feat(taxonomy): add taxonomy type selection to import tags wizard - #3208
feat(taxonomy): add taxonomy type selection to import tags wizard#3208javoconsultant wants to merge 4 commits into
Conversation
|
Thanks for the pull request, @javoconsultant! 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:
🔘 Submit a signed contributor agreement (CLA)
If you've signed an agreement in the past, you may need to re-sign. Once you've signed the CLA, please allow 1 business day for it to be processed. 🔘 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. 🔘 Update the status of your PRYour PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate. 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: 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. |
|
Hi @javoconsultant! Welcome, and thank you for this contribution! In order for your CLA check to turn green, you'll need to submit a CLA form. If you are contributing as an individual, please fill out the individual CLA form here. If you are contributing on behalf of an organization, please have your manager reach out to oscm@axim.org so you may be added to your org's existing entity agreement. Please let me know if you have any questions. Thanks! |
|
@javoconsultant when you have a moment, please also add a description of your changes to the top of this pull request. Thank you! |
Hi @mphilbrick211, I've modified description |
mgwozdz-unicon
left a comment
There was a problem hiding this comment.
Claude and I worked together on this code review and came up with the following items:
1. Consider a native Form.Control as="select" instead of SelectMenu for the Taxonomy Type field.
In this codebase, SelectMenu is the established pattern for transient filter or action menus, like the org filter on TaxonomyListPage.tsx, not for a value that lives in form state and gets submitted with a mutation. Form.Control as="select" is that pattern, it's used throughout the app and by the Name and Description fields already in this same file, and it gets label association for free from Form.Group. The current implementation has to hand-wire id/aria-labelledby instead, renders each option as a link rather than an option (confirmed by the test's own getByRole('link') queries), and gives the user no visual indication of which option is currently selected when the menu is open. A native select would resolve all of that at once.
2. TaxonomyType is imported from a feature folder into the shared data layer.
src/taxonomy/data/apiHooks.ts imports TaxonomyType from ../import-tags/constants, which inverts the usual dependency direction, the generic taxonomy data layer now depends on one specific wizard's constants file. It's really a domain and API concept, since it goes straight into the FormData payload, so it belongs in src/taxonomy/data/types.ts or a new data/constants.ts instead. The ticket that will later display the taxonomy type elsewhere will need this same enum outside the import wizard, so worth fixing the direction now rather than later.
3. Please add a test for the default "Tags" path reaching the API, and consider splitting the dropdown test out.
apiHooks.test.jsx covers both TaxonomyType values reaching the FormData at the hook level, but the full wizard test in ImportTagsWizard.test.jsx only asserts the payload for the explicit "Competency" selection.
- Please add a case for leaving the dropdown at its default ("Tags") and asserting that reaches the API too.
- Please add a test for keyboard-only interaction: opening the menu, arrow-navigating between options, and closing it. A custom menu doesn't get that from the browser the way a native select would.
- Consider pulling the dropdown's own behavior into its own smaller test rather than folding it into the large parametrized wizard test, that would make failures easier to isolate going forward.
4. A few smaller items.
- The three new i18n messages in
messages.tsdon't include adescriptionfor translators (the rest of the file doesn't either, but worth not compounding it). Suggested text:importWizardStepPopulateTaxonomyType: "Label for the dropdown where the user selects the type of taxonomy being imported."importWizardStepPopulateTaxonomyTypeTags: "Option in the Taxonomy Type dropdown for a standard tag taxonomy."importWizardStepPopulateTaxonomyTypeCompetency: "Option in the Taxonomy Type dropdown for a competency taxonomy."
- It'd help future readers to add a short comment on
TAXONOMY_TYPE_OPTIONSinconstants.tsexplaining what actually distinguishes the two types: a Tags taxonomy is just a label for content with no rules for demonstrating mastery, while a Competency taxonomy is a taxonomy of skills and enables the Competency Management page for configuring the rules used to demonstrate mastery of them. - The new tests still call
initializeMockApprather thaninitializeMocksfromtestUtils.tsx. PopulateStep.propTypesgains a newPropTypes.oneOf(...)entry, which the checklist asks to avoid in modified code.
5. Please fill out the rest of the PR description.
Only the Description section is filled in (the video and the link to #615), the rest, including Testing instructions, Other information, and the Best Practices Checklist, is still placeholder text. Since this is still a draft PR, could you also note here that it depends on #614 (import endpoint support for taxonomy_type) before it can come out of draft and be verified end to end?
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3208 +/- ##
=======================================
Coverage 95.92% 95.93%
=======================================
Files 1397 1398 +1
Lines 33581 33591 +10
Branches 7947 7934 -13
=======================================
+ Hits 32214 32224 +10
Misses 1308 1308
Partials 59 59 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…nomy type and remove unused styles
Description
openedx/openedx-core#615
Recording.at.2026-08-27.02.29.27.mp4
Supporting information
openedx/openedx-core#615
Testing instructions
Other information
Include anything else that will help reviewers and consumers understand the change.
Best Practices Checklist
We're trying to move away from some deprecated patterns in this codebase. Please
check if your PR meets these recommendations before asking for a review:
.ts,.tsx).propTypesanddefaultPropsin any new or modified code.src/testUtils.tsx(specificallyinitializeMocks)apiHooks.tsin this repo for examples.messages.tsfiles have adescriptionfor translators to use.../in import paths. To import from parent folders, use@src, e.g.import { initializeMocks } from '@src/testUtils';instead offrom '../../../../testUtils'