Repository navigation
Conversation
60530c9 to
5ddf424
Compare
31b7f7a to
250d736
Compare
133193a to
a916e83
Compare
02ac457 to
1af8cbb
Compare
a916e83 to
716766e
Compare
|
Hi @AShatsila , one thing for the PR description: I don't see the pull/4 commits in the changes here. Could you review this statement? Also there are two commits that occurred after the one you mentioned, which need to be included in this review. |
There was a problem hiding this comment.
Hi @AShatsila , this is very good work. Everything tested works very well, and I only found a few small issues that are more in the nit area - if you fix them I'll approve right away.
Here is the AI review as well, please fix the small things that you find worth it:
Review of PR #5 (frontend-app-authoring): Delete Competency Course and Rule Groups
Checked against head 716766e0. No finding blocks the merge, but the decisions below should be recorded in the PR or the ticket.
Review Details
Prerequisites, items left to other tickets, what passed, and the action list
Prerequisites (skip if you know the codebase)
These come from the code at 716766e0 and from the tickets.
- Associations panel. The right-hand side of the Competency Management page (
/taxonomy/:taxonomyId/competencies) shows which course content counts toward the selected competency.CompetencyAssociationsProviderinCompetencyAssociationsContext.tsxholds its state. - Course Group and Rule Group. A Course Group is one course's section (
CourseGroupSection). Each Rule Group in it is a card (CriteriaGroupBox) of rule boxes, each naming a grading rule and its subsections. The card's header band ("By completing … of the following") holds its select button and logic-operator toggle; the three-dot toggle sits beside the band, and the rule boxes below it. Both kinds of group are rows of one backend table, so the DELETE endpoint (openedx-core openedx#675) accepts either id. - The placeholder Rule Group. An unsaved, empty Rule Group added with "+ Rule Group" (openedx#671). It exists only in the browser, with focus
{ groupId: null, ruleKey: null }plusplaceholder.parentRuleGroupId, and removing it sends no request. - Logical focus and DOM focus. Logical focus is the provider's
focusvalue: the Rule Group and rule box that the next selected subsection goes into. DOM focus isdocument.activeElement. When a delete removes the keyboard user's button, the provider sets a one-shotkeyboardFocusRequestnaming a Rule Group (kind: 'group') or the empty state (kind: 'empty'); the named component focuses itself and clears the request. - The empty state. The message
CourseGroupListshows when it has no Course Group to show. - The snapshot, the resolver, and the in-flight guard.
deleteGrouprecords the page order before sending (snapshotPageOrderinutils.ts). When the request and its refetch have both landed (the delete settles),resolveFocusAfterDeletereads the refreshed cache and asks the pure helperfocusTargetAfterRemovalwhere logical focus goes. The helper's docstring numbers its rules 1 to 5; rule 4 falls back to the first group of a course that did not exist at the snapshot.isDeleteInFlightRef, the in-flight guard, drops a second delete while one is pending. - The three-dot menu. The delete control is a Paragon
Dropdowntoggled by aMoreVert(three-dot)IconButton. Its one item is "Remove group" on a Rule Group and "Remove from {competency name}" on a Course Group. - Sibling tickets. Ticket numbers in this review are openedx/openedx-core issues. openedx#675 is the backend DELETE endpoint, implemented in ufedaseyeuconsultant/openedx-core#4 and still in review. openedx#786 will put a confirmation dialog in front of this delete. openedx#709, this PR's ticket, tells openedx#710 (delete an association) and openedx#833 (delete a rule box) to reuse this change's focus resolver.
- How the review tested the tests. For each behavior the ticket requires, the reviewer broke its code and checked that a test in the
competency-managementfolder failed for that reason. A break that no test catches means the behavior is untested. Of 47 breaks at the reviewed heada916e833, 43 were caught; the other four are still not caught at716766e0.
Things the review is leaving to other tickets
- Merging
upstream/master(1f5cd9ce) conflicts inCompetencyAssociationsPanel.tsxandCourseSearchBrowse.tsx, and so does merging this PR's base, so PR #4 owns both conflicts. This PR adds two lines toCompetencyAssociationsPanel.tsx, so it will likely need a small rebase afterward. - The confirmation dialog (openedx#786) can sit between the menu item's click handler and
deleteGroup, and add a return value, without undoing anything here. - Deleting a rule box (openedx#833) and an association (openedx#710): the resolver finding below records what they will need.
deleteGroupdoes not checkcanEditCourse. The menu is hidden on courses the author cannot edit, and openedx#675 says the endpoint returns 403 without the tagging permission, so the server enforces it.
What passed
- The branch is correctly stacked on PR #4 (base
1af8cbb7). The 23 suites and 312 tests insrc/taxonomy/competency-managementpass;tsc,oxlint,dprint, andstylelintare clean on the changed files;criteria-groups.scsscompiles as its own root stylesheet with the bundler's load paths. - The DELETE call, the mutation hook, and resolver rules 1 to 3 match the ticket. The mutation stays pending until the refetch lands, as the PR body says, which
@tanstack/query-core's source confirms. - The select button's
.stretched-linkoverlay stays inside the header band and off the three-dot toggle, so noz-indexorstopPropagationis needed. - The
Dropdown.Toggle as={IconButton}shape matches three repo precedents; the toggle getsaria-expanded, and the passedaria-labelwins overalt. Every three-dot toggle, the placeholder's included, is disabled during a delete, and on courses the author cannot edit the menu is absent rather than disabled.
No browser run was made, so the focus-after-disable step and the scroll question are inferred.
Actions
- Record decisions on the competency-switch race, the rule-4 deviation from the ticket, the failed-refetch alert, the
preventScrollscroll position, and the openedx#786 return value. - Leave DOM focus on something after every delete that removes the keyboard user's button, give the placeholder card a focus target, and check the failed-delete path in a browser.
- Use the
-baseborder-radius token in all three places, and drop the duplicated band styles. - Consider distinct accessible names for the Rule Group three-dot menus, and renaming the resolver.
- Apply the test hygiene and the small things as you see fit, starting with the four missing tests and the PR body.
Decisions to make or write down
Switching to another competency while a delete is pending can clear the new competency's focus. The review reasoned this through without running it.
Details
The provider is not remounted on a competency change: it resets its state when tagId changes, and nothing disables the competency tree during a delete. The per-call onSettled runs the resolveFocusAfterDelete created at click time, which holds the old tagId, while the focus ref holds the new competency's focus. The review infers that this focus is absent from the old data, so the helper returns null and the code clears it, possibly with an 'empty' request. Record the tagId when sending, and skip focus resolution in onSettled if it has changed; keep the toast and the guard reset.
When no group from before the delete survives, focus can move to a course that another author added, and is lost if that course fails to load.
Details
Rule 4 reads the refreshed data unfiltered; the helper's docstring explains that the new course's outline may still be loading. If that outline fetch fails, the course never renders, the keyboard request is never consumed, and the empty state shows without focus, against the empty-state scenario. The ticket's Technical Details take this candidate from visible Course Groups. Reaching this takes another author adding a course this author cannot open while the delete is pending. Decide whether to accept the deviation, or to re-resolve focus once the outlines have loaded.
If the group list fails to reload after a successful delete, the panel shows an error alert, with no toast and focus on the page body, after an estimated seven seconds of disabled controls.
Details
A test harness confirmed that DELETE 200 followed by GET 500 leaves no cards, the "problem loading" alert, no toast (the delete succeeded), and DOM focus on body. The production QueryClient sets no retry, so TanStack Query's default three retries, with 1, 2, and 4 second backoffs, keep every three-dot toggle and subsection select disabled for about seven seconds first; that figure is computed from the defaults, not measured. Decide whether the settle path should toast when the groups query is in error.
After focus moves upward, the focused card's header band may sit just above the visible area. This has not been checked in a browser.
Details
The select button takes focus with preventScroll, and only the first rule box scrolls itself into view, with block: 'nearest'. When focus moves upward, this aligns the rule box with the top of the viewport and leaves the band, which holds DOM focus, just above it. When focus moves downward, the band stays visible. Two scenarios in the ticket require the target to be visible without further scrolling. This needs a manual check; calling scrollIntoView({ block: 'nearest' }) on the card wrapper after focusing would settle it.
deleteGroup returns nothing and toasts on failure, while openedx#786 wants the caller to learn the outcome and show no toast; openedx#786 can add a return value without undoing this change.
Details
deleteGroup toasts on every failure except a 404. openedx#786 says that on failure the dialog stays open with an inline alert and no toast, and the caller closes it only on success. This is mostly a conflict between the two tickets: note it in the PR body, or return a promise of the outcome now.
Improvements worth doing
After some deletes, keyboard focus is left on nothing, and the comment saying a failed delete keeps focus is unproven.
Details
On each path below, the delete removes the keyboard user's button and nothing takes DOM focus. All three were confirmed by reading the code.
- (a) Nothing is in logical focus, for example after the author discards a placeholder rule box, and the author deletes the only group. The helper returns
'unchanged'for null focus, as its docstring says, and the provider issues'empty'only when the helper returnsnull. This breaks example 3 of the ticket's keyboard scenario ("the only Rule Group on the panel" → "the panel's empty-state message"). - (b) The placeholder is in focus, and the author deletes a saved sibling group. No request is issued, and the placeholder card has nothing that can take focus.
- (c) The focused group sits in a collapsed section. The request carries
expand: false, so the section consumes it without focusing anything.
The ticket's Technical Details say to set a keyboard request whenever the deleted group is gone.
On a failed delete, the comment "A failed or no-op delete leaves the menu button in the DOM, which keeps the browser's focus" repeats the ticket's own assumption. The menu returns focus to its toggle, but the toggle is disabled for the request plus refetch. In jsdom, focus stays on the disabled toggle; whether Chrome and Firefox move it to body needs a browser run.
Fix: make every settle that removes the keyboard user's button leave focus on something (the group in logical focus if it is rendered, else a rendered neighbor, else the empty state), give the placeholder card a focus target, and update the comment after the browser check.
A new style rule uses a design token that does not exist, so the band's corners come out square.
Details
The new &__header-row rule in criteria-groups.scss uses var(--pgn-size-card-border-radius), but the installed Paragon defines only the -base, -inner, and -logo variants, so the radius computes to zero, although the comment by the older &__header rule says the radius matches the Card's corners. Two older rules already used the token, which is why this is an improvement rather than a blocker. Use var(--pgn-size-card-border-radius-base) in all three places. Also, &__header is now always inside &__header-row, so its own background-color and border-radius are redundant, and padding-right: var(--pgn-spacing-spacer-1) is the pr-1 utility class.
Every Rule Group three-dot menu has the same accessible name, so a screen reader user cannot tell the cards apart.
Details
Every card announces "More actions for this group", and the select button before it repeats per card too. The repo's tag list names the item in its label. The text is the ticket's own suggestion, so changing it is a ticket-level decision; something like "More actions for rule group {position} in {course name}" would distinguish the cards.
The focus resolver's name does not match the one openedx#709 gives openedx#710 and openedx#833.
Details
The resolver is private and named resolveFocusAfterDelete, while openedx#709 tells openedx#710 and openedx#833 to call resolveFocusAfterRemoval. focusTargetAfterRemoval also never reads the rule key, so a deleted rule box in a surviving group would come back 'unchanged'; the ticket leaves that extension to whichever of openedx#710 and openedx#833 lands later. Rename the resolver to match the ticket, or note the difference in the ticket.
Some tests repeat setup code, or have a name that promises an assertion they do not make.
Details
- In
api.test.ts, the test name mentions PATCH without asserting it. - In
CompetencyAssociationsContext.test.tsx, the "wait for the second GET, then release the refetch" sequence appears ten times; extract asettleRefetchhelper. - In
apiHooks.test.tsx, "invalidates on success" and "on failure too" are near copies; useit.each.
Small things
These are low-severity items in tests, code, comments, commits, and the PR body.
Details
- Four behaviors have no test that fails when their code breaks. Add or extend a provider test for each:
- Focus lands on the first rule box, not the last. The ticket's scenarios say "together with the first rule box", but every provider fixture group has one criterion, so first and last are the same box. Give the neighboring group two rule boxes and assert the first box's key.
- No
'empty'request is issued while a Course Group remains. Leave a second Course Group after the delete and assert a group request. - The in-flight guard resets after a settle; without the reset, every delete after the first is silently dropped. Delete twice in sequence and assert two DELETE requests.
focusRuleBoxclears the keyboard request; the ticket does not name this one. Extend the "clears the keyboard focus request" test tofocusRuleBox, for instance withit.each.
- The PR body describes a trash icon labeled "Delete rule group", under a "Trash controls" heading; the code renders the three-dot menu, with "More actions for…" toggles and "Remove group" / "Remove from {competencyName}" items. Its stacked-diff sentence is wrong: the PR is based on PR #4's branch, so the diff shows only this PR, and the commit it calls the last is the first of three. Rewrite the body for the three-dot menu and its labels, drop that sentence, and add a screenshot.
removePlaceholderGroup's fallback branch duplicatesdiscardPlaceholderexcept for resetting the duplicate-rejected flag; calldiscardPlaceholder().focusTargetAfterRemovalinutils.tsrestates theCriteriaFocustype inline; useimport type.apiUrls.updateCompetencyCriteriaGroupnow serves PATCH and DELETE; rename it tocompetencyCriteriaGroup.- The empty-state comment in
CourseGroupListdoes not explain why the effect depends onaccessibleCourseGroups.length. - The comment above
focusGroupAfterRemovalis convoluted; use "Lands on the first rule box, not the last asfocusGroupdoes;nullwhen the group has none, which focuses a placeholder rule box." setFocusAndDropKeyboardRequestis used only byusePlaceholderState. The provider's own focus writers clear the keyboard request by hand, and two of them,associateSubsection's success handler andupdateRuleScore, callsetFocuswithout clearing it. That contradicts the helper's comment ("Any focus write other than a delete's own drops a pending keyboard focus request") and the ticket's rule to clear the request on the next focus write. Route every focus writer through the helper.- Commit "feat: add three dods instead of cross for removing" has a typo and describes an icon rather than behavior; "feat: add delete hooks for group and courses" covers the whole feature. Both have empty bodies.
- The "Avoid
../" checklist box is unchecked, although the diff adds no such import. - The new provider tests use
fireEventandgetByTestId, where the repo'sAGENTS.mdprefers user-event and role queries; this matches that test file's existing pattern.
| /** One-shot keyboard focus target after a delete removed the focused menu button. | ||
| * `expand`: whether a collapsed section holding the target should open for it. | ||
| */ | ||
| export type KeyboardFocusRequest = { kind: 'group'; groupId: number; expand: boolean; } | { kind: 'empty'; }; |
There was a problem hiding this comment.
Question:
- does a keyboardfocusrequest just relate to tabbing on the keyboard for accessibility? If so, why are there kinds group and empty?
- I assume the
focusReact state is for the selected group instead of the keyboard focus, right? Can we make sure that we differentiate the two sufficiently clearly? For example, a rule group may be selected but the keyboard focus may be on a button somewhere else. - "target after a delete removed the focused menu button" -> why do we want to shift the keyboard focus here? To what?
| @@ -203,6 +233,12 @@ export const CompetencyAssociationsProvider = ({ | |||
|
|
|||
| const [prevTagId, setPrevTagId] = useState(tagId); | |||
| const [focus, setFocus] = useState<CriteriaFocus | null>(null); | |||
There was a problem hiding this comment.
Maybe the name focus is a bit confusing because it can be confused with keyboardFocus, however, we are talking about which criteria group is selected.
Could this and related variables be renamed to something like selectedGroup or something similar?
|
|
||
| const consumeKeyboardFocusRequest = useCallback(() => setKeyboardFocusRequest(null), []); | ||
|
|
||
| // Unlike `focusGroup`, lands on the group's first rule box, and the rule |
There was a problem hiding this comment.
Can you clarify this comment? I don't understand this. What information is the comment designed to tell me?
There was a problem hiding this comment.
The function takes a group id and selects that group together with its first rule box: ruleBoxesForGroup(groupId, ...)[0]. It never picks another rule box. The ticket's AC asks for the first rule box after a delete, while selectGroup (used on a click) selects the last one, and that difference is what the comment was trying to say. I replaced it with one line that says exactly that.
| // Unlike `focusGroup`, lands on the group's first rule box, and the rule | ||
| // key is `null` for a group with none, which focuses a placeholder rule | ||
| // box, as `focusGroup` does. | ||
| const focusGroupAfterRemoval = useCallback(( |
There was a problem hiding this comment.
Question: if I understand the logic here correctly, the way this function work is that you just give it a group id and it sets focus to that group. Is that right? So the statement that the function sets this to the first rule box would be wrong then, since it can set it to any rule box?
There was a problem hiding this comment.
The function takes a group id and selects that group together with its first rule box: ruleBoxesForGroup(groupId, ...)[0]. It never picks another rule box. The ticket's AC asks for the first rule box after a delete, while selectGroup (used on a click) selects the last one, and that difference is what the comment was trying to say. I replaced it with one line that says exactly that.
| // keeps the browser's focus; only a vanished group strands it. | ||
| const isDeletedGroupGone = !freshIndex.groupsById.has(deletedGroupId); | ||
|
|
||
| if (target === 'unchanged') { |
There was a problem hiding this comment.
Question: what does target === 'unchanged' refer to? And what does the code in the if-else statement do? Could you extract this function into a well-named function that conveys in its name what the function does?If you can use semantic variable naming to communicate intention that would help. For example, I'm not sure what the line setKeyboardFocusRequest(
isDeletedGroupGone && !hasVisibleOrNewCourseGroup(snapshot, freshIndex) ? { kind: 'empty' } : null,
);` checks. If you could put the argument in a well named variable that would tell me what it is intended to be.
There was a problem hiding this comment.
selectionTargetAfterRemoval returns one of three things: the id of the group to select next, null when nothing is left to select, or a sentinel meaning "the current selection survived the delete, keep it". I renamed that sentinel to 'keep-current' and pulled the conditions into named variables, for example isPanelEmpty for the condition behind the { kind: 'empty' } request. The function now reads as two steps: decide the new selection, then decide where keyboard focus goes.
| expect(mockShowToast).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('treats a 404 as success: no toast, and focus still moves off the vanished group', async () => { |
There was a problem hiding this comment.
Could we expect a 204 here? A 404 is an error state that communicates that a resource was expected to be found but not found. A delete request typically responds with 204 to show that a resource was successfully deleted and is not returned. If you make a read request against the resource, of course, there would now be a 404, but a well working application would not be expected to make a read request.
It would be great if you could change the code and test so it expects a 204. A 404 should show an error in the frontend. Now if we just mock a 204 response here but it's not in line with the backend, that doesn't really help. Could you please identify the PR that provides the backend for this delete function and then ask the PR author to fix this to return a 204?
Correct me if I got anything wrong here please.
There was a problem hiding this comment.
- Successful delete. The backend PR (feat: add endpoint to delete or archive a CompetencyCriteriaGroup sub… ufedaseyeuconsultant/openedx-core#4, for [BE] Build endpoint for removing a Competency Criteria Group openedx/openedx-core#675) returns 200 with a body:
{ id, archived, cascaded_group_count, cascaded_criteria_count }, the frontend never reads the body, so a 204 would work just as well. Whether to drop the body is a backend decision. If you'd still prefer a 204 - 404. The ticket's AC "Deleting a group another author already deleted counts as a successful deletion" asks for the group to disappear with no error, because the author got what they asked for. Any other failure status still shows the error toast.
| expect(screen.getByTestId('keyboard-focus-request')).toHaveTextContent('null'); | ||
| }); | ||
|
|
||
| it('drops a second call made before the first settles, so one request is sent', async () => { |
There was a problem hiding this comment.
What's the real world case here? Why don't we just disable the delete button when we make the first request, until we receive a response?
| }); | ||
| }); | ||
|
|
||
| it('deletes a criteria group at the same URL the group PATCH uses', async () => { |
There was a problem hiding this comment.
I don't understand the test name. Could you explain more clearly what the expected behavior of the test is?
| const takesKeyboardFocus = group !== undefined | ||
| && keyboardFocusRequest?.kind === 'group' | ||
| && keyboardFocusRequest.groupId === group.id; | ||
| useEffect(() => { |
There was a problem hiding this comment.
| useEffect(() => { | |
| useEffect(() => { |
| * (`ruleKey === null` with a real `groupId`) is treated as focus on that | ||
| * group. | ||
| */ | ||
| export function focusTargetAfterRemoval( |
There was a problem hiding this comment.
Could you improve readability here? I understand the intention of this function from its comment, but I don't understand the code without lots of effort.
Maybe you could decompose this into small functions that run through the rules. For example, findFirstGroupAbove(). Also it would be great if you could avoid nested if statements and nested for loops within if statements here. Maybe use a series of guard clauses instead, or something that's pretty clear?
1af8cbb to
4dd2304
Compare
716766e to
5f5e1eb
Compare
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
4dd2304 to
3a5e9be
Compare
ac52cf8 to
8570846
Compare
Description
Implements Delete Competency Course & Rule Groups (openedx/openedx-core#709). It builds on <#671 PR>. Until that PR merges, this PR's diff also shows its changes. To review only this PR's own changes, open the last commit, "feat: add delete hooks for group and courses".
Each Course Group and each Rule Group on the Competency Management page's associations panel now has a trash icon. A click deletes the group right away, with no confirmation dialog. openedx/openedx-core#786 adds that dialog later.
Data layer (
data/):deleteCompetencyCriteriaGroupsendsDELETE /api/cbe/v1/competencies/<tag_id>/criteria-groups/<group_id>/.useDeleteCompetencyCriteriaGrouprefetches the criteria groups after every delete, whether it succeeds or fails. It stays pending until that refetch lands, so the panel only ever shows what the server holds.Trash controls (
criteria-groups/):Focus after a delete (
utils.ts,CompetencyAssociationsContext.tsx): before sending the request, the provider records the page order. Once the refetch lands, the pure helperfocusTargetAfterRemovalpicks the new focus from that order and the refreshed data:The newly focused Rule Group's first rule box is the target for the next subsection the author selects. When the deleted group took the browser's keyboard focus with it, keyboard focus moves to the new Rule Group's header, or to the empty-state message when the panel is empty. A collapsed Course Group expands when focus moves into it.
Roles affected: Course Author (Competency Management page).
Supporting information
Testing instructions
These steps need a Studio instance with the openedx-core CBE API, including the DELETE endpoint from ufedaseyeuconsultant/openedx-core#4, and openedx/openedx-platform#39099 applied.
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