docs: add ADR for taxonomy_type field, amend ADR 0002#662
docs: add ADR for taxonomy_type field, amend ADR 0002#662mgwozdz-unicon wants to merge 1 commit into
Conversation
openedx_tagging needs a layering-safe way to report whether a Taxonomy is competency-enabled (openedx#618), without gaining any knowledge of the CompetencyTaxonomy subclass CBE owns. Adds a taxonomy_type field to the base Taxonomy model set by the caller that creates a CompetencyTaxonomy row, and documents why two prior approaches on openedx#618 (an openedx_tagging-side hasattr check, an overridable get_type() method) were unsound. Amends ADR 0002 to note the corresponding creation-time invariant and that its rejection of a flat taxonomy_type column still stands, unaffected by this narrower field.
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @mgwozdz-unicon! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
| whether a ``CompetencyTaxonomy`` row actually exists for that taxonomy. This is accepted | ||
| because it is enforced at a single transactional chokepoint (the creation path for a | ||
| ``CompetencyTaxonomy``), not as an ongoing invariant that application code must maintain | ||
| across multiple call sites. |
There was a problem hiding this comment.
I'm not sure about this.
Add a
taxonomy_typefield directly toTaxonomy: ATaxonomyType(models.TextChoices)enum with valuesTAGS(default) andCOMPETENCY
So we're still hard-coding a reference to COMPETENCY in the tagging app, and we now have to ensure that we keep the field's value in sync with whether or not a CompetencyTaxonomy actually exists.
Plus, per the ADR 614, we have to update a whole bunch of other tagging REST APIs to support the taxonomy_type field.
If we're going to hard-code some reference to Competencies in the tagging app, I think what I'm leaning toward is something radically simpler:
Put this in the taxonomy list/get serializer, and that's it - no other changes needed to the taxonomy app.
# Technically this is a layering violation as our tagging/taxonomy app is not supposed
# to be aware of competencies, but it simplifies a lot for us and it doesn't require
# importing any competency code.
# We can revisit this in the future if we introduce other taxonomy types.
taxonomy_type = "competency" if hasattr(instance, "competencytaxonomy") else "tags"REST APIs: I don't think there's any need to change the tagging REST APIs. To create a competency taxonomy from the frontend, you could either (A) call the "create taxonomy" REST API and on succes, call the "create competency metadata for taxonomy" REST API (from the cbe app), or (B) call a competency REST API that creates the Taxonomy and the CompetencyTaxonomy object together in a single request.
Export: You could add the taxonomy_type field to the JSON export data if that seems useful. It's not possible to include on CSVs.
Import: If the user chooses to import something as a Competency taxonomy (in the frontend UI), it's the same thing: import the taxonomy then call a second API to convert it to Competency.
There was a problem hiding this comment.
Going through in order:
On the sync-drift point: yes, I agree that this tradeoff is the risk, but hopefully it can be resolved at implementation time.
On the REST API surface per #614: I don't think ADR 13 is adding work there. Those endpoints need to accept taxonomy_type on create/import regardless of how the Get side reports it, so that's #614's scope either way, not something this ADR introduces on top.
On the hasattr suggestion: I hear the appeal, it's less code today, and I get that you're framing it as a deliberate, called-out layering violation rather than an accidental one. Thinking it through more, I don't think either approach avoids touching openedx_tagging when a new type shows up eventually, the field approach still means adding a value to the shared enum. Where I think they differ is what that touch requires: with the field, adding a type is adding a string to a catalog, the read path (TaxonomySerializer) doesn't change, it already just returns whatever's stored. With hasattr, adding a type means adding another branch to the serializer's logic that names a specific downstream app's MTI relation, so the read path itself keeps growing a fact about another app's model internals with every flavor. That feels like the thing the ADR is trying to keep out of a generic library, holding the type as a piece of data the caller owns, versus holding it as code that inspects other apps' structure. Also worth flagging: this looks close to option 2 from your comment on #618 (Adding a tiny bit of Competency awareness to the base tagging app), where you said you leaned toward 1+3 over it. Did something change your thinking there, or is this meant as a variant of option 1 rather than 2?
On separate Competency REST APIs: Since the Taxonomies landing page is meant to serve both flows, I don't think it makes sense to make the frontend juggle two calls, especially when this could become even more calls than that if the number of taxonomy types increases.
On export/import: agreed, that's already settled in #614 and not really in scope here.
There was a problem hiding this comment.
Yeah, sorry, I'm still not happy with any of the approaches here so I'm just throwing out ideas to see if there's anything that you like. I'm not so unhappy that I'm going to block anything though. I'm reluctantly fine with any of these approaches, but I want to see if we can come up with something clean that doesn't have any layering violations.
Thinking about this more, what if we kept the taxonomy code in openedx-core / oel_tagging completely free of competency awareness, but use the platform's override of the REST API at openedx/core/djangoapps/content_tagging/rest_api/v1/ to inject the competency pieces you need in the REST API? Then you could add taxonomy_type = "competency" if hasattr(instance, "competencytaxonomy") else "tags" to the serializer and also wire through the "competency creation" changes to the REST API specified in #614 while still keeping some semblance of layer separation, because the core tagging API is unchanged.
So the layers would be:
oel_taggingpython API & REST APIs in this repo - pure tagging/taxonomy only ✅content_taggingpython API in openedx-platform - pure tagging/taxonomy only ✅ (as specified on [BE] Update taxonomy Create & Import endpoints to accept Taxonomy Type #614 , "Layering constraint. openedx_tagging/api.py::create_taxonomy() creates only a plain Taxonomy. It must not import or call anything from openedx_learning. The CompetencyTaxonomy creation belongs exclusively in the CBE applet.")- CBE django app python API - can convert taxonomies to competencies, provide other competency features ✅ respects layering
content_taggingREST API in openedx-platform (openedx/core/djangoapps/content_tagging/rest_api/v1/) - injects and handles thetaxonomy_typefield (not persisted in the database), which gets routed to the competency python API as needed.⚠️ breaks layering but this is a fairly minimal "wrapper"/"router" type code anyways so it seems like a logical place to do this- Studio MFE frontend: combined handling of taxonomies & competencies as per UX mockups.
⚠️ breaks layering but the product folks think it's better UX than separating them into two separate UIs.
At least in that case there are clean boundaries drawn in several places that completely respect the layering.
3 participants
Summary
openedx_tagging's taxonomy Get endpoints need to report whether a taxonomy is aCompetency Taxonomy (#618), symmetrically with the Create/Import endpoint's
taxonomy_typefield (#614). Two approaches were tried on #618 and found unsound:CompetencyTaxonomyrow directly insideopenedx_tagging(
hasattr(instance, "competencytaxonomy")) — hardcodes a downstream applet'smulti-table-inheritance relation name into a generic, standalone library.
Taxonomy.get_type()method mirroring the existingsystem_definedbase/override pattern — unsound for two independent reasons: that pattern
(
Taxonomy._taxonomy_class/.cast()) is being removed per Remove Taxonomy subclasses functionality + system-level taxonomies #634, and separately,.cast()/.copy()'s existing implementation only copies a hardcoded list of baseTaxonomyfields in Python and never queries a subclass's own table, so it wouldsilently misrepresent a true multi-table-inheritance subclass like
CompetencyTaxonomyeven if Remove Taxonomy subclasses functionality + system-level taxonomies #634 weren't happening.
This PR adds ADR 0013 (
openedx_tagging), proposing ataxonomy_typefielddirectly on
Taxonomy, set explicitly by the caller that creates aCompetencyTaxonomyrow rather than inspected by
openedx_taggingitself. It also amends ADR 0002(
openedx_learning) with the corresponding creation-time invariant and a note that itsexisting rejection of a flat
taxonomy_typecolumn stands, unaffected by this narrowerfield (different problem: a REST API type label, not where competency data lives).
Both are
Status: Proposed, open for review.Context
taxonomy_typetaxonomy_typefield (this ADR's field mirrors it)CompetencyTaxonomy's multi-tableinheritance is unaffected; only the
system_defined/free-text mechanism is removed)openedx_learning) —CompetencyTaxonomymulti-table-inheritance modelopenedx_tagging) — prior art foropenedx_tagging-owned ADRsdriven by CBE needs