-
Notifications
You must be signed in to change notification settings - Fork 170
feat(drive-integration): display validationFindings in mapping-review UI [INTEG-4383] #11087
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 2 commits
ea9efab
733fe25
956958d
9b356de
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| import { useMemo } from 'react'; | ||
| import { Box, Button, Flex, Note, Paragraph, Text } from '@contentful/f36-components'; | ||
| import { Box, Button, Checkbox, Flex, Note, Paragraph, Text } from '@contentful/f36-components'; | ||
| import { LightbulbIcon } from '@contentful/f36-icons'; | ||
| import type { MappingReviewSuspendPayload } from '@types'; | ||
| import type { MappingReviewSuspendPayload, ValidationFinding } from '@types'; | ||
| import { buildEntryListFromEntryBlockGraph } from '../../../../utils/overviewEntryList'; | ||
| import { OverviewEntryList } from './OverviewEntryList'; | ||
| import { overviewSectionBox, overviewSectionBoxScrollable } from './OverviewSection.styles'; | ||
|
|
@@ -18,6 +18,10 @@ interface OverviewProps { | |
| isCtaLoading?: boolean; | ||
| isCtaDisabled?: boolean; | ||
| areEntrySelectionsDisabled?: boolean; | ||
| /** Called with `true` when the user checks the block-findings acknowledgement. */ | ||
| onBlockFindingsAcknowledged?: (acknowledged: boolean) => void; | ||
| /** Whether the user has acknowledged block findings. */ | ||
| blockFindingsAcknowledged?: boolean; | ||
| } | ||
|
|
||
| const OverviewSection = ({ | ||
|
|
@@ -31,6 +35,8 @@ const OverviewSection = ({ | |
| isCtaLoading = false, | ||
| isCtaDisabled = false, | ||
| areEntrySelectionsDisabled = false, | ||
| onBlockFindingsAcknowledged, | ||
| blockFindingsAcknowledged = false, | ||
| }: OverviewProps) => { | ||
| const entryRows = useMemo( | ||
| () => | ||
|
|
@@ -42,6 +48,19 @@ const OverviewSection = ({ | |
| [payload.entryBlockGraph.entries, payload.contentTypes, payload.referenceGraph.edges] | ||
| ); | ||
|
|
||
| const findingsByEntryIndex = useMemo((): ReadonlyMap<number, ValidationFinding[]> => { | ||
| const map = new Map<number, ValidationFinding[]>(); | ||
| for (const finding of payload.validationFindings ?? []) { | ||
| if (finding.entryIndex === undefined) continue; | ||
| const list = map.get(finding.entryIndex) ?? []; | ||
| list.push(finding); | ||
| map.set(finding.entryIndex, list); | ||
| } | ||
| return map; | ||
| }, [payload.validationFindings]); | ||
|
|
||
| const hasBlockFindings = (payload.validationFindings ?? []).some((f) => f.severity === 'block'); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could reuse the one that is computed in the |
||
|
|
||
| return ( | ||
| <> | ||
| <Box padding="spacingL" className={overviewSectionBox}> | ||
|
|
@@ -59,6 +78,22 @@ const OverviewSection = ({ | |
|
|
||
| <Splitter /> | ||
|
|
||
| {hasBlockFindings && ( | ||
| <Note variant="negative"> | ||
| <Flex flexDirection="column" gap="spacingXs"> | ||
| <Text> | ||
| Some entries have issues that may prevent the content from being created | ||
| correctly. Review the highlighted entries before proceeding. | ||
| </Text> | ||
| <Checkbox | ||
| isChecked={blockFindingsAcknowledged} | ||
| onChange={(event) => onBlockFindingsAcknowledged?.(event.target.checked)}> | ||
| I have reviewed the issues and want to proceed | ||
| </Checkbox> | ||
| </Flex> | ||
| </Note> | ||
| )} | ||
|
|
||
| <Flex justifyContent="space-between" alignItems="center" paddingBottom="none"> | ||
| <Flex flexDirection="column" gap="spacingXs"> | ||
| <Text fontWeight="fontWeightDemiBold" fontSize="fontSizeL"> | ||
|
|
@@ -91,6 +126,7 @@ const OverviewSection = ({ | |
| onSelect={onSelectEntryIndex} | ||
| onToggleEntrySelection={onToggleEntrySelection} | ||
| areEntrySelectionsDisabled={areEntrySelectionsDisabled} | ||
| findingsByEntryIndex={findingsByEntryIndex} | ||
| /> | ||
| </Box> | ||
| )} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -125,6 +125,23 @@ export interface TabsImagesSuspendPayload { | |
| tabs?: DocTabOption[]; | ||
| } | ||
|
|
||
| /** Severity levels for validation findings produced by the validate-payload step. */ | ||
| export type ValidationFindingSeverity = 'block' | 'warn'; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⛏️ nit: this could be an enum, as we are using it in several places |
||
|
|
||
| /** A single validation finding from the validate-payload step. */ | ||
| export interface ValidationFinding { | ||
| /** Machine-readable finding code. */ | ||
| code: string; | ||
| /** Human-readable explanation. */ | ||
| message: string; | ||
| /** `block` findings prevent advancing; `warn` findings are advisory. */ | ||
| severity: ValidationFindingSeverity; | ||
| /** Zero-based entry index, when entry-scoped. */ | ||
| entryIndex?: number; | ||
| /** Field ID, when field-scoped. */ | ||
| fieldId?: string; | ||
| } | ||
|
|
||
| export interface MappingReviewSuspendPayload { | ||
| reason?: string; | ||
| suspendStepId: 'mapping-review'; | ||
|
|
@@ -134,6 +151,8 @@ export interface MappingReviewSuspendPayload { | |
| entryBlockGraph: EntryBlockGraph; | ||
| referenceGraph: ReviewedReferenceGraph; | ||
| contentTypes: WorkflowContentType[]; | ||
| /** Present when the google-docs-agent-improvements flag is on; absent otherwise. */ | ||
| validationFindings?: ValidationFinding[]; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⛏️Nit: I would remove the comment |
||
| } | ||
|
|
||
| export type WorkflowRunResult = | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| import { cleanup, render, screen } from '@testing-library/react'; | ||
| import { afterEach, describe, expect, it, vi } from 'vitest'; | ||
| import type { ValidationFinding } from '@types'; | ||
| import type { EntryListRow } from '../../../../../src/utils/overviewEntryList'; | ||
| import { OverviewEntryList } from '../../../../../src/locations/Page/components/overview/OverviewEntryList'; | ||
|
|
||
| const makeRow = (entryIndex: number, label: string): EntryListRow => ({ | ||
| id: `row-${entryIndex}`, | ||
| entryIndex, | ||
| contentTypeName: 'Article', | ||
| entryTitle: label, | ||
| children: [], | ||
| }); | ||
|
|
||
| const renderList = ( | ||
| rows: EntryListRow[], | ||
| findingsByEntryIndex?: ReadonlyMap<number, ValidationFinding[]> | ||
| ) => | ||
| render( | ||
| <OverviewEntryList | ||
| rows={rows} | ||
| selectedEntryIndex={null} | ||
| selectedEntryKeys={new Set()} | ||
| onSelect={vi.fn()} | ||
| onToggleEntrySelection={vi.fn()} | ||
| findingsByEntryIndex={findingsByEntryIndex} | ||
| /> | ||
| ); | ||
|
|
||
| afterEach(() => cleanup()); | ||
|
|
||
| describe('OverviewEntryList — validation finding badges (INTEG-4383)', () => { | ||
| it('renders a "Needs attention" badge for entries with block findings', () => { | ||
| const rows = [makeRow(0, 'Entry A')]; | ||
| const findings: ValidationFinding[] = [ | ||
| { | ||
| code: 'required-field-missing', | ||
| message: 'title missing', | ||
| severity: 'block', | ||
| entryIndex: 0, | ||
| }, | ||
| ]; | ||
| renderList(rows, new Map([[0, findings]])); | ||
|
|
||
| expect(screen.getByText('Needs attention')).toBeTruthy(); | ||
| expect(screen.queryByText('Warning')).toBeNull(); | ||
| }); | ||
|
|
||
| it('renders a "Warning" badge for entries with only warn findings', () => { | ||
| const rows = [makeRow(0, 'Entry A')]; | ||
| const findings: ValidationFinding[] = [ | ||
| { code: 'displayField-blank', message: 'title blank', severity: 'warn', entryIndex: 0 }, | ||
| ]; | ||
| renderList(rows, new Map([[0, findings]])); | ||
|
|
||
| expect(screen.getByText('Warning')).toBeTruthy(); | ||
| expect(screen.queryByText('Needs attention')).toBeNull(); | ||
| }); | ||
|
|
||
| it('renders "Needs attention" (not Warning) when entry has both block and warn findings', () => { | ||
| const rows = [makeRow(0, 'Entry A')]; | ||
| const findings: ValidationFinding[] = [ | ||
| { | ||
| code: 'required-field-missing', | ||
| message: 'title missing', | ||
| severity: 'block', | ||
| entryIndex: 0, | ||
| }, | ||
| { code: 'displayField-blank', message: 'title blank', severity: 'warn', entryIndex: 0 }, | ||
| ]; | ||
| renderList(rows, new Map([[0, findings]])); | ||
|
|
||
| expect(screen.getByText('Needs attention')).toBeTruthy(); | ||
| expect(screen.queryByText('Warning')).toBeNull(); | ||
| }); | ||
|
|
||
| it('renders no finding badges when findingsByEntryIndex is undefined', () => { | ||
| const rows = [makeRow(0, 'Entry A')]; | ||
| renderList(rows, undefined); | ||
|
|
||
| expect(screen.queryByText('Needs attention')).toBeNull(); | ||
| expect(screen.queryByText('Warning')).toBeNull(); | ||
| }); | ||
|
|
||
| it('renders no finding badges for entries with no findings', () => { | ||
| const rows = [makeRow(0, 'Entry A'), makeRow(1, 'Entry B')]; | ||
| const findings: ValidationFinding[] = [ | ||
| { | ||
| code: 'required-field-missing', | ||
| message: 'title missing', | ||
| severity: 'block', | ||
| entryIndex: 1, | ||
| }, | ||
| ]; | ||
| renderList(rows, new Map([[1, findings]])); | ||
|
|
||
| // Only entry 1 should have the badge | ||
| expect(screen.getAllByText('Needs attention')).toHaveLength(1); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"No mapped content"and"Needs attention"/"Warning"are rendered as sibling<Badge>elements in a<Paragraph>. If an entry simultaneously has no mapped content type AND a block finding, both badges appear.Perhaps this is an expected behavior but since the PR didn't mention about this, just wanted to double check it with you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch — this is intentional. An entry with no mapped content fields that also has a block finding (e.g. a required field is missing) would legitimately show both. The badges serve different purposes: "No mapped content" means none of the field mappings have any source refs, while "Needs attention" means the validate-payload step flagged a structural issue. They're not mutually exclusive so showing both makes sense here.