Repository navigation
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. 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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3275 +/- ##
==========================================
+ Coverage 96.20% 96.30% +0.09%
==========================================
Files 1446 1455 +9
Lines 35391 35802 +411
Branches 8468 8567 +99
==========================================
+ Hits 34048 34479 +431
+ Misses 1285 1267 -18
+ Partials 58 56 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
tbain
left a comment
There was a problem hiding this comment.
-
(Note) I appreciate the comments being pared down through this PR :)
-
(AI Bug) CompetencyAssociationsContext.tsx (line 156) The request triggered by this line is mandatory according to the AI's read of these changes, but the flow is used by "course authors" who do not have access to that endpoint, only "competency administrators" can access it (according to the AI's read...), so it is concerned that this call will result in a 403 becuase we're calling it from the wrong flow and/or with the wrong role. Does this make sense to you? And if so, is there a way to either call a different endpoint for that data that would be appropriate for the course author role, or some way to circumvent it to alleviate any concerns? OR is this a non-issue becuase the flow will be used by the correct role, OR the data is not required for the page to function? (please see the raw AI output hidden below for detailed findings since I find this issue kind of hard to wrap my own head around)
-
(AI Bug) src/taxonomy/competency-management/data/apiHooks.ts::145 Codex raises a concern here that we are defaulting to 'true' here which will make it seem like the user is authorized to see all the content. The AI flags this as an issue mainly becuase while the configuration is loading, the system my 'flash' and show the user the full breadth of courses rather than the ones that they are authorized to see if the authorization is enabled after it loads, since it will default to thinking that Authorization is off, and, therefore, they can see everything becuase of the 'true' default during load. Not sure of the best resolution to this, but maybe reverse the logic so it defaults to 'false', and then selectively enable it once isAuthzEnabled is properly initialized? Unless this is a misunderstanding and/or not worth addressing since it is an edge case based on load times.
Details
Request changes: I found two actionable integration bugs. The split between #3275 and the follow-up PR is otherwise reasonable.P1
Bug. CompetencyAssociationsContext.tsx (line 156) unconditionally fetches /rule_profiles/. The endpoint introduced by openedx-core #773 is restricted to competency administrators, while the #672 workflow is performed by course authors. In the follow-up PR, a 403 makes the entire associations panel show an error, and associateSubsection silently returns when no default profile is available. Consequently, ordinary course authors cannot view or add criteria.
P2
Bug. apiHooks.ts:103-105,142-146 (line 103) treats the default enableAuthzCourseAuthoring: false flag as authoritative while waffle flags are still loading, returning every course as editable. The follow-up uses this result to expose subsection-selection controls. A user can therefore briefly see and activate controls for a course they cannot modify, contrary to #672’s permission requirements. Permission results should remain false until flag and permission loading completes, consistent with the existing authorization hook.
Scope and verification
PR #3275 is explicitly the data-layer/tree-selection half of the work; the follow-up supplies the visible criteria UI, so I did not flag the UI omitted from this PR. However, the follow-up does not resolve either issue above.
Verification passed:
- TypeScript checks
- Lint, formatting, oxlint, and stylelint
- 7 targeted Jest suites, 70 tests
The full make validate suite was not run.
My request for further details:
Can you please explain those two bugs? The first I'm not able to quite understand what the issue is with the code you cited. The second appears contradictory, since the issue is that it defaults to false but... that's what we want since it would give 'false' authorization and thus prevent the user from being able to see anything by default?
You’re right to question the second finding. My wording was misleading.
The important distinction is:
- enableAuthzCourseAuthoring is a feature flag.
- permissionsByCourseId[courseId] is the actual per-course authorization result.
When the feature flag is false, the code intentionally treats authorization as disabled and returns true:
result[courseId] = isAuthzEnabled
? (resolvedAnswers[courseId] ?? false)
: true;
So false here means “the authorization system is off,” not “the user is unauthorized.” That preserves the existing behavior where all courses are treated as editable.
The narrower issue I intended to identify is during loading:
- The waffle flag initially defaults to false.
- Therefore isAuthzEnabled is temporarily false.
- The hook returns true for every course.
- Later, if the server says authorization is enabled, the hook switches to checking permissions.
During that short interval, the follow-up PR can show selectable subsection controls before permissions have been resolved. The existing useCourseUserPermissions hook avoids this by returning false while loading, even when authorization is ultimately disabled.
So the corrected finding is:
While the waffle flag is still loading, permissionsByCourseId temporarily returns true for every course because the default flag value is false. If authorization is enabled on the server, users may briefly see or activate controls before their course permissions are known. This is a loading-state race, not a problem with the default authorization policy.
The first finding is a separate issue. The problem is not the syntax at CompetencyAssociationsContext.tsx (line 156); it is the request that line causes:
const profileQuery = useDefaultCompetencyRuleProfile();
That sends a request for the default rule profile whenever the provider mounts. According to openedx-core #773, that endpoint is restricted to competency administrators. However, the #672 workflow is used by course authors.
The follow-up PR treats that profile request as mandatory:
- If it returns 403, CourseGroupList renders an error instead of the associations.
- When the user tries to add a subsection, associateSubsection sees that systemDefaultProfile is missing and returns without creating anything.
So the issue is an authorization-contract mismatch: the frontend requires data from an endpoint that the intended user may not be allowed to read. Either the endpoint must permit the relevant course-author role, or the frontend must obtain the effective default rule through a course-author-accessible endpoint.
In short: the first finding remains substantive; the second should be described only as a transient loading race, and its severity may reasonably be reduced depending on whether that brief exposure is considered worth fixing.
Thanks, this is a real issue, and it comes from a gap between the requirements of two tickets rather than from this PR's code. @thelmick-unicon, tagging you since you wrote both openedx/openedx-core#773 and openedx/openedx-core#672. What happens: a criterion that follows the default rule stores only Why that conflicts with #672: #672's user stories are written for course authors, and the rest of the backend already lets a non-staff course author through:
So a non-staff course author can open the page, load the tree and create criteria, but gets a 403 on Proposed fix (backend): let anyone who can view a competency taxonomy read the rule profiles, i.e. loosen the @thelmick-unicon, could you confirm which roles this page is meant for? If it's global staff only, this is a non-issue and I'll note it in the PR description. Otherwise we'll need a follow-up backend ticket for #773. |
|
@AShatsila @javoconsultant @thelmick-unicon I agree that the openedx/openedx-core#773 endpoint should be updated so that course authors can use this endpoint as well. |
|
This comes out of @jesperhodge's manual test pass, which I triaged in #3321. One of his findings lands in this PR. When an author selects a subsection that is already a criterion for the active competency, The ask: show this message without the spinning icon. Two options, your pick. One is to give This fits under openedx/openedx-core#672's scenario for selecting an already-associated subsection, which says the author should be able to tell from the panel that the content is already associated "rather than seeing an unexplained failure". A spinner on that message confuses the explanation. |
@mgwozdz-unicon let's open separate ticket |
This was already addressed in these merged PRs: openedx/openedx-core#846 and this PR openedx/openedx-platform#39099 |
oh, sorry, that comment is outdated |
|
This comment is a collaboration between Claude and me. @AShatsila One requirements correction on this PR, separate from the code itself. 1. Competencies at any level must stay selectable. This PR restricts selection in the competency tree to leaf competencies ( 2. Why it matters now. @jesperhodge hit the consequence while testing #683 (see his comment on that issue): a leaf competency that already had criteria gained a child through a taxonomy edit, and its criteria became unreachable on the page, because the parent could only be expanded or collapsed and never selected. Those criteria then blocked the new child's associations with the #666 error and there was no way to view or remove them. With every row selectable, the author opens the parent, sees its criteria, and can remove them. 3. What to change. Please restore the |
There was a problem hiding this comment.
Marking this as changes requested by me to cover my 2 comments I left earlier. I have not done a full review of this PR at this time though.
#3275 (comment)
#3275 (comment)
Adds the API client, query hooks, types and index utilities for the CBE criteria-groups, criteria and rule-profile endpoints, and the associations context that the criteria UI builds on. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Expanding a course now focuses its last bottom-tier group and rule box unless focus is already in that course, and the provider exposes whether course outlines are still loading. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ProcessingNotification takes an optional icon, threaded through showToast. The already-associated toast uses InfoOutline and the create-criterion failure toasts use Error; every other toast keeps the spinning gear. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Went with the icon prop. |
Agreed, thanks. The leaf-only restriction came from a stale copy of the tree files during the stack split. I rebased the stack onto current Downstream: nothing in the second PR of this stack depends on leaf-only selection. The associations context and criteria UI work from any |
9371e9a to
befdb42
Compare
Switching the active competency reset the expanded-course set, so a course left expanded in the content panel lost its select controls until it was collapsed and expanded again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mgwozdz-unicon
left a comment
There was a problem hiding this comment.
@AShatsila Both changes I asked for, the toast icon and tree selection at every level, are addressed in the current head (4485bec).
One leftover: the PR description still describes the old behavior. The "Leaf-only selection in CompetencyTreeItem" bullet under Description, the sentence "apart from the change to tree selection" in the first paragraph, and testing steps 2 to 4 (a group row "is not selected", Enter or Space "selects a focused leaf") all need updating so the description matches the code.
To be clear about the scope of my review: I checked only those two items and have not reviewed the rest of the PR, so I am not approving. Tyler and Adolfo are the reviewers for the full pass. Adolfo, if you don't get to it this week, I will do the full review myself next week while you are out. My "changes requested" state is stale either way. I'll dismiss it myself once my Core Contributor write access lands, or whoever finishes the review can dismiss it then, so no one needs to act on it now.
tbain
left a comment
There was a problem hiding this comment.
I really, really hate to do this, I'm sorry - AI uncovered 2 (hopefully easy to address) issue that seems like we need to address that was part of the original push that we didn't catch before;
- Old request overwrites new focus. If an association request for competency A finishes after the author switches to B, its response can replace B’s focused group. The next selection for B can then omit B’s intended group and score. The unguarded completion callback was already present at the [previously reviewed commit]( ).
Summary of suggested fix (From Codex, I defer to however you see fit to fix it): Record which competency the create request belongs to. In its completion callback, update focus only if that competency is still selected. If the author has switched, let the request finish and refresh its own data, but don’t change the current competency’s focus.
- Early course expansion loses focus. If an author expands a course before association data finishes loading, the provider records the expansion but never applies the course’s group and rule when the data arrives. A later selection can omit both from the create request. The [later focus commit](d70bd0c80) added course-expansion focus for the normal, already-loaded case. This is a remaining timing gap in that new behavior; before that commit, course expansion did not move focus at all.
Summary of suggested fix (From Codex, I defer to however you see fit to fix it): Record the most recently expanded course when its association data is still loading. Once the group tree and default profile are available, apply that course’s normal focus selection. Clear or disregard the pending choice if the author has since selected another group or expanded a different course, so the delayed update doesn’t override a more recent action.
Both of these seem like they are timing issues that might erroneously write out the wrong information, or unintended information in real world circumstances where timing issues make it save data with an 'old' focus or might set focus incorrectly once a process completes.
A create request that finishes after the author switches competency no longer replaces the new competency's focus. A course expanded before the criteria groups and default profile load now gets its group and rule focus once they arrive, unless the author has picked a group or rule since. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Description updated to match the code |
fixed |
tbain
left a comment
There was a problem hiding this comment.
The code changes look good to me, though Codex still observed some concerns about stale data;
-
[P2] A pending expansion can carry over to a cached competency. In
[CompetencyAssociationsContext.tsx:202](https://github.com/openedx/frontend-app-authoring/blob/6f1d92dc186f141105a637b3c1ffb165c81345ee/src/taxonomy/competency-management/CompetencyAssociationsContext.tsx#L202), an author can expand a course while competency A is loading, then switch to a previously viewed competency B. The same render schedules the pending expansion to be cleared but still applies it using B’s cached data. B can acquire an unintended group and score focus.I [this is Codex] reproduced this with a targeted test: it passes on the previous commit and fails with the new fix. The pending expansion should be discarded before it can be applied to the new competency.
-
[P2] An old request can still overwrite focus after leaving and returning. The guard at
[CompetencyAssociationsContext.tsx:367](https://github.com/openedx/frontend-app-authoring/blob/6f1d92dc186f141105a637b3c1ffb165c81345ee/src/taxonomy/competency-management/CompetencyAssociationsContext.tsx#L367)fixes the original A → B sequence. But if the author starts a request for A, switches to B, returns to A, and chooses another group before the old request completes, the IDs match again and the old response overwrites that choice. A targeted test reproduced this. Tracking the selection instance, rather than only the competency ID, would let the callback recognize that an intervening switch occurred.
Both of these seem like they still provide some error cases that could cause some odd or erroneous behavior in niche cases, so I think they're worth addressing. I do apologize that this is thrashing between us, though. :/
A pending course expansion is no longer applied in the render that switches competency, where the index may already hold the new competency's cached tree. A create response is matched against a per-switch selection counter, so leaving and returning to the same competency no longer lets an old response overwrite the author's focus. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Description
First of two PRs for Manage & Apply Competencies (#672). This PR adds the data layer and the selection state that the criteria UI in the second PR builds on. It has no new visible UI of its own apart from the change to tree selection.
src/taxonomy/competency-management/data/): an API client, React Query hooks and types for three openedx-core CBE endpoints:GET api/cbe/v1/competencies/<tag_id>/criteria-groups/returns a competency's criteria-group tree and criteria (fix: total file count update on add and delete #681).POST api/cbe/v1/competencies/<tag_id>/criteria/associates a subsection with a competency (chore(deps-dev): bump semver from 5.7.1 to 5.7.2 #665).GET api/cbe/v1/rule_profiles/returns the rule profiles, from which the system-default profile is read (refactor: replace @edx/paragon and @edx/frontend-build #773).utils.ts): build an index over the flatgroups/criteriaresponse. The backend returns a three-level tree (root, course-level, leaf) with nodepthfield, so a group's level is inferred fromparentIdandcourseKey. The utilities also resolve each criterion's effective rule and group criteria into rule boxes.CompetencyTreeItem: only a leaf competency can be selected. A group row only expands and collapses, because a parent competency's mastery rolls up from its children. Group rows keeprole="treeitem", so the ARIA tree structure stays valid for the leaves inside them.CompetencyAssociationsContext: holds the focused group and rule box and the association action, and checks per-course tagging permissions. When the backend rejects an association because a parent or child competency already has criteria in the same course (chore(deps): bump tinymce from 5.10.5 to 5.10.8 #666), it shows a specific toast.Roles affected: Course Author (tree selection on the Competency Management page) and Developer (new data hooks).
Supporting information
Testing instructions
The data layer is covered by unit tests. It is used by the UI in the follow-up PR.
Other information
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, e.g.import { initializeMocks } from '@src/testUtils';instead offrom '../../../../testUtils'🤖 Generated with Claude Code