Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assume that scope IDs are opaque key objects #708

Open
kdmccormick opened this issue Jan 12, 2024 · 3 comments
Open

Assume that scope IDs are opaque key objects #708

kdmccormick opened this issue Jan 12, 2024 · 3 comments
Assignees

Comments

@kdmccormick
Copy link
Member

kdmccormick commented Jan 12, 2024

In edx-platform, which accounts for virtually all of real-life xblock usage, a block's def_id and usage_id are assumed to be instance of opaque key classes.. This is helpful and lets assume other things too. For example, every XBlock usage in edx-platform will have an associated UsageKey, which, in turn, will have an associated LearningContextKey.

This XBlock repo, though, doesn't make that assumption. As of right now, xblock-sdk, the only other known XBlock Runtime, uses strings for scope IDs. We would like to update both this repo as well as xblock-sdk to use opaque key objects. Discussion: #690 (comment)

Tasks

@kdmccormick
Copy link
Member Author

@bradenmacdonald , Here's the current ScopeIds definition:

class ScopeIds(namedtuple('ScopeIds', 'user_id block_type def_id usage_id')):
    ....

I'm thinking we could change it to something like:

class ScopeIds(NamedTuple):
    user_id: int  # or just 'object'?
    block_type:  str
    def_id: DefinitionKey
    usage_id: UsageKey

do you agree?

@bradenmacdonald
Copy link
Contributor

This is great, @kdmccormick

Just need to investigate the type of user_id a bit more. I know it can be None sometimes when an XBlock is not "bound" to a particular user. And at least in the "new runtime", an anonymous user gets a string ID. So I think it should be typed as int|str|None.

Example: https://github.com/openedx/edx-platform/blob/2cd1410d9951f0b5cd1639f25dd34e191d4d75f3/openedx/core/djangoapps/xblock/runtime/runtime.py#L110-L117

kdmccormick added a commit that referenced this issue Jan 12, 2024
Adds opaque-keys as a dependency for some new unit tests.
Normally I wouldn't add a dependency just for a couple tests,
but we anticipate to make the repo depend on opaque-keys soon
anyway:

* #707
* #708

Bumps version from 1.9.1 to 1.10.0
kdmccormick added a commit that referenced this issue Jan 12, 2024
Adds opaque-keys as a dependency for some new unit tests.
Normally I wouldn't add a dependency just for a couple tests,
but we anticipate to make the repo depend on opaque-keys soon
anyway:

* #707
* #708

Bumps version from 1.9.1 to 1.10.0
kdmccormick added a commit that referenced this issue Jan 23, 2024
Add two new properties to `XBlock` objects: `.usage_key` and
`.context_key``. These simply expose the values of
`.scope_ids.usage_id`` and `.scope_ids.usage_id.context_key`,
providing a convenient replacement to the deprecated,
edx-platform-specific block properties `.location` and `.course_id`,
respectively.

Note: this adds opaque-keys as a dependency for some new unit tests.
Normally I wouldn't add a dependency just for a couple tests,
but we anticipate to make the repo depend on opaque-keys soon
anyway:

* #707
* #708

Bumps version from 1.9.1 to 1.10.0
kdmccormick added a commit that referenced this issue Jan 24, 2024
Add two new properties to `XBlock` objects: `.usage_key` and
`.context_key`. These simply expose the values of
`.scope_ids.usage_id`` and `.scope_ids.usage_id.context_key`,
providing a convenient replacement to the deprecated,
edx-platform-specific block properties `.location` and `.course_id`,
respectively.

Note: this adds opaque-keys as a dependency for some new unit tests.
Normally it wouldn't make sense to add a dependency just for a couple
of tests, but we anticipate to make the repo depend on opaque-keys:
soon anyway, for:

* #707, and
* #708

Bumps version from 1.9.1 to 1.10.0
@kdmccormick kdmccormick self-assigned this Jan 31, 2024
@kdmccormick
Copy link
Member Author

I'm currently doing this in tandem with #707 , implemented by #713.

My thinking is that we'll make the change, bump the major version of XBlock, and pin xblock-sdk to an older version of XBlock.

Following up, we should either deprecate xblock-sdk, or a find a maintainer for it who is willing to remove the XBlock version pin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants