-
Notifications
You must be signed in to change notification settings - Fork 4k
feat: Explicit mapping from container_types to OLX tags #36580
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
feat: Explicit mapping from container_types to OLX tags #36580
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-review
|
||
These tag names are historical. We keep them around for the backwards compatibility of OLX | ||
and for easier interation with modulestore-powered content (e.g., copy-paste between content | ||
libraries and courses. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
libraries and courses. | |
libraries and courses). |
Section = "section" | ||
|
||
@property | ||
def olx_tag(self) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm torn between calling this block_type vs olx_tag. On one hand, we generally don't want to think of Containers as pseudo-XBlocks, so I'm trying to frame this all as an OLX thing. On the other hand, container_sync will need to understand that Unit maps to the block_type vertical
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, just one request.
raise TypeError(f"unexpected ContainerType: {self!r}") | ||
|
||
@property | ||
def source_olx_tags(self) -> set[str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get the need for this, but I'd prefer that this be a private method; I don't want the public API to involve multiple tags per container as it's confusing. i.e. lets focus on .olx_tag
and .from_source_olx_tag()
as the public API, and this is an implementation detail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. Updated, ready for re-review @bradenmacdonald .
a4eb18f
to
19fce2c
Compare
19fce2c
to
e48bdee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't test this but the code LGTM.
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
This is needed by several aspects of the Teak Libraries Overhaul (https://github.com/orgs/openedx/projects/66) including: * copy-paste of containers between courses and V2 libraries * syncing of containers in courses from V2 libraries * the import_from_modulestore API
This is needed by several aspects of the Teak Libraries Overhaul (https://github.com/orgs/openedx/projects/66) including: * copy-paste of containers between courses and V2 libraries * syncing of containers in courses from V2 libraries * the import_from_modulestore API
This is needed by several aspects of the Teak Libraries Overhaul (Libraries Overhaul) including: