Fix off-by-one error in importer error messages when first row is header #2339
Conversation
There was a problem hiding this comment.
Nice work! I can tell that you've explored the code to find the places to make these changes, so I'm curious about your reasoning. I can think of several places to fix this bug. Either in the UI, in the hook, or in the tested pure functions behind it. You've chosen the hook, and I would love to hear more about that. Some more concrete questions are in the comments below.
| @@ -29,6 +30,7 @@ export type ImportRowProblem = { | |||
| | ImportProblemKind.UNEXPECTED_ERROR | |||
| | ImportProblemKind.UNKNOWN_PERSON | |||
| | ImportProblemKind.UNKNOWN_ERROR; | |||
| rows?: number[]; | |||
There was a problem hiding this comment.
What is the benefit of adding this as a second property, over just rendering it +1 or +2 in the UI?
| status, | ||
| title, | ||
| }) => { | ||
| const messages = useMessages(messageIds); | ||
|
|
||
| const rowNumbers = rowIndices?.map((rowIndex) => rowIndex + 1); |
There was a problem hiding this comment.
What would be the downsides of just adding logic to either add +1 or +2 depending on firstLineIsHeaders here?
| const rowModifier = sheet.firstRowIsHeaders ? 2 : 1; | ||
| problems.forEach((problem) => { | ||
| if ('indices' in problem) { | ||
| problem.rows = problem.indices.map((index) => index + rowModifier); | ||
| } | ||
| return problem; | ||
| }); | ||
|
|
There was a problem hiding this comment.
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.
Description
This PR fixes the off-by-one error in the importer error messages when the first row of the imported document is a header.
Screenshots
Changes
ImportRowProblemandImportFieldProblemtypes to also include an arrayrowsin addition toindices.usePreflightto map indices to rows, +1 or +2 depending on whether the first row is a header.rowIndicesproperty torowNumbersinImportMessagein order to reflect the fact that these are row numbers and not merely indices of the array.problem.rowsinstead ofproblem.indicestoImportMessageinImportMessageItemImportMessageRelated issues
Resolves #2325