-
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
System defined taxonomies #67
Merged
pomegranited
merged 28 commits into
openedx:main
from
open-craft:chris/system-defined-taxonomies
Aug 2, 2023
Merged
Changes from 22 commits
Commits
Show all changes
28 commits
Select commit
Hold shift + click to select a range
ac21e1c
feat: System-defined taxonomies
ChrisChV 55636f7
feat: Language and Author taxonomies
ChrisChV 327bb58
feat: Generic system defined object tags and Language object tag added
ChrisChV 58fc1e8
chore: get_tags_query_set added to LanguageObjectTag
ChrisChV 9e7ed4e
chore: Adding _validate_taxonomy function to all system defined objec…
ChrisChV 899d61f
chore: Updating system_defined_taxonomy_id
ChrisChV 1e68687
refactor: consolidates ObjectTag validation
ChrisChV 9e5d854
feat: System defined taxonomies subclasses
ChrisChV 9b72fec
style: linting and formatting
pomegranited 69eeece
refactor: use negative numbers as primary keys for system taxonomies …
pomegranited 2a0b4b6
refactor: use ObjectTag subclasses where possible
pomegranited de3cc3e
refactor: LanguageTaxonomy overrides get_tags
pomegranited 437ae98
docs: System taxonomy creation doc updated with Dynamic tags approach
ChrisChV 8ca16ea
style: Updating comments
ChrisChV 0c74bb4
style: Separating the models into base and system defined
ChrisChV 9a38047
fix: Update language fixture to negative pk
ChrisChV 1847248
feat: Updates on Taxonomy and Tag admins
ChrisChV eaaba78
feat: Instance validations on ModelSystemDefinedTaxonomy
ChrisChV bfc532a
feat: use Taxonomy.cast and ObjectTag.cast in rules
pomegranited c54cdda
fix: adds unique_together
pomegranited 4f51683
fix: indexes
pomegranited a29d227
fix: Model pk validation on Model taxonomy
ChrisChV a34d401
style: comments and style
ChrisChV 4b56ec8
feat: Creating language taxonomy on fixtures
ChrisChV 034726d
test: Added system defined to api tests
ChrisChV 8f1cac1
Merge branch 'main' into chris/system-defined-taxonomies
ChrisChV cde323e
test: Fixing tests after merge with main
ChrisChV 8a9d241
chore: Package version update
ChrisChV File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
OK, I definitely like the approach described here ^^
But what I'm still caught up on is
ModelObjectTag
. Do we need to subclass ObjectTag and push this logic into it? From reading the approach described here, and from what we discussed, I assumed we'd happily just use the normalObjectTag
or evenContentObjectTag
. Because we're creatingTag
instances as needed, and the regularObjectTag
already works correctly withTag
instances in Taxonomies.Of course you need some logic to:
Tag
instances: e.g. list all known users or languages → this belongs in the Taxonomy subclass but I don't think it's even necessary for this project, because we never need to display the "available" tags of system taxonomies to users. The tags will get auto-created by the system and the system already knows what user/language is in use. There's no UI and no dropdown menu to show.Tag
instances when tagging an object, if they don't yet exist. Sure, but this logic could live anywhere - in theedx-platform
code that's applying the User and Language tags, in thetag_object()
API, in theTaxonomy.tag_object()
API - I don't think it is necessary to put this into anObjectTag
subclass, and doing so creates a lot of complexity.Tag
s in it just-in-time... What other functionality do we need here?Tag
instances when the upstream object is deleted: we don't really need to worry about this for now, as languages won't be deleted and user deletion is very rare. In fact, I don't think user deletion even really happens; it's just retirement/anonymization that happens per GDPR. So in the future we could add a signal listener that detects "user retirement" and then deletes anyTag
andObjectTag
s associated with the user, but I would leave that out of scope for now. We don't even really know how/where such user tags will be used yet.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 agree with everything you've said here, except for:
I think providing a
ModelTaxonomy
(or mixin) that handles Tag auto-creation is broadly useful.And I think it belongs in this openedx-learning library, because it's not content-specific and could be useful when tagging other types of objects, like people or forum posts. However, if it makes more sense for this to live in edx-platform for the MVP, I'm OK with that, we can always move it into openedx-learning later.
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.
That's true except for language, it's the only system-defined taxonomy that can be "edited" in a content by the content author.
I agree with Jill too, in addition to the auto creation, it checks that the instance (User, Organization) exists to see if the tag is valid.
I agree, the simplest thing is to check if the user or organization exists.
To clarify: the ModelTaxonomy is not used for languages, it is only used for the taxonomy of users and organizations
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.
👍🏻 Sure, good point. That's totally reasonable, and that sounds like a mixin to me. After all, in the platform if we keep the "ContentTaxonomy" vs "regular Taxonomy" distinction, it would have to be a mixin, to allow Content tagging with auto-creation of tags.
Oh OK. I've asked for clarification on that because I heard the opposite. But in any case, it seems we agree that "list available tags" should live in the Taxonomy. (I think it should be similar to what you implemented in
LanguageTaxonomy.get_tags() -> List[Tag]
but we'll probably eventually need an API that returns tag strings/IDs, notTag
objects because they may not exist yet. Imagine doing an auto-complete to tag users and there are 1,000,000 users in the system - we don't want to createTag
objects for each user unless they're actually used as a tag. Or maybe that would be a separatesearch_available_tags
API. In any case, future PR.)That's fine. I do think however that belongs in the Taxonomy, not the ObjectTag itself. Because it's mostly a question of validating the User/Org then creating the corresponding
Tag
instance. The ObjectTag can validate itself with the relation to theTag
and doesn't need any special logic to handle ongoing validation related to the User/Org. If necessary, the Taxonomy can have some code to keep theTag
instances in sync with the User/Org models, but I think we can leave that out of scope for now to keep it simple.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, agreed -- and note we don't need to search model tags in MVP (User and Organization system taxonomies are our only model system taxonomies, and neither are editable by the content authors).
Cool, thanks for your clarifications here @bradenmacdonald . I'll note this on my "cleanup" task: openedx/modular-learning#85