feat: XBlock.usage_key, XBlock.context_key convenience props

[kdmccormick]

Reasoning

According to these warnings, we should eventually make these updates across the board:

• block.location -> block.scope_ids.usage_id
• block.course_id -> block.scope_ids.usage_id.context_key

The thing is... these new names are (subjectively, of course) not very good 😄 They take longer to type out, they take up more space on a line, and IMO are more intimidating to devs who don't have their heads way into XBlock land. Every time I update code from location or course_id to the new, longer name, I find the resulting line harder to read. It also took me a long time to remember whether to use id versus key, which is maddeningly inconsistent in the phrase "block.scope_ids.usage_id.context_key".

I propose:

• block.location -> block.usage_key
• block.course_id -> block.context_key

I'd also be open to:

• block.location -> block.usage_id
• block.course_id -> block.context_id

or, if we want the interface to remind people that a context key is, conceptually, just a component of a usage key:

• block.location -> block.usage_key
• block.course_id -> block.usage_key.context_key

To Do

• shop this idea around
• update the changelog and version
• unit test
• update the docs
• post-merge: update warnings in https://github.com/openedx/edx-platform/blob/master/openedx/core/djangoapps/xblock/runtime/shims.py

[connorhaugh]

Things I like:

1. no (fewer) warnings :)
2. the name of the var describes what it is and can possibly be.

Therefore, I think your changes make sense.

That being said, course_id -> context_key will require some evangelization. "Course id" is common parlance, and there certainly are a broad variety of contexts outside of xblocks where it is used. A line as to where context_key is used as opposed to course_id will require good, visible documentation.

[ormsbee]

I'm good with it. I agree with usage_key and context_key.

That being said, course_id -> context_key will require some evangelization. "Course id" is common parlance, and there certainly are a broad variety of contexts outside of xblocks where it is used. A line as to where context_key is used as opposed to course_id will require good, visible documentation.

Agreed, but I think it's important that we do. There's a VideoBlock handler that currently breaks in content libraries right now because there's no course_id in a v2 library.

[bradenmacdonald]

I agree, and I like the proposed block.usage_key and block.context_key.

I have a question in turn, which is: can we specify a type hint that these return UsageKey and LearningContextKey? That would be nice (assuming we also added a py.typed file to this repo), but would be wrong in the specific case of using the new .usage_key convenience property in the XBlock-SDK which returns some other usage ID format ( a string I think?).

[bradenmacdonald]

these new names

Also FWIW, the names (mostly) aren't exactly new... block.scope_ids.usage_id is 9 years old, vs. block.location 10 years old :p Only context_key is somewhat new.

[feanil]

• block.location -> block.usage_key
• block.course_id -> block.context_key

This sounds good to me as well.

[kdmccormick]

I have a question in turn, which is: can we specify a type hint that these return UsageKey and LearningContextKey? That would be nice (assuming we also added a py.typed file to this repo), but would be wrong in the specific case of using the new .usage_key convenience property in the XBlock-SDK which returns some other usage ID format ( a string I think?).

@bradenmacdonald Oof, I had no idea that xblock-sdk's runtime had different types for scope_ids. Is that difference considered a legit feature of the XBlock framework, or was it just that the opaque-keys project never got around to moving xblock-sdk from strings to keys?

I love adding type hints, but I don't want to add ones that will be wrong in certain cases.

[jansenk]

My understanding was that "location" was a term we were trying to move away from. Am I incorrect?

[kdmccormick]

@jansenk You're correct. It's just that the current replacement is unwieldy (block.scope_ids.usage_id).

This PR would make the replacement nicer (block.usage_key).

[bradenmacdonald]

@kdmccormick

I had no idea that xblock-sdk's runtime had different types for scope_ids. Is that difference considered a legit feature of the XBlock framework, or was it just that the opaque-keys project never got around to moving xblock-sdk from strings to keys?

You can see the details here: https://github.com/openedx/xblock-sdk/blob/529c0c87a740e39f12ae42ec9ff735e198d983f1/workbench/runtime.py#L100-L157

The XBlock library has never cared what format your Scope IDs are in; they can be anything. XBlock SDK used strings, and edx-platform uses opaque keys.

The XBlock library also requires a "definition ID" as one of the Scope IDs and requires some usage of it in the API, though that hasn't proven to be a very useful concept, and Learning Core may not use it at all.

I think it would be reasonable at this point to change the XBlock SDK core lib to require that the scope IDs are opaque keys, and change the XBlock SDK accordingly. The XBlock SDK doesn't have to use the same opaque keys as edx-platform; since it uses scenarios instead of courses for example it could use its own type of usage key that references a scenario as its context, not a course. I think that would be a nice win for consistency if we're using opaque keys across the board, and let use do type hints.

[kdmccormick]

@bradenmacdonald I opened #707 and #708. I think we can merge this PR without tackling those first. Let me know if you disagree.

[bradenmacdonald]

Sounds good to me @kdmccormick 👍🏻

[kdmccormick]

@bradenmacdonald @ormsbee @feanil @jansenk This is ready for review.

[kdmccormick]

Note for reviewers.

[kdmccormick]

I added opaque-keys as a dependency for the new unit tests. Normally I wouldn't add a base dependency just for a couple tests, but we anticipate to make the repo depend on opaque-keys soon anyway, so this seemed like a good step:

• Add type hints and check them in CI #707
• Assume that scope IDs are opaque key objects #708

[bradenmacdonald]

I didn't run this, but the code LGTM. Thanks so much for taking this on, @kdmccormick !

[bradenmacdonald]

Nit: On the first line of the docstring, I would prefer to say something more helpful like "This is the ID of the context (course, library, etc.) that contains this XBlock". Then, you can mention that it's an alias.

Same above for usage_key/usage_id - what is it?

[kdmccormick]

Good call, I'll make that change.