-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
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. |
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
|
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.
@@ -3217,6 +3217,10 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring | |||
# Course Goals | |||
'lms.djangoapps.course_goals.apps.CourseGoalsConfig', | |||
|
|||
# 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_tagging
app 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.
master
openedx/devstack up and running.paver install_prereqs
in the LMS shell (fromdevstack
dir, runmake lms-shell
) and CMS shell (make studio-shell
)../manage.py lms migrate
in the LMS shell.cms/envs/private.py
which contains the following. You may need to restart Studio for these changes to take effect:make dev.restart-devserver.studio
[email protected]
/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: