Repository navigation
Conversation
Adds the API client, query hooks, types and index utilities for the CBE criteria-groups, criteria and rule-profile endpoints, restores leaf-only competency selection in the tree, and adds the associations context that the criteria UI builds on. Co-Authored-By: Claude Opus 5.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. 🔘 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 #3275 +/- ##
==========================================
- Coverage 96.11% 96.10% -0.02%
==========================================
Files 1427 1432 +5
Lines 34681 34937 +256
Branches 8293 8329 +36
==========================================
+ Hits 33334 33575 +241
- Misses 1290 1305 +15
Partials 57 57 ☔ 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. |
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