Skip to content

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

Merged

Conversation

kdmccormick
Copy link
Member

@kdmccormick kdmccormick commented Apr 22, 2025

This is needed by several aspects of the Teak Libraries Overhaul (Libraries Overhaul) including:

  • copy-paste of containers between courses and V2 libraries
  • syncing of containers in courses from V2 libraries
  • the import_from_modulestore API

Copy link
Member Author

@kdmccormick kdmccormick left a 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.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
libraries and courses.
libraries and courses).

Section = "section"

@property
def olx_tag(self) -> str:
Copy link
Member Author

@kdmccormick kdmccormick Apr 22, 2025

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.

Copy link
Contributor

@bradenmacdonald bradenmacdonald left a 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]:
Copy link
Contributor

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.

Copy link
Member Author

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 .

@kdmccormick kdmccormick changed the title feat: Explicit mapping from container_types to block_types feat: Explicit mapping from container_types to OLX tags Apr 22, 2025
@kdmccormick kdmccormick force-pushed the kdmccormick/container-type-map branch from a4eb18f to 19fce2c Compare April 22, 2025 23:43
@kdmccormick kdmccormick force-pushed the kdmccormick/container-type-map branch from 19fce2c to e48bdee Compare April 22, 2025 23:45
Copy link
Contributor

@bradenmacdonald bradenmacdonald left a 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.

@kdmccormick kdmccormick enabled auto-merge (squash) April 22, 2025 23:56
@kdmccormick kdmccormick disabled auto-merge April 22, 2025 23:57
@kdmccormick kdmccormick enabled auto-merge (squash) April 22, 2025 23:57
@kdmccormick kdmccormick merged commit 9d45f85 into openedx:master Apr 23, 2025
48 checks passed
@kdmccormick kdmccormick deleted the kdmccormick/container-type-map branch April 23, 2025 00:13
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

tonybusa pushed a commit to tonybusa/edx-platform that referenced this pull request Apr 23, 2025
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
UsamaSadiq pushed a commit that referenced this pull request May 14, 2025
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
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

Successfully merging this pull request may close these issues.

4 participants