From c866ce084545586f59e095ab7a08e13f888bff9a Mon Sep 17 00:00:00 2001 From: AShatsila <319486855+AShatsila@users.noreply.github.com> Date: Tue, 29 Sep 2026 17:15:00 +0400 Subject: [PATCH 01/12] feat: add criteria-groups UI and wire associations into the page Adds the course group, rule box and criterion chip components and wires gradeable-subsection association selection into the competency management page's course search panel. Co-Authored-By: Claude Opus 5.5 --- .../CompetencyAssociationsPanel.tsx | 37 ++-- .../CourseOutlineSubtree.test.tsx | 111 ++++++++-- .../course-search/CourseOutlineSubtree.tsx | 143 ++++++++----- .../course-search/CourseRow.tsx | 19 +- .../course-search/CourseSearchBrowse.scss | 24 ++- .../course-search/CourseSearchBrowse.test.tsx | 101 ++++++---- .../course-search/CourseSearchBrowse.tsx | 33 +-- .../CriteriaAssociationsSection.test.tsx | 77 +++++++ .../CriteriaAssociationsSection.tsx | 54 +++++ .../course-search/messages.ts | 9 +- .../criteria-groups/CourseGroupList.test.tsx | 146 ++++++++++++++ .../criteria-groups/CourseGroupList.tsx | 64 ++++++ .../CourseGroupSection.test.tsx | 190 ++++++++++++++++++ .../criteria-groups/CourseGroupSection.tsx | 96 +++++++++ .../criteria-groups/CriteriaGroupBox.test.tsx | 120 +++++++++++ .../criteria-groups/CriteriaGroupBox.tsx | 100 +++++++++ .../criteria-groups/CriterionChip.test.tsx | 13 ++ .../criteria-groups/CriterionChip.tsx | 18 ++ .../CriterionChipList.test.tsx | 55 +++++ .../criteria-groups/CriterionChipList.tsx | 42 ++++ .../criteria-groups/GroupConnector.test.tsx | 19 ++ .../criteria-groups/GroupConnector.tsx | 36 ++++ .../LogicOperatorSelect.test.tsx | 37 ++++ .../criteria-groups/LogicOperatorSelect.tsx | 51 +++++ .../criteria-groups/RuleBox.test.tsx | 77 +++++++ .../criteria-groups/RuleBox.tsx | 85 ++++++++ .../criteria-groups/RuleBoxList.test.tsx | 135 +++++++++++++ .../criteria-groups/RuleBoxList.tsx | 43 ++++ .../ScoreThresholdField.test.tsx | 32 +++ .../criteria-groups/ScoreThresholdField.tsx | 49 +++++ .../criteria-groups/criteria-groups.scss | 111 ++++++++++ .../criteria-groups/index.ts | 1 + .../criteria-groups/messages.ts | 84 ++++++++ 33 files changed, 2063 insertions(+), 149 deletions(-) create mode 100644 src/taxonomy/competency-management/course-search/CriteriaAssociationsSection.test.tsx create mode 100644 src/taxonomy/competency-management/course-search/CriteriaAssociationsSection.tsx create mode 100644 src/taxonomy/competency-management/criteria-groups/CourseGroupList.test.tsx create mode 100644 src/taxonomy/competency-management/criteria-groups/CourseGroupList.tsx create mode 100644 src/taxonomy/competency-management/criteria-groups/CourseGroupSection.test.tsx create mode 100644 src/taxonomy/competency-management/criteria-groups/CourseGroupSection.tsx create mode 100644 src/taxonomy/competency-management/criteria-groups/CriteriaGroupBox.test.tsx create mode 100644 src/taxonomy/competency-management/criteria-groups/CriteriaGroupBox.tsx create mode 100644 src/taxonomy/competency-management/criteria-groups/CriterionChip.test.tsx create mode 100644 src/taxonomy/competency-management/criteria-groups/CriterionChip.tsx create mode 100644 src/taxonomy/competency-management/criteria-groups/CriterionChipList.test.tsx create mode 100644 src/taxonomy/competency-management/criteria-groups/CriterionChipList.tsx create mode 100644 src/taxonomy/competency-management/criteria-groups/GroupConnector.test.tsx create mode 100644 src/taxonomy/competency-management/criteria-groups/GroupConnector.tsx create mode 100644 src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.test.tsx create mode 100644 src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.tsx create mode 100644 src/taxonomy/competency-management/criteria-groups/RuleBox.test.tsx create mode 100644 src/taxonomy/competency-management/criteria-groups/RuleBox.tsx create mode 100644 src/taxonomy/competency-management/criteria-groups/RuleBoxList.test.tsx create mode 100644 src/taxonomy/competency-management/criteria-groups/RuleBoxList.tsx create mode 100644 src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.test.tsx create mode 100644 src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.tsx create mode 100644 src/taxonomy/competency-management/criteria-groups/criteria-groups.scss create mode 100644 src/taxonomy/competency-management/criteria-groups/index.ts create mode 100644 src/taxonomy/competency-management/criteria-groups/messages.ts diff --git a/src/taxonomy/competency-management/associations/CompetencyAssociationsPanel.tsx b/src/taxonomy/competency-management/associations/CompetencyAssociationsPanel.tsx index f2327e87d5..4034ef182a 100644 --- a/src/taxonomy/competency-management/associations/CompetencyAssociationsPanel.tsx +++ b/src/taxonomy/competency-management/associations/CompetencyAssociationsPanel.tsx @@ -4,6 +4,7 @@ import { Col, Row, Stack } from '@openedx/paragon'; import { ResizableBox } from '@src/generic/resizable/Resizable'; import { useIsDesktop } from '@src/utils'; +import { CompetencyAssociationsProvider } from '../CompetencyAssociationsContext'; import CompetencyTree, { type CompetencyTreeNode } from '../CompetencyTree'; import { CourseSearchBrowse } from '../course-search'; @@ -24,10 +25,10 @@ const CompetencyAssociationsPanel = ({ taxonomyId, taxonomyName }: CompetencyAss const [selectedCompetency, setSelectedCompetency] = useState(null); const isDesktop = useIsDesktop(); - // Rendered once and reused, bare, in both branches below: `CompetencyTree` - // keeps its own expand/collapse state locally, so moving it to a different - // position in the element tree between states would remount it and reset - // that state. + // Rendered once and reused in both branches below at the same element-tree + // position: `CompetencyTree` keeps its own expand/collapse state locally, + // so moving it between states would make React remount it and silently + // reset what the user had expanded. const tree = ( {tree} {selectedCompetency && (
- + + +
)} @@ -60,12 +64,17 @@ const CompetencyAssociationsPanel = ({ taxonomyId, taxonomyName }: CompetencyAss return ( - + {tree} {selectedCompetency && ( - - + + + + )} diff --git a/src/taxonomy/competency-management/course-search/CourseOutlineSubtree.test.tsx b/src/taxonomy/competency-management/course-search/CourseOutlineSubtree.test.tsx index 226e4be402..3bc15b3643 100644 --- a/src/taxonomy/competency-management/course-search/CourseOutlineSubtree.test.tsx +++ b/src/taxonomy/competency-management/course-search/CourseOutlineSubtree.test.tsx @@ -7,6 +7,7 @@ import { userEvent, within, } from '@src/testUtils'; +import { buildMockCompetencyAssociationsContextValue, MockCompetencyAssociationsProvider } from '../testHelpers'; import CourseOutlineSubtree from './CourseOutlineSubtree'; let axiosMock; @@ -20,7 +21,10 @@ const mixedOutline = buildOutlineIndex({ id: 'section-1', displayName: 'Section 1', children: [ - { id: 'sub-1a', displayName: 'Subsection 1A (graded)', overrides: { graded: true } }, + // `usageKey: undefined` mirrors the real `course_index` response, + // which never populates `usage_key` - so these tests only pass if + // `SubsectionRow` reads `.id`, not `.usageKey`. + { id: 'sub-1a', displayName: 'Subsection 1A (graded)', overrides: { graded: true, usageKey: undefined } }, { id: 'sub-1b', displayName: 'Subsection 1B (ungraded)', overrides: { graded: false } }, ], }, @@ -75,6 +79,16 @@ const allUngradedOutline = buildOutlineIndex({ const noSubsectionsOutline = buildOutlineIndex([]); +// Most tests below care about outline rendering, not the association data +// layer, so a lightly-mocked provider is enough; a few near the end +// override `associatedObjectIds`/`associateSubsection` directly. +const renderSubtree = (contextOverrides: Parameters[0] = {}) => + render( + + + , + ); + describe('', () => { beforeEach(() => { ({ axiosMock } = initializeMocks()); @@ -82,14 +96,14 @@ describe('', () => { it('renders a loading state while the outline request is pending', () => { axiosMock.onGet(outlineApiUrl).reply(() => new Promise(() => {})); - render(); + renderSubtree(); expect(screen.getByRole('status')).toBeInTheDocument(); }); it('renders a scoped inline error, not the panel-level error, when the outline request fails', async () => { axiosMock.onGet(outlineApiUrl).reply(500); - render(); + renderSubtree(); const error = await screen.findByText('There was a problem loading this course\'s outline.'); expect(error).toBeInTheDocument(); @@ -102,7 +116,7 @@ describe('', () => { async () => { const user = userEvent.setup(); axiosMock.onGet(outlineApiUrl).reply(200, mixedOutline); - render(); + renderSubtree(); expect(await screen.findByText('Section 1')).toBeInTheDocument(); expect(screen.getByText('Section 2')).toBeInTheDocument(); @@ -131,7 +145,7 @@ describe('', () => { async () => { const user = userEvent.setup(); axiosMock.onGet(outlineApiUrl).reply(200, partiallyGradedOutline); - render(); + renderSubtree(); expect(await screen.findByText('Section 1')).toBeInTheDocument(); const section2Header = screen.getByText('Section 2'); @@ -157,7 +171,7 @@ describe('', () => { + 'subsection is ungraded', async () => { axiosMock.onGet(outlineApiUrl).reply(200, allUngradedOutline); - render(); + renderSubtree(); expect(await screen.findByText('This course has no gradeable subsections.')).toBeInTheDocument(); expect(screen.queryByText('Section 1')).not.toBeInTheDocument(); @@ -167,7 +181,7 @@ describe('', () => { it('renders the whole-course no-gradeable-subsections message when there are no subsections at all', async () => { axiosMock.onGet(outlineApiUrl).reply(200, noSubsectionsOutline); - render(); + renderSubtree(); expect(await screen.findByText('This course has no gradeable subsections.')).toBeInTheDocument(); }); @@ -175,7 +189,7 @@ describe('', () => { it('does nothing when a section with no graded subsections is clicked', async () => { const user = userEvent.setup(); axiosMock.onGet(outlineApiUrl).reply(200, partiallyGradedOutline); - render(); + renderSubtree(); const header = await screen.findByText('Section 2'); await user.click(header); @@ -191,20 +205,89 @@ describe('', () => { + 'one request in total', async () => { axiosMock.onGet(outlineApiUrl).reply(200, mixedOutline); - const { unmount } = render(); + const { unmount } = renderSubtree(); await screen.findByText('Section 1'); expect(axiosMock.history.get).toHaveLength(1); - // Simulate the parent CourseRow collapsing (unmount) then re-expanding - // (remount) the same course while still within the query cache's normal - // staleTime. With `refetchOnMount: false`, this must serve the cached - // data without firing a second request. + // Simulate the parent CourseRow collapsing then re-expanding the same + // course within the query cache's staleTime: must serve cached data. unmount(); - render(); + renderSubtree(); await screen.findByText('Section 1'); expect(axiosMock.history.get).toHaveLength(1); }, ); + + it( + 'marks a subsection as already associated by its real id - the field the real course_index response ' + + 'actually populates, never the always-undefined usageKey', + async () => { + const user = userEvent.setup(); + axiosMock.onGet(outlineApiUrl).reply(200, mixedOutline); + renderSubtree({ associatedObjectIds: new Set(['sub-1a']) }); + + // Expand both sections' graded subsections: `sub-1a` (Section 1) and + // `sub-2a` (Section 2) - `sub-1b` is ungraded, so it never renders a + // row at all (see the "shows a disclosure icon..." test above). + const expandButtons = await screen.findAllByRole('button', { name: 'Expand' }); + await user.click(expandButtons[0]); + await user.click(screen.getByRole('button', { name: 'Expand' })); + + expect(screen.getByRole('button', { name: 'Subsection 1A (graded)' })).toHaveAttribute( + 'data-associated', + 'true', + ); + // A sibling row whose id isn't in the set stays unmarked - proves the + // lookup is a real per-row match, not something that's always true. + expect(screen.getByRole('button', { name: 'Subsection 2A (graded)' })).toHaveAttribute( + 'data-associated', + 'false', + ); + }, + ); + + it( + 'calls associateSubsection with the subsection\'s real id when a selectable row is clicked - not the ' + + 'usageKey field the real API never populates', + async () => { + const user = userEvent.setup(); + const associateSubsection = jest.fn(); + axiosMock.onGet(outlineApiUrl).reply(200, mixedOutline); + renderSubtree({ associateSubsection }); + + await user.click((await screen.findAllByRole('button', { name: 'Expand' }))[0]); + await user.click(screen.getByRole('button', { name: 'Subsection 1A (graded)' })); + + expect(associateSubsection).toHaveBeenCalledWith('sub-1a', courseId); + }, + ); + + it( + 'renders the Competency ID badge on an already-associated row when competencyExternalId is set ' + + '(moved here from CourseSearchBrowse.test.tsx along with the badge itself)', + async () => { + axiosMock.onGet(outlineApiUrl).reply(200, mixedOutline); + renderSubtree({ associatedObjectIds: new Set(['sub-1a']), competencyExternalId: 'EXT-042' }); + + await userEvent.setup().click((await screen.findAllByRole('button', { name: 'Expand' }))[0]); + // The badge's own text becomes part of the button's accessible name + // once set, so find the row by its label text instead of `getByRole`. + const row = (await screen.findByText('Subsection 1A (graded)')).closest('button') as HTMLElement; + + expect(within(row).getByText('EXT-042')).toHaveAttribute('aria-hidden', 'true'); + expect(within(row).getByText('Competency ID: EXT-042')).toBeInTheDocument(); + }, + ); + + it('renders no Competency ID badge on an already-associated row when competencyExternalId is falsy', async () => { + axiosMock.onGet(outlineApiUrl).reply(200, mixedOutline); + renderSubtree({ associatedObjectIds: new Set(['sub-1a']), competencyExternalId: null }); + + await userEvent.setup().click((await screen.findAllByRole('button', { name: 'Expand' }))[0]); + const row = (await screen.findByText('Subsection 1A (graded)')).closest('button') as HTMLElement; + + expect(within(row).queryByText('Competency ID:', { exact: false })).not.toBeInTheDocument(); + }); }); diff --git a/src/taxonomy/competency-management/course-search/CourseOutlineSubtree.tsx b/src/taxonomy/competency-management/course-search/CourseOutlineSubtree.tsx index 166874c24f..550358f3cd 100644 --- a/src/taxonomy/competency-management/course-search/CourseOutlineSubtree.tsx +++ b/src/taxonomy/competency-management/course-search/CourseOutlineSubtree.tsx @@ -1,12 +1,25 @@ import { type ReactNode, useState } from 'react'; import { useIntl } from '@edx/frontend-platform/i18n'; -import { Button, IconButton } from '@openedx/paragon'; -import { ExpandLess, ExpandMore } from '@openedx/paragon/icons'; +import { + Badge, + Button, + Icon, + IconButton, +} from '@openedx/paragon'; +import { + AddCircleOutline, + AdsClick, + CheckCircle, + ExpandLess, + ExpandMore, +} from '@openedx/paragon/icons'; import { useCourseOutlineIndex } from '@src/course-outline/data'; import type { XBlock } from '@src/data/types'; import { LoadingSpinner } from '@src/generic/Loading'; +import { useCompetencyAssociations } from '../CompetencyAssociationsContext'; +import rootMessages from '../messages'; import messages from './messages'; export interface CourseOutlineSubtreeProps { @@ -15,29 +28,81 @@ export interface CourseOutlineSubtreeProps { interface SubsectionRowProps { subsection: XBlock; - isAssociated: boolean; + courseId: string; } /** * One row for a single graded subsection. * - * `isAssociated` is a typed seam for a later, separately-scoped ticket that - * will show a visual "already associated with the active competency" state. - * `CourseOutlineSubtree` always passes `false` today (no fetch backs it - * yet), and it is only surfaced as a `data-associated` attribute, so it - * produces no visible difference right now. + * Clicking calls `associateSubsection` unless `canEditCourse(courseId)` is + * false, in which case the row has no click behavior. The already-associated + * badge and marker icon show regardless of `canEditCourse`, so a + * view-only course still shows what's associated. Re-clicking an + * already-associated row hits the duplicate guard inside + * `associateSubsection` (an informational toast), not a failed request. */ -const SubsectionRow = ({ subsection, isAssociated }: SubsectionRowProps) => ( - -); +const SubsectionRow = ({ subsection, courseId }: SubsectionRowProps) => { + const intl = useIntl(); + const { + associatedObjectIds, + associateSubsection, + canEditCourse, + competencyExternalId, + } = useCompetencyAssociations(); + // The real `course_index` response populates `.id`, never `.usageKey` + // (declared on the shared `XBlockBase` type but always `undefined` here). + const isAssociated = associatedObjectIds.has(subsection.id); + const canSelect = canEditCourse(courseId); + + return ( + + ); +}; interface SectionHeaderProps { displayName: string; @@ -47,15 +112,10 @@ interface SectionHeaderProps { } /** - * One section (chapter) header, never itself an association target for the - * active competency (navigation only, mirroring `CourseRow`'s own course - * title). - * - * A section with at least one graded subsection gets its own disclosure - * control - reusing the plain `IconButton` + `ExpandLess`/`ExpandMore` - * pattern `CourseRow` already uses for its course-level toggle - and starts - * collapsed. A section with no graded subsections has nothing to disclose, - * so it keeps rendering as plain, non-interactive text with no icon. + * One section (chapter) header: navigation only, never an association + * target. A section with a graded subsection gets its own disclosure + * control, starting collapsed; a section with none renders as plain, + * non-interactive text with no icon. */ const SectionHeader = ({ displayName, @@ -89,28 +149,19 @@ const SectionHeader = ({ }; /** - * Real, lazily-fetched course outline shown inside an expanded `CourseRow`. + * Lazily-fetched course outline shown inside an expanded `CourseRow`. * - * Renders every section (chapter) as a header, and under each section, only - * its graded subsections as clickable rows - ungraded subsections and - * anything below a subsection (units/verticals) are out of scope and never - * rendered. A section with at least one graded subsection gets its own - * disclosure control (see `SectionHeader`), defaulting to collapsed; a - * section with none keeps its header non-interactive, same as before. + * Renders each section as a header, and under it only its graded + * subsections as clickable rows; ungraded subsections and anything below a + * subsection (units/verticals) are never rendered. */ const CourseOutlineSubtree = ({ courseId }: CourseOutlineSubtreeProps) => { const intl = useIntl(); - // `refetchOnMount: false`: this component only ever mounts while its parent - // `CourseRow` is expanded, so the component's own mount/unmount lifecycle IS - // the "is this needed" gate - a separate, manually-tracked `enabled` flag on - // top of that would be redundant. Without this option, collapsing and - // re-expanding the same course (unmount then remount) within the query - // cache's normal staleTime would trigger a wasted background refetch of - // data that's already cached (the query key is unique per course). + // `refetchOnMount: false`: without it, collapsing and re-expanding the + // same course (unmount/remount) within the query cache's staleTime would + // trigger a wasted refetch of already-cached data. const { data, isLoading, isError } = useCourseOutlineIndex(courseId, { refetchOnMount: false }); - // Ids of sections (chapters) currently expanded, showing their graded - // subsections - every section starts collapsed, mirroring `CourseRow`'s own - // default-collapsed convention for its course-level toggle. + // Ids of sections currently expanded; every section starts collapsed. const [expandedSectionIds, setExpandedSectionIds] = useState>(new Set()); const handleToggleSection = (sectionId: string) => { @@ -169,7 +220,7 @@ const CourseOutlineSubtree = ({ courseId }: CourseOutlineSubtreeProps) => { ))} diff --git a/src/taxonomy/competency-management/course-search/CourseRow.tsx b/src/taxonomy/competency-management/course-search/CourseRow.tsx index 6cfcd5c662..cc322c9319 100644 --- a/src/taxonomy/competency-management/course-search/CourseRow.tsx +++ b/src/taxonomy/competency-management/course-search/CourseRow.tsx @@ -5,6 +5,7 @@ import { IconButton } from '@openedx/paragon'; import { ExpandLess, ExpandMore } from '@openedx/paragon/icons'; import type { Course } from '@src/studio-home/data/api'; +import { useCompetencyAssociations } from '../CompetencyAssociationsContext'; import CourseOutlineSubtree from './CourseOutlineSubtree'; import messages from './messages'; @@ -23,12 +24,28 @@ export interface CourseRowProps { */ const CourseRow = ({ course }: CourseRowProps) => { const intl = useIntl(); + const { notifyCourseExpanded } = useCompetencyAssociations(); const [isExpanded, setIsExpanded] = useState(false); const toggleLabel = isExpanded ? intl.formatMessage(messages.collapseRowButtonLabel) : intl.formatMessage(messages.expandRowButtonLabel); + const handleToggle = () => { + // Reads `isExpanded` directly rather than a functional updater: React + // may invoke a functional updater more than once, so calling + // `notifyCourseExpanded` (another component's state setter) from inside + // one would risk React's "Cannot update a component while rendering a + // different component" warning. + const next = !isExpanded; + if (next) { + // Only on expand, never on collapse - see `notifyCourseExpanded`'s + // own docstring for why this direction matters. + notifyCourseExpanded(course.courseKey); + } + setIsExpanded(next); + }; + return (
@@ -38,7 +55,7 @@ const CourseRow = ({ course }: CourseRowProps) => { aria-label={toggleLabel} aria-expanded={isExpanded} size="sm" - onClick={() => setIsExpanded((prev) => !prev)} + onClick={handleToggle} />
{course.displayName}
diff --git a/src/taxonomy/competency-management/course-search/CourseSearchBrowse.scss b/src/taxonomy/competency-management/course-search/CourseSearchBrowse.scss index 27315198ee..6817b8e56b 100644 --- a/src/taxonomy/competency-management/course-search/CourseSearchBrowse.scss +++ b/src/taxonomy/competency-management/course-search/CourseSearchBrowse.scss @@ -249,6 +249,28 @@ text-align: left; // Paragon's `.btn` sets `inline-flex`/`justify-content: center`, which // (not `text-align`) controls this button's text position. - justify-content: flex-start; + justify-content: space-between; + } + + &__subsection-actions { + display: flex; + align-items: center; + flex-shrink: 0; + } + + &__subsection-badge { + display: inline-flex; + align-items: center; + gap: 4px; + margin-right: 6px; + } + + &__subsection-associated-icon, + &__subsection-select-icon { + color: var(--pgn-color-info-500); + } + + &__subsection-associated-icon { + margin-right: 6px; } } diff --git a/src/taxonomy/competency-management/course-search/CourseSearchBrowse.test.tsx b/src/taxonomy/competency-management/course-search/CourseSearchBrowse.test.tsx index 1faefe3841..edc0c20683 100644 --- a/src/taxonomy/competency-management/course-search/CourseSearchBrowse.test.tsx +++ b/src/taxonomy/competency-management/course-search/CourseSearchBrowse.test.tsx @@ -10,6 +10,8 @@ import { import { getApiBaseUrl, type Course } from '@src/studio-home/data/api'; import { getCourseOutlineIndexApiUrl } from '@src/course-outline/data'; import { buildOutlineIndex } from '@src/course-outline/__mocks__'; +import { CompetencyAssociationsProvider } from '../CompetencyAssociationsContext'; +import { apiUrls as competencyManagementApiUrls } from '../data/api'; import type { CompetencyTreeNode } from '../CompetencyTree'; import CourseRow from './CourseRow'; import CourseSearchBrowse from './CourseSearchBrowse'; @@ -38,7 +40,29 @@ const buildResponse = (courses: Course[], numPages: number = 1) => ({ count: courses.length, }); -const lastRequestParams = () => axiosMock.history.get[axiosMock.history.get.length - 1].params; +// Scoped to the courses-list endpoint: `axiosMock.history.get` also holds +// `CompetencyAssociationsProvider`'s own requests. +const lastRequestParams = () => { + const courseRequests = axiosMock.history.get.filter((req) => req.url === coursesApiUrl); + return courseRequests[courseRequests.length - 1].params; +}; + +const renderCourseSearchBrowse = (competency: CompetencyTreeNode = activeCompetency) => + render( + + + , + ); + +const renderCourseRow = (course: Course) => + render( + + + , + ); describe(' and ', () => { beforeAll(() => { @@ -47,6 +71,21 @@ describe(' and ', () => { beforeEach(() => { ({ axiosMock } = initializeMocks()); + axiosMock.onGet(competencyManagementApiUrls.competencyCriteriaGroups(Number(activeCompetency.id))) + .reply(200, { groups: [], criteria: [] }); + axiosMock.onGet(competencyManagementApiUrls.defaultCompetencyRuleProfile()) + .reply(200, { + count: 1, + next: null, + previous: null, + results: [{ + id: 1, + scope_type: 'system_default', + rule_type: 'grade', + rule_payload: { op: 'gte', value: 0.7, scale: 'percent' }, + archived: false, + }], + }); }); afterAll(() => { @@ -55,39 +94,23 @@ describe(' and ', () => { describe('', () => { it( - 'renders the active competency\'s name in the "Demonstrate Mastery For" line, and the static ' - + 'associations empty state', + 'renders the active competency\'s name in the "Demonstrate Mastery For" line, and the ' + + 'empty-associations state once the associations queries resolve', async () => { axiosMock.onGet(coursesApiUrl).reply(200, buildResponse([buildCourse()])); - render(); + renderCourseSearchBrowse(); - // The competency name renders inside its own `` node (see - // `CourseSearchBrowse.tsx`), so the full sentence isn't a single text - // node - check the line's own full text, then separately confirm the - // name within it is bolded. + // The name renders in its own ``, so the sentence is not a single text node. const masteryLine = await screen.findByText( (_, element) => - element?.tagName.toLowerCase() === 'div' - && element.classList.contains('course-search-browse__associations-mastery') + element?.classList.contains('course-search-browse__associations-mastery') === true && element.textContent === 'Demonstrate Mastery For Test Competency', ); - expect(masteryLine).toBeInTheDocument(); expect(within(masteryLine).getByText('Test Competency', { selector: 'strong' })).toBeInTheDocument(); - // The competency name renders inside its own `` node (see - // `CourseSearchBrowse.tsx`), so the full sentence isn't a single text - // node - check the paragraph's own full text, then separately confirm - // the name within it is bolded. - const emptyStateMessage = screen.getByText( - (_, element) => - element?.tagName.toLowerCase() === 'p' - && element.textContent === 'No content associated with Test Competency yet.', - ); - expect(emptyStateMessage).toBeInTheDocument(); - expect(within(emptyStateMessage).getByText('Test Competency', { selector: 'strong' })).toBeInTheDocument(); - + expect(await screen.findByText('No content associated.')).toBeInTheDocument(); expect( - screen.getByText('Make content selections to create competency criteria associations.'), + screen.getByText('Make content selections to associate this competency with course content.'), ).toBeInTheDocument(); }, ); @@ -95,7 +118,7 @@ describe(' and ', () => { it('renders the Competency ID badge next to the "Demonstrate Mastery For" line when externalId is set', async () => { axiosMock.onGet(coursesApiUrl).reply(200, buildResponse([buildCourse()])); const competencyWithExternalId: CompetencyTreeNode = { ...activeCompetency, externalId: 'EXT-042' }; - render(); + renderCourseSearchBrowse(competencyWithExternalId); // The competency name renders inside its own `` node (see // `CourseSearchBrowse.tsx`), and the badge/sr-only text follow it in @@ -115,7 +138,7 @@ describe(' and ', () => { it('renders no Competency ID badge when externalId is falsy', async () => { axiosMock.onGet(coursesApiUrl).reply(200, buildResponse([buildCourse()])); - render(); + renderCourseSearchBrowse(); // The competency name renders inside its own `` node (see // `CourseSearchBrowse.tsx`), so the full sentence isn't a single text @@ -132,9 +155,13 @@ describe(' and ', () => { it('renders course rows once the request resolves, and paginates on page-button click', async () => { axiosMock.onGet(coursesApiUrl).reply(200, buildResponse([buildCourse()], 2)); - render(); + renderCourseSearchBrowse(); - expect(screen.getByRole('status')).toBeInTheDocument(); + // Scoped to the course list: the associations section briefly shows its own spinner too. + const coursesContainer = screen.getByText('Courses & Content').closest( + '.course-search-browse__container', + ) as HTMLElement; + expect(within(coursesContainer).getByRole('status')).toBeInTheDocument(); expect(await screen.findByText('Intro to Testing')).toBeInTheDocument(); fireEvent.click(screen.getByRole('button', { name: 'Page 2' })); @@ -146,7 +173,7 @@ describe(' and ', () => { it('renders an inline error, not the empty state, when the request fails', async () => { axiosMock.onGet(coursesApiUrl).reply(500); - render(); + renderCourseSearchBrowse(); expect(await screen.findByText('There was a problem loading courses. Please try again.')) .toBeInTheDocument(); @@ -156,7 +183,7 @@ describe(' and ', () => { it('shows distinct empty-result messages for no search vs. a search value, and clears the search on demand', async () => { axiosMock.onGet(coursesApiUrl).reply(200, buildResponse([])); - render(); + renderCourseSearchBrowse(); expect(await screen.findByText('You do not have access to any courses.')).toBeInTheDocument(); expect(screen.queryByRole('button', { name: 'Clear search' })).not.toBeInTheDocument(); @@ -180,7 +207,7 @@ describe(' and ', () => { it('shows the "no matching courses" message, not "no accessible courses", when only a date filter (no search text) narrows the results to zero', async () => { axiosMock.onGet(coursesApiUrl).reply(200, buildResponse([buildCourse()])); - render(); + renderCourseSearchBrowse(); await screen.findByText('Intro to Testing'); axiosMock.onGet(coursesApiUrl).reply(200, buildResponse([])); @@ -197,7 +224,7 @@ describe(' and ', () => { it('debounces the search field by 400ms and resets the page back to 1', async () => { axiosMock.onGet(coursesApiUrl).reply(200, buildResponse([buildCourse()], 3)); - render(); + renderCourseSearchBrowse(); await screen.findByText('Intro to Testing'); // Move off page 1 first, so the reset back to page 1 below is observable. @@ -234,7 +261,7 @@ describe(' and ', () => { it('keeps an active search filter in the request when only the page changes', async () => { axiosMock.onGet(coursesApiUrl).reply(200, buildResponse([buildCourse()], 3)); - render(); + renderCourseSearchBrowse(); await screen.findByText('Intro to Testing'); const searchBox = screen.getByRole('searchbox'); @@ -259,7 +286,7 @@ describe(' and ', () => { it('sends start_date_on_or_after when a start date is picked in the range calendar, and resets the page back to 1', async () => { axiosMock.onGet(coursesApiUrl).reply(200, buildResponse([buildCourse()], 3)); - render(); + renderCourseSearchBrowse(); await screen.findByText('Intro to Testing'); // Move off page 1 first, so the reset back to page 1 below is observable. @@ -296,7 +323,7 @@ describe(' and ', () => { it('sends both start_date_on_or_after and start_date_on_or_before once an end date is also picked', async () => { axiosMock.onGet(coursesApiUrl).reply(200, buildResponse([buildCourse()], 3)); - render(); + renderCourseSearchBrowse(); await screen.findByText('Intro to Testing'); fireEvent.click(screen.getByLabelText('Start Date')); @@ -326,7 +353,7 @@ describe(' and ', () => { it('omits start_date_on_or_after and start_date_on_or_before from the request when neither date is set', async () => { axiosMock.onGet(coursesApiUrl).reply(200, buildResponse([buildCourse()])); - render(); + renderCourseSearchBrowse(); await screen.findByText('Intro to Testing'); // The mock adapter records the params object exactly as handed to axios, @@ -357,7 +384,7 @@ describe(' and ', () => { ], }), ); - render(); + renderCourseRow(course); expect(screen.queryByText(outlineSectionHeading)).not.toBeInTheDocument(); diff --git a/src/taxonomy/competency-management/course-search/CourseSearchBrowse.tsx b/src/taxonomy/competency-management/course-search/CourseSearchBrowse.tsx index f34d1a6799..638c603a92 100644 --- a/src/taxonomy/competency-management/course-search/CourseSearchBrowse.tsx +++ b/src/taxonomy/competency-management/course-search/CourseSearchBrowse.tsx @@ -10,7 +10,6 @@ import { import { useIntl } from '@edx/frontend-platform/i18n'; import { - Badge, Button, IconButton, Pagination, @@ -29,6 +28,7 @@ import { DATE_FORMAT } from '@src/constants'; import { useStudioHomeCoursesV2 } from '@src/studio-home/data/apiHooks'; import type { CompetencyTreeNode } from '../CompetencyTree'; import CourseRow from './CourseRow'; +import CriteriaAssociationsSection from './CriteriaAssociationsSection'; import messages from './messages'; // @ts-ignore import './CourseSearchBrowse.scss'; @@ -287,36 +287,7 @@ const CourseSearchBrowse = ({ activeCompetency }: CourseSearchBrowseProps) => {
-
-
- {intl.formatMessage(messages.associationsSectionLabel)} -
-
- {intl.formatMessage(messages.demonstrateMasteryForLabel, { - competencyName: {activeCompetency.value}, - })} - {activeCompetency.externalId && ( - <> - - {intl.formatMessage(messages.competencyIdAccessibleLabel, { - externalId: activeCompetency.externalId, - })} - - - - )} -
-
-

- {intl.formatMessage(messages.noAssociationsMessage, { - competencyName: {activeCompetency.value}, - })} -

-

{intl.formatMessage(messages.noAssociationsPromptMessage)}

-
-
+
{intl.formatMessage(messages.coursesAndContentLabel)} diff --git a/src/taxonomy/competency-management/course-search/CriteriaAssociationsSection.test.tsx b/src/taxonomy/competency-management/course-search/CriteriaAssociationsSection.test.tsx new file mode 100644 index 0000000000..4ec67075ac --- /dev/null +++ b/src/taxonomy/competency-management/course-search/CriteriaAssociationsSection.test.tsx @@ -0,0 +1,77 @@ +import { initializeMocks, render, screen, within } from '@src/testUtils'; +import { CompetencyAssociationsProvider } from '../CompetencyAssociationsContext'; +import type { CompetencyTreeNode } from '../CompetencyTree'; +import { apiUrls } from '../data/api'; +import CriteriaAssociationsSection from './CriteriaAssociationsSection'; + +let axiosMock: ReturnType['axiosMock']; + +const tagId = 42; +const competency: CompetencyTreeNode = { id: String(tagId), value: 'Data Literacy', externalId: null }; + +const renderSection = () => ( + render( + + + , + ) +); + +describe('', () => { + beforeEach(() => { + ({ axiosMock } = initializeMocks()); + }); + + it('renders the associations header and the "Demonstrate Mastery For" line for the given competency', () => { + axiosMock.onGet(apiUrls.competencyCriteriaGroups(tagId)).reply(() => new Promise(() => {})); + axiosMock.onGet(apiUrls.defaultCompetencyRuleProfile()).reply(() => new Promise(() => {})); + renderSection(); + + expect(screen.getByText('Competency Criteria Associations')).toBeInTheDocument(); + const masteryLine = screen.getByText( + (_, element) => + element?.classList.contains('course-search-browse__associations-mastery') === true + && element.textContent === 'Demonstrate Mastery For Data Literacy', + ); + expect(within(masteryLine).getByText('Data Literacy', { selector: 'strong' })).toBeInTheDocument(); + }); + + it('renders the empty-associations state once both queries resolve with no groups', async () => { + axiosMock.onGet(apiUrls.competencyCriteriaGroups(tagId)).reply(200, { groups: [], criteria: [] }); + axiosMock.onGet(apiUrls.defaultCompetencyRuleProfile()).reply(200, { + count: 1, + next: null, + previous: null, + results: [{ + id: 1, + scope_type: 'system_default', + rule_type: 'grade', + rule_payload: { op: 'gte', value: 0.7, scale: 'percent' }, + archived: false, + }], + }); + renderSection(); + + expect(await screen.findByText('No content associated.')).toBeInTheDocument(); + }); + + it('renders a failed state when the groups request fails', async () => { + axiosMock.onGet(apiUrls.competencyCriteriaGroups(tagId)).reply(500); + axiosMock.onGet(apiUrls.defaultCompetencyRuleProfile()).reply(200, { + count: 1, + next: null, + previous: null, + results: [{ + id: 1, + scope_type: 'system_default', + rule_type: 'grade', + rule_payload: { op: 'gte', value: 0.7, scale: 'percent' }, + archived: false, + }], + }); + renderSection(); + + expect(await screen.findByText('There was a problem loading this competency\'s associations. Please try again.')) + .toBeInTheDocument(); + }); +}); diff --git a/src/taxonomy/competency-management/course-search/CriteriaAssociationsSection.tsx b/src/taxonomy/competency-management/course-search/CriteriaAssociationsSection.tsx new file mode 100644 index 0000000000..5eaf68959d --- /dev/null +++ b/src/taxonomy/competency-management/course-search/CriteriaAssociationsSection.tsx @@ -0,0 +1,54 @@ +import { useIntl } from '@edx/frontend-platform/i18n'; +import { Badge } from '@openedx/paragon'; + +import type { CompetencyTreeNode } from '../CompetencyTree'; +import { CourseGroupList } from '../criteria-groups'; +import messages from './messages'; + +export interface CriteriaAssociationsSectionProps { + /** The currently selected competency, named in the "Demonstrate Mastery + * For" line along with its Competency ID badge. + */ + competency: CompetencyTreeNode; +} + +/** The associations section above "Courses & Content": the selected + * competency's existing criteria groups, rendered as a tree of course-level + * groups, bottom-tier groups, and rule boxes. + * + * `CourseGroupList` reads its own data and focus state from + * `CompetencyAssociationsContext`; this component only renders the static + * label/mastery lines, which need `competency` directly since the context + * doesn't track its name. + */ +const CriteriaAssociationsSection = ({ competency }: CriteriaAssociationsSectionProps) => { + const intl = useIntl(); + + return ( +
+
+ {intl.formatMessage(messages.associationsSectionLabel)} +
+
+ {intl.formatMessage(messages.demonstrateMasteryForLabel, { + competencyName: {competency.value}, + })} + {competency.externalId && ( + <> + + {intl.formatMessage(messages.competencyIdAccessibleLabel, { + externalId: competency.externalId, + })} + + + + )} +
+ +
+ ); +}; + +export default CriteriaAssociationsSection; diff --git a/src/taxonomy/competency-management/course-search/messages.ts b/src/taxonomy/competency-management/course-search/messages.ts index bc2ddc7f82..f6de3b1ea7 100644 --- a/src/taxonomy/competency-management/course-search/messages.ts +++ b/src/taxonomy/competency-management/course-search/messages.ts @@ -94,14 +94,13 @@ const messages = defineMessages({ }, noAssociationsMessage: { id: 'course-authoring.competency-management.course-search.no-associations-message', - defaultMessage: 'No content associated with {competencyName} yet.', - description: 'First line of the static empty state shown in the associations block, since there is no ' - + 'backend endpoint yet to list a competency\'s actual associations. {competencyName} names the currently ' - + 'active competency.', + defaultMessage: 'No content associated.', + description: 'First line of the empty state shown in the associations block when the selected competency ' + + 'has no course content associated with it yet.', }, noAssociationsPromptMessage: { id: 'course-authoring.competency-management.course-search.no-associations-prompt-message', - defaultMessage: 'Make content selections to create competency criteria associations.', + defaultMessage: 'Make content selections to associate this competency with course content.', description: 'Second line of the static empty state shown in the associations block, prompting the user ' + 'to make content selections below.', }, diff --git a/src/taxonomy/competency-management/criteria-groups/CourseGroupList.test.tsx b/src/taxonomy/competency-management/criteria-groups/CourseGroupList.test.tsx new file mode 100644 index 0000000000..8706a0cc70 --- /dev/null +++ b/src/taxonomy/competency-management/criteria-groups/CourseGroupList.test.tsx @@ -0,0 +1,146 @@ +import type { UseQueryResult } from '@tanstack/react-query'; +import { initializeMocks, render, screen } from '@src/testUtils'; +import { buildMockCompetencyAssociationsContextValue, MockCompetencyAssociationsProvider } from '../testHelpers'; +import type { CompetencyCriteriaGroupsResponse, CompetencyRuleProfile } from '../data/types'; +import { buildCompetencyCriteriaGroupsIndex } from '../utils'; +import CourseGroupList from './CourseGroupList'; + +const systemDefaultProfile: CompetencyRuleProfile = { + id: 1, + scopeType: 'system_default', + ruleType: 'grade', + rulePayload: { op: 'gte', value: 0.7, scale: 'percent' }, + archived: false, +}; + +const emptyGroupsResponse: CompetencyCriteriaGroupsResponse = { groups: [], criteria: [] }; + +const populatedGroupsResponse: CompetencyCriteriaGroupsResponse = { + groups: [ + { + id: 100, + parentId: null, + tagId: 42, + courseKey: null, + name: 'root', + ordering: 0, + logicOperator: 'AND', + archived: false, + }, + { + id: 1, + parentId: 100, + tagId: 42, + courseKey: 'course-v1:OrgX+CS101+2024', + name: 'course', + ordering: 0, + logicOperator: 'AND', + archived: false, + }, + ], + criteria: [], +}; + +const buildQueryResult = (overrides: Partial>): UseQueryResult => ({ + isLoading: false, + isError: false, + data: undefined, + ...overrides, +} as UseQueryResult); + +const loadingGroupsQuery = buildQueryResult({ isLoading: true }); +const loadedEmptyGroupsQuery = buildQueryResult({ data: emptyGroupsResponse }); +const loadedPopulatedGroupsQuery = buildQueryResult({ + data: populatedGroupsResponse, +}); +const erroredGroupsQuery = buildQueryResult({ isError: true }); + +const loadingProfileQuery = buildQueryResult({ isLoading: true }); +const loadedProfileQuery = buildQueryResult({ data: systemDefaultProfile }); +const erroredProfileQuery = buildQueryResult({ isError: true }); + +const renderList = (contextOverrides: Parameters[0] = {}) => ( + render( + + + , + ) +); + +describe('', () => { + beforeEach(() => { + initializeMocks(); + }); + + it('renders a loading state while the groups query is still loading', () => { + renderList({ groupsQuery: loadingGroupsQuery, profileQuery: loadedProfileQuery }); + expect(screen.getByRole('status')).toBeInTheDocument(); + }); + + it('renders a loading state while the default-rule-profile query is still loading, even if groups already loaded', () => { + renderList({ groupsQuery: loadedEmptyGroupsQuery, profileQuery: loadingProfileQuery }); + expect(screen.getByRole('status')).toBeInTheDocument(); + }); + + it('renders a failed state when the groups query errors', () => { + renderList({ groupsQuery: erroredGroupsQuery, profileQuery: loadedProfileQuery }); + expect(screen.getByText('There was a problem loading this competency\'s associations. Please try again.')) + .toBeInTheDocument(); + }); + + it('renders a failed state when the default-rule-profile query errors, even if groups already loaded', () => { + renderList({ + groupsQuery: loadedEmptyGroupsQuery, + profileQuery: erroredProfileQuery, + index: buildCompetencyCriteriaGroupsIndex(emptyGroupsResponse), + }); + expect(screen.getByText('There was a problem loading this competency\'s associations. Please try again.')) + .toBeInTheDocument(); + }); + + it('renders the empty state when accessibleCourseGroups is empty, not throwing', () => { + renderList({ + groupsQuery: loadedEmptyGroupsQuery, + profileQuery: loadedProfileQuery, + index: buildCompetencyCriteriaGroupsIndex(emptyGroupsResponse), + systemDefaultProfile, + accessibleCourseGroups: [], + }); + expect(screen.getByText('No content associated.')).toBeInTheDocument(); + expect(screen.getByText('Make content selections to associate this competency with course content.')) + .toBeInTheDocument(); + }); + + it( + 'renders the empty state when every course-level group is inaccessible, even though the raw ' + + 'groups response has one - accessibleCourseGroups decides this, not the raw response\'s own count', + () => { + renderList({ + groupsQuery: loadedPopulatedGroupsQuery, + profileQuery: loadedProfileQuery, + index: buildCompetencyCriteriaGroupsIndex(populatedGroupsResponse), + systemDefaultProfile, + // The one course-level group didn't pass the outline-accessibility + // check, so it must render identically to no associations at all. + accessibleCourseGroups: [], + }); + expect(screen.getByText('No content associated.')).toBeInTheDocument(); + }, + ); + + it('renders course group sections for every accessible course-level group', async () => { + const index = buildCompetencyCriteriaGroupsIndex(populatedGroupsResponse); + renderList({ + groupsQuery: loadedPopulatedGroupsQuery, + profileQuery: loadedProfileQuery, + index, + systemDefaultProfile, + accessibleCourseGroups: index.courseGroups, + }); + expect(screen.queryByText('No content associated.')).not.toBeInTheDocument(); + // The outline isn't mocked here, so it degrades to the raw course id via + // its defensive fallback - enough to prove a real `CourseGroupSection` + // was mounted. + expect(await screen.findByText('course-v1:OrgX+CS101+2024')).toBeInTheDocument(); + }); +}); diff --git a/src/taxonomy/competency-management/criteria-groups/CourseGroupList.tsx b/src/taxonomy/competency-management/criteria-groups/CourseGroupList.tsx new file mode 100644 index 0000000000..ebf37f0550 --- /dev/null +++ b/src/taxonomy/competency-management/criteria-groups/CourseGroupList.tsx @@ -0,0 +1,64 @@ +import { useIntl } from '@edx/frontend-platform/i18n'; + +import AlertMessage from '@src/generic/alert-message'; +import { LoadingSpinner } from '@src/generic/Loading'; +import { useCompetencyAssociations } from '../CompetencyAssociationsContext'; +import courseSearchMessages from '../course-search/messages'; +import CourseGroupSection from './CourseGroupSection'; +import messages from './messages'; +// @ts-ignore +import './criteria-groups.scss'; + +/** The top-of-panel associations tree for the currently selected + * competency: loading, failed, empty, or the list of accessible course-level + * groups. Reads everything from `CompetencyAssociationsContext`. + * + * "Loading" covers both the groups query and the default-rule-profile query, + * since `effectiveRuleOf` needs the profile to resolve an unoverridden + * criterion. The empty state is decided on `accessibleCourseGroups` (already + * filtered), never the raw payload's own counts, so a course the author + * can't see renders identically to no associations at all. + */ +const CourseGroupList = () => { + const intl = useIntl(); + const { groupsQuery, profileQuery, index, accessibleCourseGroups } = useCompetencyAssociations(); + + if (groupsQuery.isLoading || profileQuery.isLoading) { + return ( +
+ +
+ ); + } + + if (groupsQuery.isError || profileQuery.isError || !index || !profileQuery.data) { + return ( + + ); + } + + if (accessibleCourseGroups.length === 0) { + return ( +
+

{intl.formatMessage(courseSearchMessages.noAssociationsMessage)}

+

{intl.formatMessage(courseSearchMessages.noAssociationsPromptMessage)}

+
+ ); + } + + return ( +
+ {accessibleCourseGroups.map((courseGroup) => ( + + ))} +
+ ); +}; + +export default CourseGroupList; diff --git a/src/taxonomy/competency-management/criteria-groups/CourseGroupSection.test.tsx b/src/taxonomy/competency-management/criteria-groups/CourseGroupSection.test.tsx new file mode 100644 index 0000000000..3aab3407cd --- /dev/null +++ b/src/taxonomy/competency-management/criteria-groups/CourseGroupSection.test.tsx @@ -0,0 +1,190 @@ +import { buildOutlineIndex } from '@src/course-outline/__mocks__'; +import { getCourseOutlineIndexApiUrl } from '@src/course-outline/data'; +import { + fireEvent, + initializeMocks, + render, + screen, +} from '@src/testUtils'; +import { buildMockCompetencyAssociationsContextValue, MockCompetencyAssociationsProvider } from '../testHelpers'; +import type { + CompetencyCriteriaGroupsResponse, + CompetencyRuleProfile, + CourseCompetencyCriteriaGroup, +} from '../data/types'; +import { buildCompetencyCriteriaGroupsIndex } from '../utils'; +import CourseGroupSection from './CourseGroupSection'; + +let axiosMock: ReturnType['axiosMock']; + +const courseKey = 'course-v1:OrgX+CS101+2024'; +const outlineApiUrl = getCourseOutlineIndexApiUrl(courseKey); + +const courseGroup: CourseCompetencyCriteriaGroup = { + id: 1, + parentId: 100, + tagId: 42, + name: 'course', + ordering: 0, + logicOperator: 'OR', + archived: false, + courseKey, +}; + +// The competency's root group (id 100, never rendered), plus two leaf +// groups (10, 11) under the same course, each with one criterion pointing +// at a real subsection in the outline fixture below. +const response: CompetencyCriteriaGroupsResponse = { + groups: [ + { + id: 100, + parentId: null, + tagId: 42, + courseKey: null, + name: 'root', + ordering: 0, + logicOperator: 'AND', + archived: false, + }, + courseGroup, + { + id: 10, + parentId: 1, + tagId: 42, + courseKey: null, + name: 'leaf', + ordering: 0, + logicOperator: 'AND', + archived: false, + }, + { + id: 11, + parentId: 1, + tagId: 42, + courseKey: null, + name: 'leaf', + ordering: 1, + logicOperator: 'AND', + archived: false, + }, + ], + criteria: [ + { + id: 101, + objectId: 'sub-1a', + groupId: 10, + ruleProfileId: 1, + ruleTypeOverride: null, + rulePayloadOverride: null, + }, + { + id: 102, + objectId: 'sub-2a', + groupId: 11, + ruleProfileId: 1, + ruleTypeOverride: null, + rulePayloadOverride: null, + }, + ], +}; + +const systemDefaultProfile: CompetencyRuleProfile = { + id: 1, + scopeType: 'system_default', + ruleType: 'grade', + rulePayload: { op: 'gte', value: 0.7, scale: 'percent' }, + archived: false, +}; + +const index = buildCompetencyCriteriaGroupsIndex(response); + +// `usageKey: undefined` mirrors the real `course_index` response, which +// never populates `usage_key` - so these tests only pass if +// `CourseGroupSection` reads `.id`, not `.usageKey`. +const outlineFixture = buildOutlineIndex({ + sections: [ + { + id: 'section-1', + displayName: 'Section 1', + children: [{ id: 'sub-1a', displayName: 'Subsection 1A', overrides: { usageKey: undefined } }], + }, + { + id: 'section-2', + displayName: 'Section 2', + children: [{ id: 'sub-2a', displayName: 'Subsection 2A', overrides: { usageKey: undefined } }], + }, + ], + overrides: { courseStructure: { displayName: 'Demo Course' } }, +}); + +const renderSection = ( + contextOverrides: Parameters[0] = {}, +) => ( + render( + + + , + ) +); + +describe('', () => { + beforeEach(() => { + ({ axiosMock } = initializeMocks()); + Element.prototype.scrollIntoView = jest.fn(); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('resolves its header and chip names from its own outline fetch', async () => { + axiosMock.onGet(outlineApiUrl).reply(200, outlineFixture); + renderSection(); + + expect(await screen.findByText('Demo Course')).toBeInTheDocument(); + expect(screen.getByText('Subsection 1A')).toBeInTheDocument(); + expect(screen.getByText('Subsection 2A')).toBeInTheDocument(); + }); + + it( + 'does not throw if its own outline fetch fails (defensive only)', + async () => { + axiosMock.onGet(outlineApiUrl).reply(403); + expect(() => renderSection()).not.toThrow(); + expect(await screen.findByText(courseKey)).toBeInTheDocument(); + }, + ); + + it('renders exactly one connector for its two bottom-tier groups (N children -> N-1 connectors)', async () => { + axiosMock.onGet(outlineApiUrl).reply(200, outlineFixture); + renderSection(); + + await screen.findByText('Demo Course'); + expect(screen.getAllByTestId('group-connector')).toHaveLength(1); + }); + + it('collapses and re-expands independently via its own chevron', async () => { + axiosMock.onGet(outlineApiUrl).reply(200, outlineFixture); + renderSection(); + + await screen.findByText('Demo Course'); + expect(screen.getByText('Subsection 1A')).toBeInTheDocument(); + + fireEvent.click(screen.getByRole('button', { name: 'Collapse' })); + expect(screen.queryByText('Subsection 1A')).not.toBeInTheDocument(); + + fireEvent.click(screen.getByRole('button', { name: 'Expand' })); + expect(screen.getByText('Subsection 1A')).toBeInTheDocument(); + }); + + it('renders nothing as focused, without throwing, when a collapsed section holds the focused group/rule box', async () => { + axiosMock.onGet(outlineApiUrl).reply(200, outlineFixture); + renderSection({ focus: { groupId: 10, ruleKey: 'grade:gte:0.7:percent' } }); + + await screen.findByText('Demo Course'); + fireEvent.click(screen.getByRole('button', { name: 'Collapse' })); + + expect(screen.queryByText('Subsection 1A')).not.toBeInTheDocument(); + expect(screen.queryByText(/By completing/)).not.toBeInTheDocument(); + }); +}); diff --git a/src/taxonomy/competency-management/criteria-groups/CourseGroupSection.tsx b/src/taxonomy/competency-management/criteria-groups/CourseGroupSection.tsx new file mode 100644 index 0000000000..0e31fd44e9 --- /dev/null +++ b/src/taxonomy/competency-management/criteria-groups/CourseGroupSection.tsx @@ -0,0 +1,96 @@ +import { Fragment, useMemo, useState } from 'react'; +import { useIntl } from '@edx/frontend-platform/i18n'; +import { Card, IconButton } from '@openedx/paragon'; +import { ExpandLess, ExpandMore } from '@openedx/paragon/icons'; + +import { useCourseOutlineIndex } from '@src/course-outline/data'; +import { useCompetencyAssociations } from '../CompetencyAssociationsContext'; +import type { CourseCompetencyCriteriaGroup } from '../data/types'; +import { bottomTierGroupsForCourse } from '../utils'; +import CriteriaGroupBox from './CriteriaGroupBox'; +import GroupConnector from './GroupConnector'; +import messages from './messages'; + +export interface CourseGroupSectionProps { + courseGroup: CourseCompetencyCriteriaGroup; +} + +/** One accessible course-level group: its own header ("From within + * **{course name}** ...") and its bottom-tier group cards, connected + * pairwise by `GroupConnector`. + * + * `CourseGroupList` only mounts this for a course already confirmed + * accessible, so `useCourseOutlineIndex` below is normally reading an + * already-resolved, already-cached success (same hook/query key + * `CourseOutlineSubtree` uses). The `isError`/raw-course-key fallback is + * defensive only (e.g. a cache eviction between the check and this render), + * not a real "inaccessible course" state. + */ +const CourseGroupSection = ({ courseGroup }: CourseGroupSectionProps) => { + const intl = useIntl(); + const { index } = useCompetencyAssociations(); + const [isCollapsed, setIsCollapsed] = useState(false); + // `refetchOnMount: false`: mirrors `CourseOutlineSubtree` - avoids a + // wasted refetch of already-cached data on every mount. + const { data, isError } = useCourseOutlineIndex(courseGroup.courseKey, { refetchOnMount: false }); + + const courseDisplayName = (!isError && data) ? data.courseStructure.displayName : courseGroup.courseKey; + + const subsectionNamesByUsageKey = useMemo(() => { + const names: Record = {}; + if (!isError && data) { + (data.courseStructure.childInfo?.children ?? []).forEach((section) => { + (section.childInfo?.children ?? []).forEach((subsection) => { + // The real `course_index` response populates `.id`, never + // `.usageKey` (see `CourseOutlineSubtree`'s `SubsectionRow`). + names[subsection.id] = subsection.displayName; + }); + }); + } + return names; + }, [data, isError]); + + // `index` is only `undefined` during `CourseGroupList`'s own loading/error + // states, before this component ever mounts. + const bottomTierGroups = index ? bottomTierGroupsForCourse(index, courseGroup.courseKey) : []; + + const toggleLabel = isCollapsed + ? intl.formatMessage(messages.expandCourseGroupButtonLabel) + : intl.formatMessage(messages.collapseCourseGroupButtonLabel); + + return ( + + {courseDisplayName}
, + })} + actions={ + setIsCollapsed((prev) => !prev)} + /> + } + /> + {!isCollapsed && ( + + {bottomTierGroups.map((group, groupIndex) => ( + + {groupIndex > 0 && } + + + ))} + + )} + + ); +}; + +export default CourseGroupSection; diff --git a/src/taxonomy/competency-management/criteria-groups/CriteriaGroupBox.test.tsx b/src/taxonomy/competency-management/criteria-groups/CriteriaGroupBox.test.tsx new file mode 100644 index 0000000000..db27ae6e81 --- /dev/null +++ b/src/taxonomy/competency-management/criteria-groups/CriteriaGroupBox.test.tsx @@ -0,0 +1,120 @@ +import { + fireEvent, + initializeMocks, + render, + screen, +} from '@src/testUtils'; +import { buildMockCompetencyAssociationsContextValue, MockCompetencyAssociationsProvider } from '../testHelpers'; +import type { + BottomTierCompetencyCriteriaGroup, + CompetencyCriteriaGroupsResponse, + CompetencyRuleProfile, +} from '../data/types'; +import { buildCompetencyCriteriaGroupsIndex } from '../utils'; +import CriteriaGroupBox from './CriteriaGroupBox'; + +const systemDefaultProfile: CompetencyRuleProfile = { + id: 1, + scopeType: 'system_default', + ruleType: 'grade', + rulePayload: { op: 'gte', value: 0.7, scale: 'percent' }, + archived: false, +}; + +const group: BottomTierCompetencyCriteriaGroup = { + id: 10, + parentId: 1, + tagId: 42, + courseKey: null, + name: 'leaf', + ordering: 0, + logicOperator: 'AND', + archived: false, +}; + +const response: CompetencyCriteriaGroupsResponse = { + groups: [group], + criteria: [ + { + id: 101, + objectId: 'block-a', + groupId: 10, + ruleProfileId: 1, + ruleTypeOverride: null, + rulePayloadOverride: null, + }, + ], +}; + +const index = buildCompetencyCriteriaGroupsIndex(response); + +const renderBox = ( + contextOverrides: Parameters[0] = {}, + subsectionNamesByUsageKey: Record = {}, +) => ( + render( + + + , + ) +); + +describe('', () => { + beforeEach(() => { + initializeMocks(); + Element.prototype.scrollIntoView = jest.fn(); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('renders the group\'s any/all label as plain text with no control', () => { + const { container } = renderBox({}, { 'block-a': 'Subsection A' }); + + // The header sentence is split across sibling text nodes (plain text + + // the any/all ``), so its full text is checked via textContent + // rather than `getByText`, which doesn't match text split across nodes. + expect(container.querySelector('.criteria-group-box__header')).toHaveTextContent( + 'By completing all of the following', + ); + // Two `role="button"` elements exist (the group's own container and the + // one rendered rule box) - neither is a `SelectMenu` trigger, since no + // `onChange` was given to the any/all control. + expect(screen.getAllByRole('button')).toHaveLength(2); + expect(screen.getByText('Subsection A')).toBeInTheDocument(); + }); + + it('calls focusGroup with its own id when the group container itself is clicked', () => { + const focusGroup = jest.fn(); + const { container } = renderBox({ focusGroup }); + + fireEvent.click(container.querySelector('.criteria-group-box__header')!); + expect(focusGroup).toHaveBeenCalledWith(10); + }); + + it('clicking a rule box inside focuses only the rule box, not also the group', () => { + const focusGroup = jest.fn(); + const focusRuleBox = jest.fn(); + renderBox({ focusGroup, focusRuleBox }); + + // The rule box is the second `role="button"` (the group container is first). + fireEvent.click(screen.getAllByRole('button')[1]); + + expect(focusRuleBox).toHaveBeenCalledTimes(1); + expect(focusGroup).not.toHaveBeenCalled(); + }); + + it('scrolls the group container into view when it is focused with no rule box focused', () => { + renderBox({ focus: { groupId: 10, ruleKey: null } }); + expect(Element.prototype.scrollIntoView).toHaveBeenCalledTimes(1); + }); + + it('does not scroll the group container when a rule box within it is the focused one', () => { + renderBox({ focus: { groupId: 10, ruleKey: 'grade:gte:0.7:percent' } }); + + // The rule box itself still scrolls (it's the innermost focused + // element), but the group container must not also call it a second time. + expect(Element.prototype.scrollIntoView).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/taxonomy/competency-management/criteria-groups/CriteriaGroupBox.tsx b/src/taxonomy/competency-management/criteria-groups/CriteriaGroupBox.tsx new file mode 100644 index 0000000000..c65f1a1907 --- /dev/null +++ b/src/taxonomy/competency-management/criteria-groups/CriteriaGroupBox.tsx @@ -0,0 +1,100 @@ +import { useEffect, useRef } from 'react'; +import { useIntl } from '@edx/frontend-platform/i18n'; +import { Card } from '@openedx/paragon'; +import classNames from 'classnames'; +import { useCompetencyAssociations } from '../CompetencyAssociationsContext'; +import type { BottomTierCompetencyCriteriaGroup } from '../data/types'; +import LogicOperatorSelect from './LogicOperatorSelect'; +import RuleBoxList from './RuleBoxList'; +import messages from './messages'; + +export interface CriteriaGroupBoxProps { + group: BottomTierCompetencyCriteriaGroup; + subsectionNamesByUsageKey: Record; +} + +/** One bottom-tier group's "By completing any/all of the following" + * bracket, plus its rule boxes. Scrolls itself into view when it's the + * focused group but no rule box within it is focused - a focused rule box, + * being the more specific/innermost target, scrolls itself instead (see + * `RuleBox`), so exactly one of the two ever scrolls for a given focus. + * + * `Card` supplies the bordered/rounded box itself; the "By completing..." + * band is a plain `
` with its own scoped styling + * (`criteria-groups.scss`), since neither `Card.Header` (its own distinct + * title/subtitle typography) nor any Paragon prop covers an inline-sentence + * band like this one. + */ +const CriteriaGroupBox = ({ group, subsectionNamesByUsageKey }: CriteriaGroupBoxProps) => { + const intl = useIntl(); + const { focus, focusGroup, index, systemDefaultProfile } = useCompetencyAssociations(); + const ref = useRef(null); + + const isFocused = focus?.groupId === group.id; + // `null` whenever this group isn't the focused one, so a `RuleBox` in a + // different group never matches it by accident. + const focusedRuleKey = isFocused ? focus!.ruleKey : null; + + useEffect(() => { + if (isFocused && focusedRuleKey === null) { + ref.current?.scrollIntoView({ block: 'nearest', behavior: 'smooth' }); + } + }, [isFocused, focusedRuleKey]); + + const handleClick: React.MouseEventHandler = (event) => { + event.stopPropagation(); + focusGroup(group.id); + }; + + const handleKeyDown: React.KeyboardEventHandler = (event) => { + if (event.key === 'Enter' || event.key === ' ') { + event.preventDefault(); + event.stopPropagation(); + focusGroup(group.id); + } + }; + + return ( + // Interactive/focus/scroll semantics live on this wrapping `
`, not + // `Card` itself: `Card`'s `ref` forwarding doesn't reliably reach a real + // DOM node (see `RuleBox.tsx`). +
+ +
+ {intl.formatMessage(messages.criteriaGroupBoxLabel, { + operator: ( + + ), + })} +
+ {index && systemDefaultProfile && ( + + + + )} +
+
+ ); +}; + +export default CriteriaGroupBox; diff --git a/src/taxonomy/competency-management/criteria-groups/CriterionChip.test.tsx b/src/taxonomy/competency-management/criteria-groups/CriterionChip.test.tsx new file mode 100644 index 0000000000..00d3c5ba17 --- /dev/null +++ b/src/taxonomy/competency-management/criteria-groups/CriterionChip.test.tsx @@ -0,0 +1,13 @@ +import { initializeMocks, render, screen } from '@src/testUtils'; +import CriterionChip from './CriterionChip'; + +describe('', () => { + beforeEach(() => { + initializeMocks(); + }); + + it('renders the given display name', () => { + render(); + expect(screen.getByText('Subsection 1')).toBeInTheDocument(); + }); +}); diff --git a/src/taxonomy/competency-management/criteria-groups/CriterionChip.tsx b/src/taxonomy/competency-management/criteria-groups/CriterionChip.tsx new file mode 100644 index 0000000000..cdbb78dc08 --- /dev/null +++ b/src/taxonomy/competency-management/criteria-groups/CriterionChip.tsx @@ -0,0 +1,18 @@ +import { Chip } from '@openedx/paragon'; + +export interface CriterionChipProps { + /** Already resolved by the caller (`CriterionChipList`) - this component + * never fetches or looks anything up itself. + */ + displayName: string; +} + +/** One chip inside a rule box: the associated subsection's display name, + * truncated rather than wrapping or overflowing for a long one. No remove + * ("x") affordance: deleting an association is out of scope here. + */ +const CriterionChip = ({ displayName }: CriterionChipProps) => ( + {displayName} +); + +export default CriterionChip; diff --git a/src/taxonomy/competency-management/criteria-groups/CriterionChipList.test.tsx b/src/taxonomy/competency-management/criteria-groups/CriterionChipList.test.tsx new file mode 100644 index 0000000000..0b53770980 --- /dev/null +++ b/src/taxonomy/competency-management/criteria-groups/CriterionChipList.test.tsx @@ -0,0 +1,55 @@ +import { initializeMocks, render, screen } from '@src/testUtils'; +import type { CompetencyCriterion } from '../data/types'; +import CriterionChipList from './CriterionChipList'; + +const buildCriterion = (overrides: Partial = {}): CompetencyCriterion => ({ + id: 1, + objectId: 'block-a', + groupId: 10, + ruleProfileId: 1, + ruleTypeOverride: null, + rulePayloadOverride: null, + ...overrides, +}); + +describe('', () => { + beforeEach(() => { + initializeMocks(); + }); + + it('renders one chip per criterion, using the resolved subsection name', () => { + const criteria = [ + buildCriterion({ id: 1, objectId: 'block-a' }), + buildCriterion({ id: 2, objectId: 'block-b' }), + ]; + render( + , + ); + + expect(screen.getByText('Subsection A')).toBeInTheDocument(); + expect(screen.getByText('Subsection B')).toBeInTheDocument(); + }); + + it('falls back to a neutral label when a criterion\'s objectId isn\'t in the resolved names', () => { + render( + , + ); + + expect(screen.getByText('Content unavailable')).toBeInTheDocument(); + }); + + it('renders nothing for an empty criteria list', () => { + const { container } = render( + , + ); + // `container` isn't empty outright - the test harness's own provider + // wrappers still render - so check no chip list markup was produced. + expect(container.querySelector('.criterion-chip-list')).not.toBeInTheDocument(); + }); +}); diff --git a/src/taxonomy/competency-management/criteria-groups/CriterionChipList.tsx b/src/taxonomy/competency-management/criteria-groups/CriterionChipList.tsx new file mode 100644 index 0000000000..3cbae6d8e9 --- /dev/null +++ b/src/taxonomy/competency-management/criteria-groups/CriterionChipList.tsx @@ -0,0 +1,42 @@ +import { useIntl } from '@edx/frontend-platform/i18n'; +import type { CompetencyCriterion } from '../data/types'; +import CriterionChip from './CriterionChip'; +import messages from './messages'; + +export interface CriterionChipListProps { + criteria: CompetencyCriterion[]; + /** Subsection display names, keyed by usage key - resolved once per + * course by `CourseGroupSection` from its own outline fetch. A criterion + * whose `objectId` has no entry here (deleted content or search-index + * lag) falls back to a neutral label. + */ + subsectionNamesByUsageKey: Record; +} + +/** The chips inside one rule box, one per criterion. + * + * Empty (renders nothing) when given no criteria - not expected for a + * persisted box per ADR 0002's "no empty groups" rule, but a freshly + * created, not-yet-saved box can start with none. + */ +const CriterionChipList = ({ criteria, subsectionNamesByUsageKey }: CriterionChipListProps) => { + const intl = useIntl(); + + if (criteria.length === 0) { + return null; + } + + return ( +
+ {criteria.map((criterion) => ( + + ))} +
+ ); +}; + +export default CriterionChipList; diff --git a/src/taxonomy/competency-management/criteria-groups/GroupConnector.test.tsx b/src/taxonomy/competency-management/criteria-groups/GroupConnector.test.tsx new file mode 100644 index 0000000000..342d38c992 --- /dev/null +++ b/src/taxonomy/competency-management/criteria-groups/GroupConnector.test.tsx @@ -0,0 +1,19 @@ +import { initializeMocks, render, screen } from '@src/testUtils'; +import GroupConnector from './GroupConnector'; + +describe('', () => { + beforeEach(() => { + initializeMocks(); + }); + + it('renders \'And\' as plain text with no control for an AND operator', () => { + render(); + expect(screen.getByText('And')).toBeInTheDocument(); + expect(screen.queryByRole('button')).not.toBeInTheDocument(); + }); + + it('renders \'Or\' as plain text for an OR operator', () => { + render(); + expect(screen.getByText('Or')).toBeInTheDocument(); + }); +}); diff --git a/src/taxonomy/competency-management/criteria-groups/GroupConnector.tsx b/src/taxonomy/competency-management/criteria-groups/GroupConnector.tsx new file mode 100644 index 0000000000..33460a4a72 --- /dev/null +++ b/src/taxonomy/competency-management/criteria-groups/GroupConnector.tsx @@ -0,0 +1,36 @@ +import { useIntl } from '@edx/frontend-platform/i18n'; +import type { CompetencyGroupLogicOperator } from '../data/types'; +import LogicOperatorSelect from './LogicOperatorSelect'; +import messages from './messages'; + +export interface GroupConnectorProps { + /** The shared course-level group's own logic operator - every connector + * between that course's sibling bottom-tier groups reads the same value, + * not one of its own. + */ + logicOperator: CompetencyGroupLogicOperator; +} + +/** The Or/And connector rendered between two adjacent sibling bottom-tier + * groups within the same course. N sibling groups render N-1 of these. + */ +const GroupConnector = ({ logicOperator }: GroupConnectorProps) => { + const intl = useIntl(); + + return ( +
+
+ ); +}; + +export default GroupConnector; diff --git a/src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.test.tsx b/src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.test.tsx new file mode 100644 index 0000000000..6a280b1e85 --- /dev/null +++ b/src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.test.tsx @@ -0,0 +1,37 @@ +import { + fireEvent, + initializeMocks, + render, + screen, +} from '@src/testUtils'; +import LogicOperatorSelect from './LogicOperatorSelect'; + +const labels = { and: 'all', or: 'any' }; + +describe('', () => { + beforeEach(() => { + initializeMocks(); + }); + + it('renders plain, non-interactive text when no onChange is given', () => { + render(); + + expect(screen.getByText('all')).toBeInTheDocument(); + expect(screen.queryByRole('button')).not.toBeInTheDocument(); + }); + + it('renders the \'OR\' label\'s text when value is \'OR\'', () => { + render(); + expect(screen.getByText('any')).toBeInTheDocument(); + }); + + it('renders an interactive control and calls onChange when given', () => { + const onChange = jest.fn(); + render(); + + fireEvent.click(screen.getByRole('button')); + fireEvent.click(screen.getByText('any')); + + expect(onChange).toHaveBeenCalledWith('OR'); + }); +}); diff --git a/src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.tsx b/src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.tsx new file mode 100644 index 0000000000..868a56db0a --- /dev/null +++ b/src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.tsx @@ -0,0 +1,51 @@ +import { MenuItem, SelectMenu } from '@openedx/paragon'; +import type { CompetencyGroupLogicOperator } from '../data/types'; + +export interface LogicOperatorSelectLabels { + and: string; + or: string; +} + +export interface LogicOperatorSelectProps { + value: CompetencyGroupLogicOperator; + /** Display text for each of the two states - "all"/"any" on a bottom-tier + * group's own bracket, "And"/"Or" on a connector between sibling groups. + * Supplied by the caller rather than looked up here, since the same + * control serves both contexts with different wording. + */ + labels: LogicOperatorSelectLabels; + /** Omitted where only the value is read: renders plain, non-interactive + * text in that case. Passing it renders the interactive `SelectMenu`. + */ + onChange?: (value: CompetencyGroupLogicOperator) => void; + className?: string; +} + +/** Shared any/all (bottom-tier group) or Or/And (connector) control for how + * a group's children combine. Read-only (plain text) unless `onChange` is + * given. + */ +const LogicOperatorSelect = ({ + value, + labels, + onChange, + className, +}: LogicOperatorSelectProps) => { + const label = value === 'AND' ? labels.and : labels.or; + + if (!onChange) { + return {label}; + } + + return ( + + onChange('AND')}>{labels.and} + onChange('OR')}>{labels.or} + + ); +}; + +export default LogicOperatorSelect; diff --git a/src/taxonomy/competency-management/criteria-groups/RuleBox.test.tsx b/src/taxonomy/competency-management/criteria-groups/RuleBox.test.tsx new file mode 100644 index 0000000000..c0c0237c94 --- /dev/null +++ b/src/taxonomy/competency-management/criteria-groups/RuleBox.test.tsx @@ -0,0 +1,77 @@ +import { + fireEvent, + initializeMocks, + render, + screen, +} from '@src/testUtils'; +import { buildMockCompetencyAssociationsContextValue, MockCompetencyAssociationsProvider } from '../testHelpers'; +import type { CompetencyCriterion } from '../data/types'; +import RuleBox from './RuleBox'; + +const criteria: CompetencyCriterion[] = [ + { + id: 1, + objectId: 'block-a', + groupId: 10, + ruleProfileId: null, + ruleTypeOverride: 'grade', + rulePayloadOverride: { op: 'gte', value: 0.7, scale: 'percent' }, + }, +]; + +const rule = { ruleType: 'grade', rulePayload: { op: 'gte' as const, value: 0.7, scale: 'percent' as const } }; + +const renderRuleBox = (contextOverrides: Parameters[0] = {}) => ( + render( + + + , + ) +); + +describe('', () => { + beforeEach(() => { + initializeMocks(); + Element.prototype.scrollIntoView = jest.fn(); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('renders the given rule\'s score threshold and its chips', () => { + renderRuleBox(); + + expect(screen.getByText('With a score of 70% or higher')).toBeInTheDocument(); + expect(screen.getByText('Subsection A')).toBeInTheDocument(); + }); + + it('calls focusRuleBox with its own groupId/ruleKey when clicked', () => { + const focusRuleBox = jest.fn(); + renderRuleBox({ focusRuleBox }); + + fireEvent.click(screen.getByRole('button')); + expect(focusRuleBox).toHaveBeenCalledWith(10, 'grade:gte:0.7:percent'); + }); + + it('does not call scrollIntoView when not the focused box', () => { + renderRuleBox({ focus: null }); + expect(Element.prototype.scrollIntoView).not.toHaveBeenCalled(); + }); + + it('does not call scrollIntoView when a different box is focused', () => { + renderRuleBox({ focus: { groupId: 10, ruleKey: 'grade:lte:0.5:percent' } }); + expect(Element.prototype.scrollIntoView).not.toHaveBeenCalled(); + }); + + it('scrolls itself into view when it is the focused box', () => { + renderRuleBox({ focus: { groupId: 10, ruleKey: 'grade:gte:0.7:percent' } }); + expect(Element.prototype.scrollIntoView).toHaveBeenCalledWith({ block: 'nearest', behavior: 'smooth' }); + }); +}); diff --git a/src/taxonomy/competency-management/criteria-groups/RuleBox.tsx b/src/taxonomy/competency-management/criteria-groups/RuleBox.tsx new file mode 100644 index 0000000000..5b66570c21 --- /dev/null +++ b/src/taxonomy/competency-management/criteria-groups/RuleBox.tsx @@ -0,0 +1,85 @@ +import { useEffect, useRef } from 'react'; +import { Card } from '@openedx/paragon'; +import classNames from 'classnames'; +import { useCompetencyAssociations } from '../CompetencyAssociationsContext'; +import type { CompetencyCriterion, EffectiveRule } from '../data/types'; +import CriterionChipList from './CriterionChipList'; +import ScoreThresholdField from './ScoreThresholdField'; + +export interface RuleBoxProps { + /** The group this box belongs to, and this box's own rule key - together + * this box's identity for focus comparisons against + * `CompetencyAssociationsContext`'s `focus`. + */ + groupId: number; + ruleKey: string; + /** The rule this box displays, given as a prop rather than read from a + * criterion - so this same component can later serve a not-yet-saved box + * that has no criterion of its own yet. + */ + rule: EffectiveRule; + criteria: CompetencyCriterion[]; + subsectionNamesByUsageKey: Record; +} + +/** One rule box: the rule it's given, its chips, and focus/click behavior. + * Scrolls itself into view (`block: 'nearest'`) when it becomes the focused + * box - the innermost focused element, so its containing `CriteriaGroupBox` + * does not also scroll itself in that case. + */ +const RuleBox = ({ + groupId, + ruleKey, + rule, + criteria, + subsectionNamesByUsageKey, +}: RuleBoxProps) => { + const { focus, focusRuleBox } = useCompetencyAssociations(); + const isFocused = focus?.groupId === groupId && focus?.ruleKey === ruleKey; + const ref = useRef(null); + + useEffect(() => { + if (isFocused) { + ref.current?.scrollIntoView({ block: 'nearest', behavior: 'smooth' }); + } + }, [isFocused]); + + const handleClick: React.MouseEventHandler = (event) => { + // Stops the click from also bubbling into the containing + // `CriteriaGroupBox`'s own click handler, which would otherwise + // overwrite this box's more specific focus with the group's broader one. + event.stopPropagation(); + focusRuleBox(groupId, ruleKey); + }; + + const handleKeyDown: React.KeyboardEventHandler = (event) => { + if (event.key === 'Enter' || event.key === ' ') { + event.preventDefault(); + event.stopPropagation(); + focusRuleBox(groupId, ruleKey); + } + }; + + return ( + // Interactive/focus/scroll semantics live on this wrapping `
`, not + // `Card` itself: `Card`'s `ref` forwarding doesn't reliably reach a real + // DOM node (confirmed directly - `ref.current` had no `scrollIntoView`). +
+ + + + + + +
+ ); +}; + +export default RuleBox; diff --git a/src/taxonomy/competency-management/criteria-groups/RuleBoxList.test.tsx b/src/taxonomy/competency-management/criteria-groups/RuleBoxList.test.tsx new file mode 100644 index 0000000000..5ce9cb0a52 --- /dev/null +++ b/src/taxonomy/competency-management/criteria-groups/RuleBoxList.test.tsx @@ -0,0 +1,135 @@ +import { initializeMocks, render, screen } from '@src/testUtils'; +import { MockCompetencyAssociationsProvider } from '../testHelpers'; +import type { CompetencyCriteriaGroupsResponse, CompetencyRuleProfile } from '../data/types'; +import { buildCompetencyCriteriaGroupsIndex } from '../utils'; +import RuleBoxList from './RuleBoxList'; + +const systemDefaultProfile: CompetencyRuleProfile = { + id: 1, + scopeType: 'system_default', + ruleType: 'grade', + rulePayload: { op: 'gte', value: 0.7, scale: 'percent' }, + archived: false, +}; + +// Group 10 holds two criteria sharing the default profile's rule (101, 102) +// and one with a different override (103). +const response: CompetencyCriteriaGroupsResponse = { + groups: [ + { + id: 10, + parentId: null, + tagId: 42, + courseKey: null, + name: 'leaf', + ordering: 0, + logicOperator: 'AND', + archived: false, + }, + ], + criteria: [ + { + id: 101, + objectId: 'block-a', + groupId: 10, + ruleProfileId: 1, + ruleTypeOverride: null, + rulePayloadOverride: null, + }, + { + id: 102, + objectId: 'block-b', + groupId: 10, + ruleProfileId: 1, + ruleTypeOverride: null, + rulePayloadOverride: null, + }, + { + id: 103, + objectId: 'block-c', + groupId: 10, + ruleProfileId: null, + ruleTypeOverride: 'grade', + rulePayloadOverride: { op: 'lte', value: 0.9, scale: 'percent' }, + }, + ], +}; + +describe('', () => { + beforeEach(() => { + initializeMocks(); + Element.prototype.scrollIntoView = jest.fn(); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('renders two criteria sharing a rule as one box, and a different rule as a second box', () => { + const index = buildCompetencyCriteriaGroupsIndex(response); + render( + + + , + ); + + const boxes = screen.getAllByRole('button'); + expect(boxes).toHaveLength(2); + // Box 1 (min criterion id 101) holds both Subsection A and B. + expect(boxes[0]).toHaveTextContent('Subsection A'); + expect(boxes[0]).toHaveTextContent('Subsection B'); + expect(boxes[0]).toHaveTextContent('With a score of 70% or higher'); + // Box 2 (min criterion id 103) holds only Subsection C, with its own rule. + expect(boxes[1]).toHaveTextContent('Subsection C'); + expect(boxes[1]).toHaveTextContent('With a score of 90% or lower'); + }); + + it('marks only the box matching the context focus as focused', () => { + const index = buildCompetencyCriteriaGroupsIndex(response); + render( + + + , + ); + + const boxes = screen.getAllByRole('button'); + expect(boxes[0].className).not.toContain('rule-box--focused'); + expect(boxes[1].className).toContain('rule-box--focused'); + }); + + it('renders no box as focused, without throwing, when the focused key matches none rendered', () => { + const index = buildCompetencyCriteriaGroupsIndex(response); + expect(() => + render( + + + , + ) + ).not.toThrow(); + + screen.getAllByRole('button').forEach((box) => { + expect(box.className).not.toContain('rule-box--focused'); + }); + }); +}); diff --git a/src/taxonomy/competency-management/criteria-groups/RuleBoxList.tsx b/src/taxonomy/competency-management/criteria-groups/RuleBoxList.tsx new file mode 100644 index 0000000000..a92c6667e5 --- /dev/null +++ b/src/taxonomy/competency-management/criteria-groups/RuleBoxList.tsx @@ -0,0 +1,43 @@ +import type { CompetencyCriteriaGroupsIndex } from '../utils'; +import { ruleBoxesForGroup } from '../utils'; +import type { CompetencyRuleProfile } from '../data/types'; +import RuleBox from './RuleBox'; + +export interface RuleBoxListProps { + groupId: number; + index: CompetencyCriteriaGroupsIndex; + systemDefaultProfile: CompetencyRuleProfile; + subsectionNamesByUsageKey: Record; +} + +/** Derives one bottom-tier group's rule boxes via `ruleBoxesForGroup` and + * renders one `RuleBox` per box. Each `RuleBox` reads its own focus state + * from `CompetencyAssociationsContext` directly (given `groupId` and its + * own box key to compare against), so no focus state is threaded through + * here. + */ +const RuleBoxList = ({ + groupId, + index, + systemDefaultProfile, + subsectionNamesByUsageKey, +}: RuleBoxListProps) => { + const boxes = ruleBoxesForGroup(groupId, index, systemDefaultProfile); + + return ( +
+ {boxes.map((box) => ( + + ))} +
+ ); +}; + +export default RuleBoxList; diff --git a/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.test.tsx b/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.test.tsx new file mode 100644 index 0000000000..84c4dd845c --- /dev/null +++ b/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.test.tsx @@ -0,0 +1,32 @@ +import { initializeMocks, render, screen } from '@src/testUtils'; +import ScoreThresholdField from './ScoreThresholdField'; + +describe('', () => { + beforeEach(() => { + initializeMocks(); + }); + + it('renders "or higher" for a gte rule, as plain text with no control', () => { + render(); + + expect(screen.getByText('With a score of 70% or higher')).toBeInTheDocument(); + expect(screen.queryByRole('button')).not.toBeInTheDocument(); + expect(screen.queryByRole('textbox')).not.toBeInTheDocument(); + }); + + it('renders "or lower" for a lte rule', () => { + render(); + expect(screen.getByText('With a score of 50% or lower')).toBeInTheDocument(); + }); + + it('renders no suffix for an eq rule', () => { + render(); + expect(screen.getByText('With a score of 100%')).toBeInTheDocument(); + expect(screen.queryByText(/or higher|or lower/)).not.toBeInTheDocument(); + }); + + it('rounds a fractional value to the nearest whole percent', () => { + render(); + expect(screen.getByText('With a score of 67% or higher')).toBeInTheDocument(); + }); +}); diff --git a/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.tsx b/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.tsx new file mode 100644 index 0000000000..089eb5f0df --- /dev/null +++ b/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.tsx @@ -0,0 +1,49 @@ +import { useIntl } from '@edx/frontend-platform/i18n'; +import type { MessageDescriptor } from 'react-intl'; +import type { GradeRulePayload } from '../data/types'; +import messages from './messages'; + +export interface ScoreThresholdFieldProps { + rulePayload: GradeRulePayload; + /** Not exercised yet (nothing here is editable): reserved so a later + * editing ticket can wire in a change handler without this component + * needing to be reshaped. + */ + onChange?: (rulePayload: GradeRulePayload) => void; + /** Same reservation as `onChange`, mirroring `EditableCell`'s prop of the + * same name (`src/taxonomy/tree-table/EditableCell.tsx`). + */ + getInlineValidationMessage?: (value: string) => string; +} + +/** Which suffix follows the percentage for each comparison operator - + * `null` for `eq`, which reads as an exact score with no suffix. + */ +const SUFFIX_MESSAGE_BY_OP: Record = { + gte: messages.scoreThresholdOrHigherSuffix, + lte: messages.scoreThresholdOrLowerSuffix, + eq: null, +}; + +/** Closed-state label for a rule's score threshold: "With a score of X% + * or higher"/"or lower"/(no suffix, for an exact match). Read-only - see + * `onChange`/`getInlineValidationMessage` above for why the props still + * exist. + */ +const ScoreThresholdField = ({ rulePayload }: ScoreThresholdFieldProps) => { + const intl = useIntl(); + // `rulePayload.value` is a 0.0-1.0 fraction (ADR 0002), not a 0-100 + // percentage - same conversion `src/grading-settings/credit-section/ + // index.jsx` uses for the same kind of field. + const percent = Math.round(rulePayload.value * 100); + const suffixMessage = SUFFIX_MESSAGE_BY_OP[rulePayload.op]; + + return ( + + {intl.formatMessage(messages.scoreThresholdLabel, { percent })} + {suffixMessage && ` ${intl.formatMessage(suffixMessage)}`} + + ); +}; + +export default ScoreThresholdField; diff --git a/src/taxonomy/competency-management/criteria-groups/criteria-groups.scss b/src/taxonomy/competency-management/criteria-groups/criteria-groups.scss new file mode 100644 index 0000000000..aa32f68540 --- /dev/null +++ b/src/taxonomy/competency-management/criteria-groups/criteria-groups.scss @@ -0,0 +1,111 @@ +// Scoped under `.course-group-list` (this subtree's own root - see +// `CourseGroupList.tsx`), mirroring `../CompetencyTree.scss` and +// `../course-search/CourseSearchBrowse.scss`'s own single-root-class scoping. +// +// `Card`/`Card.Body` (Paragon) supply every box's own border/radius/fill. +// What's added here is what Paragon has no prop for: the inline-sentence +// layout of "By completing any of the following" and "With a score of X% +// or higher", the connector line between sibling bottom-tier groups, and +// chip truncation. +.course-group-list { + // Band fill and connector line/card-border color, read off the design + // screenshot; not an existing Paragon or app-wide token. + $sentence-band-bg: #F6F6F5; + $connector-line-color: #DDDDDD; + // Reuses this feature's own established accent blue (`../CompetencyTree.scss`'s + // `.badge-info` retint and `../course-search/CourseSearchBrowse.scss`'s + // section-label text) rather than introducing a third, similar blue. + $focused-accent-color: #006DAA; + + // Vertical gap between adjacent course-level group cards. + > * + * { + margin-top: 16px; + } + + .course-group-section { + &__body > * + * { + margin-top: 12px; + } + } + + .criteria-group-box { + cursor: pointer; + + &--focused { + border-color: $focused-accent-color; + box-shadow: 0 0 0 1px $focused-accent-color; + } + + &__header { + background-color: $sentence-band-bg; + // Matches `Card`'s own top corners, so the band's top edge reads as + // part of the same rounded box rather than a squared-off strip + // sitting inside it. + border-radius: var(--pgn-size-card-border-radius) var(--pgn-size-card-border-radius) 0 0; + padding: 10px 12px; + } + + // The any/all word itself - bold, matching the screenshot - passed as + // `LogicOperatorSelect`'s own `className` from `CriteriaGroupBox.tsx`. + &__header-operator { + font-weight: 700; + } + + &__rules { + // `Card.Body`'s own default padding already insets these from the + // card's edges; only the gap between adjacent rule boxes is added + // here. + .rule-box-list > * + * { + margin-top: 10px; + } + } + } + + .rule-box { + cursor: pointer; + + &--focused { + border-color: $focused-accent-color; + box-shadow: 0 0 0 1px $focused-accent-color; + } + + &__body > * + * { + margin-top: 8px; + } + } + + // Each chip's own truncation - a long subsection name shows an ellipsis + // rather than wrapping the chip onto a second line or overflowing its + // row. `Chip`'s own label is a nested `
` (`.pgn__chip__label`, a + // Paragon-internal class - confirmed via `Chip`'s own source), so the + // truncation properties land there, not on the chip's own root element. + .criterion-chip { + max-width: 160px; + + .pgn__chip__label { + overflow: hidden; + white-space: nowrap; + text-overflow: ellipsis; + } + } + + // A thin line - label - line row between two adjacent sibling bottom-tier + // groups within the same course-level group (see `GroupConnector.tsx`). + .group-connector { + display: flex; + align-items: center; + gap: 8px; + + &__line { + flex: 1; + height: 1px; + background-color: $connector-line-color; + } + + &__label { + font-size: var(--pgn-typography-font-size-sm); + font-weight: 700; + white-space: nowrap; + } + } +} diff --git a/src/taxonomy/competency-management/criteria-groups/index.ts b/src/taxonomy/competency-management/criteria-groups/index.ts new file mode 100644 index 0000000000..d24391d784 --- /dev/null +++ b/src/taxonomy/competency-management/criteria-groups/index.ts @@ -0,0 +1 @@ +export { default as CourseGroupList } from './CourseGroupList'; diff --git a/src/taxonomy/competency-management/criteria-groups/messages.ts b/src/taxonomy/competency-management/criteria-groups/messages.ts new file mode 100644 index 0000000000..ff82642eab --- /dev/null +++ b/src/taxonomy/competency-management/criteria-groups/messages.ts @@ -0,0 +1,84 @@ +import { defineMessages } from '@edx/frontend-platform/i18n'; + +const messages = defineMessages({ + logicOperatorAnyLabel: { + id: 'course-authoring.competency-management.criteria-groups.logic-operator-any-label', + defaultMessage: 'any', + description: 'Label for a bottom-tier group\'s "or" logic operator, shown inline in ' + + '"By completing any of the following".', + }, + logicOperatorAllLabel: { + id: 'course-authoring.competency-management.criteria-groups.logic-operator-all-label', + defaultMessage: 'all', + description: 'Label for a bottom-tier group\'s "and" logic operator, shown inline in ' + + '"By completing all of the following".', + }, + logicOperatorOrLabel: { + id: 'course-authoring.competency-management.criteria-groups.logic-operator-or-label', + defaultMessage: 'Or', + description: 'Label for a course-level group\'s "or" logic operator, shown on the connector between ' + + 'two adjacent sibling bottom-tier groups.', + }, + logicOperatorAndLabel: { + id: 'course-authoring.competency-management.criteria-groups.logic-operator-and-label', + defaultMessage: 'And', + description: 'Label for a course-level group\'s "and" logic operator, shown on the connector between ' + + 'two adjacent sibling bottom-tier groups.', + }, + criteriaGroupBoxLabel: { + id: 'course-authoring.competency-management.criteria-groups.criteria-group-box-label', + defaultMessage: 'By completing {operator} of the following', + description: 'Header sentence on a bottom-tier group\'s bracket, naming how its rule boxes combine. ' + + '"{operator}" is rendered as the any/all control, not plain text.', + }, + scoreThresholdLabel: { + id: 'course-authoring.competency-management.criteria-groups.score-threshold-label', + defaultMessage: 'With a score of {percent}%', + description: 'Closed-state label for a rule box\'s score threshold, before the or-higher/or-lower suffix. ' + + '"{percent}" is a whole-number percentage (0-100), converted from the rule\'s 0.0-1.0 fraction.', + }, + scoreThresholdOrHigherSuffix: { + id: 'course-authoring.competency-management.criteria-groups.score-threshold-or-higher-suffix', + defaultMessage: 'or higher', + description: 'Suffix appended to the score threshold label when the rule\'s comparison is "at least" ' + + '(op: gte).', + }, + scoreThresholdOrLowerSuffix: { + id: 'course-authoring.competency-management.criteria-groups.score-threshold-or-lower-suffix', + defaultMessage: 'or lower', + description: 'Suffix appended to the score threshold label when the rule\'s comparison is "at most" ' + + '(op: lte). No suffix is appended for an exact-match rule (op: eq).', + }, + unknownSubsectionLabel: { + id: 'course-authoring.competency-management.criteria-groups.unknown-subsection-label', + defaultMessage: 'Content unavailable', + description: 'Fallback chip text for a criterion whose associated subsection isn\'t found in its ' + + 'course\'s outline (deleted content or search-index lag, not a permission boundary).', + }, + fromWithinCourseLabel: { + id: 'course-authoring.competency-management.criteria-groups.from-within-course-label', + defaultMessage: 'From within {courseName} ...', + description: 'Header sentence on a course-level group\'s card, naming the course its bottom-tier groups ' + + 'belong to. "{courseName}" is rendered in bold, not plain text.', + }, + expandCourseGroupButtonLabel: { + id: 'course-authoring.competency-management.criteria-groups.expand-course-group.button-label', + defaultMessage: 'Expand', + description: 'Accessible label for the disclosure control that expands a collapsed course-level ' + + 'associations group.', + }, + collapseCourseGroupButtonLabel: { + id: 'course-authoring.competency-management.criteria-groups.collapse-course-group.button-label', + defaultMessage: 'Collapse', + description: 'Accessible label for the disclosure control that collapses an expanded course-level ' + + 'associations group.', + }, + courseGroupListErrorMessage: { + id: 'course-authoring.competency-management.criteria-groups.course-group-list-error-message', + defaultMessage: 'There was a problem loading this competency\'s associations. Please try again.', + description: 'Inline error message shown when loading a competency\'s existing criteria groups, or the ' + + 'system default rule profile, fails.', + }, +}); + +export default messages; From c7a8ff25589eb688e86d5be3dd4545effd5c15f0 Mon Sep 17 00:00:00 2001 From: AShatsila <319486855+AShatsila@users.noreply.github.com> Date: Tue, 6 Oct 2026 13:17:09 +0400 Subject: [PATCH 02/12] fix: drop inert subsection button and nested group/rule-box controls --- .../CourseOutlineSubtree.test.tsx | 23 ++++++++++ .../course-search/CourseOutlineSubtree.tsx | 42 +++++++++++++------ .../course-search/CourseSearchBrowse.scss | 3 +- .../criteria-groups/CriteriaGroupBox.test.tsx | 36 ++++++++++++++-- .../criteria-groups/CriteriaGroupBox.tsx | 30 +++++++------ .../criteria-groups/RuleBox.tsx | 8 +--- .../criteria-groups/criteria-groups.scss | 8 +++- 7 files changed, 112 insertions(+), 38 deletions(-) diff --git a/src/taxonomy/competency-management/course-search/CourseOutlineSubtree.test.tsx b/src/taxonomy/competency-management/course-search/CourseOutlineSubtree.test.tsx index 3bc15b3643..5c31e00e2a 100644 --- a/src/taxonomy/competency-management/course-search/CourseOutlineSubtree.test.tsx +++ b/src/taxonomy/competency-management/course-search/CourseOutlineSubtree.test.tsx @@ -290,4 +290,27 @@ describe('', () => { expect(within(row).queryByText('Competency ID:', { exact: false })).not.toBeInTheDocument(); }); + + it( + 'renders a graded subsection of a non-writable course with no button role, still showing the ' + + 'already-associated marking', + async () => { + const user = userEvent.setup(); + axiosMock.onGet(outlineApiUrl).reply(200, mixedOutline); + renderSubtree({ + canEditCourse: () => false, + associatedObjectIds: new Set(['sub-1a']), + competencyExternalId: 'EXT-042', + }); + + await user.click((await screen.findAllByRole('button', { name: 'Expand' }))[0]); + + const row = screen.getByText('Subsection 1A (graded)').closest('.course-search-browse__subsection')!; + expect(row.tagName).toBe('DIV'); + expect(row).toHaveAttribute('data-associated', 'true'); + expect(within(row as HTMLElement).queryByRole('button')).not.toBeInTheDocument(); + expect(screen.queryByRole('button', { name: /Subsection 1A/ })).not.toBeInTheDocument(); + expect(within(row as HTMLElement).getByText('Competency ID: EXT-042')).toBeInTheDocument(); + }, + ); }); diff --git a/src/taxonomy/competency-management/course-search/CourseOutlineSubtree.tsx b/src/taxonomy/competency-management/course-search/CourseOutlineSubtree.tsx index 550358f3cd..3e93fde04f 100644 --- a/src/taxonomy/competency-management/course-search/CourseOutlineSubtree.tsx +++ b/src/taxonomy/competency-management/course-search/CourseOutlineSubtree.tsx @@ -34,10 +34,11 @@ interface SubsectionRowProps { /** * One row for a single graded subsection. * - * Clicking calls `associateSubsection` unless `canEditCourse(courseId)` is - * false, in which case the row has no click behavior. The already-associated - * badge and marker icon show regardless of `canEditCourse`, so a - * view-only course still shows what's associated. Re-clicking an + * Renders a button that calls `associateSubsection` when clicked, unless + * `canEditCourse(courseId)` is false, in which case it renders the same + * content as a non-interactive `div`. The already-associated badge and + * marker icon show regardless of `canEditCourse`, so a view-only course + * still shows what's associated. Re-clicking an * already-associated row hits the duplicate guard inside * `associateSubsection` (an informational toast), not a failed request. */ @@ -54,15 +55,9 @@ const SubsectionRow = ({ subsection, courseId }: SubsectionRowProps) => { const isAssociated = associatedObjectIds.has(subsection.id); const canSelect = canEditCourse(courseId); - return ( - ); }; diff --git a/src/taxonomy/competency-management/course-search/CourseSearchBrowse.scss b/src/taxonomy/competency-management/course-search/CourseSearchBrowse.scss index 6817b8e56b..7d5b31273d 100644 --- a/src/taxonomy/competency-management/course-search/CourseSearchBrowse.scss +++ b/src/taxonomy/competency-management/course-search/CourseSearchBrowse.scss @@ -232,7 +232,7 @@ // One level deeper than `&__group`, further indented, with the lightest // border color continuing the darkest-to-lightest progression. Font-size/ // weight read off Figma, overriding the tertiary `Button`'s own 18px/ - // normal default. + // normal default. A non-selectable row is a plain `div` with the same box. &__subsection { // This is a Paragon ` + +
); }; @@ -650,4 +665,114 @@ describe('CompetencyAssociationsProvider', () => { }); }); }); + + describe('updateGroupOperator', () => { + it('sends the uppercased operator, with no toast, on success', async () => { + axiosMock.onGet(groupsUrl).reply(200, singleGroupResponse); + axiosMock.onGet(profileUrl).reply(200, profileResponse); + axiosMock.onGet(getCourseOutlineIndexApiUrl(courseA)).reply(200, outlineFixture); + axiosMock.onPatch(updateGroupOperatorUrl).reply(200, { + id: 10, + parent_id: 1, + tag_id: tagId, + course_key: null, + name: 'leaf', + ordering: 0, + logic_operator: 'OR', + archived: false, + }); + renderProvider(); + await waitFor(() => expect(screen.getByTestId('groups-status')).toHaveTextContent('groups-success')); + + fireEvent.click(screen.getByText('update-group-operator')); + + await waitFor(() => expect(axiosMock.history.patch).toHaveLength(1)); + expect(axiosMock.history.patch[0].url).toEqual(updateGroupOperatorUrl); + expect(JSON.parse(axiosMock.history.patch[0].data)).toEqual({ logic_operator: 'OR' }); + expect(mockShowToast).not.toHaveBeenCalled(); + }); + + it('shows a failure toast on rejection', async () => { + axiosMock.onGet(groupsUrl).reply(200, singleGroupResponse); + axiosMock.onGet(profileUrl).reply(200, profileResponse); + axiosMock.onGet(getCourseOutlineIndexApiUrl(courseA)).reply(200, outlineFixture); + axiosMock.onPatch(updateGroupOperatorUrl).reply(500); + renderProvider(); + await waitFor(() => expect(screen.getByTestId('groups-status')).toHaveTextContent('groups-success')); + + fireEvent.click(screen.getByText('update-group-operator')); + + await waitFor(() => { + expect(mockShowToast).toHaveBeenCalledWith( + 'There was a problem updating this group\'s logic. Please try again.', + ); + }); + }); + }); + + describe('updateRuleScore', () => { + it('sends the batch payload and repairs focus from the response on success', async () => { + axiosMock.onGet(groupsUrl).reply(200, singleGroupResponse); + axiosMock.onGet(profileUrl).reply(200, profileResponse); + axiosMock.onGet(getCourseOutlineIndexApiUrl(courseA)).reply(200, outlineFixture); + axiosMock.onPatch(updateRuleScoreUrl).reply(200, [ + { + id: 900, + object_id: 'existing-sub', + group_id: 10, + rule_profile_id: null, + rule_type_override: 'grade', + rule_payload_override: { op: 'gte', value: 0.8, scale: 'percent' }, + }, + ]); + renderProvider(); + await waitFor(() => expect(screen.getByTestId('groups-status')).toHaveTextContent('groups-success')); + + fireEvent.click(screen.getByText('update-rule-score')); + + await waitFor(() => expect(axiosMock.history.patch).toHaveLength(1)); + expect(axiosMock.history.patch[0].url).toEqual(updateRuleScoreUrl); + expect(JSON.parse(axiosMock.history.patch[0].data)).toEqual({ + criterion_ids: [900], + // `existing-sub` (criterion 900) carries no override of its own in + // `singleGroupResponse` - its effective rule type is resolved from + // the system default profile (`'grade'`), pinned unchanged, not + // accepted from the test consumer's own call. + rule_type_override: 'grade', + rule_payload_override: { op: 'gte', value: 0.8, scale: 'percent' }, + }); + + // The new key is computed from the response's own echoed value + // (0.8), not the request - proven distinctly since both happen to + // agree here, but the assertion still pins the exact expected key. + await waitFor(() => { + expect(screen.getByTestId('focus')).toHaveTextContent('"groupId":10'); + }); + expect(screen.getByTestId('focus')).toHaveTextContent('"ruleKey":"grade:gte:0.8:percent"'); + }); + + it('shows a failure toast on rejection, without changing focus', async () => { + axiosMock.onGet(groupsUrl).reply(200, singleGroupResponse); + axiosMock.onGet(profileUrl).reply(200, profileResponse); + axiosMock.onGet(getCourseOutlineIndexApiUrl(courseA)).reply(200, outlineFixture); + axiosMock.onPatch(updateRuleScoreUrl).reply(500); + renderProvider(); + // `singleGroupResponse` has exactly one bottom-tier group, so the + // initial-focus effect (covered above under "initial focus") already + // auto-focuses it once the queries resolve - captured here as the + // known-good baseline this test proves a rejection leaves untouched. + await waitFor(() => { + expect(screen.getByTestId('focus')).toHaveTextContent('"ruleKey":"grade:gte:0.7:percent"'); + }); + + fireEvent.click(screen.getByText('update-rule-score')); + + await waitFor(() => { + expect(mockShowToast).toHaveBeenCalledWith( + 'There was a problem updating this rule\'s score. Please try again.', + ); + }); + expect(screen.getByTestId('focus')).toHaveTextContent('"ruleKey":"grade:gte:0.7:percent"'); + }); + }); }); diff --git a/src/taxonomy/competency-management/CompetencyAssociationsContext.tsx b/src/taxonomy/competency-management/CompetencyAssociationsContext.tsx index 7b267dbece..59aced6ab4 100644 --- a/src/taxonomy/competency-management/CompetencyAssociationsContext.tsx +++ b/src/taxonomy/competency-management/CompetencyAssociationsContext.tsx @@ -25,9 +25,12 @@ import { useCourseTaggingPermissions, useCreateCompetencyCriterion, useDefaultCompetencyRuleProfile, + useUpdateCompetencyCriteriaGroupOperator, + useUpdateCompetencyCriteriaRule, } from './data/apiHooks'; import type { CompetencyCriteriaGroupsResponse, + CompetencyGroupLogicOperator, CompetencyRuleProfile, CourseCompetencyCriteriaGroup, CreateCompetencyCriterionPayload, @@ -39,6 +42,7 @@ import { associatedObjectIds, bottomTierGroupsForCourse, buildCompetencyCriteriaGroupsIndex, + effectiveRuleOf, lastBottomTierGroupForCourse, lastRealRuleKeyIn, ruleBoxesForGroup, @@ -56,6 +60,9 @@ export interface CriteriaFocus { } export interface CompetencyAssociationsContextValue { + /** Both start unset (`null`); see `CriteriaFocus` above for why the pair + * is always written together. + */ focus: CriteriaFocus | null; /** Focuses a group and its last real rule box (`lastRealRuleKeyIn`). A * no-op when `groupId` is already focused, so re-clicking the group @@ -75,8 +82,25 @@ export interface CompetencyAssociationsContextValue { * active competency. */ associateSubsection: (objectId: string, courseId: string) => void; - /** Whether the signed-in author can create/manage associations for the - * given course (`useCourseTaggingPermissions`'s `courses.manage_tags`). + /** Updates a bottom-tier group's any/all combining logic (`#760`). A + * rejected save is a no-op besides a failure toast: `group.logicOperator` + * is only ever written by the next successful `groupsQuery` refetch. + */ + updateGroupOperator: (groupId: number, logicOperator: CompetencyGroupLogicOperator) => void; + /** Updates a rule box's score threshold, across every criterion listed in + * `criterionIds` (`#759`). The rule type is pinned to the box's current + * effective rule, resolved here rather than accepted from the caller, + * since only the numeric value is user-editable. On success, repairs + * focus using the mutation's own response rather than the request, since + * a "reset to default" edit can be echoed back differently than it was + * sent. Returns a `Promise` (not `void`) so `ScoreThresholdField` can + * revert its own local input on rejection, without owning the mutation. + */ + updateRuleScore: (groupId: number, criterionIds: number[], rulePayload: GradeRulePayload) => Promise; + /** + * Whether the signed-in author can create/manage associations for the + * given course, via `useCourseTaggingPermissions`'s `courses.manage_tags` + * check. */ canEditCourse: (courseId: string) => boolean; groupsQuery: UseQueryResult; @@ -163,6 +187,8 @@ export const CompetencyAssociationsProvider = ({ const groupsQuery = useCompetencyCriteriaGroups(tagId); const profileQuery = useDefaultCompetencyRuleProfile(); const createCriterion = useCreateCompetencyCriterion(); + const updateGroupOperatorMutation = useUpdateCompetencyCriteriaGroupOperator(); + const updateRuleScoreMutation = useUpdateCompetencyCriteriaRule(); const index = useMemo( () => (groupsQuery.data ? buildCompetencyCriteriaGroupsIndex(groupsQuery.data) : undefined), @@ -363,12 +389,61 @@ export const CompetencyAssociationsProvider = ({ }); }, [associatedIds, index, systemDefaultProfile, focus, tagId, createCriterion, showToast, intl]); + const updateGroupOperator = useCallback((groupId: number, logicOperator: CompetencyGroupLogicOperator) => { + updateGroupOperatorMutation.mutate({ tagId, groupId, logicOperator }, { + onError: () => { + // No local rollback needed: `LogicOperatorSelect` always renders + // from `group.logicOperator`, which a rejected mutation never touches. + showToast(intl.formatMessage(messages.updateGroupOperatorFailedToastMessage)); + }, + }); + }, [tagId, updateGroupOperatorMutation, showToast, intl]); + + const updateRuleScore = useCallback(( + groupId: number, + criterionIds: number[], + rulePayload: GradeRulePayload, + ): Promise => { + if (!index || !systemDefaultProfile) { + // Shouldn't happen: a rule box is only ever rendered - let alone made + // editable - once both queries have resolved. + return Promise.resolve(); + } + // Rule type is pinned to the box's current effective rule, resolved + // from any one criterion already in it (they all share it, by definition). + const groupCriteria = index.criteriaByGroupId.get(groupId) ?? []; + const anchorCriterion = groupCriteria.find((criterion) => criterionIds.includes(criterion.id)); + if (!anchorCriterion) { + return Promise.resolve(); + } + const { ruleType } = effectiveRuleOf(anchorCriterion, systemDefaultProfile); + + return updateRuleScoreMutation.mutateAsync({ tagId, groupId, criterionIds, ruleType, rulePayload }) + .then((updatedCriteria) => { + // From the response, not the request: a "reset to default" edit can + // echo back a shared profile reference instead of the sent values, + // so the new focus key has to come from what was actually persisted. + const [updatedCriterion] = updatedCriteria; + if (updatedCriterion) { + setFocus({ groupId, ruleKey: ruleKeyOf(updatedCriterion, systemDefaultProfile) }); + } + }) + .catch((error) => { + showToast(intl.formatMessage(messages.updateRuleScoreFailedToastMessage)); + // Re-thrown so ScoreThresholdField's own commit handler also sees + // the rejection and reverts its local input. + throw error; + }); + }, [index, systemDefaultProfile, tagId, updateRuleScoreMutation, showToast, intl]); + const contextValue = useMemo(() => ({ focus, focusGroup, focusRuleBox, notifyCourseExpanded, associateSubsection, + updateGroupOperator, + updateRuleScore, canEditCourse, groupsQuery, profileQuery, @@ -384,6 +459,8 @@ export const CompetencyAssociationsProvider = ({ focusRuleBox, notifyCourseExpanded, associateSubsection, + updateGroupOperator, + updateRuleScore, canEditCourse, groupsQuery, profileQuery, diff --git a/src/taxonomy/competency-management/criteria-groups/CourseGroupSection.tsx b/src/taxonomy/competency-management/criteria-groups/CourseGroupSection.tsx index 7c2f188271..e6c410cf9a 100644 --- a/src/taxonomy/competency-management/criteria-groups/CourseGroupSection.tsx +++ b/src/taxonomy/competency-management/criteria-groups/CourseGroupSection.tsx @@ -29,7 +29,7 @@ export interface CourseGroupSectionProps { */ const CourseGroupSection = ({ courseGroup }: CourseGroupSectionProps) => { const intl = useIntl(); - const { focus, index } = useCompetencyAssociations(); + const { focus, index, canEditCourse } = useCompetencyAssociations(); const [isCollapsed, setIsCollapsed] = useState(false); // `refetchOnMount: false`: mirrors `CourseOutlineSubtree` - avoids a // wasted refetch of already-cached data on every mount. @@ -91,6 +91,7 @@ const CourseGroupSection = ({ courseGroup }: CourseGroupSectionProps) => { ))} diff --git a/src/taxonomy/competency-management/criteria-groups/CriteriaGroupBox.edit.test.tsx b/src/taxonomy/competency-management/criteria-groups/CriteriaGroupBox.edit.test.tsx new file mode 100644 index 0000000000..975096ae8a --- /dev/null +++ b/src/taxonomy/competency-management/criteria-groups/CriteriaGroupBox.edit.test.tsx @@ -0,0 +1,125 @@ +import { + initializeMocks, + render, + screen, + userEvent, +} from '@src/testUtils'; +import { buildMockCompetencyAssociationsContextValue, MockCompetencyAssociationsProvider } from '../testHelpers'; +import type { + BottomTierCompetencyCriteriaGroup, + CompetencyCriteriaGroupsResponse, + CompetencyRuleProfile, + CourseCompetencyCriteriaGroup, +} from '../data/types'; +import { buildCompetencyCriteriaGroupsIndex } from '../utils'; +import CriteriaGroupBox from './CriteriaGroupBox'; + +const systemDefaultProfile: CompetencyRuleProfile = { + id: 1, + scopeType: 'system_default', + ruleType: 'grade', + rulePayload: { op: 'gte', value: 0.7, scale: 'percent' }, + archived: false, +}; + +const courseGroup: CourseCompetencyCriteriaGroup = { + id: 1, + parentId: null, + depth: 1, + ordering: 0, + logicOperator: 'and', + courseKey: 'course-v1:OrgX+CS101+2024', +}; + +const group: BottomTierCompetencyCriteriaGroup = { + id: 10, + parentId: 1, + depth: 2, + ordering: 0, + logicOperator: 'and', +}; + +const response: CompetencyCriteriaGroupsResponse = { + groups: [courseGroup, group], + criteria: [ + { + id: 101, + objectId: 'block-a', + competencyCriteriaGroupId: 10, + ruleProfileId: 1, + ruleTypeOverride: null, + rulePayloadOverride: null, + }, + ], +}; + +const index = buildCompetencyCriteriaGroupsIndex(response); + +const renderBox = ( + contextOverrides: Parameters[0] = {}, + canEdit: boolean = true, + boxGroup: BottomTierCompetencyCriteriaGroup = group, +) => ( + render( + + + , + ) +); + +describe(' editing (#794)', () => { + beforeEach(() => { + initializeMocks(); + Element.prototype.scrollIntoView = jest.fn(); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('renders the any/all label as plain text with no control when canEdit is false', () => { + const { container } = renderBox({}, false); + + expect(screen.getByText('all')).toBeInTheDocument(); + // Two `role="button"` elements exist (the group's own container and its + // rule box) - neither is a `Dropdown` trigger, since `canEdit` is false. + expect(screen.getAllByRole('button')).toHaveLength(2); + expect(container.querySelector('.dropdown-toggle')).not.toBeInTheDocument(); + }); + + it('calls updateGroupOperator with the group id and the selected operator when canEdit is true', async () => { + const user = userEvent.setup(); + const updateGroupOperator = jest.fn(); + renderBox({ updateGroupOperator }); + + await user.click(screen.getByRole('button', { name: 'all' })); + await user.click(screen.getByText('any')); + + expect(updateGroupOperator).toHaveBeenCalledWith(10, 'or'); + }); + + it( + 'keeps showing the prior label after a rejected save, since the control is controlled by the group ' + + 'prop, not by the click that was made', + async () => { + const user = userEvent.setup(); + const updateGroupOperator = jest.fn(); + const { container, rerender } = renderBox({ updateGroupOperator }); + + await user.click(screen.getByRole('button', { name: 'all' })); + await user.click(screen.getByText('any')); + expect(updateGroupOperator).toHaveBeenCalledWith(10, 'or'); + + // A rejected save never updates `group.logicOperator` - simulated by + // re-rendering with the same, unchanged `group` (the real caller's + // `groupsQuery` data is likewise unchanged after a rejected mutation). + rerender( + + + , + ); + + expect(container.querySelector('.dropdown-toggle')).toHaveTextContent('all'); + }, + ); +}); diff --git a/src/taxonomy/competency-management/criteria-groups/CriteriaGroupBox.test.tsx b/src/taxonomy/competency-management/criteria-groups/CriteriaGroupBox.test.tsx index ad8fbe633c..4f1277e765 100644 --- a/src/taxonomy/competency-management/criteria-groups/CriteriaGroupBox.test.tsx +++ b/src/taxonomy/competency-management/criteria-groups/CriteriaGroupBox.test.tsx @@ -10,6 +10,7 @@ import type { BottomTierCompetencyCriteriaGroup, CompetencyCriteriaGroupsResponse, CompetencyRuleProfile, + CourseCompetencyCriteriaGroup, } from '../data/types'; import { buildCompetencyCriteriaGroupsIndex } from '../utils'; import CriteriaGroupBox from './CriteriaGroupBox'; @@ -22,6 +23,22 @@ const systemDefaultProfile: CompetencyRuleProfile = { archived: false, }; +// A real course-level parent for the bottom-tier group below - a bottom-tier +// group never exists without one in the real API response. Without this, +// any `canEdit`/course-id resolution derived from the tree would silently +// default to false, and a "renders read-only" test would keep passing for +// the wrong reason. +const courseGroup: CourseCompetencyCriteriaGroup = { + id: 1, + parentId: null, + tagId: 42, + courseKey: 'course-v1:OrgX+CS101+2024', + name: 'course', + ordering: 0, + logicOperator: 'AND', + archived: false, +}; + const group: BottomTierCompetencyCriteriaGroup = { id: 10, parentId: 1, @@ -34,7 +51,7 @@ const group: BottomTierCompetencyCriteriaGroup = { }; const response: CompetencyCriteriaGroupsResponse = { - groups: [group], + groups: [courseGroup, group], criteria: [ { id: 101, @@ -52,10 +69,13 @@ const index = buildCompetencyCriteriaGroupsIndex(response); const renderBox = ( contextOverrides: Parameters[0] = {}, subsectionNamesByUsageKey: Record = {}, + // Defaults to `true`, matching `testHelpers.tsx`'s own `canEditCourse: () => true` + // convention (tests default to "editable," and opt out explicitly). + canEdit: boolean = true, ) => ( render( - + , ) ); @@ -70,8 +90,8 @@ describe('', () => { jest.clearAllMocks(); }); - it('renders the group\'s any/all label as plain text with no control', () => { - const { container } = renderBox({}, { 'block-a': 'Subsection A' }); + it('renders the group\'s any/all label as plain text with no control when canEdit is false', () => { + const { container } = renderBox({}, { 'block-a': 'Subsection A' }, false); // The header sentence is split across sibling text nodes (plain text + // the any/all ``), so its full text is checked via textContent @@ -79,9 +99,9 @@ describe('', () => { expect(container.querySelector('.criteria-group-box__header')).toHaveTextContent( 'By completing all of the following', ); - // Two `role="button"` elements exist (the group's own header band and the - // one rendered rule box) - neither is a `SelectMenu` trigger, since no - // `onChange` was given to the any/all control. + // Two `role="button"` elements exist (the group's own header band and + // the one rendered rule box) - neither is a `Dropdown` trigger, since + // `canEdit` is false here. expect(screen.getAllByRole('button')).toHaveLength(2); expect(screen.getByText('Subsection A')).toBeInTheDocument(); }); @@ -128,8 +148,11 @@ describe('', () => { const focusRuleBox = jest.fn(); renderBox({ focusGroup, focusRuleBox }); - // The rule box is the second `role="button"` (the header band is first). - fireEvent.click(screen.getAllByRole('button')[1]); + // With the default `canEdit: true`, three `role="button"` elements + // exist: the header band, the any/all `Dropdown` trigger, and the rule + // box itself - the rule box is the third, not the second. + expect(screen.getAllByRole('button')).toHaveLength(3); + fireEvent.click(screen.getAllByRole('button')[2]); expect(focusRuleBox).toHaveBeenCalledTimes(1); expect(focusGroup).not.toHaveBeenCalled(); diff --git a/src/taxonomy/competency-management/criteria-groups/CriteriaGroupBox.tsx b/src/taxonomy/competency-management/criteria-groups/CriteriaGroupBox.tsx index 4e1a5ceff4..406995d808 100644 --- a/src/taxonomy/competency-management/criteria-groups/CriteriaGroupBox.tsx +++ b/src/taxonomy/competency-management/criteria-groups/CriteriaGroupBox.tsx @@ -10,6 +10,12 @@ import messages from './messages'; export interface CriteriaGroupBoxProps { group: BottomTierCompetencyCriteriaGroup; subsectionNamesByUsageKey: Record; + /** Whether the signed-in author can edit this group's own any/all logic + * and its rule boxes' scores - resolved once by `CourseGroupSection` via + * `canEditCourse(courseGroup.courseKey)` and threaded down as a plain + * prop, rather than re-derived here from `index`. + */ + canEdit: boolean; } /** One bottom-tier group's "By completing any/all of the following" @@ -20,9 +26,9 @@ export interface CriteriaGroupBoxProps { * being the more specific/innermost target, scrolls itself instead (see * `RuleBox`), so exactly one of the two ever scrolls for a given focus. */ -const CriteriaGroupBox = ({ group, subsectionNamesByUsageKey }: CriteriaGroupBoxProps) => { +const CriteriaGroupBox = ({ group, subsectionNamesByUsageKey, canEdit }: CriteriaGroupBoxProps) => { const intl = useIntl(); - const { focus, focusGroup, index, systemDefaultProfile } = useCompetencyAssociations(); + const { focus, focusGroup, index, systemDefaultProfile, updateGroupOperator } = useCompetencyAssociations(); const ref = useRef(null); const isFocused = focus?.groupId === group.id; @@ -41,6 +47,12 @@ const CriteriaGroupBox = ({ group, subsectionNamesByUsageKey }: CriteriaGroupBox }; const handleKeyDown: React.KeyboardEventHandler = (event) => { + // Only this wrapper's own key events, not ones bubbled up from the + // any/all dropdown once `canEdit` is true - otherwise this handler's + // `preventDefault` would swallow the dropdown's own Enter/Space. + if (event.target !== event.currentTarget) { + return; + } if (event.key === 'Enter' || event.key === ' ') { event.preventDefault(); focusGroup(group.id); @@ -72,6 +84,7 @@ const CriteriaGroupBox = ({ group, subsectionNamesByUsageKey }: CriteriaGroupBox and: intl.formatMessage(messages.logicOperatorAllLabel), or: intl.formatMessage(messages.logicOperatorAnyLabel), }} + onChange={canEdit ? (logicOperator) => updateGroupOperator(group.id, logicOperator) : undefined} /> ), })} @@ -83,6 +96,7 @@ const CriteriaGroupBox = ({ group, subsectionNamesByUsageKey }: CriteriaGroupBox index={index} systemDefaultProfile={systemDefaultProfile} subsectionNamesByUsageKey={subsectionNamesByUsageKey} + canEdit={canEdit} />
)} diff --git a/src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.test.tsx b/src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.test.tsx index 6a280b1e85..adcb00dfcc 100644 --- a/src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.test.tsx +++ b/src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.test.tsx @@ -29,9 +29,33 @@ describe('', () => { const onChange = jest.fn(); render(); - fireEvent.click(screen.getByRole('button')); + fireEvent.click(screen.getByRole('button', { name: 'all' })); fireEvent.click(screen.getByText('any')); expect(onChange).toHaveBeenCalledWith('OR'); }); + + it( + 'keeps showing the prior label after a rejected save, since the trigger is controlled by value, not by ' + + 'the click that was made', + () => { + const onChange = jest.fn(); + const { container, rerender } = render(); + + fireEvent.click(screen.getByRole('button', { name: 'all' })); + fireEvent.click(screen.getByText('any')); + expect(onChange).toHaveBeenCalledWith('or'); + + // A rejected save never updates `group.logicOperator` - simulated + // here by re-rendering with `value` unchanged, exactly what the real + // caller does when the mutation's `onError` fires and no local + // rollback of its own is needed (see this component's own docstring). + // The trigger itself (`.dropdown-toggle`, not the still-mounted-but- + // hidden menu items) is checked directly, since Paragon's `Dropdown` + // keeps its closed menu's items in the DOM rather than removing them. + rerender(); + + expect(container.querySelector('.dropdown-toggle')).toHaveTextContent('all'); + }, + ); }); diff --git a/src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.tsx b/src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.tsx index 868a56db0a..8db39b73b9 100644 --- a/src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.tsx +++ b/src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.tsx @@ -1,4 +1,5 @@ -import { MenuItem, SelectMenu } from '@openedx/paragon'; +import { useId } from 'react'; +import { Dropdown } from '@openedx/paragon'; import type { CompetencyGroupLogicOperator } from '../data/types'; export interface LogicOperatorSelectLabels { @@ -15,7 +16,7 @@ export interface LogicOperatorSelectProps { */ labels: LogicOperatorSelectLabels; /** Omitted where only the value is read: renders plain, non-interactive - * text in that case. Passing it renders the interactive `SelectMenu`. + * text in that case. Passing it renders the interactive control instead. */ onChange?: (value: CompetencyGroupLogicOperator) => void; className?: string; @@ -23,7 +24,14 @@ export interface LogicOperatorSelectProps { /** Shared any/all (bottom-tier group) or Or/And (connector) control for how * a group's children combine. Read-only (plain text) unless `onChange` is - * given. + * given, in which case a controlled Paragon `Dropdown` renders instead, + * with its trigger label always computed from `value`. + * + * Deliberately not Paragon's `SelectMenu`: it's uncontrolled (holds its own + * "selected" state once a `MenuItem` is clicked), so its trigger would keep + * showing whatever the author last clicked even after a rejected save left + * `value` unchanged. `Dropdown` has no equivalent internal state, so a + * rejected save's rollback is just a matter of never having changed `value`. */ const LogicOperatorSelect = ({ value, @@ -32,19 +40,22 @@ const LogicOperatorSelect = ({ className, }: LogicOperatorSelectProps) => { const label = value === 'AND' ? labels.and : labels.or; + const toggleId = useId(); if (!onChange) { return {label}; } return ( - - onChange('AND')}>{labels.and} - onChange('OR')}>{labels.or} - + + + {label} + + + onChange('AND')}>{labels.and} + onChange('OR')}>{labels.or} + + ); }; diff --git a/src/taxonomy/competency-management/criteria-groups/RuleBox.edit.test.tsx b/src/taxonomy/competency-management/criteria-groups/RuleBox.edit.test.tsx new file mode 100644 index 0000000000..34bfd317ca --- /dev/null +++ b/src/taxonomy/competency-management/criteria-groups/RuleBox.edit.test.tsx @@ -0,0 +1,136 @@ +import { + initializeMocks, + render, + screen, + userEvent, + waitFor, +} from '@src/testUtils'; +import { buildMockCompetencyAssociationsContextValue, MockCompetencyAssociationsProvider } from '../testHelpers'; +import type { CompetencyCriterion } from '../data/types'; +import RuleBox from './RuleBox'; + +const criteria: CompetencyCriterion[] = [ + { + id: 1, + objectId: 'block-a', + competencyCriteriaGroupId: 10, + ruleProfileId: null, + ruleTypeOverride: 'grade', + rulePayloadOverride: { op: 'gte', value: 0.7, scale: 'percent' }, + }, +]; + +const rule = { ruleType: 'grade', rulePayload: { op: 'gte' as const, value: 0.7, scale: 'percent' as const } }; + +const renderRuleBox = ( + contextOverrides: Parameters[0] = {}, + props: Partial> = {}, +) => ( + render( + + + , + ) +); + +describe(' editing (#794)', () => { + beforeEach(() => { + initializeMocks(); + Element.prototype.scrollIntoView = jest.fn(); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('renders the score as plain text, not an input, when canEdit is false', () => { + renderRuleBox({}, { canEdit: false }); + + expect(screen.getByText('With a score of 70% or higher')).toBeInTheDocument(); + expect(screen.queryByRole('textbox')).not.toBeInTheDocument(); + }); + + it('renders the score as plain text, not an input, when canEdit is omitted (default false)', () => { + renderRuleBox(); + + expect(screen.getByText('With a score of 70% or higher')).toBeInTheDocument(); + expect(screen.queryByRole('textbox')).not.toBeInTheDocument(); + }); + + it( + 'commits an edited score by calling updateRuleScore with the group id and every criterion id in the box', + async () => { + const user = userEvent.setup(); + const updateRuleScore = jest.fn().mockResolvedValue(undefined); + renderRuleBox({ updateRuleScore }, { canEdit: true }); + + const input = screen.getByRole('textbox'); + await user.clear(input); + await user.type(input, '85'); + await user.keyboard('{Enter}'); + + expect(updateRuleScore).toHaveBeenCalledWith(10, [1], { op: 'gte', value: 0.85, scale: 'percent' }); + }, + ); + + it('blocks the commit and shows the inline message when getInlineValidationMessage flags a duplicate', async () => { + const user = userEvent.setup(); + const updateRuleScore = jest.fn().mockResolvedValue(undefined); + const getInlineValidationMessage = jest.fn().mockReturnValue( + 'Another rule box in this group already uses this score.', + ); + renderRuleBox({ updateRuleScore }, { canEdit: true, getInlineValidationMessage }); + + const input = screen.getByRole('textbox'); + await user.clear(input); + await user.type(input, '85'); + await user.keyboard('{Enter}'); + + expect(updateRuleScore).not.toHaveBeenCalled(); + expect(screen.getByText('Another rule box in this group already uses this score.')).toBeInTheDocument(); + }); + + it('does not call updateRuleScore when the input round-trips to the same displayed percent', async () => { + const user = userEvent.setup(); + const updateRuleScore = jest.fn().mockResolvedValue(undefined); + renderRuleBox({ updateRuleScore }, { canEdit: true }); + + const input = screen.getByRole('textbox'); + await user.clear(input); + await user.type(input, '70'); + await user.keyboard('{Enter}'); + + expect(updateRuleScore).not.toHaveBeenCalled(); + }); + + it('reverts the input to the prop value when the commit is rejected', async () => { + const user = userEvent.setup(); + const updateRuleScore = jest.fn().mockRejectedValue(new Error('save failed')); + renderRuleBox({ updateRuleScore }, { canEdit: true }); + + const input = screen.getByRole('textbox'); + await user.clear(input); + await user.type(input, '85'); + await user.keyboard('{Enter}'); + + expect(updateRuleScore).toHaveBeenCalledWith(10, [1], { op: 'gte', value: 0.85, scale: 'percent' }); + await waitFor(() => expect(input).toHaveValue('70')); + }); + + it('does not call focusRuleBox when the score input itself is clicked', async () => { + const user = userEvent.setup(); + const focusRuleBox = jest.fn(); + renderRuleBox({ focusRuleBox }, { canEdit: true }); + + await user.click(screen.getByRole('textbox')); + + expect(focusRuleBox).not.toHaveBeenCalled(); + }); +}); diff --git a/src/taxonomy/competency-management/criteria-groups/RuleBox.tsx b/src/taxonomy/competency-management/criteria-groups/RuleBox.tsx index 93586e92bc..5cbc4c99c0 100644 --- a/src/taxonomy/competency-management/criteria-groups/RuleBox.tsx +++ b/src/taxonomy/competency-management/criteria-groups/RuleBox.tsx @@ -1,7 +1,7 @@ import { useEffect, useRef } from 'react'; import classNames from 'classnames'; import { useCompetencyAssociations } from '../CompetencyAssociationsContext'; -import type { CompetencyCriterion, EffectiveRule } from '../data/types'; +import type { CompetencyCriterion, EffectiveRule, GradeRulePayload } from '../data/types'; import CriterionChipList from './CriterionChipList'; import ScoreThresholdField from './ScoreThresholdField'; @@ -19,6 +19,17 @@ export interface RuleBoxProps { rule: EffectiveRule; criteria: CompetencyCriterion[]; subsectionNamesByUsageKey: Record; + /** Threaded down from `RuleBoxList` (originally resolved by + * `CourseGroupSection` via `canEditCourse`) - see `CriteriaGroupBoxProps.canEdit`. + * Optional, defaulting to `false`, so every caller that predates this prop + * keeps working unchanged. + */ + canEdit?: boolean; + /** Built once by `RuleBoxList` for this specific box (the duplicate-score + * check) - passed straight through to `ScoreThresholdField`. Only + * meaningful when `canEdit` is true. + */ + getInlineValidationMessage?: (value: string) => string; } /** One rule box: the rule it's given, its chips, and focus/click behavior. @@ -32,8 +43,10 @@ const RuleBox = ({ rule, criteria, subsectionNamesByUsageKey, + canEdit = false, + getInlineValidationMessage, }: RuleBoxProps) => { - const { focus, focusRuleBox } = useCompetencyAssociations(); + const { focus, focusRuleBox, updateRuleScore } = useCompetencyAssociations(); const isFocused = focus?.groupId === groupId && focus?.ruleKey === ruleKey; const ref = useRef(null); @@ -48,12 +61,22 @@ const RuleBox = ({ }; const handleKeyDown: React.KeyboardEventHandler = (event) => { + // Only this wrapper's own key events, not ones bubbled up from the + // score input once `canEdit` is true - otherwise this handler's + // `preventDefault` would swallow the input's own Enter/Escape. + if (event.target !== event.currentTarget) { + return; + } if (event.key === 'Enter' || event.key === ' ') { event.preventDefault(); focusRuleBox(groupId, ruleKey); } }; + const handleScoreChange = (rulePayload: GradeRulePayload): Promise => ( + updateRuleScore(groupId, criteria.map((criterion) => criterion.id), rulePayload) + ); + return (
- +
); diff --git a/src/taxonomy/competency-management/criteria-groups/RuleBoxList.tsx b/src/taxonomy/competency-management/criteria-groups/RuleBoxList.tsx index a92c6667e5..d9ce5da1d2 100644 --- a/src/taxonomy/competency-management/criteria-groups/RuleBoxList.tsx +++ b/src/taxonomy/competency-management/criteria-groups/RuleBoxList.tsx @@ -1,13 +1,21 @@ +import { useIntl } from '@edx/frontend-platform/i18n'; import type { CompetencyCriteriaGroupsIndex } from '../utils'; import { ruleBoxesForGroup } from '../utils'; -import type { CompetencyRuleProfile } from '../data/types'; +import type { CompetencyRuleProfile, RuleBox as RuleBoxData } from '../data/types'; import RuleBox from './RuleBox'; +import messages from './messages'; export interface RuleBoxListProps { groupId: number; index: CompetencyCriteriaGroupsIndex; systemDefaultProfile: CompetencyRuleProfile; subsectionNamesByUsageKey: Record; + /** Threaded down from `CriteriaGroupBox` (originally resolved by + * `CourseGroupSection` via `canEditCourse`) - see `CriteriaGroupBoxProps.canEdit`. + * Optional, defaulting to `false`, so every caller that predates this prop + * keeps working unchanged. + */ + canEdit?: boolean; } /** Derives one bottom-tier group's rule boxes via `ruleBoxesForGroup` and @@ -15,15 +23,36 @@ export interface RuleBoxListProps { * from `CompetencyAssociationsContext` directly (given `groupId` and its * own box key to compare against), so no focus state is threaded through * here. + * + * Also builds each box's own `getInlineValidationMessage` (the + * duplicate-score check) here, using `boxes`/`index`/`systemDefaultProfile`, + * which this component already has - `RuleBox` takes no new data dependency + * for it. */ const RuleBoxList = ({ groupId, index, systemDefaultProfile, subsectionNamesByUsageKey, + canEdit = false, }: RuleBoxListProps) => { + const intl = useIntl(); const boxes = ruleBoxesForGroup(groupId, index, systemDefaultProfile); + // Compares rounded percent + rule type + op, not `ruleKeyOf`'s raw string + // key (built from a raw fraction, which risks a formatting mismatch that + // silently never fires). Excludes the box being edited by its own `key`. + const getInlineValidationMessage = (candidateBox: RuleBoxData) => (value: string): string => { + const candidatePercent = Math.round(Number(value)); + const isDuplicate = boxes.some((other) => ( + other.key !== candidateBox.key + && other.rule.ruleType === candidateBox.rule.ruleType + && other.rule.rulePayload.op === candidateBox.rule.rulePayload.op + && Math.round(other.rule.rulePayload.value * 100) === candidatePercent + )); + return isDuplicate ? intl.formatMessage(messages.duplicateScoreValidationMessage) : ''; + }; + return (
{boxes.map((box) => ( @@ -34,6 +63,8 @@ const RuleBoxList = ({ rule={box.rule} criteria={box.criteria} subsectionNamesByUsageKey={subsectionNamesByUsageKey} + canEdit={canEdit} + getInlineValidationMessage={getInlineValidationMessage(box)} /> ))}
diff --git a/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.test.tsx b/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.test.tsx index f5c93c003c..66ccff045b 100644 --- a/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.test.tsx +++ b/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.test.tsx @@ -1,4 +1,10 @@ -import { initializeMocks, render, screen } from '@src/testUtils'; +import { + initializeMocks, + render, + screen, + userEvent, + waitFor, +} from '@src/testUtils'; import ScoreThresholdField from './ScoreThresholdField'; describe('', () => { @@ -39,4 +45,132 @@ describe('', () => { render(); expect(screen.getByText('67% or higher')).toBeInTheDocument(); }); + + describe('editable (onChange given)', () => { + const rulePayload = { op: 'gte' as const, value: 0.7, scale: 'percent' as const }; + + it('commits the typed percent, as a fraction, on Enter - op/scale pinned unchanged', async () => { + const user = userEvent.setup(); + const onChange = jest.fn().mockResolvedValue(undefined); + render(); + + const input = screen.getByRole('textbox'); + await user.clear(input); + await user.type(input, '85'); + await user.keyboard('{Enter}'); + + expect(onChange).toHaveBeenCalledWith({ op: 'gte', value: 0.85, scale: 'percent' }); + }); + + it('commits the typed percent on blur', async () => { + const user = userEvent.setup(); + const onChange = jest.fn().mockResolvedValue(undefined); + render( + <> + + + , + ); + + const input = screen.getByRole('textbox'); + await user.clear(input); + await user.type(input, '90'); + await user.click(screen.getByRole('button', { name: 'elsewhere' })); + + expect(onChange).toHaveBeenCalledWith({ op: 'gte', value: 0.9, scale: 'percent' }); + }); + + it('Escape reverts the input to the last-persisted value without committing', async () => { + const user = userEvent.setup(); + const onChange = jest.fn().mockResolvedValue(undefined); + render(); + + const input = screen.getByRole('textbox'); + await user.clear(input); + await user.type(input, '99'); + await user.keyboard('{Escape}'); + + expect(input).toHaveValue('70'); + expect(onChange).not.toHaveBeenCalled(); + }); + + it('reverts the input to the prop value when the commit is rejected', async () => { + const user = userEvent.setup(); + const onChange = jest.fn().mockRejectedValue(new Error('save failed')); + render(); + + const input = screen.getByRole('textbox'); + await user.clear(input); + await user.type(input, '85'); + await user.keyboard('{Enter}'); + + expect(onChange).toHaveBeenCalledWith({ op: 'gte', value: 0.85, scale: 'percent' }); + await waitFor(() => expect(input).toHaveValue('70')); + }); + + it('blocks the commit and shows the inline message when getInlineValidationMessage returns one', async () => { + const user = userEvent.setup(); + const onChange = jest.fn().mockResolvedValue(undefined); + const getInlineValidationMessage = jest.fn().mockReturnValue('Duplicate score.'); + render( + , + ); + + const input = screen.getByRole('textbox'); + await user.clear(input); + await user.type(input, '85'); + await user.keyboard('{Enter}'); + + expect(getInlineValidationMessage).toHaveBeenCalledWith('85'); + expect(onChange).not.toHaveBeenCalled(); + expect(screen.getByText('Duplicate score.')).toBeInTheDocument(); + }); + + it('does not call onChange on blur when the input still matches the persisted value', async () => { + const user = userEvent.setup(); + const onChange = jest.fn().mockResolvedValue(undefined); + render( + <> + + + , + ); + + // Focus and blur without typing anything - a plain tab-through. + await user.click(screen.getByRole('textbox')); + await user.click(screen.getByRole('button', { name: 'elsewhere' })); + + expect(onChange).not.toHaveBeenCalled(); + }); + + it('does not call onChange when the typed value round-trips to the same displayed percent', async () => { + const user = userEvent.setup(); + const onChange = jest.fn().mockResolvedValue(undefined); + render(); + + const input = screen.getByRole('textbox'); + await user.clear(input); + await user.type(input, '70'); + await user.keyboard('{Enter}'); + + expect(onChange).not.toHaveBeenCalled(); + }); + + it('reverts, rather than committing 0%, when the input is cleared entirely', async () => { + const user = userEvent.setup(); + const onChange = jest.fn().mockResolvedValue(undefined); + render(); + + const input = screen.getByRole('textbox'); + await user.clear(input); + await user.keyboard('{Enter}'); + + expect(onChange).not.toHaveBeenCalled(); + expect(input).toHaveValue('70'); + }); + }); }); diff --git a/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.tsx b/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.tsx index 84592dea6d..cb5daeb36a 100644 --- a/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.tsx +++ b/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.tsx @@ -1,5 +1,7 @@ +import { useState } from 'react'; +import type { KeyboardEvent } from 'react'; import { useIntl } from '@edx/frontend-platform/i18n'; -import { Icon } from '@openedx/paragon'; +import { Form, Icon } from '@openedx/paragon'; import { ExpandMore } from '@openedx/paragon/icons'; import type { MessageDescriptor } from 'react-intl'; import type { GradeRulePayload } from '../data/types'; @@ -7,13 +9,16 @@ import messages from './messages'; export interface ScoreThresholdFieldProps { rulePayload: GradeRulePayload; - /** Not exercised yet (nothing here is editable): reserved so a later - * editing ticket can wire in a change handler without this component - * needing to be reshaped. + /** Omitted where only `rulePayload` is read: renders plain, non-interactive + * text in that case. Passing it renders the editable input instead. + * Returns a `Promise` so this component's own commit handler can revert + * its local input on a rejected save - the mutation itself still lives in + * `CompetencyAssociationsContext`. */ - onChange?: (rulePayload: GradeRulePayload) => void; + onChange?: (rulePayload: GradeRulePayload) => Promise; /** Same reservation as `onChange`, mirroring `EditableCell`'s prop of the - * same name (`src/taxonomy/tree-table/EditableCell.tsx`). + * same name. Per-keystroke and display-only - it never blocks a commit by + * itself; `commit` below is what gates the commit on it. */ getInlineValidationMessage?: (value: string) => string; } @@ -27,30 +32,123 @@ const SUFFIX_MESSAGE_BY_OP: Record String(Math.round(rulePayload.value * 100)); + /** Closed-state label for a rule's score threshold: "With a score of X% - * or higher"/"or lower"/(no suffix, for an exact match). Read-only - the trailing - * chevron is decorative. See `onChange`/`getInlineValidationMessage` above for why the props still - * exist. + * or higher"/"or lower"/(no suffix, for an exact match). Read-only unless + * `onChange` is given, in which case the percentage renders as an editable + * input instead - same "read-only unless given a handler" shape + * `LogicOperatorSelect` already uses. */ -const ScoreThresholdField = ({ rulePayload }: ScoreThresholdFieldProps) => { +const ScoreThresholdField = ({ rulePayload, onChange, getInlineValidationMessage }: ScoreThresholdFieldProps) => { const intl = useIntl(); - // `rulePayload.value` is a 0.0-1.0 fraction (ADR 0002), not a 0-100 - // percentage - same conversion `src/grading-settings/credit-section/ - // index.jsx` uses for the same kind of field. const percent = Math.round(rulePayload.value * 100); const suffixMessage = SUFFIX_MESSAGE_BY_OP[rulePayload.op]; + // Local input state, mirroring `EditableCell`'s own pattern - never + // resynced from `rulePayload` while mounted. A successful edit gets a + // fresh value for free, since `RuleBoxList`'s `key={box.key}` remounts + // this box once its rule key changes. A rejected edit doesn't change that + // key, so it needs its own explicit revert (see `commit` below). + const [inputValue, setInputValue] = useState(() => percentOf(rulePayload)); + const [validationMessage, setValidationMessage] = useState(''); + + if (!onChange) { + return ( + + {intl.formatMessage(messages.scoreThresholdLabel, { + threshold: ( + + {intl.formatMessage(messages.scoreThresholdValue, { percent })} + {suffixMessage && ` ${intl.formatMessage(suffixMessage)}`} + + + ), + })} + + ); + } + + const revertToLastPersisted = () => { + setInputValue(percentOf(rulePayload)); + setValidationMessage(''); + }; + + const commit = () => { + const trimmedInput = inputValue.trim(); + const numericPercent = Number(trimmedInput); + if (trimmedInput === '' || !Number.isFinite(numericPercent)) { + revertToLastPersisted(); + return; + } + if (numericPercent === Math.round(rulePayload.value * 100)) { + // Nothing actually changed - e.g. a blur or Enter with no edit made, + // or a re-typed value that round-trips to the same displayed percent. + // Skip the request rather than re-sending the current value, which + // (given the display itself is a rounded percent) could otherwise + // silently move a stored fraction by a rounding error. + setValidationMessage(''); + return; + } + const message = getInlineValidationMessage ? getInlineValidationMessage(inputValue) : ''; + if (message) { + // getInlineValidationMessage only sets the inline message by itself; + // gating the actual commit on it is this component's own work. + setValidationMessage(message); + return; + } + setValidationMessage(''); + const newRulePayload: GradeRulePayload = { ...rulePayload, value: numericPercent / 100 }; + onChange(newRulePayload).catch(() => { + // Unlike `LogicOperatorSelect`'s `Dropdown` (which always renders + // from its `value` prop), this input's local state doesn't + // self-correct on rejection - revert it explicitly. + revertToLastPersisted(); + }); + }; + + const handleKeyDown = (event: KeyboardEvent) => { + if (event.key === 'Enter') { + commit(); + } else if (event.key === 'Escape') { + revertToLastPersisted(); + } + // Stops the wrapping `RuleBox`'s own `onKeyDown` (Enter/Space -> + // `focusRuleBox`) from also firing while typing inside this input. + event.stopPropagation(); + }; + return ( - + {intl.formatMessage(messages.scoreThresholdLabel, { threshold: ( - - {intl.formatMessage(messages.scoreThresholdValue, { percent })} - {suffixMessage && ` ${intl.formatMessage(suffixMessage)}`} - + + setInputValue(event.target.value)} + onKeyDown={handleKeyDown} + onBlur={commit} + onClick={(event: React.MouseEvent) => event.stopPropagation()} + /> + % ), })} + {suffixMessage && ` ${intl.formatMessage(suffixMessage)}`} + {validationMessage && ( +
+ {validationMessage} +
+ )}
); }; diff --git a/src/taxonomy/competency-management/criteria-groups/criteria-groups.scss b/src/taxonomy/competency-management/criteria-groups/criteria-groups.scss index d39de54bee..2a9de3c3a2 100644 --- a/src/taxonomy/competency-management/criteria-groups/criteria-groups.scss +++ b/src/taxonomy/competency-management/criteria-groups/criteria-groups.scss @@ -103,6 +103,19 @@ &__header-operator { font-weight: 700; line-height: 1.5rem; + + // Once `canEdit` is true, this class lands on a Paragon `Dropdown` + // (a `
`), which has no `display` rule of its own and would + // otherwise break "By completing [x] of the following" onto separate + // lines. + display: inline-block; + + // Nudges the toggle's baseline to line up with the surrounding + // sentence text - the one thing `.btn-inline` doesn't already handle + // for a `Dropdown.Toggle` specifically. + .dropdown-toggle { + vertical-align: baseline; + } } &__rules { @@ -153,6 +166,16 @@ color: var(--pgn-color-info-600); } + // "With a score of [input]% or higher" - the editable percent input + // itself. Paragon's `Form.Control` renders block-level and full-width by + // default, which would otherwise blow out this inline sentence to the + // width of the whole rule box. + .score-threshold-field__input { + display: inline-block; + width: 3.5em; + text-align: center; + } + // Each chip's own truncation - a long subsection name shows an ellipsis // rather than wrapping the chip onto a second line or overflowing its // row. `Chip`'s own label is a nested `
` (`.pgn__chip__label`, a diff --git a/src/taxonomy/competency-management/criteria-groups/messages.ts b/src/taxonomy/competency-management/criteria-groups/messages.ts index 23ff868b64..9ebce681d3 100644 --- a/src/taxonomy/competency-management/criteria-groups/messages.ts +++ b/src/taxonomy/competency-management/criteria-groups/messages.ts @@ -85,6 +85,19 @@ const messages = defineMessages({ description: 'Inline error message shown when loading a competency\'s existing criteria groups, or the ' + 'system default rule profile, fails.', }, + scoreThresholdInputAccessibleLabel: { + id: 'course-authoring.competency-management.criteria-groups.score-threshold-input-accessible-label', + defaultMessage: 'Score threshold percentage', + description: 'Accessible name for the editable score-threshold input, which otherwise relies on the ' + + 'surrounding sentence ("With a score of ___% or higher") for sighted users only.', + }, + duplicateScoreValidationMessage: { + id: 'course-authoring.competency-management.criteria-groups.duplicate-score-validation-message', + defaultMessage: 'Another rule box in this group already uses this score.', + description: 'Inline validation message shown while editing a rule box\'s score threshold, when the typed ' + + 'value would exactly duplicate another rule box\'s own score, rule type, and comparison in the same ' + + 'group.', + }, }); export default messages; diff --git a/src/taxonomy/competency-management/data/api.test.ts b/src/taxonomy/competency-management/data/api.test.ts index 2256b74f79..b51ae8a4e7 100644 --- a/src/taxonomy/competency-management/data/api.test.ts +++ b/src/taxonomy/competency-management/data/api.test.ts @@ -4,6 +4,8 @@ import { createCompetencyCriterion, getCompetencyCriteriaGroups, getDefaultCompetencyRuleProfile, + updateCompetencyCriteriaGroupOperator, + updateCompetencyCriteriaRule, } from './api'; import type { CompetencyCriteriaGroupsResponse } from './types'; @@ -130,4 +132,66 @@ describe('competency-management api calls', () => { objectTagId: tagId, }); }); + + it('updates a group\'s operator, uppercasing the request and lowercasing the response', async () => { + const { axiosMock } = initializeMocks(); + axiosMock.onPatch(apiUrls.updateCompetencyCriteriaGroup(tagId, 10)).reply(200, { + id: 10, + parent_id: 1, + depth: 2, + ordering: 0, + logic_operator: 'OR', + }); + + const result = await updateCompetencyCriteriaGroupOperator(tagId, 10, 'or'); + + expect(axiosMock.history.patch[0].url).toEqual(apiUrls.updateCompetencyCriteriaGroup(tagId, 10)); + expect(JSON.parse(axiosMock.history.patch[0].data)).toEqual({ logic_operator: 'OR' }); + expect(result).toEqual({ + id: 10, + parentId: 1, + depth: 2, + ordering: 0, + logicOperator: 'or', + }); + }); + + it('batch-updates a rule box\'s score, sending a snake_case payload and camelCasing the response', async () => { + const { axiosMock } = initializeMocks(); + const groupId = 10; + axiosMock.onPatch(apiUrls.updateCompetencyCriteriaRule(groupId)).reply(200, [ + { + id: 101, + object_id: 'block-a', + competency_criteria_group_id: groupId, + rule_profile_id: null, + rule_type_override: 'grade', + rule_payload_override: { op: 'gte', value: 0.8, scale: 'percent' }, + }, + ]); + + const result = await updateCompetencyCriteriaRule( + groupId, + [101], + 'grade', + { op: 'gte', value: 0.8, scale: 'percent' }, + ); + + expect(axiosMock.history.patch[0].url).toEqual(apiUrls.updateCompetencyCriteriaRule(groupId)); + expect(JSON.parse(axiosMock.history.patch[0].data)).toEqual({ + criterion_ids: [101], + rule_type_override: 'grade', + rule_payload_override: { op: 'gte', value: 0.8, scale: 'percent' }, + }); + expect(result).toEqual([ + { + id: 101, + objectId: 'block-a', + competencyCriteriaGroupId: groupId, + ruleProfileId: null, + ruleTypeOverride: 'grade', + rulePayloadOverride: { op: 'gte', value: 0.8, scale: 'percent' }, + }, + ]); + }); }); diff --git a/src/taxonomy/competency-management/data/api.ts b/src/taxonomy/competency-management/data/api.ts index ccdc64cdd1..d4e1cb1e78 100644 --- a/src/taxonomy/competency-management/data/api.ts +++ b/src/taxonomy/competency-management/data/api.ts @@ -1,11 +1,17 @@ import { camelCaseObject, getConfig } from '@edx/frontend-platform'; import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth'; import type { + CompetencyCriteriaGroup, CompetencyCriteriaGroupsResponse, + CompetencyCriterion, + CompetencyGroupLogicOperator, CompetencyRuleProfile, CompetencyRuleProfileListResponse, CreateCompetencyCriterionPayload, CreateCompetencyCriterionResponse, + GradeRulePayload, + UpdateCompetencyCriteriaGroupPayload, + UpdateCompetencyCriteriaRulePayload, } from './types'; const getApiBaseUrl = () => getConfig().STUDIO_BASE_URL; @@ -40,6 +46,12 @@ export const apiUrls = { defaultCompetencyRuleProfile: () => makeCbeV1Url('rule_profiles/'), /** POST: create a new criterion under this competency (tag). Backs `#665`. */ createCompetencyCriterion: (tagId: number) => makeUrl(`${tagId}/criteria/`), + /** PATCH: update a bottom-tier group's any/all combining logic. Backs `#760`. */ + updateCompetencyCriteriaGroup: (tagId: number, groupId: number) => makeUrl(`${tagId}/criteria-groups/${groupId}/`), + /** PATCH: batch-update a rule box's score across every criterion sharing it. Backs `#759` - + * not nested under `competencies//`, so built with `makeCbeV1Url` instead of `makeUrl`. + */ + updateCompetencyCriteriaRule: (groupId: number) => makeCbeV1Url(`criteria-groups/${groupId}/criteria/bulk-update/`), } satisfies Record string>; /** @@ -86,3 +98,53 @@ export async function createCompetencyCriterion( const { data } = await getAuthenticatedHttpClient().post(apiUrls.createCompetencyCriterion(tagId), payload); return camelCaseObject(data); } + +/** + * Update a bottom-tier group's any/all combining logic (`#760`). + * @param tagId The competency (tag) the group belongs to. + * @param groupId The bottom-tier group to update. + * @param logicOperator The new operator. + */ +export async function updateCompetencyCriteriaGroupOperator( + tagId: number, + groupId: number, + logicOperator: CompetencyGroupLogicOperator, +): Promise { + const payload: UpdateCompetencyCriteriaGroupPayload = { logic_operator: logicOperator }; + const { data } = await getAuthenticatedHttpClient().patch( + apiUrls.updateCompetencyCriteriaGroup(tagId, groupId), + payload, + ); + return camelCaseObject(data); +} + +/** + * Batch-update a rule box's score threshold, across every criterion that + * currently shares it (`#759`). Always sends explicit override values, + * never a shared rule-profile reference - the backend reassigns a + * criterion back to a matching default profile itself when the submitted + * value matches it. Atomic; a `criterionIds` entry outside the target + * group, or nonexistent, rejects the whole request and changes nothing. + * @param groupId The bottom-tier group the batched criteria belong to. + * @param criterionIds Every criterion currently in the rule box being edited. + * @param ruleType The rule box's own rule type, pinned unchanged - only the + * numeric value is user-editable (`#794`'s own scope). + * @param rulePayload The new score threshold, applied to every listed criterion. + * @returns The updated criteria as actually persisted - the caller derives + * the box's new focus key from this, not from the request, since a "reset + * to default" edit may echo back a profile reference instead. + */ +export async function updateCompetencyCriteriaRule( + groupId: number, + criterionIds: number[], + ruleType: string, + rulePayload: GradeRulePayload, +): Promise { + const payload: UpdateCompetencyCriteriaRulePayload = { + criterion_ids: criterionIds, + rule_type_override: ruleType, + rule_payload_override: rulePayload, + }; + const { data } = await getAuthenticatedHttpClient().patch(apiUrls.updateCompetencyCriteriaRule(groupId), payload); + return camelCaseObject(data); +} diff --git a/src/taxonomy/competency-management/data/apiHooks.test.tsx b/src/taxonomy/competency-management/data/apiHooks.test.tsx index f0c1320711..3d75e66597 100644 --- a/src/taxonomy/competency-management/data/apiHooks.test.tsx +++ b/src/taxonomy/competency-management/data/apiHooks.test.tsx @@ -16,6 +16,8 @@ import { useCourseTaggingPermissions, useCreateCompetencyCriterion, useDefaultCompetencyRuleProfile, + useUpdateCompetencyCriteriaGroupOperator, + useUpdateCompetencyCriteriaRule, } from './apiHooks'; jest.mock('@src/authz/data/apiHooks', () => ({ @@ -162,6 +164,84 @@ describe('useCreateCompetencyCriterion', () => { }); }); +describe('useUpdateCompetencyCriteriaGroupOperator', () => { + it('invalidates that tagId\'s groups query on success', async () => { + const { axiosMock, queryClient } = initializeMocks(); + axiosMock.onPatch(apiUrls.updateCompetencyCriteriaGroup(tagId, 10)).reply(200, { + id: 10, + parent_id: 1, + depth: 2, + ordering: 0, + logic_operator: 'OR', + }); + queryClient.setQueryData(competencyQueryKeys.competencyCriteriaGroups(tagId), { groups: [], criteria: [] }); + + const { result } = renderHook(() => useUpdateCompetencyCriteriaGroupOperator(), { wrapper: makeWrapper() }); + + await act(async () => { + await result.current.mutateAsync({ tagId, groupId: 10, logicOperator: 'or' }); + }); + + const state = queryClient.getQueryState(competencyQueryKeys.competencyCriteriaGroups(tagId)); + expect(state?.isInvalidated).toBe(true); + }); + + it('does not invalidate a different tagId\'s groups query', async () => { + const { axiosMock, queryClient } = initializeMocks(); + const otherTagId = 7; + axiosMock.onPatch(apiUrls.updateCompetencyCriteriaGroup(tagId, 10)).reply(200, { + id: 10, + parent_id: 1, + depth: 2, + ordering: 0, + logic_operator: 'OR', + }); + queryClient.setQueryData(competencyQueryKeys.competencyCriteriaGroups(otherTagId), { groups: [], criteria: [] }); + + const { result } = renderHook(() => useUpdateCompetencyCriteriaGroupOperator(), { wrapper: makeWrapper() }); + + await act(async () => { + await result.current.mutateAsync({ tagId, groupId: 10, logicOperator: 'or' }); + }); + + const state = queryClient.getQueryState(competencyQueryKeys.competencyCriteriaGroups(otherTagId)); + expect(state?.isInvalidated).toBe(false); + }); +}); + +describe('useUpdateCompetencyCriteriaRule', () => { + it('invalidates that tagId\'s groups query on success', async () => { + const { axiosMock, queryClient } = initializeMocks(); + const groupId = 10; + axiosMock.onPatch(apiUrls.updateCompetencyCriteriaRule(groupId)).reply(200, [ + { + id: 101, + object_id: 'block-a', + competency_criteria_group_id: groupId, + rule_profile_id: null, + rule_type_override: 'grade', + rule_payload_override: { op: 'gte', value: 0.8, scale: 'percent' }, + }, + ]); + queryClient.setQueryData(competencyQueryKeys.competencyCriteriaGroups(tagId), { groups: [], criteria: [] }); + + const { result } = renderHook(() => useUpdateCompetencyCriteriaRule(), { wrapper: makeWrapper() }); + + await act(async () => { + await result.current.mutateAsync({ + tagId, + groupId, + criterionIds: [101], + ruleType: 'grade', + rulePayload: { op: 'gte', value: 0.8, scale: 'percent' }, + }); + }); + + const state = queryClient.getQueryState(competencyQueryKeys.competencyCriteriaGroups(tagId)); + expect(state?.isInvalidated).toBe(true); + }); +}); + describe('useCourseTaggingPermissions', () => { beforeEach(() => { initializeMocks(); diff --git a/src/taxonomy/competency-management/data/apiHooks.ts b/src/taxonomy/competency-management/data/apiHooks.ts index 55717df873..eb2776cbf2 100644 --- a/src/taxonomy/competency-management/data/apiHooks.ts +++ b/src/taxonomy/competency-management/data/apiHooks.ts @@ -8,7 +8,14 @@ import { useWaffleFlags } from '@src/data/apiHooks'; import type { PermissionValidationQuery } from '@src/authz/types'; import * as api from './api'; -import type { CreateCompetencyCriterionPayload, CreateCompetencyCriterionResponse } from './types'; +import type { + CompetencyCriteriaGroup, + CompetencyCriterion, + CompetencyGroupLogicOperator, + CreateCompetencyCriterionPayload, + CreateCompetencyCriterionResponse, + GradeRulePayload, +} from './types'; /** * Query key factory for competency-management data, following the same @@ -76,6 +83,59 @@ export const useCreateCompetencyCriterion = () => { }); }; +/** + * Build the mutation to update a bottom-tier group's any/all combining + * logic (`#760`). Invalidates that competency's criteria-groups query on + * success so the new operator shows up without a manual refetch. + */ +export const useUpdateCompetencyCriteriaGroupOperator = () => { + const queryClient = useQueryClient(); + return useMutation< + CompetencyCriteriaGroup, + AxiosError>, + { tagId: number; groupId: number; logicOperator: CompetencyGroupLogicOperator; } + >({ + mutationFn: ( + { tagId, groupId, logicOperator }: { + tagId: number; + groupId: number; + logicOperator: CompetencyGroupLogicOperator; + }, + ) => api.updateCompetencyCriteriaGroupOperator(tagId, groupId, logicOperator), + onSuccess: (_data, { tagId }) => { + queryClient.invalidateQueries({ queryKey: competencyQueryKeys.competencyCriteriaGroups(tagId) }); + }, + }); +}; + +/** + * Build the mutation to batch-update a rule box's score threshold (`#759`). + * Invalidates that competency's criteria-groups query on success. `tagId` + * is only used for that invalidation - the endpoint's own URL isn't nested + * under a competency. + */ +export const useUpdateCompetencyCriteriaRule = () => { + const queryClient = useQueryClient(); + return useMutation< + CompetencyCriterion[], + AxiosError>, + { tagId: number; groupId: number; criterionIds: number[]; ruleType: string; rulePayload: GradeRulePayload; } + >({ + mutationFn: ( + { groupId, criterionIds, ruleType, rulePayload }: { + tagId: number; + groupId: number; + criterionIds: number[]; + ruleType: string; + rulePayload: GradeRulePayload; + }, + ) => api.updateCompetencyCriteriaRule(groupId, criterionIds, ruleType, rulePayload), + onSuccess: (_data, { tagId }) => { + queryClient.invalidateQueries({ queryKey: competencyQueryKeys.competencyCriteriaGroups(tagId) }); + }, + }); +}; + export interface UseCourseTaggingPermissionsReturn { isLoading: boolean; isAuthzEnabled: boolean; diff --git a/src/taxonomy/competency-management/data/types.ts b/src/taxonomy/competency-management/data/types.ts index 5bd428c2ba..fb945abd28 100644 --- a/src/taxonomy/competency-management/data/types.ts +++ b/src/taxonomy/competency-management/data/types.ts @@ -3,9 +3,10 @@ * `docs/openedx_learning/decisions/0002-competency-criteria-model.rst` * (ADR 0002) in `openedx-core`. Every read-hook response is normalized * through `camelCaseObject` (see `./apiHooks.ts`), so these describe the - * camelCase shape the app consumes, never the wire snake_case - except - * `CreateCompetencyCriterionPayload`, a request body, which goes out as the - * API expects (snake_case). + * camelCase shape the app consumes, never the wire snake_case - except the + * request-body payload types (`CreateCompetencyCriterionPayload`, + * `UpdateCompetencyCriteriaGroupPayload`, `UpdateCompetencyCriteriaRulePayload`), + * which go out as the API expects (snake_case). */ /** How a group's immediate children combine: "AND" (all) or "OR" (any). A @@ -164,3 +165,27 @@ export interface CreateCompetencyCriterionPayload { * linking the content object to the competency. */ export type CreateCompetencyCriterionResponse = Omit & { objectTagId: number; }; + +/** Request body for updating a bottom-tier group's any/all combining logic + * (`#760`). `logic_operator` is the only field, so this is just + * `CompetencyGroupLogicOperator` under its own wire key - named separately + * from `CompetencyGroupLogicOperator` itself for symmetry with + * `CreateCompetencyCriterionPayload`/`UpdateCompetencyCriteriaRulePayload`, + * the other request-body types in this file. + */ +export interface UpdateCompetencyCriteriaGroupPayload { + logic_operator: CompetencyGroupLogicOperator; +} + +/** Request body for updating a rule box's score threshold (`#759`), batched + * across every criterion that currently shares the box. Always carries + * explicit override values, never a `competency_rule_profile_id` - the + * backend reassigns a criterion back to its default profile itself when + * the submitted value matches it, so "reset to default" needs no special + * payload here. + */ +export interface UpdateCompetencyCriteriaRulePayload { + criterion_ids: number[]; + rule_type_override: string; + rule_payload_override: GradeRulePayload; +} diff --git a/src/taxonomy/competency-management/messages.ts b/src/taxonomy/competency-management/messages.ts index 9fdc811ff2..ed6aebe5f9 100644 --- a/src/taxonomy/competency-management/messages.ts +++ b/src/taxonomy/competency-management/messages.ts @@ -56,6 +56,16 @@ const messages = defineMessages({ + 'new criterion because an ancestor or descendant competency already has criteria in the same course ' + '(a 400 response with a "tag_id" field error).', }, + updateGroupOperatorFailedToastMessage: { + id: 'course-authoring.competency-management.update-group-operator-failed-toast-message', + defaultMessage: 'There was a problem updating this group\'s logic. Please try again.', + description: 'Toast shown when changing a bottom-tier group\'s any/all combining logic fails.', + }, + updateRuleScoreFailedToastMessage: { + id: 'course-authoring.competency-management.update-rule-score-failed-toast-message', + defaultMessage: 'There was a problem updating this rule\'s score. Please try again.', + description: 'Toast shown when changing a rule box\'s score threshold fails.', + }, }); export default messages; diff --git a/src/taxonomy/competency-management/testHelpers.tsx b/src/taxonomy/competency-management/testHelpers.tsx index 8d02f352d5..0eb8d9e16f 100644 --- a/src/taxonomy/competency-management/testHelpers.tsx +++ b/src/taxonomy/competency-management/testHelpers.tsx @@ -19,6 +19,8 @@ export const buildMockCompetencyAssociationsContextValue = ( focusRuleBox: jest.fn(), notifyCourseExpanded: jest.fn(), associateSubsection: jest.fn(), + updateGroupOperator: jest.fn(), + updateRuleScore: jest.fn().mockResolvedValue(undefined), canEditCourse: () => true, groupsQuery: { isLoading: false, From 2eb343360818cfb3f587be843114efe358ba2f67 Mon Sep 17 00:00:00 2001 From: AShatsila <319486855+AShatsila@users.noreply.github.com> Date: Tue, 6 Oct 2026 15:34:50 +0400 Subject: [PATCH 06/12] fix: a11y --- .../criteria-groups/ScoreThresholdField.tsx | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.tsx b/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.tsx index cb5daeb36a..580e0b4d21 100644 --- a/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.tsx +++ b/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.tsx @@ -1,4 +1,4 @@ -import { useState } from 'react'; +import { useId, useState } from 'react'; import type { KeyboardEvent } from 'react'; import { useIntl } from '@edx/frontend-platform/i18n'; import { Form, Icon } from '@openedx/paragon'; @@ -45,6 +45,7 @@ const percentOf = (rulePayload: GradeRulePayload): string => String(Math.round(r */ const ScoreThresholdField = ({ rulePayload, onChange, getInlineValidationMessage }: ScoreThresholdFieldProps) => { const intl = useIntl(); + const inputId = useId(); const percent = Math.round(rulePayload.value * 100); const suffixMessage = SUFFIX_MESSAGE_BY_OP[rulePayload.op]; @@ -122,7 +123,7 @@ const ScoreThresholdField = ({ rulePayload, onChange, getInlineValidationMessage }; return ( - + {intl.formatMessage(messages.scoreThresholdLabel, { threshold: ( @@ -133,6 +134,7 @@ const ScoreThresholdField = ({ rulePayload, onChange, getInlineValidationMessage className="score-threshold-field__input" value={inputValue} aria-label={intl.formatMessage(messages.scoreThresholdInputAccessibleLabel)} + aria-invalid={!!validationMessage} isInvalid={!!validationMessage} onChange={(event) => setInputValue(event.target.value)} onKeyDown={handleKeyDown} @@ -145,11 +147,11 @@ const ScoreThresholdField = ({ rulePayload, onChange, getInlineValidationMessage })} {suffixMessage && ` ${intl.formatMessage(suffixMessage)}`} {validationMessage && ( -
+ {validationMessage} -
+ )} -
+
); }; From 15fbc6094275b1d1b45c68e3d9d9a306f72efec6 Mon Sep 17 00:00:00 2001 From: AShatsila <319486855+AShatsila@users.noreply.github.com> Date: Wed, 7 Oct 2026 19:11:13 +0400 Subject: [PATCH 07/12] feat: update test cases --- .../CompetencyAssociationsContext.test.tsx | 4 +-- .../CriteriaGroupBox.edit.test.tsx | 21 +++++++++------ .../criteria-groups/CriteriaGroupBox.test.tsx | 27 ++++++++++++++----- .../LogicOperatorSelect.test.tsx | 6 ++--- .../criteria-groups/RuleBox.edit.test.tsx | 2 +- .../competency-management/data/api.test.ts | 20 +++++++++----- .../data/apiHooks.test.tsx | 16 +++++++---- 7 files changed, 64 insertions(+), 32 deletions(-) diff --git a/src/taxonomy/competency-management/CompetencyAssociationsContext.test.tsx b/src/taxonomy/competency-management/CompetencyAssociationsContext.test.tsx index 6f1fcbf8b9..51c14bdf30 100644 --- a/src/taxonomy/competency-management/CompetencyAssociationsContext.test.tsx +++ b/src/taxonomy/competency-management/CompetencyAssociationsContext.test.tsx @@ -219,7 +219,7 @@ const TestConsumer = () => { > associate-duplicate - + + , + ); + + const input = screen.getByRole('textbox'); + await user.clear(input); + await user.type(input, '85'); + await user.keyboard('{Enter}'); + // Enter's own commit is now in flight (its mutation promise hasn't + // resolved yet) - a second attempt, from blurring right after, must + // not issue a second, possibly-out-of-order, request. + expect(input).toBeDisabled(); + await user.click(screen.getByRole('button', { name: 'elsewhere' })); + + expect(onChange).toHaveBeenCalledTimes(1); + resolveOnChange!(); + await waitFor(() => expect(input).not.toBeDisabled()); + }); + it('blocks the commit and shows the inline message when getInlineValidationMessage returns one', async () => { const user = userEvent.setup(); const onChange = jest.fn().mockResolvedValue(undefined); diff --git a/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.tsx b/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.tsx index 580e0b4d21..71d500ee21 100644 --- a/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.tsx +++ b/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.tsx @@ -1,4 +1,4 @@ -import { useId, useState } from 'react'; +import { useId, useRef, useState } from 'react'; import type { KeyboardEvent } from 'react'; import { useIntl } from '@edx/frontend-platform/i18n'; import { Form, Icon } from '@openedx/paragon'; @@ -56,6 +56,18 @@ const ScoreThresholdField = ({ rulePayload, onChange, getInlineValidationMessage // key, so it needs its own explicit revert (see `commit` below). const [inputValue, setInputValue] = useState(() => percentOf(rulePayload)); const [validationMessage, setValidationMessage] = useState(''); + // Serializes commits: Enter fires `commit`, and the same Enter keypress + // typically also blurs the field (or a user tabs away right after), + // firing `commit` again before the first mutation has resolved - two + // concurrent requests whose responses can land out of order, leaving an + // earlier edit's value persisted over a later one. `saveInFlight` (a ref, + // not state, so it's already set before React re-renders - synchronously + // blocking a second call from the same event-handling pass) and + // `isSaving` (state, so the input can visibly disable) together make a + // second commit attempt, from either Enter or blur, a no-op until the + // in-flight one settles. + const saveInFlight = useRef(false); + const [isSaving, setIsSaving] = useState(false); if (!onChange) { return ( @@ -78,7 +90,10 @@ const ScoreThresholdField = ({ rulePayload, onChange, getInlineValidationMessage setValidationMessage(''); }; - const commit = () => { + const commit = async () => { + if (saveInFlight.current) { + return; + } const trimmedInput = inputValue.trim(); const numericPercent = Number(trimmedInput); if (trimmedInput === '' || !Number.isFinite(numericPercent)) { @@ -103,17 +118,24 @@ const ScoreThresholdField = ({ rulePayload, onChange, getInlineValidationMessage } setValidationMessage(''); const newRulePayload: GradeRulePayload = { ...rulePayload, value: numericPercent / 100 }; - onChange(newRulePayload).catch(() => { + saveInFlight.current = true; + setIsSaving(true); + try { + await onChange(newRulePayload); + } catch { // Unlike `LogicOperatorSelect`'s `Dropdown` (which always renders // from its `value` prop), this input's local state doesn't // self-correct on rejection - revert it explicitly. revertToLastPersisted(); - }); + } finally { + saveInFlight.current = false; + setIsSaving(false); + } }; const handleKeyDown = (event: KeyboardEvent) => { if (event.key === 'Enter') { - commit(); + void commit(); } else if (event.key === 'Escape') { revertToLastPersisted(); } @@ -136,9 +158,12 @@ const ScoreThresholdField = ({ rulePayload, onChange, getInlineValidationMessage aria-label={intl.formatMessage(messages.scoreThresholdInputAccessibleLabel)} aria-invalid={!!validationMessage} isInvalid={!!validationMessage} + disabled={isSaving} onChange={(event) => setInputValue(event.target.value)} onKeyDown={handleKeyDown} - onBlur={commit} + onBlur={() => { + void commit(); + }} onClick={(event: React.MouseEvent) => event.stopPropagation()} /> % From d3b82439a0d07bf447f53debc9a06b1dd012739e Mon Sep 17 00:00:00 2001 From: AShatsila <319486855+AShatsila@users.noreply.github.com> Date: Thu, 15 Oct 2026 19:41:40 +0400 Subject: [PATCH 09/12] refactor: comments, test cases, criteria group box and score --- .../CompetencyAssociationsContext.test.tsx | 82 ++++++++--------- .../CompetencyAssociationsContext.tsx | 28 +++--- .../CriteriaGroupBox.edit.test.tsx | 34 +------- .../criteria-groups/CriteriaGroupBox.test.tsx | 82 ++++++----------- .../criteria-groups/CriteriaGroupBox.tsx | 57 +++++------- .../LogicOperatorSelect.test.tsx | 6 -- .../criteria-groups/LogicOperatorSelect.tsx | 3 - .../criteria-groups/RuleBox.edit.test.tsx | 60 +------------ .../criteria-groups/RuleBox.test.tsx | 7 +- .../criteria-groups/RuleBox.tsx | 47 +++------- .../criteria-groups/RuleBoxList.test.tsx | 87 ++++++++++++++++++- .../criteria-groups/RuleBoxList.tsx | 15 +--- .../ScoreThresholdField.test.tsx | 62 ++++++++++--- .../criteria-groups/ScoreThresholdField.tsx | 67 ++++---------- .../criteria-groups/criteria-groups.scss | 24 +++++ .../criteria-groups/messages.ts | 12 +++ .../competency-management/data/api.test.ts | 4 +- .../competency-management/data/api.ts | 8 +- .../data/apiHooks.test.tsx | 25 ------ .../competency-management/data/apiHooks.ts | 27 +++--- .../competency-management/data/types.ts | 9 +- 21 files changed, 329 insertions(+), 417 deletions(-) diff --git a/src/taxonomy/competency-management/CompetencyAssociationsContext.test.tsx b/src/taxonomy/competency-management/CompetencyAssociationsContext.test.tsx index 51c14bdf30..e9e7000802 100644 --- a/src/taxonomy/competency-management/CompetencyAssociationsContext.test.tsx +++ b/src/taxonomy/competency-management/CompetencyAssociationsContext.test.tsx @@ -223,10 +223,7 @@ const TestConsumer = () => { {intl.formatMessage(messages.criteriaGroupBoxLabel, { operator: ( ', () => { fireEvent.click(screen.getByText('any')); expect(onChange).toHaveBeenCalledWith('OR'); - // A rejected save never updates `group.logicOperator` - simulated - // here by re-rendering with `value` unchanged, exactly what the real - // caller does when the mutation's `onError` fires and no local - // rollback of its own is needed (see this component's own docstring). - // The trigger itself (`.dropdown-toggle`, not the still-mounted-but- - // hidden menu items) is checked directly, since Paragon's `Dropdown` // keeps its closed menu's items in the DOM rather than removing them. rerender(); diff --git a/src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.tsx b/src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.tsx index 8db39b73b9..81abc3083e 100644 --- a/src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.tsx +++ b/src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.tsx @@ -15,9 +15,6 @@ export interface LogicOperatorSelectProps { * control serves both contexts with different wording. */ labels: LogicOperatorSelectLabels; - /** Omitted where only the value is read: renders plain, non-interactive - * text in that case. Passing it renders the interactive control instead. - */ onChange?: (value: CompetencyGroupLogicOperator) => void; className?: string; } diff --git a/src/taxonomy/competency-management/criteria-groups/RuleBox.edit.test.tsx b/src/taxonomy/competency-management/criteria-groups/RuleBox.edit.test.tsx index bbc62acb4a..3f9b50dcb3 100644 --- a/src/taxonomy/competency-management/criteria-groups/RuleBox.edit.test.tsx +++ b/src/taxonomy/competency-management/criteria-groups/RuleBox.edit.test.tsx @@ -3,7 +3,6 @@ import { render, screen, userEvent, - waitFor, } from '@src/testUtils'; import { buildMockCompetencyAssociationsContextValue, MockCompetencyAssociationsProvider } from '../testHelpers'; import type { CompetencyCriterion } from '../data/types'; @@ -22,7 +21,7 @@ const criteria: CompetencyCriterion[] = [ const rule = { ruleType: 'grade', rulePayload: { op: 'gte' as const, value: 0.7, scale: 'percent' as const } }; -const renderRuleBox = ( +const render70PercentRuleBox = ( contextOverrides: Parameters[0] = {}, props: Partial> = {}, ) => ( @@ -50,15 +49,8 @@ describe(' editing (#794)', () => { jest.clearAllMocks(); }); - it('renders the score as plain text, not an input, when canEdit is false', () => { - renderRuleBox({}, { canEdit: false }); - - expect(screen.getByText('With a score of 70% or higher')).toBeInTheDocument(); - expect(screen.queryByRole('textbox')).not.toBeInTheDocument(); - }); - it('renders the score as plain text, not an input, when canEdit is omitted (default false)', () => { - renderRuleBox(); + render70PercentRuleBox(); expect(screen.getByText('With a score of 70% or higher')).toBeInTheDocument(); expect(screen.queryByRole('textbox')).not.toBeInTheDocument(); @@ -69,7 +61,7 @@ describe(' editing (#794)', () => { async () => { const user = userEvent.setup(); const updateRuleScore = jest.fn().mockResolvedValue(undefined); - renderRuleBox({ updateRuleScore }, { canEdit: true }); + render70PercentRuleBox({ updateRuleScore }, { canEdit: true }); const input = screen.getByRole('textbox'); await user.clear(input); @@ -80,54 +72,10 @@ describe(' editing (#794)', () => { }, ); - it('blocks the commit and shows the inline message when getInlineValidationMessage flags a duplicate', async () => { - const user = userEvent.setup(); - const updateRuleScore = jest.fn().mockResolvedValue(undefined); - const getInlineValidationMessage = jest.fn().mockReturnValue( - 'Another rule box in this group already uses this score.', - ); - renderRuleBox({ updateRuleScore }, { canEdit: true, getInlineValidationMessage }); - - const input = screen.getByRole('textbox'); - await user.clear(input); - await user.type(input, '85'); - await user.keyboard('{Enter}'); - - expect(updateRuleScore).not.toHaveBeenCalled(); - expect(screen.getByText('Another rule box in this group already uses this score.')).toBeInTheDocument(); - }); - - it('does not call updateRuleScore when the input round-trips to the same displayed percent', async () => { - const user = userEvent.setup(); - const updateRuleScore = jest.fn().mockResolvedValue(undefined); - renderRuleBox({ updateRuleScore }, { canEdit: true }); - - const input = screen.getByRole('textbox'); - await user.clear(input); - await user.type(input, '70'); - await user.keyboard('{Enter}'); - - expect(updateRuleScore).not.toHaveBeenCalled(); - }); - - it('reverts the input to the prop value when the commit is rejected', async () => { - const user = userEvent.setup(); - const updateRuleScore = jest.fn().mockRejectedValue(new Error('save failed')); - renderRuleBox({ updateRuleScore }, { canEdit: true }); - - const input = screen.getByRole('textbox'); - await user.clear(input); - await user.type(input, '85'); - await user.keyboard('{Enter}'); - - expect(updateRuleScore).toHaveBeenCalledWith(10, [1], { op: 'gte', value: 0.85, scale: 'percent' }); - await waitFor(() => expect(input).toHaveValue('70')); - }); - it('does not call focusRuleBox when the score input itself is clicked', async () => { const user = userEvent.setup(); const focusRuleBox = jest.fn(); - renderRuleBox({ focusRuleBox }, { canEdit: true }); + render70PercentRuleBox({ focusRuleBox }, { canEdit: true }); await user.click(screen.getByRole('textbox')); diff --git a/src/taxonomy/competency-management/criteria-groups/RuleBox.test.tsx b/src/taxonomy/competency-management/criteria-groups/RuleBox.test.tsx index 28045733e6..2c03f97fdf 100644 --- a/src/taxonomy/competency-management/criteria-groups/RuleBox.test.tsx +++ b/src/taxonomy/competency-management/criteria-groups/RuleBox.test.tsx @@ -52,14 +52,11 @@ describe('', () => { expect(screen.getByText('Subsection A')).toBeInTheDocument(); }); - it('calls focusRuleBox with its own groupId/ruleKey when clicked', () => { + it('calls focusRuleBox with its own groupId/ruleKey when the select button is clicked', () => { const focusRuleBox = jest.fn(); const { container } = renderRuleBox({ focusRuleBox }); - // Queried by class, not `getByRole('button')`: the wrapper has no - // `role="button"` (see `RuleBox.tsx`'s own comment on why), since a real - // ` ', () => { , ); - // Queried by class, not `getAllByRole('button')`: a rule box has no - // `role="button"` (see `RuleBox.tsx`'s own comment on why). const boxes = container.querySelectorAll('.rule-box'); expect(boxes).toHaveLength(2); // Box 1 (min criterion id 101) holds both Subsection A and B. @@ -135,4 +138,82 @@ describe('', () => { expect(box.className).not.toContain('rule-box--focused'); }); }); + + describe('duplicate-score validation (real getInlineValidationMessage, not mocked)', () => { + const duplicateCheckResponse: CompetencyCriteriaGroupsResponse = { + groups: [ + { + id: 10, + parentId: null, + tagId: 42, + courseKey: null, + name: 'leaf', + ordering: 0, + logicOperator: 'AND', + archived: false, + }, + ], + criteria: [ + { + id: 201, + objectId: 'block-x', + groupId: 10, + ruleProfileId: null, + ruleTypeOverride: 'grade', + rulePayloadOverride: { op: 'gte', value: 0.6, scale: 'percent' }, + }, + { + id: 202, + objectId: 'block-y', + groupId: 10, + ruleProfileId: null, + ruleTypeOverride: 'grade', + rulePayloadOverride: { op: 'gte', value: 0.8, scale: 'percent' }, + }, + ], + }; + + const renderEditableBoxes = (updateRuleScore: jest.Mock) => { + const index = buildCompetencyCriteriaGroupsIndex(duplicateCheckResponse); + return render( + + + , + ); + }; + + it('refuses to save a score that would duplicate another box\'s own rule in the same group', async () => { + const user = userEvent.setup(); + const updateRuleScore = jest.fn().mockResolvedValue(undefined); + renderEditableBoxes(updateRuleScore); + + const [firstBoxInput] = screen.getAllByRole('textbox'); + await user.clear(firstBoxInput); + await user.type(firstBoxInput, '80'); + await user.keyboard('{Enter}'); + + expect(updateRuleScore).not.toHaveBeenCalled(); + expect(screen.getByText('Another rule box in this group already uses this score.')).toBeInTheDocument(); + }); + + it('does not treat retyping a box\'s own current score as a duplicate of itself', async () => { + const user = userEvent.setup(); + const updateRuleScore = jest.fn().mockResolvedValue(undefined); + renderEditableBoxes(updateRuleScore); + + const [firstBoxInput] = screen.getAllByRole('textbox'); + await user.clear(firstBoxInput); + await user.type(firstBoxInput, '60'); + await user.keyboard('{Enter}'); + + expect(updateRuleScore).not.toHaveBeenCalled(); + expect(screen.queryByText('Another rule box in this group already uses this score.')).not.toBeInTheDocument(); + }); + }); }); diff --git a/src/taxonomy/competency-management/criteria-groups/RuleBoxList.tsx b/src/taxonomy/competency-management/criteria-groups/RuleBoxList.tsx index d9ce5da1d2..e1b5bbc954 100644 --- a/src/taxonomy/competency-management/criteria-groups/RuleBoxList.tsx +++ b/src/taxonomy/competency-management/criteria-groups/RuleBoxList.tsx @@ -10,11 +10,6 @@ export interface RuleBoxListProps { index: CompetencyCriteriaGroupsIndex; systemDefaultProfile: CompetencyRuleProfile; subsectionNamesByUsageKey: Record; - /** Threaded down from `CriteriaGroupBox` (originally resolved by - * `CourseGroupSection` via `canEditCourse`) - see `CriteriaGroupBoxProps.canEdit`. - * Optional, defaulting to `false`, so every caller that predates this prop - * keeps working unchanged. - */ canEdit?: boolean; } @@ -23,11 +18,6 @@ export interface RuleBoxListProps { * from `CompetencyAssociationsContext` directly (given `groupId` and its * own box key to compare against), so no focus state is threaded through * here. - * - * Also builds each box's own `getInlineValidationMessage` (the - * duplicate-score check) here, using `boxes`/`index`/`systemDefaultProfile`, - * which this component already has - `RuleBox` takes no new data dependency - * for it. */ const RuleBoxList = ({ groupId, @@ -39,9 +29,8 @@ const RuleBoxList = ({ const intl = useIntl(); const boxes = ruleBoxesForGroup(groupId, index, systemDefaultProfile); - // Compares rounded percent + rule type + op, not `ruleKeyOf`'s raw string - // key (built from a raw fraction, which risks a formatting mismatch that - // silently never fires). Excludes the box being edited by its own `key`. + // Compares rounded percent + rule type + op (not `ruleKeyOf`'s raw key, which risks a + // formatting-mismatch false negative); excludes the box being edited by its own `key`. const getInlineValidationMessage = (candidateBox: RuleBoxData) => (value: string): string => { const candidatePercent = Math.round(Number(value)); const isDuplicate = boxes.some((other) => ( diff --git a/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.test.tsx b/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.test.tsx index 2473df7d60..b86a00ed72 100644 --- a/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.test.tsx +++ b/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.test.tsx @@ -47,12 +47,12 @@ describe('', () => { }); describe('editable (onChange given)', () => { - const rulePayload = { op: 'gte' as const, value: 0.7, scale: 'percent' as const }; + const rulePayload70Percent = { op: 'gte' as const, value: 0.7, scale: 'percent' as const }; it('commits the typed percent, as a fraction, on Enter - op/scale pinned unchanged', async () => { const user = userEvent.setup(); const onChange = jest.fn().mockResolvedValue(undefined); - render(); + render(); const input = screen.getByRole('textbox'); await user.clear(input); @@ -67,7 +67,7 @@ describe('', () => { const onChange = jest.fn().mockResolvedValue(undefined); render( <> - + , ); @@ -83,7 +83,7 @@ describe('', () => { it('Escape reverts the input to the last-persisted value without committing', async () => { const user = userEvent.setup(); const onChange = jest.fn().mockResolvedValue(undefined); - render(); + render(); const input = screen.getByRole('textbox'); await user.clear(input); @@ -97,7 +97,7 @@ describe('', () => { it('reverts the input to the prop value when the commit is rejected', async () => { const user = userEvent.setup(); const onChange = jest.fn().mockRejectedValue(new Error('save failed')); - render(); + render(); const input = screen.getByRole('textbox'); await user.clear(input); @@ -118,7 +118,7 @@ describe('', () => { ); render( <> - + , ); @@ -144,7 +144,7 @@ describe('', () => { const getInlineValidationMessage = jest.fn().mockReturnValue('Duplicate score.'); render( , @@ -165,7 +165,7 @@ describe('', () => { const onChange = jest.fn().mockResolvedValue(undefined); render( <> - + , ); @@ -180,7 +180,7 @@ describe('', () => { it('does not call onChange when the typed value round-trips to the same displayed percent', async () => { const user = userEvent.setup(); const onChange = jest.fn().mockResolvedValue(undefined); - render(); + render(); const input = screen.getByRole('textbox'); await user.clear(input); @@ -193,7 +193,7 @@ describe('', () => { it('reverts, rather than committing 0%, when the input is cleared entirely', async () => { const user = userEvent.setup(); const onChange = jest.fn().mockResolvedValue(undefined); - render(); + render(); const input = screen.getByRole('textbox'); await user.clear(input); @@ -202,5 +202,47 @@ describe('', () => { expect(onChange).not.toHaveBeenCalled(); expect(input).toHaveValue('70'); }); + + it('blocks the commit and shows the range message for a value above 100', async () => { + const user = userEvent.setup(); + const onChange = jest.fn().mockResolvedValue(undefined); + render(); + + const input = screen.getByRole('textbox'); + await user.clear(input); + await user.type(input, '150'); + await user.keyboard('{Enter}'); + + expect(onChange).not.toHaveBeenCalled(); + expect(screen.getByText('Enter a whole number from 0 to 100.')).toBeInTheDocument(); + }); + + it('blocks the commit and shows the range message for a negative value', async () => { + const user = userEvent.setup(); + const onChange = jest.fn().mockResolvedValue(undefined); + render(); + + const input = screen.getByRole('textbox'); + await user.clear(input); + await user.type(input, '-5'); + await user.keyboard('{Enter}'); + + expect(onChange).not.toHaveBeenCalled(); + expect(screen.getByText('Enter a whole number from 0 to 100.')).toBeInTheDocument(); + }); + + it('blocks the commit and shows the range message for a decimal value', async () => { + const user = userEvent.setup(); + const onChange = jest.fn().mockResolvedValue(undefined); + render(); + + const input = screen.getByRole('textbox'); + await user.clear(input); + await user.type(input, '85.5'); + await user.keyboard('{Enter}'); + + expect(onChange).not.toHaveBeenCalled(); + expect(screen.getByText('Enter a whole number from 0 to 100.')).toBeInTheDocument(); + }); }); }); diff --git a/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.tsx b/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.tsx index 71d500ee21..64dbebb674 100644 --- a/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.tsx +++ b/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.tsx @@ -9,16 +9,12 @@ import messages from './messages'; export interface ScoreThresholdFieldProps { rulePayload: GradeRulePayload; - /** Omitted where only `rulePayload` is read: renders plain, non-interactive - * text in that case. Passing it renders the editable input instead. - * Returns a `Promise` so this component's own commit handler can revert - * its local input on a rejected save - the mutation itself still lives in - * `CompetencyAssociationsContext`. + /** Omitted: renders plain text. Given: renders the editable input, and the returned promise lets + * `saveScoreIfChanged` revert its local input on a rejected save. */ onChange?: (rulePayload: GradeRulePayload) => Promise; - /** Same reservation as `onChange`, mirroring `EditableCell`'s prop of the - * same name. Per-keystroke and display-only - it never blocks a commit by - * itself; `commit` below is what gates the commit on it. + /** Called once at commit time, from `saveScoreIfChanged` below; a non-empty return blocks the + * commit and shows the message inline instead. */ getInlineValidationMessage?: (value: string) => string; } @@ -32,40 +28,23 @@ const SUFFIX_MESSAGE_BY_OP: Record String(Math.round(rulePayload.value * 100)); -/** Closed-state label for a rule's score threshold: "With a score of X% - * or higher"/"or lower"/(no suffix, for an exact match). Read-only unless - * `onChange` is given, in which case the percentage renders as an editable - * input instead - same "read-only unless given a handler" shape - * `LogicOperatorSelect` already uses. - */ +/** Closed-state label for a rule's score threshold: "With a score of X% or higher"/"or lower"/ + * (no suffix, for an exact match). Read-only unless `onChange` is given. */ const ScoreThresholdField = ({ rulePayload, onChange, getInlineValidationMessage }: ScoreThresholdFieldProps) => { const intl = useIntl(); const inputId = useId(); const percent = Math.round(rulePayload.value * 100); const suffixMessage = SUFFIX_MESSAGE_BY_OP[rulePayload.op]; - // Local input state, mirroring `EditableCell`'s own pattern - never - // resynced from `rulePayload` while mounted. A successful edit gets a - // fresh value for free, since `RuleBoxList`'s `key={box.key}` remounts - // this box once its rule key changes. A rejected edit doesn't change that - // key, so it needs its own explicit revert (see `commit` below). + // Local input state, mirroring `EditableCell`'s pattern - never resynced from `rulePayload` + // while mounted; a rejected edit needs its own explicit revert (see `saveScoreIfChanged` below). const [inputValue, setInputValue] = useState(() => percentOf(rulePayload)); const [validationMessage, setValidationMessage] = useState(''); - // Serializes commits: Enter fires `commit`, and the same Enter keypress - // typically also blurs the field (or a user tabs away right after), - // firing `commit` again before the first mutation has resolved - two - // concurrent requests whose responses can land out of order, leaving an - // earlier edit's value persisted over a later one. `saveInFlight` (a ref, - // not state, so it's already set before React re-renders - synchronously - // blocking a second call from the same event-handling pass) and - // `isSaving` (state, so the input can visibly disable) together make a - // second commit attempt, from either Enter or blur, a no-op until the - // in-flight one settles. + // Guards against a second commit (e.g. blur right after Enter) firing before the first + // mutation resolves, which could otherwise let two out-of-order responses land in the wrong order. const saveInFlight = useRef(false); const [isSaving, setIsSaving] = useState(false); @@ -90,7 +69,7 @@ const ScoreThresholdField = ({ rulePayload, onChange, getInlineValidationMessage setValidationMessage(''); }; - const commit = async () => { + const saveScoreIfChanged = async () => { if (saveInFlight.current) { return; } @@ -100,19 +79,16 @@ const ScoreThresholdField = ({ rulePayload, onChange, getInlineValidationMessage revertToLastPersisted(); return; } + if (!Number.isInteger(numericPercent) || numericPercent < 0 || numericPercent > 100) { + setValidationMessage(intl.formatMessage(messages.scoreThresholdRangeValidationMessage)); + return; + } if (numericPercent === Math.round(rulePayload.value * 100)) { - // Nothing actually changed - e.g. a blur or Enter with no edit made, - // or a re-typed value that round-trips to the same displayed percent. - // Skip the request rather than re-sending the current value, which - // (given the display itself is a rounded percent) could otherwise - // silently move a stored fraction by a rounding error. setValidationMessage(''); return; } const message = getInlineValidationMessage ? getInlineValidationMessage(inputValue) : ''; if (message) { - // getInlineValidationMessage only sets the inline message by itself; - // gating the actual commit on it is this component's own work. setValidationMessage(message); return; } @@ -123,9 +99,6 @@ const ScoreThresholdField = ({ rulePayload, onChange, getInlineValidationMessage try { await onChange(newRulePayload); } catch { - // Unlike `LogicOperatorSelect`'s `Dropdown` (which always renders - // from its `value` prop), this input's local state doesn't - // self-correct on rejection - revert it explicitly. revertToLastPersisted(); } finally { saveInFlight.current = false; @@ -135,13 +108,10 @@ const ScoreThresholdField = ({ rulePayload, onChange, getInlineValidationMessage const handleKeyDown = (event: KeyboardEvent) => { if (event.key === 'Enter') { - void commit(); + void saveScoreIfChanged(); } else if (event.key === 'Escape') { revertToLastPersisted(); } - // Stops the wrapping `RuleBox`'s own `onKeyDown` (Enter/Space -> - // `focusRuleBox`) from also firing while typing inside this input. - event.stopPropagation(); }; return ( @@ -162,9 +132,8 @@ const ScoreThresholdField = ({ rulePayload, onChange, getInlineValidationMessage onChange={(event) => setInputValue(event.target.value)} onKeyDown={handleKeyDown} onBlur={() => { - void commit(); + void saveScoreIfChanged(); }} - onClick={(event: React.MouseEvent) => event.stopPropagation()} /> %
diff --git a/src/taxonomy/competency-management/criteria-groups/criteria-groups.scss b/src/taxonomy/competency-management/criteria-groups/criteria-groups.scss index 2a9de3c3a2..76ff84d463 100644 --- a/src/taxonomy/competency-management/criteria-groups/criteria-groups.scss +++ b/src/taxonomy/competency-management/criteria-groups/criteria-groups.scss @@ -91,6 +91,13 @@ line-height: 1.75rem; color: var(--pgn-color-gray-700); cursor: pointer; + // Anchors `&__select-button`'s `.stretched-link` overlay to this band. + position: relative; + } + + &__select-button { + all: unset; + cursor: pointer; &:focus-visible { outline: 2px solid var(--pgn-color-info-600); @@ -115,6 +122,9 @@ // for a `Dropdown.Toggle` specifically. .dropdown-toggle { vertical-align: baseline; + // Layers above `&__select-button`'s stretched overlay so clicks land here. + position: relative; + z-index: 1; } } @@ -137,6 +147,7 @@ border-radius: var(--pgn-size-border-radius-base); background-color: var(--pgn-color-white); cursor: pointer; + position: relative; &--focused { padding-left: var(--pgn-spacing-spacer-1-5); @@ -144,6 +155,16 @@ background-color: var(--pgn-color-info-100); box-shadow: var(--pgn-elevation-box-shadow-centered-2); } + + &__select-button { + all: unset; + cursor: pointer; + + &:focus-visible { + outline: 2px solid var(--pgn-color-info-400); + outline-offset: -2px; + } + } } .score-threshold-field { @@ -174,6 +195,9 @@ display: inline-block; width: 3.5em; text-align: center; + // Layers above `.rule-box__select-button`'s stretched overlay so clicks land here. + position: relative; + z-index: 1; } // Each chip's own truncation - a long subsection name shows an ellipsis diff --git a/src/taxonomy/competency-management/criteria-groups/messages.ts b/src/taxonomy/competency-management/criteria-groups/messages.ts index 9ebce681d3..63a7adffe9 100644 --- a/src/taxonomy/competency-management/criteria-groups/messages.ts +++ b/src/taxonomy/competency-management/criteria-groups/messages.ts @@ -98,6 +98,18 @@ const messages = defineMessages({ + 'value would exactly duplicate another rule box\'s own score, rule type, and comparison in the same ' + 'group.', }, + scoreThresholdRangeValidationMessage: { + id: 'course-authoring.competency-management.criteria-groups.score-threshold-range-validation-message', + defaultMessage: 'Enter a whole number from 0 to 100.', + description: 'Inline validation message shown while editing a rule box\'s score threshold, when the typed ' + + 'value is not a whole number from 0 to 100.', + }, + selectRuleBoxButtonLabel: { + id: 'course-authoring.competency-management.criteria-groups.select-rule-box-button-label', + defaultMessage: 'Select this rule', + description: 'Visually hidden accessible label for the button that selects a rule box, stretched to cover ' + + 'the whole box - the box has no single short visible phrase of its own to reuse as this label.', + }, }); export default messages; diff --git a/src/taxonomy/competency-management/data/api.test.ts b/src/taxonomy/competency-management/data/api.test.ts index 726895ce12..1cac3cabe6 100644 --- a/src/taxonomy/competency-management/data/api.test.ts +++ b/src/taxonomy/competency-management/data/api.test.ts @@ -133,7 +133,7 @@ describe('competency-management api calls', () => { }); }); - it('updates a group\'s operator, sending and returning it unchanged (already uppercase on the wire)', async () => { + it('updates a group\'s operator at its URL, sending a snake_case body and camelCasing the response', async () => { const { axiosMock } = initializeMocks(); axiosMock.onPatch(apiUrls.updateCompetencyCriteriaGroup(tagId, 10)).reply(200, { id: 10, @@ -162,7 +162,7 @@ describe('competency-management api calls', () => { }); }); - it('batch-updates a rule box\'s score, sending a snake_case payload and camelCasing the response', async () => { + it('batch-updates a rule box\'s score at its URL, sending a snake_case payload and camelCasing the response', async () => { const { axiosMock } = initializeMocks(); const groupId = 10; axiosMock.onPatch(apiUrls.updateCompetencyCriteriaRule(groupId)).reply(200, [ diff --git a/src/taxonomy/competency-management/data/api.ts b/src/taxonomy/competency-management/data/api.ts index d4e1cb1e78..f3240d7edf 100644 --- a/src/taxonomy/competency-management/data/api.ts +++ b/src/taxonomy/competency-management/data/api.ts @@ -48,9 +48,7 @@ export const apiUrls = { createCompetencyCriterion: (tagId: number) => makeUrl(`${tagId}/criteria/`), /** PATCH: update a bottom-tier group's any/all combining logic. Backs `#760`. */ updateCompetencyCriteriaGroup: (tagId: number, groupId: number) => makeUrl(`${tagId}/criteria-groups/${groupId}/`), - /** PATCH: batch-update a rule box's score across every criterion sharing it. Backs `#759` - - * not nested under `competencies//`, so built with `makeCbeV1Url` instead of `makeUrl`. - */ + /** PATCH: batch-update a rule box's score across every criterion sharing it. Backs `#759`. */ updateCompetencyCriteriaRule: (groupId: number) => makeCbeV1Url(`criteria-groups/${groupId}/criteria/bulk-update/`), } satisfies Record string>; @@ -130,9 +128,7 @@ export async function updateCompetencyCriteriaGroupOperator( * @param ruleType The rule box's own rule type, pinned unchanged - only the * numeric value is user-editable (`#794`'s own scope). * @param rulePayload The new score threshold, applied to every listed criterion. - * @returns The updated criteria as actually persisted - the caller derives - * the box's new focus key from this, not from the request, since a "reset - * to default" edit may echo back a profile reference instead. + * @returns The updated criteria as actually persisted. */ export async function updateCompetencyCriteriaRule( groupId: number, diff --git a/src/taxonomy/competency-management/data/apiHooks.test.tsx b/src/taxonomy/competency-management/data/apiHooks.test.tsx index b6e57138a0..1512cb36a1 100644 --- a/src/taxonomy/competency-management/data/apiHooks.test.tsx +++ b/src/taxonomy/competency-management/data/apiHooks.test.tsx @@ -188,31 +188,6 @@ describe('useUpdateCompetencyCriteriaGroupOperator', () => { const state = queryClient.getQueryState(competencyQueryKeys.competencyCriteriaGroups(tagId)); expect(state?.isInvalidated).toBe(true); }); - - it('does not invalidate a different tagId\'s groups query', async () => { - const { axiosMock, queryClient } = initializeMocks(); - const otherTagId = 7; - axiosMock.onPatch(apiUrls.updateCompetencyCriteriaGroup(tagId, 10)).reply(200, { - id: 10, - parent_id: 1, - tag_id: tagId, - course_key: null, - name: 'leaf', - ordering: 0, - logic_operator: 'OR', - archived: false, - }); - queryClient.setQueryData(competencyQueryKeys.competencyCriteriaGroups(otherTagId), { groups: [], criteria: [] }); - - const { result } = renderHook(() => useUpdateCompetencyCriteriaGroupOperator(), { wrapper: makeWrapper() }); - - await act(async () => { - await result.current.mutateAsync({ tagId, groupId: 10, logicOperator: 'OR' }); - }); - - const state = queryClient.getQueryState(competencyQueryKeys.competencyCriteriaGroups(otherTagId)); - expect(state?.isInvalidated).toBe(false); - }); }); describe('useUpdateCompetencyCriteriaRule', () => { diff --git a/src/taxonomy/competency-management/data/apiHooks.ts b/src/taxonomy/competency-management/data/apiHooks.ts index eb2776cbf2..b3b6934556 100644 --- a/src/taxonomy/competency-management/data/apiHooks.ts +++ b/src/taxonomy/competency-management/data/apiHooks.ts @@ -77,9 +77,10 @@ export const useCreateCompetencyCriterion = () => { mutationFn: ( { tagId, payload }: { tagId: number; payload: CreateCompetencyCriterionPayload; }, ) => api.createCompetencyCriterion(tagId, payload), - onSuccess: (_data, { tagId }) => { - queryClient.invalidateQueries({ queryKey: competencyQueryKeys.competencyCriteriaGroups(tagId) }); - }, + onSuccess: (_data, { tagId }) => + queryClient.invalidateQueries({ + queryKey: competencyQueryKeys.competencyCriteriaGroups(tagId), + }), }); }; @@ -102,17 +103,18 @@ export const useUpdateCompetencyCriteriaGroupOperator = () => { logicOperator: CompetencyGroupLogicOperator; }, ) => api.updateCompetencyCriteriaGroupOperator(tagId, groupId, logicOperator), - onSuccess: (_data, { tagId }) => { - queryClient.invalidateQueries({ queryKey: competencyQueryKeys.competencyCriteriaGroups(tagId) }); - }, + onSuccess: (_data, { tagId }) => + queryClient.invalidateQueries({ + queryKey: competencyQueryKeys.competencyCriteriaGroups(tagId), + }), }); }; /** * Build the mutation to batch-update a rule box's score threshold (`#759`). - * Invalidates that competency's criteria-groups query on success. `tagId` - * is only used for that invalidation - the endpoint's own URL isn't nested - * under a competency. + * Invalidates that competency's criteria-groups query on success so the new score shows up + * without a manual refetch. `tagId` is only used for that invalidation - the endpoint's own + * URL isn't nested under a competency. */ export const useUpdateCompetencyCriteriaRule = () => { const queryClient = useQueryClient(); @@ -130,9 +132,10 @@ export const useUpdateCompetencyCriteriaRule = () => { rulePayload: GradeRulePayload; }, ) => api.updateCompetencyCriteriaRule(groupId, criterionIds, ruleType, rulePayload), - onSuccess: (_data, { tagId }) => { - queryClient.invalidateQueries({ queryKey: competencyQueryKeys.competencyCriteriaGroups(tagId) }); - }, + onSuccess: (_data, { tagId }) => + queryClient.invalidateQueries({ + queryKey: competencyQueryKeys.competencyCriteriaGroups(tagId), + }), }); }; diff --git a/src/taxonomy/competency-management/data/types.ts b/src/taxonomy/competency-management/data/types.ts index fb945abd28..2b26f3d732 100644 --- a/src/taxonomy/competency-management/data/types.ts +++ b/src/taxonomy/competency-management/data/types.ts @@ -166,13 +166,8 @@ export interface CreateCompetencyCriterionPayload { */ export type CreateCompetencyCriterionResponse = Omit & { objectTagId: number; }; -/** Request body for updating a bottom-tier group's any/all combining logic - * (`#760`). `logic_operator` is the only field, so this is just - * `CompetencyGroupLogicOperator` under its own wire key - named separately - * from `CompetencyGroupLogicOperator` itself for symmetry with - * `CreateCompetencyCriterionPayload`/`UpdateCompetencyCriteriaRulePayload`, - * the other request-body types in this file. - */ +/** Request body for updating a bottom-tier group's any/all combining logic (`#760`): just + * `logic_operator`, under its own wire key. */ export interface UpdateCompetencyCriteriaGroupPayload { logic_operator: CompetencyGroupLogicOperator; } From b2f69dad9d6f530163f7c5fe46aee57e1fed3213 Mon Sep 17 00:00:00 2001 From: AShatsila <319486855+AShatsila@users.noreply.github.com> Date: Tue, 20 Oct 2026 19:20:20 +0400 Subject: [PATCH 10/12] refactor: comments and test cases --- .../criteria-groups/CriteriaGroupBox.test.tsx | 2 -- .../LogicOperatorSelect.test.tsx | 1 - .../criteria-groups/LogicOperatorSelect.tsx | 10 +-------- .../criteria-groups/RuleBoxList.test.tsx | 14 ------------- .../criteria-groups/criteria-groups.scss | 21 +++++++------------ 5 files changed, 8 insertions(+), 40 deletions(-) diff --git a/src/taxonomy/competency-management/criteria-groups/CriteriaGroupBox.test.tsx b/src/taxonomy/competency-management/criteria-groups/CriteriaGroupBox.test.tsx index f0cf54dd50..35701edc7b 100644 --- a/src/taxonomy/competency-management/criteria-groups/CriteriaGroupBox.test.tsx +++ b/src/taxonomy/competency-management/criteria-groups/CriteriaGroupBox.test.tsx @@ -52,8 +52,6 @@ const index = buildCompetencyCriteriaGroupsIndex(response); const renderBox = ( contextOverrides: Parameters[0] = {}, subsectionNamesByUsageKey: Record = {}, - // Defaults to `true`, matching `testHelpers.tsx`'s own `canEditCourse: () => true` - // convention (tests default to "editable," and opt out explicitly). canEdit: boolean = true, ) => ( render( diff --git a/src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.test.tsx b/src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.test.tsx index b9f49fd39d..1f4b80060c 100644 --- a/src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.test.tsx +++ b/src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.test.tsx @@ -46,7 +46,6 @@ describe('', () => { fireEvent.click(screen.getByText('any')); expect(onChange).toHaveBeenCalledWith('OR'); - // keeps its closed menu's items in the DOM rather than removing them. rerender(); expect(container.querySelector('.dropdown-toggle')).toHaveTextContent('all'); diff --git a/src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.tsx b/src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.tsx index 81abc3083e..740bcb50ae 100644 --- a/src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.tsx +++ b/src/taxonomy/competency-management/criteria-groups/LogicOperatorSelect.tsx @@ -20,15 +20,7 @@ export interface LogicOperatorSelectProps { } /** Shared any/all (bottom-tier group) or Or/And (connector) control for how - * a group's children combine. Read-only (plain text) unless `onChange` is - * given, in which case a controlled Paragon `Dropdown` renders instead, - * with its trigger label always computed from `value`. - * - * Deliberately not Paragon's `SelectMenu`: it's uncontrolled (holds its own - * "selected" state once a `MenuItem` is clicked), so its trigger would keep - * showing whatever the author last clicked even after a rejected save left - * `value` unchanged. `Dropdown` has no equivalent internal state, so a - * rejected save's rollback is just a matter of never having changed `value`. + * a group's children combine. Read-only unless `onChange` is given. */ const LogicOperatorSelect = ({ value, diff --git a/src/taxonomy/competency-management/criteria-groups/RuleBoxList.test.tsx b/src/taxonomy/competency-management/criteria-groups/RuleBoxList.test.tsx index a6cfe274f0..d8fb19593d 100644 --- a/src/taxonomy/competency-management/criteria-groups/RuleBoxList.test.tsx +++ b/src/taxonomy/competency-management/criteria-groups/RuleBoxList.test.tsx @@ -201,19 +201,5 @@ describe('', () => { expect(updateRuleScore).not.toHaveBeenCalled(); expect(screen.getByText('Another rule box in this group already uses this score.')).toBeInTheDocument(); }); - - it('does not treat retyping a box\'s own current score as a duplicate of itself', async () => { - const user = userEvent.setup(); - const updateRuleScore = jest.fn().mockResolvedValue(undefined); - renderEditableBoxes(updateRuleScore); - - const [firstBoxInput] = screen.getAllByRole('textbox'); - await user.clear(firstBoxInput); - await user.type(firstBoxInput, '60'); - await user.keyboard('{Enter}'); - - expect(updateRuleScore).not.toHaveBeenCalled(); - expect(screen.queryByText('Another rule box in this group already uses this score.')).not.toBeInTheDocument(); - }); }); }); diff --git a/src/taxonomy/competency-management/criteria-groups/criteria-groups.scss b/src/taxonomy/competency-management/criteria-groups/criteria-groups.scss index 76ff84d463..21603898cd 100644 --- a/src/taxonomy/competency-management/criteria-groups/criteria-groups.scss +++ b/src/taxonomy/competency-management/criteria-groups/criteria-groups.scss @@ -99,9 +99,10 @@ all: unset; cursor: pointer; - &:focus-visible { + // Outline the stretched `::after` overlay; the button's own box is near zero size. + &:focus-visible::after { outline: 2px solid var(--pgn-color-info-600); - outline-offset: 2px; + outline-offset: -2px; } } @@ -111,15 +112,10 @@ font-weight: 700; line-height: 1.5rem; - // Once `canEdit` is true, this class lands on a Paragon `Dropdown` - // (a `
`), which has no `display` rule of its own and would - // otherwise break "By completing [x] of the following" onto separate - // lines. + // Paragon's `Dropdown` has no `display` rule of its own, and would otherwise wrap onto its own line. display: inline-block; - // Nudges the toggle's baseline to line up with the surrounding - // sentence text - the one thing `.btn-inline` doesn't already handle - // for a `Dropdown.Toggle` specifically. + // Aligns the toggle's text baseline with the surrounding sentence text. .dropdown-toggle { vertical-align: baseline; // Layers above `&__select-button`'s stretched overlay so clicks land here. @@ -160,7 +156,7 @@ all: unset; cursor: pointer; - &:focus-visible { + &:focus-visible::after { outline: 2px solid var(--pgn-color-info-400); outline-offset: -2px; } @@ -187,10 +183,7 @@ color: var(--pgn-color-info-600); } - // "With a score of [input]% or higher" - the editable percent input - // itself. Paragon's `Form.Control` renders block-level and full-width by - // default, which would otherwise blow out this inline sentence to the - // width of the whole rule box. + // Paragon's `Form.Control` renders block-level and full-width by default, which would otherwise blow out this inline sentence. .score-threshold-field__input { display: inline-block; width: 3.5em; From e96b2ab80b0ebeb5a24574e0b557760aa2a133fe Mon Sep 17 00:00:00 2001 From: AShatsila <319486855+AShatsila@users.noreply.github.com> Date: Tue, 20 Oct 2026 20:29:34 +0400 Subject: [PATCH 11/12] refactor: comments --- .../competency-management/criteria-groups/criteria-groups.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/taxonomy/competency-management/criteria-groups/criteria-groups.scss b/src/taxonomy/competency-management/criteria-groups/criteria-groups.scss index 21603898cd..9624660f4d 100644 --- a/src/taxonomy/competency-management/criteria-groups/criteria-groups.scss +++ b/src/taxonomy/competency-management/criteria-groups/criteria-groups.scss @@ -183,7 +183,7 @@ color: var(--pgn-color-info-600); } - // Paragon's `Form.Control` renders block-level and full-width by default, which would otherwise blow out this inline sentence. + // `Form.Control` is block-level and full-width by default; keep it inline in "With a score of X% or higher". .score-threshold-field__input { display: inline-block; width: 3.5em; From fdcf0fb3c7214cb7193dfaa491f3720a580ec162 Mon Sep 17 00:00:00 2001 From: AShatsila <319486855+AShatsila@users.noreply.github.com> Date: Thu, 29 Oct 2026 21:02:35 +0400 Subject: [PATCH 12/12] feat: update score threshold component --- .../criteria-groups/RuleBox.edit.test.tsx | 4 +++- .../criteria-groups/ScoreThresholdField.tsx | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/taxonomy/competency-management/criteria-groups/RuleBox.edit.test.tsx b/src/taxonomy/competency-management/criteria-groups/RuleBox.edit.test.tsx index 3f9b50dcb3..d11e8ad6c5 100644 --- a/src/taxonomy/competency-management/criteria-groups/RuleBox.edit.test.tsx +++ b/src/taxonomy/competency-management/criteria-groups/RuleBox.edit.test.tsx @@ -52,7 +52,9 @@ describe(' editing (#794)', () => { it('renders the score as plain text, not an input, when canEdit is omitted (default false)', () => { render70PercentRuleBox(); - expect(screen.getByText('With a score of 70% or higher')).toBeInTheDocument(); + expect(screen.getByText('70% or higher').closest('.score-threshold-field')).toHaveTextContent( + 'With a score of 70% or higher', + ); expect(screen.queryByRole('textbox')).not.toBeInTheDocument(); }); diff --git a/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.tsx b/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.tsx index 64dbebb674..878d422610 100644 --- a/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.tsx +++ b/src/taxonomy/competency-management/criteria-groups/ScoreThresholdField.tsx @@ -129,7 +129,8 @@ const ScoreThresholdField = ({ rulePayload, onChange, getInlineValidationMessage aria-invalid={!!validationMessage} isInvalid={!!validationMessage} disabled={isSaving} - onChange={(event) => setInputValue(event.target.value)} + onChange={(event) => + setInputValue(event.target.value)} onKeyDown={handleKeyDown} onBlur={() => { void saveScoreIfChanged();