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 6 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:
🔘 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3208 +/- ##
=======================================
Coverage 95.92% 95.93%
=======================================
Files 1397 1403 +6
Lines 33581 33640 +59
Branches 7947 7962 +15
=======================================
+ Hits 32214 32273 +59
- Misses 1308 1309 +1
+ Partials 59 58 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…nomy type and remove unused styles
6994092 to
79190a2
Compare
mgwozdz-unicon
left a comment
There was a problem hiding this comment.
Thanks for the updates in the latest commit. Here's where the earlier review items stand.
Done:
- Item 1 (native select):
PopulateStep.tsxnow usesForm.Control as="select"inside aForm.Group/Form.Label, so the label association is automatic and the options render as real<option>elements. - Item 2 (dependency direction):
TaxonomyTypenow lives insrc/taxonomy/data/constants.ts, and bothapiHooks.tsand the import-tags feature import it from there instead of the other way around. - The
PopulateStep.propTypesconcern from item 4 is moot now that the file has been converted to TypeScript and no longer usespropTypesat all.
Still open:
- Item 3: the wizard-level test in
ImportTagsWizard.test.tsx("can upload new taxonomies from the dialog") still only exercises the explicit "Competency" selection and assertstaxonomy_type=competencyin the posted FormData. Could you add a case that leaves the dropdown at its default ("Tags") and assertstaxonomy_type=tagsreaches the API the same way? (apiHooks.test.jsxwas updated to loop over bothTaxonomyTypevalues, but that's the hook-level test, not the wizard test this item was about.) The keyboard-only interaction test (open the dropdown, arrow between options, close it) and splitting the dropdown assertions out of the large parametrized test are both still outstanding too. - Item 4: the three new i18n messages (
importWizardStepPopulateTaxonomyType,importWizardStepPopulateTaxonomyTypeTags,importWizardStepPopulateTaxonomyTypeCompetency) still don't have adescriptionfor translators. The suggested text from the earlier review still applies: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."
TAXONOMY_TYPE_OPTIONSinsteps/constants.tshas a docstring now, but it doesn't explain what actually distinguishes the two types. Worth adding: 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.ImportTagsWizard.test.tsx, where the new dropdown tests live, still sets up its mocks withinitializeMockAppand a manually constructedMockAdapterinbeforeEach, rather thaninitializeMocksfromtestUtils.tsx.- Item 5: Testing instructions are filled in now, but Other information is still the placeholder text, the Best Practices Checklist is still fully unchecked, and there's no note that this depends on #614 (import endpoint support for
taxonomy_type) before it can come out of draft and be verified end to end.
…or import tags wizard
mgwozdz-unicon
left a comment
There was a problem hiding this comment.
Thank you for addressing the rest of those items! I'm approving, but could you please still update the Description so that the Other Information section mentions that this PR is stuck in draft blocked by openedx-core#614 and check the boxes in the Best Practices section?
@mgwozdz-unicon done! thank you |
Description
openedx/openedx-core#615
Recording.at.2026-08-27.02.29.27.mp4
Supporting information
openedx/openedx-core#615
Testing instructions
Other information
This PR is stuck in draft, blocked by openedx/openedx-core#614
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'