Skip to content
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.

Sign up for GitHub

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

Fix off-by-one error in importer error messages when first row is header #2339

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+16 −8
Open

Fix off-by-one error in importer error messages when first row is header #2339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2af4ecb
Fix off-by-one error in importer error messages when first row is header
niklasva82 Nov 9, 2024
fff5a2a
map should be forEach
niklasva82 Nov 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions src/features/import/components/ImportDialog/elements/ImportMessageList/ImportMessage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ type Props = {
description?: string;
onCheck?: (checked: boolean) => void;
onClickBack?: () => void;
rowIndices?: number[];
rowNumbers?: number[];
status: 'error' | 'info' | 'success' | 'warning';
title: string;
};
Expand All @@ -26,14 +26,12 @@ const ImportMessage: FC<Props> = ({
description,
onCheck,
onClickBack,
rowIndices,
rowNumbers,
status,
title,
}) => {
const messages = useMessages(messageIds);

const rowNumbers = rowIndices?.map((rowIndex) => rowIndex + 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the downsides of just adding logic to either add +1 or +2 depending on firstLineIsHeaders here?


return (
<Alert
action={
Expand Down
8 changes: 4 additions & 4 deletions src/features/import/components/ImportDialog/elements/ImportMessageList/ImportMessageItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ const ImportMessageItem: FC<Props> = ({ onCheck, onClickBack, problem }) => {
<ImportMessage
onCheck={onCheck}
onClickBack={onClickBack}
rowIndices={problem.indices}
rowNumbers={problem.rows}
status="error"
title={messages.preflight.messages.invalidFormat.title({
field: getFieldTitle(problem.field),
Expand All @@ -52,7 +52,7 @@ const ImportMessageItem: FC<Props> = ({ onCheck, onClickBack, problem }) => {
description={messages.preflight.messages.missingIdAndName.description()}
onCheck={onCheck}
onClickBack={onClickBack}
rowIndices={problem.indices}
rowNumbers={problem.rows}
status="error"
title={messages.preflight.messages.missingIdAndName.title()}
/>
Expand Down Expand Up @@ -103,7 +103,7 @@ const ImportMessageItem: FC<Props> = ({ onCheck, onClickBack, problem }) => {
description={messages.preflight.messages.unknownPerson.description()}
onCheck={onCheck}
onClickBack={onClickBack}
rowIndices={problem.indices}
rowNumbers={problem.rows}
status="error"
title={messages.preflight.messages.unknownPerson.title()}
/>
Expand All @@ -114,7 +114,7 @@ const ImportMessageItem: FC<Props> = ({ onCheck, onClickBack, problem }) => {
description={messages.preflight.messages.unknownError.description()}
onCheck={onCheck}
onClickBack={onClickBack}
rowIndices={problem.indices}
rowNumbers={problem.rows}
status="error"
title={messages.preflight.messages.unknownError.title()}
/>
Expand Down
8 changes: 8 additions & 0 deletions src/features/import/hooks/usePreflight.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,14 @@ export default function usePreflight(orgId: number) {
);
}

const rowModifier = sheet.firstRowIsHeaders ? 2 : 1;
problems.forEach((problem) => {
if ('indices' in problem) {
problem.rows = problem.indices.map((index) => index + rowModifier);
}
return problem;
});

Comment on lines +62 to +69
Copy link
Member

@richardolsson richardolsson Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is quite "far back" in the stack of responsibilities, but still not so far back that it can be tested by any of our existing tests. It also relies on runtime type-checking logic which to me gives off just a little bit of code smell.

I'm curious about why you decided to put the logic here, and not further back (predictProblems(), problemsFromPreview()) where it can be tested, or in the UI (<ImportMessageItem>) where I think that it could probably be simpler.

const hasError = problems.some(
(problem) => levelForProblem(problem) == 'error'
);
Expand Down
2 changes: 2 additions & 0 deletions src/features/import/utils/problems/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export type ImportFieldProblem = {
field: string;
indices: number[];
kind: ImportProblemKind.INVALID_FORMAT;
rows?: number[];
};

export type ImportFieldMetaProblem = {
Expand All @@ -29,6 +30,7 @@ export type ImportRowProblem = {
| ImportProblemKind.UNEXPECTED_ERROR
| ImportProblemKind.UNKNOWN_PERSON
| ImportProblemKind.UNKNOWN_ERROR;
rows?: number[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the benefit of adding this as a second property, over just rendering it +1 or +2 in the UI?

};

export type ImportSheetProblem = {
Expand Down
Loading