feat: accept taxonomy_type on taxonomy create and import endpoints

[alezconsultant]

Description

Adds taxonomy_type to the taxonomy create and import endpoints: "competency"
creates a CompetencyTaxonomy alongside the base Taxonomy, "tags" (default)
creates a plain one.

This PR lands the openedx-core side only. A companion PR against
openedx-platform will wire TaxonomyOrgView to CompetencyTaxonomyView and
pass taxonomy_type through from the client.

Changes

• src/openedx_tagging/api.py: TaxonomyType enum (TAGS, COMPETENCY).
• src/openedx_tagging/rest_api/v1/serializers.py: taxonomy_type, a write-only
  ChoiceField on TaxonomySerializer, default "tags".
• src/openedx_tagging/rest_api/v1/views.py: TaxonomyView.perform_create() and
  create_import() discard taxonomy_type after validation — this app must never
  import openedx_learning. create_import()'s taxonomy-creation step is extracted
  into an overridable _create_taxonomy_for_import() hook.
• src/openedx_learning/applets/cbe/api.py: create_competency_taxonomy() — creates
  the Taxonomy and linked CompetencyTaxonomy rows in one transaction.
• src/openedx_learning/applets/cbe/views.py (new): CompetencyTaxonomyView, a
  TaxonomyView subclass overriding perform_create()/_create_taxonomy_for_import()
  to dispatch to create_competency_taxonomy() for taxonomy_type="competency".

Design notes

• openedx_tagging stays unaware CompetencyTaxonomy exists (lint-imports enforces
  it never imports openedx_learning). Plain TaxonomyView accepts
  taxonomy_type="competency" as a valid value but always creates a plain Taxonomy
  for it, since it structurally can't do anything else.
• CompetencyTaxonomyView overrides TaxonomyView's creation methods rather than
  building on top of them, because that layering rule blocks the other direction:
  openedx_tagging can't extend itself with competency-aware behavior, since it
  can't reference openedx_learning at all. Overriding in a subclass that lives in
  openedx_learning (where both apps are visible).
• CompetencyTaxonomyView is 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

[openedx-webhooks]

Thanks for the pull request, @alezconsultant!

This repository is currently maintained by @axim-engineering.

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 approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

• If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
  • This process (including the steps you'll need to take) is documented here.
• If it doesn't, simply proceed with the next step.

🔘 Provide context

To 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:

• Dependencies

  This PR must be merged before / after / at the same time as ...

• Blockers

  This PR is waiting for OEP-1234 to be accepted.

• Timeline information

  This PR must be merged by XX date because ...

• Partner information

  This is for a course on edx.org.

• Supporting documentation
• Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

---

Where 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:

• Overview of Review Process for Community Contributions
• Pull Request Status Guide
• Making changes to your pull request

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:

• The size and impact of the changes that it introduces
• The need for product review
• Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

[mgwozdz-unicon]

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.

[alezconsultant]

1. Done, wrap create_competency_taxonomy to try/except and raise DRF ValidationError
2. Add tests for CompetencyTaxonomyView in tests/openedx_learning/applets/cbe/test_views.py
3. Add tests test_create_import_tags_creates_no_competency_row and test_perform_create_tags_creates_no_competency_row in tests/openedx_learning/applets/cbe/test_views.py
4. Add docstring to create_taxonomy to explain logic for ommited export_id passed to create_taxonomy
5. Add statement about future openedx-platform PR

[mgwozdz-unicon]

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?

[mgwozdz-unicon]

Suggested change

	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.

[mgwozdz-unicon]

Suggested change

	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.

[mgwozdz-unicon]

Suggested change

	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.

[alezconsultant]

Thanks. I updated the PR description and applied your changes to the test comments.

[ormsbee]

I am concerned that by subclassing TaxonomyView, we're adding an unnecessary amount of coupling. If we only need to change how these taxonomies are created and deleted, I would prefer that we implement a narrow endpoint for that and not inherit the rest of the API interface—with the assumption that the frontend would point to the existing REST API for tags for most operations.

I believe @kdmccormick has been more in the loop on the discussions regarding this endpoint, so I defer to him on this review.

[ormsbee]

@alezconsultant: I think this is fine for the purposes of this PR, but we probably want to have a short discussion about whether to extract this into a helper for openedx_django_lib. This pattern of extending via subclass is something that's likely to happen more and more often given how we set up our apps in openedx-core. I believe that

@bradenmacdonald, @kdmccormick, @mgwozdz-unicon, @jesperhodge: Want to get your thoughts here as well. FWIW, django-polymorphic does this kind of downcasting via create_from_super. With containers, we flip the responsibilities by passing the subclass to create_container. I prefer doing something like create_from_super (and the code in this PR), because I think it's less confusing to have the extension happen in the code dealing with the subclass... so long as we have some common and well-maintained utility for doing it in a consistent way.

[jesperhodge]

Hi @ormsbee , before I look deeper into this:

• "we probably want to have a short discussion about whether to extract this into a helper for openedx_django_lib" -> Great idea! If this is something that doesn't need to block this PR, can we move this out into a separate followup discussion so the PR can move along? But if you want a decision before the PR gets approved, we can discuss it first, of course.
• "I prefer doing something like create_from_super (and the code in this PR)" - for clarification, is this an action item / suggestion for this PR? Or is it part of a separate discussion you mentioned above?

[bradenmacdonald]

Personally, I would suggest simplifying this. Remove the for field in Taxonomy._meta.fields: ... competency_taxonomy.save_base(raw=True) block above and just keep it simple with competency_taxonomy = CompetencyTaxonomy.objects.create(taxonomy_ptr=taxonomy).

Yeah it may use one more query, but creating competency taxonomies is something that happens fairly rarely, and nobody is going to notice the performance difference at all. So why over-complicate the code will all this advanced Django usage.

I prefer doing something like create_from_super (and the code in this PR), because I think it's less confusing to have the extension happen in the code dealing with the subclass... so long as we have some common and well-maintained utility for doing it in a consistent way.

I don't have a strong opinion, but in the case of Container, the raw base class cannot be created on its own, so where we have it now seems like a natural place to put the logic that enforces the consistency. In other words, it seems fine to enforce it in the one place that every usage already has in common, rather than expecting that every subclass implements subclassing using the correct helper. But either way is fine with me in general.

[bradenmacdonald]

I am in favor of having a helper in openedx_django_lib to make things consistent in general. But wouldn't it be even better to just use django-polymorphic?

[jesperhodge]

Here's my take if it helps, but I'll defer to your thoughts on this since you have more experience there.

Yes I agree with Braden. This seems unnecessarily complicated. If I understand correctly, this is because of an extra query just in the create function, which is insignificant? Or am I wrong and this affects reads elsewhere in the code somehow?
If there is no important reason for downclassing, I would second Braden's simplification.
If there is a good reason, I also think it would be better to use django-polymorphic. Since that's already a standard way of doing things, I think wrapping this in another standard inside of openedx_django_lib is not adding any benefit.

[mgwozdz-unicon]

I agree with Braden's simplification suggestion. I'm not strongly opposed to django-polymorphic, but I don't think it fits in our scope, and we're already planning to take a different approach on #618 (which is where I think this part of the discussion might be more relevant) by adding select_related in TaxonomyOrgView.get_queryset() to avoid the extra per-row query without a new dependency.

[mgwozdz-unicon]

Claude is now flagging to me that the CompetencyTaxonomy.objects.create(taxonomy_ptr=taxonomy) simplification won't work for our needs. It's saying that this approach will call Django's normal save() which will overwrite the taxonomy's name and export_id values with blank strings, but the current code avoids this. It might be worth it to update the comment on this code to also warn that a plain .save() or .objects.create() there would blank the parent row, since we missed that this first time.

[ormsbee]

Isn't this introducing a lot of implicit coupling? If a client calls the list endpoint on this class-based-view, they'll get all taxonomies instead of just CBE ones, right?

[alezconsultant]

Yes, we get all taxonomies, GET logic is part of another ticket, if i got it right.

[alezconsultant]

@ormsbee @bradenmacdonald @mgwozdz-unicon I agree with the concerns about coupling in TaxonomyView, but we still have only one consumer in openedx-platform which is TaxonomyOrgView . As far as I understand, the main goal of this task is to allow the existing endpoints to create and import competency taxonomies by providing taxonomy_type in the request body. So this seems more like an extension of the current endpoints rather than a reason to introduce new ones. To avoid tight coupling, I suggest creating a mixin that can be plugged into TaxonomyOrgView from openedx-platform, for example:
TaxonomyOrgView(MixinName, TaxonomyView) Alternatively, we could create separate endpoints dedicated to creating and importing competency taxonomy records. In that case, I need to understand for what taxonomy types it will be.

[jesperhodge]

My thoughts are that the tight coupling is a problem, and that we have a natural separation of concerns here. The Competency Taxonomy endpoints are part of the CBE applet, which is a different API segment than tagging. The endpoints are supposed to be different from each other, and defined separately.
If we want to reuse pieces of code, that's a good idea. Following good DDD principles, the views ideally just define the API endpoints - in two different places, separately, not coupled. But the business logic for DDD doesn't belong in the view but in a shared service or helper file that is used by both endpoints. Exactly for this reason: to enable separation of concerns between different endpoints.
Serializers are already extracted to a separate place in Django. Using the same serializer for two different endpoint seems completely fine, but again I would go with composition over inheritance there.
So I would suggest to keep the two endpoints separate without inheritance, and to keep it simple without mixins as well, and just extract any shared logic into a file that is shared between tagging and CBE.

[mgwozdz-unicon]

Worked through this with Claude, and I think "keep the two endpoints separate" reads two different ways, and they lead to different places.

If it means two different URLs, the existing tags create endpoint plus a second, CBE-owned endpoint for competency taxonomies, that's the "New combined competency REST API" alternative that ADR 0013 (competency-taxonomy-detection) already considered and rejected. The reasoning there was that a separate CBE endpoint is a new surface to design, build, and maintain, and the frontend still has to choose which one to call before it makes the request, since nothing upstream tells it in advance whether it's creating a Tags or Competency taxonomy. That just moves the type-awareness into the frontend instead of removing it, which is the opposite of what this PR's taxonomy_type field is meant to do.

If instead it means two separately-defined view classes in code, with only TaxonomyOrgView ever bound to a real URL, and TaxonomyOrgView.perform_create() / _create_taxonomy_for_import() calling create_taxonomy() or create_competency_taxonomy() directly based on taxonomy_type, with no CBE-owned view class sitting in between, then I agree with that approach. It keeps this as a single endpoint, avoids any inheritance between the tagging and CBE view layers, and matches what I've been describing as the plan for openedx-platform: that repo is where the branching lives to create a CompetencyTaxonomy record versus a plain Taxonomy record, not a shared view class in openedx-core.

Concretely, that would mean this PR doesn't need a CompetencyTaxonomyView in openedx-core at all. TaxonomyView keeps the _create_taxonomy_for_import() hook it already has, and create_competency_taxonomy() stays in cbe/api.py. The follow-up openedx-platform PR wires TaxonomyOrgView to call both directly based on taxonomy_type. That PR needs an edit to perform_create() regardless, since it currently calls create_taxonomy() directly without going through super(), so routing it through this dispatch instead isn't extra work on top of what's already required there.

[jesperhodge]

The field values in the database only exist on the Taxonomy table though, right? The CompetencyTaxonomy table only has a pointer id

[mgwozdz-unicon]

Yes

[jesperhodge]

Is this true? My understanding was that save() will save the child and the parent, and save_base is just there to prevent a double write (but I think the double write is desired here...)
This sounds like it would overwrite the parent fields with empty values?

[mgwozdz-unicon]

Yes, this is true as written. Claude tested this and confirmed that the parent fields would get overwritten by a plain save().

[jesperhodge]

Great work! Awesome to see this coming together.

Update: I am a bit confused about the requirements, so I need to look through this again. Some of my previous PR comments were incorrect, so I'm reevaluating.

[jesperhodge]

Please note that I previously left some pretty heavy pushback about some of the unit tests, but I was wrong. Apologies.

[alezconsultant]

I removed CompetencyTaxonomyView and unrelated tests. That i will move wiring and dispatching logic to openedx-platform TaxonomyOrgView, but for now i leave save_base(raw=True) logic because save without it will fill parent obj with blank values.