Skip to content

feat(taxonomy): add taxonomy type selection to import tags wizard - #3208

Draft
javoconsultant wants to merge 4 commits into
openedx:masterfrom
javoconsultant:615-taxonomy-type-dropdown
Draft

feat(taxonomy): add taxonomy type selection to import tags wizard#3208
javoconsultant wants to merge 4 commits into
openedx:masterfrom
javoconsultant:615-taxonomy-type-dropdown

Conversation

@javoconsultant

@javoconsultant javoconsultant commented Aug 26, 2026

Copy link
Copy Markdown

Description

openedx/openedx-core#615

Recording.at.2026-08-27.02.29.27.mp4

Supporting information

openedx/openedx-core#615

Testing instructions

  • Log in to Studio as an Admin or Staff user.
  • Click on the Taxonomies tab.
  • Click the Import button.
  • Upload a JSON or CSV file in the required template format.
  • Click Continue.
  • The modal advances to a "Populate Taxonomy Information" step with 3 fields: Taxonomy Nam, Taxonomy Description and Taxonomy Type (new field)

Other information

Include anything else that will help reviewers and consumers understand the change.

  • Does this change depend on other changes elsewhere?
  • Any special concerns or limitations? For example: deprecations, migrations, security, or accessibility.

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:

  • Any new files are using TypeScript (.ts, .tsx).
  • Avoid propTypes and defaultProps in any new or modified code.
  • Tests should use the helpers in src/testUtils.tsx (specifically initializeMocks)
  • Do not add new fields to the Redux state/store. Use React Context to share state among multiple components.
  • Use React Query to load data from REST APIs. See any apiHooks.ts in this repo for examples.
  • All new i18n messages in messages.ts files have a description for translators to use.
  • Avoid using ../ in import paths. To import from parent folders, use @src, e.g. import { initializeMocks } from '@src/testUtils'; instead of from '../../../../testUtils'

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Aug 26, 2026
@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @javoconsultant!

This repository is currently maintained by @bradenmacdonald.

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
🔘 Submit a signed contributor agreement (CLA)

⚠️ We ask all contributors to the Open edX project to submit a signed contributor agreement or indicate their institutional affiliation.
Please see the CONTRIBUTING file for more information.

If you've signed an agreement in the past, you may need to re-sign.
See The New Home of the Open edX Codebase for details.

Once you've signed the CLA, please allow 1 business day for it to be processed.
After this time, you can re-run the CLA check by adding a comment below that you have signed it.
If the CLA check continues to fail, you can tag the @openedx/cla-problems team in a comment for further assistance.

🔘 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.

🔘 Update the status of your PR

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

  • 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.

@github-project-automation github-project-automation Bot moved this to Needs Triage in Contributions Aug 26, 2026
@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Aug 26, 2026
@mphilbrick211 mphilbrick211 moved this from Needs Triage to Needs Tests Run or CLA Signed in Contributions Aug 26, 2026
@mphilbrick211

Copy link
Copy Markdown

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!

@mphilbrick211

Copy link
Copy Markdown

@javoconsultant when you have a moment, please also add a description of your changes to the top of this pull request. Thank you!

@javoconsultant

Copy link
Copy Markdown
Author

@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

@mphilbrick211 mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Aug 27, 2026

@mgwozdz-unicon mgwozdz-unicon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.ts don't include a description for 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_OPTIONS in constants.ts explaining 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 initializeMockApp rather than initializeMocks from testUtils.tsx.
  • PopulateStep.propTypes gains a new PropTypes.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

codecov Bot commented Aug 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.93%. Comparing base (16e19f1) to head (ba2f697).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

Status: Needs Tests Run or CLA Signed

Development

Successfully merging this pull request may close these issues.

4 participants