feat: add Competency Tree on the Competency Management page - #3218
feat: add Competency Tree on the Competency Management page#3218AShatsila wants to merge 6 commits into
Conversation
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks for the pull request, @AShatsila! 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. |
There was a problem hiding this comment.
Claude and I worked together on this code review and came up with the following items:
1. CompetencyExpandIcon.tsx duplicates an existing component.
src/taxonomy/tag-list/OptionalExpandLink.tsx already renders the same disclosure button (invisible placeholder when a row can't expand, aria-expanded toggle when it can). It takes a TanStack Row, which doesn't fit this page's plain <ul>/<li> tree, but CompetencyExpandIcon's own canExpand/isExpanded/onToggle interface is the more reusable shape. Suggest hoisting it into a shared location (it isn't competency-specific) and making OptionalExpandLink a thin wrapper around it, so there's one component instead of two near-identical ones.
2. Test coverage doesn't reach the platform's documented depth limit.
The backend enforces TAXONOMY_MAX_DEPTH = 5 (0-indexed, openedx_tagging/models/base.py), so a taxonomy can legally nest 6 levels deep, and the frontend has a matching constant in src/taxonomy/taxonomy-detail/constants.ts. The deepest fixture in either the existing tree-table tests or CompetencyTree.test.tsx is depth 2. That matters here because CompetencyTree/CompetencyTreeItem recurse through nested <ul> elements with CSS-driven indentation rather than TanStack's built-in depth handling, and that path is untested at real depth (the synthetic taxonomy-root row adds one more level on top, so the real max case is 7 nested <ul>s, not 6). Could you add a test with a 6-level tag chain confirming every label is reachable via Expand All?
3. CompetencyTree.test.tsx:179 is testing an impossible failure.
The test renders as a plain list, not a table, with no CRUD affordances anywhere in the tree asserts the absence of six buttons copied from the other page's action menu (src/taxonomy/tag-list/Actions.tsx). Since CompetencyTreeItem never renders anything from that file, none of those assertions can fail from a change within this feature folder, and they couple this suite to another file's button labels. Suggest dropping those six and keeping the queryByRole('table')/columnheader checks just above them, since those verify what this PR actually decided: a list, not a table.
4. #680 doesn't specify the tree's default expand state, and we think it should require the root expanded.
Checked the ticket directly: the "Accordion expand/collapse" acceptance criterion only says a node's children hide or show when toggled, it never states the initial state, and nothing else in the ticket does either. CompetencyTree.tsx calls the collapsed default "this page's deliberate initial state," matching the existing Taxonomy Detail page's tree-table, which also defaults to fully collapsed. But this page's default is more aggressive than that precedent, because the thing collapsed here is the taxonomy root itself. A first-time visitor sees exactly one row, the taxonomy's own name, and zero competencies until they click. On the existing tag-editing page, top-level tags are already visible on load; only their children start collapsed. Our recommendation is to default the root row expanded (showing top-level groups/tags immediately) while leaving deeper nodes collapsed, so the page is useful without a click on first load. @thelmick-unicon, could you update #680 to specify this as a requirement, so it's explicit rather than left to each implementation's own judgment?
- Deduplicate expand/collapse icon into a shared ExpandCollapseIconButton, replacing CompetencyExpandIcon and simplifying OptionalExpandLink - Add a max-depth (6-level) tree test matching TAXONOMY_MAX_DEPTH - Remove CRUD-absence assertions that could never fail from this feature - Default the taxonomy root row to expanded on first load
There was a problem hiding this comment.
| it('renders as a plain list, not a table', async () => { |
| * message objects, so this component stays free of any particular i18n | ||
| * message set - each caller owns its own wording. | ||
| */ | ||
| const ExpandCollapseIconButton = ({ |
There was a problem hiding this comment.
Thank you for putting this in src/generic :)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3218 +/- ##
========================================
Coverage 95.92% 95.93%
========================================
Files 1397 1403 +6
Lines 33581 33737 +156
Branches 7947 7729 -218
========================================
+ Hits 32214 32367 +153
- Misses 1308 1327 +19
+ Partials 59 43 -16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
mgwozdz-unicon
left a comment
There was a problem hiding this comment.
Thank you for addressing my comments! I'm approving this, pending the one cleanup item I flagged.
arbrandes
left a comment
There was a problem hiding this comment.
Good work, thank you! Claude helped me find a few issues, though. Comments inline.
| // Grows the hit area to the WCAG 24px minimum, then cancels that | ||
| // growth's effect on the row's layout with matching negative margins | ||
| // so the glyph and the 25px label inset don't move. The right margin | ||
| // also covers the icon-to-label gap, replacing the `mr-1` utility | ||
| // this page's disclosure button doesn't apply (`ExpandCollapseIconButton`'s | ||
| // `!important` couldn't otherwise be overridden here). | ||
| padding: $disclosure-hit-area-inset; | ||
| margin: (-$disclosure-hit-area-inset) 0 (-$disclosure-hit-area-inset) (-$disclosure-hit-area-inset); // top right bottom left |
There was a problem hiding this comment.
Nit: the comment says "The right margin also covers the icon-to-label gap", but the shorthand sets right margin to 0 - it's the 4px hit-area padding that provides that gap.
| expandedLabel: string; | ||
| /** Label for the "collapse" action - shown while the row is expanded. */ | ||
| collapsedLabel: string; |
There was a problem hiding this comment.
Nit: these two hold the opposite of what their names suggest - expandedLabel is the label shown while collapsed - which is why both call sites and these doc comments have to explain the inversion. expandLabel/collapseLabel, naming the action rather than the state, reads correctly without the explanation.
|
This can merge once you've addressed @arbrandes review and gotten a ✅ from him; no need for further review from me. |
Description
Adds a new read-only Competency Management page at
/taxonomy/:taxonomyId/competencies. It shows a competency taxonomy's tags as a nested expand/collapse tree (taxonomy name → groups → sub-competencies), with each row's Competency ID (external_id) displayed next to it. The page is fully read-only: no editing of names or IDs, no add/edit/delete affordances anywhere, and no backend/API/data-layer changes.This is a new page, so it doesn't change behavior for any existing role. It's intended for Platform Administrators who need to browse a taxonomy's competencies by their stable Competency ID before applying competency criteria elsewhere.
Supporting information
Testing instructions
ENABLE_TAGGING_TAXONOMY_PAGESfeature flag.external_idset./taxonomy/:taxonomyId/competenciesfor that taxonomy.external_idshows it as a badge, and the tag without one shows no badge (not the literal text "undefined" or "null")./taxonomy/:taxonomyIdpage (tag editing) and confirm it's unaffected — same table, same columns, same zebra striping as before.Other information
src/taxonomy/tree-table/) that the editable taxonomy detail page uses. That shell is built around an HTML<table>/TanStack Table, which can't express per-row gaps or nested "card" grouping without hacky workarounds (<tr>can't takemargin, and abox-shadow-based border approach bled across row boundaries). Since this page needs neither the shared shell's editing machinery nor its tabular semantics, it renders its own lightweight<ul>/<li>tree instead, reusing only the taxonomy-tag-to-tree data shaping (TagTree) from the existing tag-list feature.#006DAA) doesn't correspond to any existing Paragon semantic color variant in this app's theme (variant="info"resolves to teal) - it's retinted via a page-scoped CSS custom-property override on top of the realBadgecomponent. Flagging per the standing internal reminder to surface anything not natively available in Paragon.aria-labelsince there's no table header for screen readers to associate it with.Best Practices Checklist
Any new files are using TypeScript (
.ts,.tsx).Avoid
propTypesanddefaultPropsin any new or modified code.Tests should use the helpers in
src/testUtils.tsx(specificallyinitializeMocks)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.tsin this repo for examples.All new i18n messages in
messages.tsfiles have adescriptionfor translators to use.Avoid using
../in import paths. To import from parent folders, use@src.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:
.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'