-
Notifications
You must be signed in to change notification settings - Fork 8
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
Allow custom Taxonomy, ObjectTag subclasses to customize tagging behavior #62
Allow custom Taxonomy, ObjectTag subclasses to customize tagging behavior #62
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. |
so that system taxonomies can store this field and ensure no one edits them.
allowing ObjectTags to be subclassed to validate themselves with different taxonomies.
4c34a54
to
bf6d6c2
Compare
for completeness.
so that the various object tag classes can register themselves as candidates for casting/resyncing ObjectTags. * Updates tests to ensure full coverage for this change * Updates Tag model to cascade delete if the Taxonomy or parent Tag is deleted. Fixes an oversight in the data model.
bf6d6c2
to
926a8d3
Compare
👍 to this. Yes, some system defined taxonomies need to overwrite get_tags |
@pomegranited In systems-defined taxonomies, one more field is necessary: visible. Maybe we can add that field in the |
* system_defined cannot be set with api.create_taxonomy, and is not editable once set. * adds taxonomy.visible_to_authors, which can be set to False for fully automated tagging.
to allow subclasses to be registered wherever they want to be.
f260ddd
to
9d07be9
Compare
9d07be9
to
a8566d3
Compare
@pomegranited Looks good 👍
|
Hi @ormsbee CC @bradenmacdonald This and openedx/edx-platform#32661 are ready for your eyes :) |
* uses the (overwritable) is_valid method instead of the (difficult to override) class method valid_for * moves cast_object_tag to the registry, since we had to cast ObjectTags to do ^ * removes ObjectTag base class from the registry, so we can still use cast_object_tag for validation
and removes the Closed vs Open ObjectTag subclasses, and registry. Tests are passing, but coverage isn't at 100%
c4f5cc7
to
a7661bb
Compare
* adds optional taxonomy_class property+field to Taxonomy * adds Taxonomy cast() method to use this class * oel_tagging.api uses Taxonomy.cast() whenever practical * moves ObjectTag validation back to Taxonomy * removes ObjectTag.resync() logic -- we don't need it yet. * removes ObjectTag.object_type field -- we're not using it for anything. * squashes migrations from previous commits * reduces diff
a7661bb
to
0807949
Compare
* adds ContentTaxonomy and ContentTag proxy models * updates api to create/return ContentTaxonomy and ContentTag models * other updates to align with changes to openedx/openedx-learning#62 * fixes linting & test coverage
Subclasses can override this method to perform their own validation checks, e.g. against dynamically generated | ||
tag lists. | ||
Subclasses should override the internal _validate* methods to perform their own validation checks, e.g. against | ||
dynamically generated tag lists. |
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.
It would be good to have a fully dynamic taxonomy (perhaps prime numbers or fibonacci numbers, something like that) in the test suite here. That can come in a follow-up PR though, based on learnings from the system taxonomies 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.
Yep, good idea. @ChrisChV is there time/space to cover this test case as part of your system taxonomy 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.
Yes, I will keep that in mind now that I make the changes in my PR
also updates some docs as per PR review.
Thanks @ormsbee. Could you please hit merge on this? It seems I don't have permission and I'm not sure who else does. |
@pomegranited has permissions. Do you mind if I wait for her to come on so that she can squash as she deems appropriate? Also, 🤦 that I didn't put up a thread to expand your permissions to include this repo. I'll just do that now... |
@ormsbee Ah ok that's great 👍🏻 Thanks! |
Huge thank you @bradenmacdonald , @ChrisChV and @ormsbee ! I think retaining the discussions here is important for this PR, but I don't want to muddy the |
@pomegranited 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
Adds fields and methods to Taxonomy to support use of Taxonomy subclasses to customize tagging behavior, e.g. when returning the list of available tags. Ensures that Taxonomy instances returned from the API are cast where possible, and makes it easier for these subclasses to be used.
Also adds support for ObjectTag subclasses via
cast
andcopy
methods, however full use of this must be handled by the implementing API -- here in oel_tagging, we just assume ObjectTag.Taxonomy._taxonomy_class
field to store the Taxonomy subclass that should be used to instantiate this instance.system_defined
, andvisible_to_authors
.Supporting information
Part of openedx/modular-learning#63, addresses openedx/edx-platform#32518 (comment)
Testing instructions
Manual testing is possible but not very useful, since we don't yet have views for these models and APIs.
But you can check that the added tests cover the expected polymorphism behavior supported by this change, and that tests are still covering 100% of the
openedx_tagging
module.Other information
See openedx/edx-platform#32661 for this branch's use with content tagging.