System-defined taxonomies architecture decisions - #56
Conversation
openedx-webhooks
added
the
open-source-contribution
|
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. |
pomegranited
left a comment
pomegranited
left a comment
There was a problem hiding this comment.
Thanks for writing up these decisions @ChrisChV ! I've left some suggestions, but feel free to push back if any don't make sense?
| Create a ``Content-side`` class that lives on ``openedx.features.tagging``. | ||
| You must create a content-side model for each content and taxonomy to be used. | ||
| You can use ``ContentSystemTaxonomyMixin`` and the mixin of the Taxonomy (e.g. ``LanguageSystemTaxonomy``) to create the new class. | ||
| Then, all system-defined taxonomies must be created initially in a fixture. |
There was a problem hiding this comment.
This isn't true for all system taxonomies.. some are free text, as described below. So I think this sentence should be removed.
There was a problem hiding this comment.
This sentence has been misunderstood. I have updated the document to make the separation between the creation of instances for the system-defined taxonomies and the creation of their tags
| You can use ``ContentSystemTaxonomyMixin`` and the mixin of the Taxonomy (e.g. ``LanguageSystemTaxonomy``) to create the new class. | ||
| Then, all system-defined taxonomies must be created initially in a fixture. | ||
|
|
||
| Each Taxonomy mixin has ``get_tags``; to configure the valid tags, and ``validate_tags``; to check if a list of tags are valid. |
There was a problem hiding this comment.
Each Taxonomy mixin has
get_tags; to configure the valid tags,
From what I understand, most system taxonomies won't need to override get_tags. Language is an exception, since we're filtering down the available ISO languages using Django settings.LANGUAGES , but from what I understand, most system taxonomies won't need to do this.
- hardcoded by fixtures/migrations: Tags are added to the database, linked to the SystemTaxonomy, so the base class
get_tagswill return them. - Free-form tags: no Tags exist in the database for the SystemTaxonomy; instead the user can add any free-text tag they like, and they are validated dynamically below. The base class
get_tagswill return an empty set, which is correct for free-form taxonomies.
and
validate_tags; to check if a list of tags are valid.
I'd removed validate_tags from the Taxonomy data architecture, and instead proposed just validating individual object tags with validate_object_tag.
There was a problem hiding this comment.
From what I understand, most system taxonomies won't need to override get_tags. Language is an exception, since we're filtering down the available ISO languages using Django settings.LANGUAGES , but from what I understand, most system taxonomies won't need to do this.
@pomegranited Yes it's correct. I will update the docs so this is not misunderstood
I'd removed validate_tags from the Taxonomy data architecture, and instead proposed just validating individual object tags with validate_object_tag.
Ohh Ok, Noted 👍
|
|
||
| Each Taxonomy mixin has ``get_tags``; to configure the valid tags, and ``validate_tags``; to check if a list of tags are valid. | ||
|
|
||
| We have two ways to handle tags in this type of taxonomies: |
There was a problem hiding this comment.
nit: for clarity
| We have two ways to handle tags in this type of taxonomies: | |
| We have two ways to handle tag creation and validation for system taxonomies: |
| ~~~~~~~~~~~~~~ | ||
|
|
||
| This taxonomy depends on a core data model, but simplifies the creation of Tags by allowing free-form tags, | ||
| but we can validate the tags using the ``validate_tags`` method. For example we can put the Author taxonomy associated with |
There was a problem hiding this comment.
(ditto re validate_object_tag.)
| Auto-generated from the codebase | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| Taxonomies that depend on a core data model and it is necessary to create a Tag for each object created. |
There was a problem hiding this comment.
nit: adding some justification here
| Taxonomies that depend on a core data model and it is necessary to create a Tag for each object created. | |
| Taxonomies that depend on a core data model could create a Tag for each eligible object. Maintaining this dynamic list of available Tags is cumbersome: we'd need triggers for creation, editing, and deletion. And if it's a large list of objects (e.g. Users), then copying that list into the Tag table is overkill. It is better to dynamically generate the list of available Tags, and/or dynamically validate a submitted object tag than to store the options in the database. |
| After create the ``Content-side`` class, `register a pipeline`_ with the respective filter. | ||
| Some filters must to be created, like ``CourseCreation``, ``LibraryCreation``, etc. | ||
|
|
||
| All this logic will be live on ``openedx.features.tagging``. |
There was a problem hiding this comment.
I think it's better for the new filters themselves to live outside of openedx.features.tagging?
The filter logic itself will still live in openedx-filters, and the events that trigger the filters should live as close to the content creation as possible. The openedx.features.tagging code should simply register to receive these events and act on them -- as may other openedx libraries that care about content creation.
There was a problem hiding this comment.
@pomegranited Yes, I updated the doc to be more clear about it 👍
| ~~~~~~~~~~~~~~ | ||
|
|
||
| Implement a function to add the tag from the content metadata and register that function | ||
| as a Django signal. Use openedx-filters is better in the edx context, but if there is |
There was a problem hiding this comment.
nit: u
| as a Django signal. Use openedx-filters is better in the edx context, but if there is | |
| as a Django signal. This works for Django database models, but some of the content lives in Mongo, outside of the Django models. Also, Using openedx-filters is better in the edx context, but if there is |
mphilbrick211
added
the
needs test run
| Context | ||
| -------- | ||
|
|
||
| The System-defined are closed taxonomies created by the system. Some of this are totally static (e.g Language) |
There was a problem hiding this comment.
| The System-defined are closed taxonomies created by the system. Some of this are totally static (e.g Language) | |
| System-defined taxonomies are closed taxonomies created by the system. Some of these are totally static (e.g. Language) |
| Context | ||
| -------- | ||
|
|
||
| One main characteristic of the System-defined is the automatic content tagging. |
There was a problem hiding this comment.
| One main characteristic of the System-defined is the automatic content tagging. | |
| One main characteristic of system-defined taxonomies is automatic content tagging. |
pomegranited
left a comment
pomegranited
left a comment
There was a problem hiding this comment.
👍 Couple of nits to fix, but this is good to merge once they're addressed.
Thank you @ChrisChV !
-
I tested thisN/A -- only ADRs - I read through the code
-
I checked for accessibility issuesN/A - Includes documentation
| outside of the Django models. Also, using openedx-filters is better in the edx context, but if there is | ||
| other no-edX project that need to use ``openedx-tagging``, can use the Django Signals approach. | ||
|
|
||
| .. _openedx-filters: https://github.com/openedx/openedx-filters/tree/a4a192e1cac0b70bed31e0db8e4c4b058848c5c4 |
There was a problem hiding this comment.
Better to link to project home:
| .. _openedx-filters: https://github.com/openedx/openedx-filters/tree/a4a192e1cac0b70bed31e0db8e4c4b058848c5c4 | |
| .. _openedx-filters: https://github.com/openedx/openedx-filters |
| ~~~~~~~~ | ||
|
|
||
| It is necessary to create `filters`_ for each content for creation/edition: ``CourseCreation``, ``LibraryCreation``, etc. | ||
| This filters will live on ``openedx-filters``. |
There was a problem hiding this comment.
nit:
| This filters will live on ``openedx-filters``. | |
| These filters will live in ``openedx-filters``. |
| After create the ``Content-side`` class for each content and taxonomy, | ||
| create a function inside each class with the logic of the auto tagging. | ||
| Then you need to register each class as `a pipeline`_ with the respective filter. | ||
|
|
||
| This pipelines will live on ``openedx.features.tagging`` |
There was a problem hiding this comment.
I think this can be simpler, since it's only the code encapsulation we need to specify here:
| After create the ``Content-side`` class for each content and taxonomy, | |
| create a function inside each class with the logic of the auto tagging. | |
| Then you need to register each class as `a pipeline`_ with the respective filter. | |
| This pipelines will live on ``openedx.features.tagging`` | |
| Auto-tagging pipelines will live under ``openedx.features.tagging``, registered as `a pipeline`_ with the respective filter. |
e60097f to
45add7c
Compare
|
@ormsbee It's ready for your review |
|
@ChrisChV the conventional commit labels are case sensitive. Can you amend the last commit when you get a second? |
e0d
removed
the
needs test run
45add7c to
69ee8dc
Compare
|
@ChrisChV 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
7 participants
This PR documents the decisions that arose from openedx/modular-learning#61
The ADRs here continues the enumeration of #55
Testing instructions
Supporting information
Closes openedx/modular-learning#61