-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat: adds Content Tagging #32661
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: adds Content Tagging #32661
Conversation
|
Thanks for the pull request, @pomegranited! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
ChrisChV
left a comment
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.
@pomegranited I left a minor comment, but it looks good to me 👍
- I tested folowing the testing instructions
- I read through the code and tests with care
fb4a1e2 to
19e3663
Compare
d780cc7 to
180a29c
Compare
|
@pomegranited 👍 to the new changes
|
bradenmacdonald
left a comment
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.
Looks good, just minor comments. I do hope that we can simplify the casting/validation even more in the future.
|
|
||
| # Tagging | ||
| 'openedx_tagging.core.tagging.apps.TaggingConfig', | ||
| 'openedx.features.content_tagging', |
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.
(This can come in a separate PR)
It would be good to add openedx.features.content_tagging to mypy.ini so you get static type checking, and to setup.cfg isolated_apps so you can make sure all other parts of the code are only importing from content_tagging.api. I think you can add openedx_tagging.core.tagging to isolated_apps as well to make sure we're only importing from api.
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.
Oo that's very cool.. will remember this for a future PR.
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.
ef6d2f9 to
0e86583
Compare
|
@pomegranited Let me know when the pypi thing is fixed, and I'll merge this for you. |
76d8dcc to
1a3c38f
Compare
|
@bradenmacdonald Resolved :) requirements/edx/kernel.in#L118 I also squashed my commits down and rebased on latest |
from cms.djangoapps.contentstore.helpers to common.djangoapps.student.auth
Adds models and APIs to support tagging content objects (e.g. XBlocks, content libraries) by content authors. Content tags can be thought of as "name:value" fields, though underneath they are a bit more complicated. * adds dependency on openedx-learning<=0.1.0 * adds tagging app to LMS and CMS * adds content tagging models, api, rules, admin, and tests. * content taxonomies and tags can be maintained per organization by content creators for that organization.
1a3c38f to
6206316
Compare
|
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. |
Description
Adds the models, APIs, and permissions/rules for
openedx.features.content_tagging, which builds on theoel_taggingapp changes made by openedx/openedx-learning#62These changes are the first step towards allow Course Authors to tag content objects (courses, units, libraries) in edx-platform, but none of these changes are user-facing yet.
Supporting information
Part of openedx/modular-learning#63
Testing instructions
This change should be 100% covered by unit tests.
But since there's no public-facing APIs yet, if you want to do any further verification, you can test the functionality from the devstack and shell.
masteropenedx/devstack up and running.paver install_prereqsin the LMS shell (fromdevstackdir, runmake lms-shell) and CMS shell (make studio-shell)../manage.py lms migratein the LMS shell.cms/envs/private.pywhich contains the following. You may need to restart Studio for these changes to take effect:make dev.restart-devserver.studioedx@example.com/edx)granted, and select the OpenCraft organization as the only org you can create courses for.From here, you'll have to play around in the shell, because we only have Django Admin capabilities for ContentTaxonomies right now, and you have to be global staff to access Django Admin, which doesn't let us test out our org restrictions.
For example:
Deadline
None
Other information
Depends on openedx/openedx-learning#62 and openedx/openedx-learning#65
Data migrations added here can be easily rolled back.
Course creator groups added by #26616.
Author to do before merge: