feat: accept taxonomy_type on taxonomy create and import endpoints - #803
feat: accept taxonomy_type on taxonomy create and import endpoints#803alezconsultant wants to merge 4 commits into
Conversation
Add taxonomy_type as a write-only ChoiceField on TaxonomySerializer ("tags" default,
"competency" via a new TaxonomyType enum). TaxonomyView.perform_create() and
create_import() discard it after validation: openedx_tagging must never import
openedx_learning, so it can't act on it. create_import()'s taxonomy-creation step is
extracted into an overridable _create_taxonomy_for_import() hook for subclasses.
Add create_competency_taxonomy() to the CBE applet: creates the Taxonomy and linked
CompetencyTaxonomy row in one transaction, via save_base(raw=True) since save() would
re-save Taxonomy with unpopulated field values.
Add CompetencyTaxonomyView(TaxonomyView), overriding perform_create() and
_create_taxonomy_for_import() to dispatch to create_competency_taxonomy() when
taxonomy_type="competency". Lives here since this is the layer that can see both
openedx_tagging and openedx_learning.
See openedx#628
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks for the pull request, @alezconsultant! 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. |
mgwozdz-unicon
left a comment
There was a problem hiding this comment.
Claude and I worked together on this code review and are requesting the enumerated changes below. The layering is done right: openedx_tagging's serializer and view changes never import or reference openedx_learning/CompetencyTaxonomy anywhere, create_competency_taxonomy() is the only place that touches both packages, and lint-imports confirms both contracts still hold. That matches the architecture constraint in #614 ("openedx_tagging must not import or instantiate CompetencyTaxonomy").
1. CompetencyTaxonomyView.perform_create() returns a 500 instead of a 400 on a validation failure.
In src/openedx_learning/applets/cbe/views.py, the competency branch of perform_create() calls create_competency_taxonomy(**serializer.validated_data) directly, with no error handling. The base TaxonomyView.perform_create() (the branch taken for "tags", in src/openedx_tagging/rest_api/v1/views.py) wraps the equivalent call in try/except exceptions.ValidationError and re-raises DRF's ValidationError so it comes back as a 400. The competency branch skips that.
Claude reproduced it: create_competency_taxonomy() calls create_taxonomy() internally, which calls taxonomy.full_clean(). Posting taxonomy_type="competency" with an export_id that already exists raises django.core.exceptions.ValidationError from that full_clean() call, and it propagates unhandled, so the response is a 500. The same duplicate export_id through plain TaxonomyView returns a 400. Can you wrap the competency branch's call the same way the base class does, so both paths return a 400 on the same kind of failure?
2. Please add a test that exercises CompetencyTaxonomyView itself.
Every new test in tests/openedx_tagging/test_views.py posts to TAXONOMY_LIST_URL / TAXONOMY_CREATE_IMPORT_URL, which route to plain TaxonomyView (see src/openedx_tagging/rest_api/v1/urls.py), not CompetencyTaxonomyView. The two new methods on CompetencyTaxonomyView, i.e. the actual taxonomy_type dispatch this ticket exists to add, have no test going through the view at all. That's how item 1 got through. CompetencyTaxonomyView isn't registered on a URL in openedx-core yet, but it can still be tested directly via APIRequestFactory + .as_view() without one. At minimum: the competency branch of perform_create() returns a CompetencyTaxonomy, and it returns a 400 (not a 500) on a validation failure, same as the "tags" branch.
3. test_create_taxonomy_type_tags_or_omitted and test_import_taxonomy_type_tags_or_omitted claim more than they check, and nothing else in the repo checks it either.
Both docstrings say the endpoint "creates a plain Taxonomy" with no way to create a CompetencyTaxonomy row, but both only assert Taxonomy.objects.filter(name=...).exists(). Since CompetencyTaxonomy is multi-table inheritance from Taxonomy, that assertion would pass either way, so it doesn't test the claim. It looks like there's no test_views.py under tests/openedx_learning/ at all, so nothing in the repo actually asserts that a CompetencyTaxonomy row is absent here. Since tests/openedx_tagging can't import CompetencyTaxonomy without breaking the layering rule it's demonstrating, can you add that assertion as its own test in tests/openedx_learning, with a docstring describing what it actually checks, and amend these two docstrings to only claim what their own assertions verify?
4. The comment on where export_id gets auto-generated got dropped, and the replacement docstring doesn't cover it.
The old create_import() had # If no taxonomy_export_id provided, a unique export id will be generated above the create_taxonomy() call. That's gone, and the new _create_taxonomy_for_import() base method doesn't explain it either. create_taxonomy()'s own docstring in src/openedx_tagging/api.py still just says "Creates, saves, and returns a new Taxonomy with the given attributes," it never documents the auto-generation behavior (if not export_id: export_id = f"{count+1}-{slug}").
Rather than restoring that exact comment at one call site, can you add a line to create_taxonomy()'s own docstring instead? After this PR there are three places that can pass export_id=None into that auto-generation path: the base _create_taxonomy_for_import(), CompetencyTaxonomyView._create_taxonomy_for_import(), and create_competency_taxonomy(). Documenting it once at the function that owns the behavior avoids it going stale at whichever call site happens to keep the comment.
5. Please explicitly state that there will be a openedx-platform follow-up in the PR description itself.
|
mgwozdz-unicon
left a comment
There was a problem hiding this comment.
Approved, but I left some suggestions that I think clean up the comments better. Could you please also update the PR description to say Related to openedx/openedx-core#628 instead of See #628 to see if that connects the Github Issue and PR together?
|
|
||
| tests/openedx_tagging/test_views.py can't check this directly: it would have | ||
| to import CompetencyTaxonomy, breaking the layering rule it's demonstrating. |
There was a problem hiding this comment.
| tests/openedx_tagging/test_views.py can't check this directly: it would have | |
| to import CompetencyTaxonomy, breaking the layering rule it's demonstrating. |
This feels like an odd AI relic to me.
|
|
||
| Doesn't check whether a CompetencyTaxonomy row also gets created for | ||
| "competency". See tests/openedx_learning/applets/cbe/test_views.py for | ||
| that assertion. |
There was a problem hiding this comment.
| Doesn't check whether a CompetencyTaxonomy row also gets created for | |
| "competency". See tests/openedx_learning/applets/cbe/test_views.py for | |
| that assertion. |
This feels like an odd AI relic to me.
|
|
||
| Doesn't check whether a CompetencyTaxonomy row also gets created for | ||
| "competency". | ||
| See tests/openedx_learning/applets/cbe/test_views.py for that assertion. |
There was a problem hiding this comment.
| Doesn't check whether a CompetencyTaxonomy row also gets created for | |
| "competency". | |
| See tests/openedx_learning/applets/cbe/test_views.py for that assertion. |
This feels like an odd AI relic to me.
|
Thanks. I updated the PR description and applied your changes to the test comments. |
78c9715 to
a3d573a
Compare
Description
Adds
taxonomy_typeto the taxonomy create and import endpoints:"competency"creates a
CompetencyTaxonomyalongside the baseTaxonomy,"tags"(default)creates a plain one.
This PR lands the
openedx-coreside only. A companion PR againstopenedx-platformwill wireTaxonomyOrgViewtoCompetencyTaxonomyViewandpass
taxonomy_typethrough from the client.Changes
src/openedx_tagging/api.py:TaxonomyTypeenum (TAGS,COMPETENCY).src/openedx_tagging/rest_api/v1/serializers.py:taxonomy_type, a write-onlyChoiceFieldonTaxonomySerializer, default"tags".src/openedx_tagging/rest_api/v1/views.py:TaxonomyView.perform_create()andcreate_import()discardtaxonomy_typeafter validation — this app must neverimport
openedx_learning.create_import()'s taxonomy-creation step is extractedinto an overridable
_create_taxonomy_for_import()hook.src/openedx_learning/applets/cbe/api.py:create_competency_taxonomy()— createsthe
Taxonomyand linkedCompetencyTaxonomyrows in one transaction.src/openedx_learning/applets/cbe/views.py(new):CompetencyTaxonomyView, aTaxonomyViewsubclass overridingperform_create()/_create_taxonomy_for_import()to dispatch to
create_competency_taxonomy()fortaxonomy_type="competency".Design notes
openedx_taggingstays unawareCompetencyTaxonomyexists (lint-importsenforcesit never imports
openedx_learning). PlainTaxonomyViewacceptstaxonomy_type="competency"as a valid value but always creates a plainTaxonomyfor it, since it structurally can't do anything else.
CompetencyTaxonomyViewoverridesTaxonomyView's creation methods rather thanbuilding on top of them, because that layering rule blocks the other direction:
openedx_taggingcan't extend itself with competency-aware behavior, since itcan't reference
openedx_learningat all. Overriding in a subclass that lives inopenedx_learning(where both apps are visible).CompetencyTaxonomyViewis a real subclass, not a mixin: one consumer for now(openedx-platform's
TaxonomyOrgView).Verification
pytest tests/openedx_learning tests/openedx_tagging --no-cov: 492 passed.pylint,pycodestyle,isort --check-only,mypy,lint-imports: all clean.Related to #628