feat competency-selection tree, Course Search, and gradeable-subsection browse UI for Competency Criteria Associations - #3235
Conversation
|
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. 🔘 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. |
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
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.
-
Indentation compounds across four separate places, producing much more gap than the design calls for.
CourseOutlineSubtree.tsx's outline wrapper carriesml-4 pl-2(32px), each section box then adds its ownml-3(16px) on top of that, the section box'spadding: 10px 8pxadds another 8px, andCourseSearchBrowse.scss's&__subsectionrule adds a furthermargin-left: 16pxon 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? -
Subsection rows overflow the right edge of their section box.
SubsectionRowrenders a Paragon<Button block ...>, andblocksetswidth: 100%on it.CourseSearchBrowse.scss's&__subsectionthen addsmargin-left: 16pxto that same element. A block element atwidth: 100%plus an additionalmargin-leftdoesn'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. Suggestwidth: calc(100% - 16px)alongside the margin, or moving to a padding-based indent on the parent instead of a margin on the child. -
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}>inCourseSearchBrowse.tsx, so there's already an established pattern for this in the same diff. -
Nit: duplicated predicate in
CourseOutlineSubtree.tsx:79and:88. BothhasGradedSubsectionandgradedSubsectionsrecompute the identicalsubsection.category === 'sequential' && subsection.gradedfilter separately. Could you compute each section's graded subsections once, then derivehasGradedSubsectionfrom whether any of those lists is non-empty? -
Hardcoded hex colors in
CourseSearchBrowse.scss.$group-border-color-course,-section,-subsection, and the#FFFFFFfills 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? -
No test coverage for
Resizable.tsx's drag behavior. This PR adds thehandleSidebranch 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 simulatesonMouseDown/onMouseMove/onMouseUpfor bothhandleSidevalues, including the min/max width clamping? -
The right-hand panel shouldn't render at all until a competency is selected. Right now
CourseSearchBrowse.tsx:191-197renders 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 inCompetencyAssociationsPanel.tsxto only mountCourseSearchBrowseonceselectedCompetencyis set, rather than always mounting it and letting it branch internally onactiveCompetency. 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. -
The course row's org/number/run line doesn't appear in the Figma design.
CourseRow.tsxrenders{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? -
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? -
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'sisSelectable = !hasChildren && !!onSelectCompetencyneeds to drop the!hasChildrencondition so a group row gets the samerole="button"/aria-selected/click and keyboard handling a leaf row already gets. The tests inCompetencyTree.test.tsxthat currently assert a group row is never selectable (around lines 253 and 288) will need to be rewritten to assert the opposite. -
The subsection-click callback should come out of this PR.
CourseOutlineSubtree.tsxfiresonSubsectionSelected({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 callsassociateSubsection(objectId, courseId), not by reusing a click on the whole row. That needscourseId, 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 theonSubsectionSelectedprop 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.
…ency management page
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>
5d5350b to
43c8904
Compare
There was a problem hiding this comment.
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:
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.
-
The associations empty state doesn't match Figma on either line, not just the wording gap flagged before.
messages.ts:95-100'snoAssociationsMessagerenders 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 waydemonstrateMasteryForLabelone line above it already does it. The message needs a{competencyName}placeholder and the call site needs to passactiveCompetency.valuein. Separately,noAssociationsPromptMessage's text should read "Make content selections to create competency criteria associations." per the screenshot, replacing the current placeholder text. -
The "Demonstrate Mastery For" line is missing the competency ID badge.
CourseSearchBrowse.tsx:286renders justactiveCompetency.value. Your screenshot shows a badge (CCRS-1.3) next to the name, matching the same badge styleCompetencyTreeItem.tsxalready renders in the tree (competency-row__badge/.badge-info).activeCompetency's type already carriesexternalId, so this should be a straightforward addition, reusing the existing badge styling rather than introducing a new one. -
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--selectedrule), but a selected group row gets none of that — the more specific.competency-group > .competency-rowrule wins the cascade and renders it identically to an unselected group row.aria-selectedand the--selectedclass 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:
-
Comment length. Several comments added in this round are longer than they need to be for what they explain, e.g.
CompetencyAssociationsPanel.tsx:27-33and:45-50,CompetencyAssociationsPanel.test.tsx:197-205, and theDateRangeTriggerdocstring atCourseSearchBrowse.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.scssfiles in this feature:CompetencyTree.scssandCourseSearchBrowse.scssare roughly half comment lines by volume, well outside what the rest of this repo's.scssfiles carry (a handful of other.scssfiles I checked have 0-3 comment lines total). Not asking for a rewrite, just flagging that this is worth a trim pass. -
Test gap: filter persistence across pagination.
CourseSearchBrowse.test.tsx:156and 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. -
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 onmaster, 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.
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.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.
usageKey/blockTypeselection callback for a later ticket to wire into an actual "associate" action).Supporting information
Testing instructions
ENABLE_TAGGING_TAXONOMY_PAGESfeature flag./taxonomy/:taxonomyId/competencies.Other information
GET /api/contentstore/v2/home/coursesendpoint (now withstart_date_on_or_after/start_date_on_or_beforequery params from feat: add open managed group type #669) and the existingCourseIndexView/outline endpoint via the already-shippeduseCourseOutlineIndexhook.DatepickerControlcomponent 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 onreact-datepicker's own range mode (selectsRange) with a custom trigger button instead.DatepickerControl.jsxwas converted to.tsxin the process; its behavior is unchanged.ResizableBox(src/generic/resizable/) gained ahandleSideprop ('left'default, unchanged for existing callers such asSidebar.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.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.<button>s (native keyboard support); the competency tree's rows userole="button"/tabIndex/aria-selectedwith 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
.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. (One exception:DatepickerControl.tsxkeeps the relative../../utils/../../constantsimports carried over from the.jsxfile it was renamed from — out of scope to touch here.)🤖 Generated with Claude Code