feat: add taxonomy_type to TaxonomyOrgView - #39086
kdmccormick merged 4 commits into
Conversation
|
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. |
7da623e to
7d42c07
Compare
perform_create() branches on taxonomy_type itself: competency calls create_competency_taxonomy(), anything else defers to super().perform_create(). _create_taxonomy_for_import() does the same for the import path; create_import() itself is unchanged, since virtual dispatch already routes through the override. See openedx/openedx-core#614 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
7d42c07 to
c46d634
Compare
mgwozdz-unicon
left a comment
There was a problem hiding this comment.
This looks good to me, pending a green build once openedx-core is released with a new version after openedx/openedx-core#803 merges. Can you please update the description of this PR to indicate that it's dependent on openedx/openedx-core#803 and switch this PR to Draft until openedx/openedx-core#803 merges?
|
Done. |
kdmccormick
left a comment
There was a problem hiding this comment.
Just one request, looks good otherwise.
Did you manually test this? Please describe how you manually tested in the PR description.
Lastly, please make sure you've read the contributor requirements in our AI Policy.
Thanks!
| serializer.instance = create_taxonomy(**serializer.validated_data, orgs=user_admin_orgs) | ||
| set_taxonomy_orgs(taxonomy=serializer.instance, all_orgs=False, orgs=user_admin_orgs) | ||
|
|
||
| def _create_taxonomy_for_import(self, validated_data: dict) -> Taxonomy: |
There was a problem hiding this comment.
Thanks for using type annotations!
| return queryset | ||
|
|
||
| def perform_create(self, serializer): | ||
| def perform_create(self, serializer) -> None: |
There was a problem hiding this comment.
This class's docstring says: This view extends the TaxonomyView to add Organization filters. With this PR's change, TaxonomyOrgView will now also be responsible for switching between create_taxonomy and create_competency_taxonomy.
Can you update the docstring to reflect the increased scope of responsibility?
|
@kdmccormick I try to consistently add Claude as a co-author to commits where I use its assistance. After your notification, I also added a clarification to the pr description thanks for pointing that out. I also updated the docstring for |
|
I should've squash it. |
|
That's OK @alezconsultant , if you put a good commit message in your PR description, then we can just squash while merging and use that as the commit message body. |
|
@kdmccormick update PR description with commit message. |
|
@alezconsultant sorry, I don't understand what you mean. |
|
@kdmccormick My bad, thought i need to manually squash it locally before merge to update squash message, but as you said it's alright, misunderstanding from my side. |
Description
TaxonomyOrgViewnow dispatchestaxonomy_typeitself, callingcreate_taxonomy()orcreate_competency_taxonomy()directly depending on the value, instead of subclassing aCBE-owned view. Implements the acceptance criteria from openedx/openedx-core#614.
Changes
TaxonomyOrgViewsubclassesTaxonomyView(fromopenedx_tagging) directly.perform_create(): branches ontaxonomy_type."competency"callscreate_competency_taxonomy(), wrapped in the same try/except the base class usesaround
create_taxonomy(), so a validation failure (e.g. duplicateexport_id) 400son both paths instead of 500ing on only one. Anything else defers to
super().perform_create()._create_taxonomy_for_import(): same dispatch for the import path.create_import(): unchanged. It already callssuper().create_import(), whichnow routes through this override via normal virtual dispatch.
Tests
Seven test methods, one per #614 Given/When/Then scenario: creating and importing a
competency taxonomy (asserting the
CompetencyTaxonomyrow exists), creating andimporting with
"tags"or notaxonomy_type(asserting it doesn't), and rejectingan unsupported value on both endpoints. Plus a regression test for the duplicate
export_idcase the try/except protects against.Manual testing instructions
To be able to test this PR locally you need to work with local version of openedx-core.
This PR depends on
openedx-core#803(taxonomy_typesupport,create_competency_taxonomy()), mount a localopenedx-corecheckout with that work instead ( or pin 1.4.0 ver).Mount
openedx-core:Check out this PR's branch and restart Studio:
I asked claude to wrote a curl based script (with all cases)
that captures coockies and CSRF tokens needed for the requests so I don't copy requests from developer tools every time (which should also work and for non default/tags requests testing can be done via ui):
Then I use curl (Postman is fine too) to run all cases:
Create,
taxonomy_type=competency— expect201:Create,
taxonomy_type=tags— expect201, noCompetencyTaxonomyrow:Create, no
taxonomy_type— expect201, noCompetencyTaxonomyrow (same as"tags"):Create,
taxonomy_type=system(unsupported) — expect400,taxonomy_typenamed in the response body:Import,
taxonomy_type=competency— expect201:Import, no
taxonomy_type— expect201:Import,
taxonomy_type=system— expect400:After each query confirm that
CompetencyTaxonomyrecords appears or omitted in mysql tables.Related to openedx/openedx-core#614
Dependent on openedx/openedx-core#803
🤖 Generated with help of Claude Code