-
Notifications
You must be signed in to change notification settings - Fork 12
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: Tagging Django App setup #54
Conversation
Thanks for the pull request, @ChrisChV! 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.
This doesn't do much yet, but it can't really until the data architecture is decided, so 👍 with one nit, and one question about "pluggability":
- I tested this by creating a virtualenv, running migrations, running a shell, and ensuring I could save a new TagContent object to the database.
- I read through the code
-
I checked for accessibility issuesN/A - Includes documentation
- Commit structure follows OEP-0051
@@ -35,6 +35,7 @@ | |||
"openedx_learning.core.publishing.apps.PublishingConfig", | |||
# Learning Contrib Apps | |||
"openedx_learning.contrib.media_server.apps.MediaServerConfig", | |||
"openedx_learning.contrib.tagging.apps.TaggingConfig", |
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.
Hmm.. this tagging app is supposed to be a "plugin", which to me means:
- it's installed alongside edx-django-utils whose plugin utils which ensures that it gets automatically added to the
INSTALLED_APPS
list in the right context - it can be installed separately into apps where needed, and
edx-django-utils is specifically tailored to the lms
and cms
apps in edx-platform, and doesn't have an equivalent on this app, so..
Maybe we need to deal with the "pluggability" aspect when it comes time to extract this app out for reuse outside of openedx-learning?
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 Good point! I didn't account for edx-django-utils
for this. I have taken into account that it had to be "installed alongside learning-core as a dependency in the edx-platform"
Another option would be to build this app to be installed alongside edx-django-utils so that we can use them with the current open-edx content and install it as a direct dependency on learning-core when we used the learning-core content. The problem is that I don't know how complicated this approach is in the long run.
Maybe we need to deal with the "pluggability" aspect when it comes time to extract this app out for reuse outside of openedx-learning?
I will leave this as an open question
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.
Maybe we need to deal with the "pluggability" aspect when it comes time to extract this app out for reuse outside of openedx-learning?
Yeah, I think this is something that we can address later. I don't think it needs to be a plugin-utils styled plugin, but more of a library-style app that handles tagging related concerns and is used by our other apps. But I do think this means that the tagging functionality modeling in this repo should be split up so that there is:
- An app or group of tagging-related apps that is entirely self contained, i.e. has no foreign keys to anything in the
openedx_learning
package. If we're pretty confident that this is going to be its own top level thing that will eventually be used in other parts of the platform, we should give it its own top level package, e.g.openedx_tagging
(or some other unique name that we can eventually register on PyPI). - The
openedx_learning.contrib.tagging
app, which would be where we apply tagging toopenedx_learning
models likePublishableEntity
andPublishableEntityVersion
. This would probably stay in this repo over the long term.
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.
Another option would be to build this app to be installed alongside edx-django-utils so that we can use them with the current open-edx content and install it as a direct dependency on learning-core when we used the learning-core content. The problem is that I don't know how complicated this approach is in the long run.
Didn't see this reply when I wrote my message. I like this approach.
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.
Another option would be to build this app to be installed alongside edx-django-utils
I like this approach too, but it has the limitation that edx-django-utils is specifically tailored to edx-platform -- i.e., you can currently only register plugins for "lms" and "cms" -- so we'd need to enhance it to also register plugins for learning core.
So I'd rather not convert this into a plugin right now, but plan for it by respecting the encapsulation @ormsbee laid out above:
- move this to a new
openedx_tagging
package in this repo, sibling ofopenedx_learning
openedx_tagging
has no foreign keys to anything in theopenedx_learning
packageopenedx_learning.contrib.tagging
contains any models that need to link the two packages.
@ChrisChV I'm happy to do this move as part of the data architecture ADR which is coming this week, ref openedx/modular-learning#60
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 like the approach 👍
@ChrisChV I'm happy to do this move as part of the data architecture ADR which is coming this week, ref openedx/modular-learning#60
Sure, thanks @pomegranited 👍
@ChrisChV Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future. |
In this PR, the tagging application and a base model for the
TagContent
have been created.Testing instructions