Skip to content

feat: competency criteria data layer and associations context(#672-01) - #3275

Open
AShatsila wants to merge 7 commits into
openedx:masterfrom
AShatsila:ashatsila/672-01-data-and-tree-selection
Open

AShatsila wants to merge 7 commits into
openedx:masterfrom
AShatsila:ashatsila/672-01-data-and-tree-selection

Conversation

@AShatsila

Copy link
Copy Markdown
Contributor

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.

  • Data layer (src/taxonomy/competency-management/data/): an API client, React Query hooks and types for three openedx-core CBE endpoints:
  • Index utilities (utils.ts): build an index over the flat groups/criteria response. The backend returns a three-level tree (root, course-level, leaf) with no depth field, so a group's level is inferred from parentId and courseKey. The utilities also resolve each criterion's effective rule and group criteria into rule boxes.
  • Leaf-only selection in 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 keep role="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

  1. Open a competency taxonomy on the Competency Management page.
  2. Click a group row. It expands or collapses and is not selected.
  3. Click a leaf competency. It is selected, and the course panel opens on the right.
  4. Check with a keyboard: Enter or Space selects a focused leaf, and a group's chevron button expands and collapses it.

The data layer is covered by unit tests. It is used by the UI in the follow-up PR.

Other information

  • Depends on the openedx-core CBE endpoints listed above, which are still in review. The endpoints are only called once the follow-up PR wires the UI in.
  • Follow-up PR: <link to PR 2>.

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, e.g. import { initializeMocks } from '@src/testUtils'; instead of from '../../../../testUtils'

🤖 Generated with Claude Code

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

openedx-webhooks commented Sep 29, 2026 •

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.

Details
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 29, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.91781% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.30%. Comparing base (08c1331) to head (76b5408).
⚠️ Report is 15 commits behind head on master.

Files with missing lines Patch % Lines
src/taxonomy/competency-management/testHelpers.tsx 0.00% 7 Missing ⚠️
...tency-management/CompetencyAssociationsContext.tsx 98.59% 2 Missing ⚠️
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.
📢 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.

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

  1. (Note) I appreciate the comments being pared down through this PR :)

  2. (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)

  3. (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:

  1. The waffle flag initially defaults to false.
  2. Therefore isAuthzEnabled is temporarily false.
  3. The hook returns true for every course.
  4. 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.

@AShatsila

Copy link
Copy Markdown
Contributor Author
  1. (Note) I appreciate the comments being pared down through this PR :)
  2. (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)
  3. (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

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 rule_profile_id, not the score itself. To display "With a score of X% or higher" (and to group criteria into rule boxes), the page reads the system default profile from GET /api/cbe/v1/rule_profiles/. Per #773, that endpoint is readable only by taxonomy admins, and in openedx_tagging that means global staff (is_taxonomy_admin = rules.is_staff).

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:

  • reading the competency's criteria tree, which org admins, org course creators and instructors pass for an enabled taxonomy of their org;
  • creating a criterion .

So a non-staff course author can open the page, load the tree and create criteria, but gets a 403 on rule_profiles/, and the associations panel can't render. The frontend can't work around this: the permission is checked server-side, and hardcoding a default threshold would show the wrong score on any instance whose default differs.

Proposed fix (backend): let anyone who can view a competency taxonomy read the rule profiles, i.e. loosen the can_view_competency_rule_profile predicate to match the read gate of the criteria-tree endpoint. #773 already notes that loosening this read gate later is not a breaking change. An alternative is for #681 to return each criterion's effective rule (plus the default rule) so the frontend doesn't need rule_profiles/ at all, but that's a bigger change on both sides.

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

@mgwozdz-unicon @javoconsultant

@mgwozdz-unicon

Copy link
Copy Markdown
Contributor

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

@mgwozdz-unicon

Copy link
Copy Markdown
Contributor

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, associateSubsection in CompetencyAssociationsContext.tsx calls showToast with the "This content is already associated with this competency." message and returns without a request. The shared ToastProvider in src/generic/toast-context renders every toast through src/generic/processing-notification, which always shows the rotating gear icon and has no option to turn it off. So this purely informational message appears with a spinner next to it, which reads as if something is still being processed when nothing is.

The ask: show this message without the spinning icon. Two options, your pick. One is to give ProcessingNotification an optional icon prop, passed through the toast context, and show this message with Paragon's InfoOutline icon, since it is a neutral fact rather than a problem or something in progress; the existing callers of the processing notification elsewhere in the app would keep the gear. The other is to show this message through Paragon's own Toast component, which renders text with no icon and is already what src/taxonomy/TaxonomyLayout.tsx uses on the taxonomy page next door.

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.

@javoconsultant

Copy link
Copy Markdown
Contributor

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

@mgwozdz-unicon let's open separate ticket

@mgwozdz-unicon

Copy link
Copy Markdown
Contributor

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

@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
So I don't think a ticket is needed for this.

@javoconsultant

Copy link
Copy Markdown
Contributor

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

@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 So I don't think a ticket is needed for this.

oh, sorry, that comment is outdated

@mgwozdz-unicon

Copy link
Copy Markdown
Contributor

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 (isSelectable = isSelectionEnabled && !hasChildren in CompetencyTreeItem.tsx, and "leaf-only tree selection" in the title). That restriction is not a requirement. A competency at any level of the taxonomy can carry criteria. The only limit is the dominance rule in #666: a competency can't get a criterion in a course where one of its taxonomy ancestors or descendants already has one. On the frontend, #797 is the ticket that enforces that rule, by disabling the ineligible gradable subsections in the content panel with a tooltip, once the competency is selected. The tree itself should let every row be selected, which is what master does today after #3291 (isSelectable = !!onSelectCompetency).

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 master behavior so group rows select the same way leaf rows do, keeping the separate expand/collapse control, and update the PR title and description so they no longer describe leaf-only selection. If anything downstream in the stack (#794, #671, #709, #710, #683) was built on the leaf-only assumption, it would be good to flag that here so we can check it too.

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

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)

AShatsila and others added 4 commits October 26, 2026 18:42
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>
@AShatsila AShatsila changed the title feat: add competency criteria data layer and leaf-only tree selection(#672-01) feat: competency criteria data layer and associations context(#672-01) Oct 26, 2026
@AShatsila

Copy link
Copy Markdown
Contributor Author

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, associateSubsection in CompetencyAssociationsContext.tsx calls showToast with the "This content is already associated with this competency." message and returns without a request. The shared ToastProvider in src/generic/toast-context renders every toast through src/generic/processing-notification, which always shows the rotating gear icon and has no option to turn it off. So this purely informational message appears with a spinner next to it, which reads as if something is still being processed when nothing is.

The ask: show this message without the spinning icon. Two options, your pick. One is to give ProcessingNotification an optional icon prop, passed through the toast context, and show this message with Paragon's InfoOutline icon, since it is a neutral fact rather than a problem or something in progress; the existing callers of the processing notification elsewhere in the app would keep the gear. The other is to show this message through Paragon's own Toast component, which renders text with no icon and is already what src/taxonomy/TaxonomyLayout.tsx uses on the taxonomy page next door.

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.

Went with the icon prop. ProcessingNotification takes an optional icon, passed through showToast, and the spinning class is only applied when no icon is given, so every existing caller keeps the gear. The already-associated toast now shows InfoOutline. The two create-criterion failure toasts (generic failure and the #666 hierarchy conflict) had the same spinner problem, so they now show Error. Commit befdb42.

@AShatsila

Copy link
Copy Markdown
Contributor Author

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 (isSelectable = isSelectionEnabled && !hasChildren in CompetencyTreeItem.tsx, and "leaf-only tree selection" in the title). That restriction is not a requirement. A competency at any level of the taxonomy can carry criteria. The only limit is the dominance rule in #666: a competency can't get a criterion in a course where one of its taxonomy ancestors or descendants already has one. On the frontend, #797 is the ticket that enforces that rule, by disabling the ineligible gradable subsections in the content panel with a tooltip, once the competency is selected. The tree itself should let every row be selected, which is what master does today after #3291 (isSelectable = !!onSelectCompetency).

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 master behavior so group rows select the same way leaf rows do, keeping the separate expand/collapse control, and update the PR title and description so they no longer describe leaf-only selection. If anything downstream in the stack (#794, #671, #709, #710, #683) was built on the leaf-only assumption, it would be good to flag that here so we can check it too.

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 master and took CompetencyTreeItem.tsx and its tests as they are after #3291, so every row is selectable and group rows keep their separate expand/collapse control. The PR title and description no longer mention leaf-only selection.

Downstream: nothing in the second PR of this stack depends on leaf-only selection. The associations context and criteria UI work from any tagId, and master's "group selection mounts the right panel" test passes on it. While rebasing it I also found that its feature commit had reverted the Competency ID badge, the <strong> competency name, the __panel wrapper and the Paragon tokens in CourseSearchBrowse; those are restored.

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>
@AShatsila
AShatsila marked this pull request as ready for review October 29, 2026 14:43

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

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

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;

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

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

Copy link
Copy Markdown
Contributor Author

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

Description updated to match the code

@AShatsila

Copy link
Copy Markdown
Contributor Author

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;

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

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

fixed

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

The code changes look good to me, though Codex still observed some concerns about stale data;

  1. [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.

  2. [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>
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.

5 participants