Skip to content

feat competency-selection tree, Course Search, and gradeable-subsection browse UI for Competency Criteria Associations - #3235

Draft
AShatsila wants to merge 6 commits into
openedx:masterfrom
AShatsila:ashatsila/670-competency-selection-tree
Draft

AShatsila wants to merge 6 commits into
openedx:masterfrom
AShatsila:ashatsila/670-competency-selection-tree

Conversation

@AShatsila

@AShatsila AShatsila commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Description

Wires up the "Manage & Apply Competencies" page (/taxonomy/:taxonomyId/competencies) with a course search/browse panel next to the existing competency tree, so a Platform Administrator can select a competency and find the course content to associate it with. Selecting a competency in the left-hand tree (a leaf or a group row — either is selectable) activates a right-hand panel where they can search courses by title, filter by course-run start date, and expand a course to browse its gradeable subsections.

image image

This builds on the read-only competency tree page (openedx/openedx-core#680, merged as #3218) and consumes the accessible-courses date-range filtering added on the backend in openedx/openedx-platform#669.

  • Left panel: the existing competency tree, unchanged in this PR except that any row — a leaf (sub-competency) or a group — is now selectable by clicking it; the expand/collapse chevron is a separate control, so toggling a group's children doesn't also select it.
  • Right panel (new): a "Competency Criteria Associations" block naming the selected competency ("Demonstrate Mastery For {name}", with its Competency ID shown as a badge) and a static empty-state placeholder, since there's no backend endpoint yet to list a competency's actual associations. Below that: a title search field, a start-date range picker, and a paginated list of accessible courses. Each course row expands to show its sections (navigation only) and graded subsections (clickable, firing a usageKey/blockType selection callback for a later ticket to wire into an actual "associate" action).
  • Distinct states throughout: no-competency-selected prompt, loading, inline error (separate from empty-results), and two different empty-result messages depending on whether a search term is active.

Supporting information

Testing instructions

  1. Enable the ENABLE_TAGGING_TAXONOMY_PAGES feature flag.
  2. Have (or create) a competency taxonomy with a nested tag tree at least 2 levels deep.
  3. As a Platform Administrator (staff) user, navigate to /taxonomy/:taxonomyId/competencies.
  4. Confirm the right panel shows a "select a competency" prompt and no course request fires until you click a competency row in the tree. Click a group row's label (not its expand/collapse chevron) and confirm it selects the group and mounts the right panel; click the same group's chevron and confirm it only expands/collapses, without changing the selection.
  5. Click a leaf competency and confirm the right panel loads: the "Competency Criteria Associations" block shows "Demonstrate Mastery For" with that competency's name and Competency ID badge plus the empty-state placeholder, and below it the full list of accessible courses, labeled "Courses & Content".
  6. Type a search term that matches no course and confirm a "no courses match your search" message with a "Clear search" button appears (distinct from the initial "no accessible courses" wording you'd see with an empty search).
  7. Click "Start Date", pick a start and/or end date in the calendar, and confirm the request re-runs and the page resets to 1; clear the dates via the "×" button next to the trigger and confirm it re-runs unfiltered.
  8. Expand a course row and confirm its sections render as plain (non-clickable) headers and only graded subsections render as clickable rows; clicking a section header does nothing, clicking/Enter/Space on a graded subsection fires the selection callback.
  9. Confirm a course with no gradeable subsections shows the "no gradeable subsections" message instead of an empty section list.
  10. Force the course-search request to fail (e.g. via devtools network throttling/blocking) and confirm a distinct inline error message appears in place of the course list, not the empty-result state.
  11. Resize to a narrow viewport (below 992px) and confirm the tree, search field, date picker, and expand/collapse controls all remain usable, stacked vertically.

Other information

  • No backend/API changes in this repo; consumes the existing GET /api/contentstore/v2/home/courses endpoint (now with start_date_on_or_after/start_date_on_or_before query params from feat: add open managed group type #669) and the existing CourseIndexView/outline endpoint via the already-shipped useCourseOutlineIndex hook.
  • The ticket's suggestion to reuse the existing DatepickerControl component wasn't viable as-is: that component only supports picking a single date, not a start/end range, so the date filter is built directly on react-datepicker's own range mode (selectsRange) with a custom trigger button instead. DatepickerControl.jsx was converted to .tsx in the process; its behavior is unchanged.
  • ResizableBox (src/generic/resizable/) gained a handleSide prop ('left' default, unchanged for existing callers such as Sidebar.tsx; 'right' used here) so the competency tree's resize handle can sit on its right edge, matching a panel on the left side of a layout instead of the right.
  • Known limitation, not fixed in this PR: the backend's start-date filter (openedx-platform, CourseOverview.get_all_courses()) excludes courses with no start date whenever either date bound is set. The original ticket for feat: add open managed group type #669 explicitly calls for null-inclusive filtering ("Courses with no start date always appear, regardless of the range"), and the shipped implementation's own test (test_get_all_courses_by_start_date_excludes_no_start_date_course) asserts the opposite. This means a course without a start date will disappear from this panel's results as soon as a date filter is applied. Filed as a backend follow-up rather than fixed here since it touches already-merged code outside this repo.
  • Accessibility: the disclosure control that expands/collapses a course row carries an accessible label that toggles between "Expand"/"Collapse"; graded subsection rows are real <button>s (native keyboard support); the competency tree's rows use role="button"/tabIndex/aria-selected with Enter/Space handling when selectable (leaf and group rows alike), since they're plain <div>s rather than native buttons, to keep the existing row styling.

Best Practices Checklist

  • 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. (One exception: DatepickerControl.tsx keeps the relative ../../utils/../../constants imports carried over from the .jsx file it was renamed from — out of scope to touch here.)

🤖 Generated with Claude Code

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

Copy link
Copy Markdown

Thanks for the pull request, @AShatsila!

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

@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.07834% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.09%. Comparing base (87cba01) to head (3028aea).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...c/generic/datepicker-control/DatepickerControl.tsx 83.33% 1 Missing ⚠️
...-management/course-search/CourseOutlineSubtree.tsx 98.21% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3235      +/-   ##
==========================================
+ Coverage   96.03%   96.09%   +0.05%     
==========================================
  Files        1407     1414       +7     
  Lines       34287    34506     +219     
  Branches     7882     7954      +72     
==========================================
+ Hits        32928    33157     +229     
+ Misses       1318     1308      -10     
  Partials       41       41              

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

@mphilbrick211 mphilbrick211 moved this from Needs Triage to Waiting on Author in Contributions Sep 9, 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.

Thank you for the quick turnaround on this! I reviewed this PR using AShatsila/frontend-app-authoring@ashatsila/680-feat-competency-tree...ashatsila/670-competency-selection-tree while we wait for #3218 to merge. All of these items were identified by me, though I did have Claude draft this comment.

  1. Indentation compounds across four separate places, producing much more gap than the design calls for. CourseOutlineSubtree.tsx's outline wrapper carries ml-4 pl-2 (32px), each section box then adds its own ml-3 (16px) on top of that, the section box's padding: 10px 8px adds another 8px, and CourseSearchBrowse.scss's &__subsection rule adds a further margin-left: 16px on top of all of it. By the time you reach a subsection row that's roughly 70-80px of stacked indent from four independent sources instead of one consistent step per nesting level. Could you consolidate this to a single indent value applied once per level?

  2. Subsection rows overflow the right edge of their section box. SubsectionRow renders a Paragon <Button block ...>, and block sets width: 100% on it. CourseSearchBrowse.scss's &__subsection then adds margin-left: 16px to that same element. A block element at width: 100% plus an additional margin-left doesn't shrink to compensate, it overflows its parent's right edge by exactly that 16px, which is visible in the screenshot in the PR description. Suggest width: calc(100% - 16px) alongside the margin, or moving to a padding-based indent on the parent instead of a margin on the child.

  3. Inline style at CompetencyAssociationsPanel.tsx:29. <div className="d-flex" style={{ gap: '20px' }}> could use a Paragon/Bootstrap gap utility instead, the same PR already uses <Stack direction="horizontal" gap={3}> in CourseSearchBrowse.tsx, so there's already an established pattern for this in the same diff.

  4. Nit: duplicated predicate in CourseOutlineSubtree.tsx:79 and :88. Both hasGradedSubsection and gradedSubsections recompute the identical subsection.category === 'sequential' && subsection.graded filter separately. Could you compute each section's graded subsections once, then derive hasGradedSubsection from whether any of those lists is non-empty?

  5. Hardcoded hex colors in CourseSearchBrowse.scss. $group-border-color-course, -section, -subsection, and the #FFFFFF fills are literal values, while the same file uses Paragon tokens elsewhere (var(--pgn-color-light-400), var(--pgn-color-info-100)). Could these map to the closest Paragon neutral/gray tokens instead, for consistency with the rest of the file and for theming?

  6. No test coverage for Resizable.tsx's drag behavior. This PR adds the handleSide branch to the drag math (onMouseMove's left-vs-right delta calculation), but there's no test file for this component at all, before or after this change. Could you add coverage that simulates onMouseDown/onMouseMove/onMouseUp for both handleSide values, including the min/max width clamping?

  7. The right-hand panel shouldn't render at all until a competency is selected. Right now CourseSearchBrowse.tsx:191-197 renders a "select a competency" prompt in place of the search field and course list, which means the panel itself, its background, its border, its column space, is still visible before anything is selected. We'd like the whole right-hand panel to not render at all in that state, not show a prompt inside a visible panel. This will need a change in CompetencyAssociationsPanel.tsx to only mount CourseSearchBrowse once selectedCompetency is set, rather than always mounting it and letting it branch internally on activeCompetency. We're updating the ticket's AC to say this explicitly, flagging now so you're not blocked waiting on that ticket update to start on it.

  8. The course row's org/number/run line doesn't appear in the Figma design. CourseRow.tsx renders {course.org} / {course.number} / {course.run} as a subtitle under every course title. Checking this against the Figma screens, course rows there show only an icon and title, no subtitle line. Could you remove it, or let us know if there's a reason to keep it, for example disambiguating two courses with the same title, that we should account for in the design instead?

  9. Question, not a change request: does the "Courses & Content" container styling deviate from Figma on purpose? Figma shows each course row as its own separately-carded white box with a gap and shadow between rows, and "Courses & Content" as plain uppercase text with a rule beneath it, not a filled background. This PR instead wraps every row inside one shared white bordered container (&__container), with "Courses & Content" as a filled, rounded-top colored strip (&__section-label). Was this an intentional simplification to better match how Paragon's own components typically group content, or should it be brought back in line with Figma's per-row cards and plain text label?

  10. All competency rows need to be selectable, not just leaves. This one needs to change: we're updating the ticket's AC so that clicking any competency row, group or leaf, selects it and opens the right-hand panel for it, the same behavior leaves already have. The expand/collapse arrow stays a separate control that only expands or collapses, it should not also select. Concretely, CompetencyTreeItem.tsx's isSelectable = !hasChildren && !!onSelectCompetency needs to drop the !hasChildren condition so a group row gets the same role="button"/aria-selected/click and keyboard handling a leaf row already gets. The tests in CompetencyTree.test.tsx that currently assert a group row is never selectable (around lines 253 and 288) will need to be rewritten to assert the opposite.

  11. The subsection-click callback should come out of this PR. CourseOutlineSubtree.tsx fires onSubsectionSelected({usageKey, blockType}) on a graded-subsection click, intended as plumbing for #672 to consume later. We checked this against #672's own technical spec: #672 actually adds its own separate select control to the rows this PR renders, and calls associateSubsection(objectId, courseId), not by reusing a click on the whole row. That needs courseId, which this callback never carries, and it needs to be conditionally hidden per subsection based on the author's write access to that course, which a single always-on row click can't express either. blockType, the one extra field this callback does carry, isn't used anywhere in #672's spec. Given that mismatch, could you drop the onSubsectionSelected prop and its wiring from this PR entirely? #672 will add its own control when it lands.

One more thing for your awareness, not a change request: the "Known limitation" note about the backend excluding null-start-date courses turns out to be moot, we've confirmed with the Open edX community that a course can never actually have a null start date. We'll get the ticket's AC updated to drop that line, no action needed on your end for it.

AShatsila and others added 5 commits September 14, 2026 17:34
Rebasing onto master picked up its reviewed initial-expand-state change
(the taxonomy root now starts expanded, not collapsed), so this test's
tree-expanding steps no longer matched reality - the first "Expand" click
it made was against a root that was already open.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@AShatsila
AShatsila force-pushed the ashatsila/670-competency-selection-tree branch from 5d5350b to 43c8904 Compare September 14, 2026 13:47

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

Thanks you for the changes, this is much better! A few smaller things came out of my latest pass with Claude that included a comparison against the Figma screenshot and a couple items I hadn't mentioned before but probably should have.

Item 3 (inline style), partially carried forward: the flagged style={{gap:'20px'}} is gone, replaced by <Stack direction="horizontal" gap={...}> as suggested. A new inline style was added in the same component though: style={{ alignItems: 'stretch' }} on that same Stack (CompetencyAssociationsPanel.tsx:51), to override .pgn__hstack's default centering. Bootstrap's align-items-stretch utility class would do the same thing without an inline style, if you want to swap it.

Item 5 (hardcoded hex colors), partially resolved: the three border colors now use Paragon gray tokens (CourseSearchBrowse.scss:39-41). The #FFFFFF container fills are still literal, but that now has a stated reason (this app's theme remaps Paragon's white tokens away from literal white, matching CompetencyTree.scss's existing precedent), so I'm fine leaving those. Two new literals came in with this round though: #F0F6FA and #004C77 for the "Courses & Content" label (CourseSearchBrowse.scss:118-129). Each has a comment tying it to an exact Figma value, so there may not be a token that matches, just flagging that the original ask (map to a token) isn't fully closed out for these two.

New from comparing the Figma screenshot to the current implementation:

Image

Note: The line of text explaining the filters is out of scope and covered in a separate Github Issue because it will require updates to the criteria endpoints first.

  1. The associations empty state doesn't match Figma on either line, not just the wording gap flagged before. messages.ts:95-100's noAssociationsMessage renders as a static "No content associated." with no competency name. Your screenshot shows "No content associated with Inference yet." — the competency name is interpolated, the same way demonstrateMasteryForLabel one line above it already does it. The message needs a {competencyName} placeholder and the call site needs to pass activeCompetency.value in. Separately, noAssociationsPromptMessage's text should read "Make content selections to create competency criteria associations." per the screenshot, replacing the current placeholder text.

  2. The "Demonstrate Mastery For" line is missing the competency ID badge. CourseSearchBrowse.tsx:286 renders just activeCompetency.value. Your screenshot shows a badge (CCRS-1.3) next to the name, matching the same badge style CompetencyTreeItem.tsx already renders in the tree (competency-row__badge/.badge-info). activeCompetency's type already carries externalId, so this should be a straightforward addition, reusing the existing badge styling rather than introducing a new one.

  3. A selected group row needs the same visual treatment as a selected leaf. Right now a selected leaf gets a light-blue background and a left accent bar (CompetencyTree.scss's .competency-row--selected rule), but a selected group row gets none of that — the more specific .competency-group > .competency-row rule wins the cascade and renders it identically to an unselected group row. aria-selected and the --selected class do land on the group row's DOM node either way, so this is a CSS-specificity fix, not new selection logic: the selected-row styling needs to win over .competency-group > .competency-row's own reset for a selected group row too. The "Demonstrate Mastery For" text already updates correctly for a group selection, but a sighted user needs the tree itself to show which row is active, the same way it already does for a leaf.

Two smaller items carried over, still worth a look:

  1. Comment length. Several comments added in this round are longer than they need to be for what they explain, e.g. CompetencyAssociationsPanel.tsx:27-33 and :45-50, CompetencyAssociationsPanel.test.tsx:197-205, and the DateRangeTrigger docstring at CourseSearchBrowse.tsx:76-93. Each has real "why" content, they'd just read easier trimmed to the load-bearing sentence rather than the full rationale. Same goes for the .scss files in this feature: CompetencyTree.scss and CourseSearchBrowse.scss are roughly half comment lines by volume, well outside what the rest of this repo's .scss files carry (a handful of other .scss files I checked have 0-3 comment lines total). Not asking for a rewrite, just flagging that this is worth a trim pass.

  2. Test gap: filter persistence across pagination. CourseSearchBrowse.test.tsx:156 and the debounce test near it both set a filter, move to page 2, and assert the page resets to 1 when the filter changes. Neither one (nor anything else in that file) tests the reverse: set a filter, then change pages, and confirm the filter is still applied in the resulting request. The implementation looks correct for this (page state is independent of search/date-range state), but there's no test proving it.

  3. Please update the PR description The "⚠️ Stacked on openedx-core#680 — merge openedx-core#680 first" warning is stale now that #680's PR (#3218) has merged; this PR is already based on master, so that line should come out. The screenshots are also out of date against the current branch, given the text and badge differences covered above, so please update them once the fixes in this comment land.

Since #3218 is merged, this PR's dependency is cleared, so it can come out of Draft and go up for a real review pass once the above is addressed.

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: Waiting on Author

Development

Successfully merging this pull request may close these issues.

4 participants