tweak(adminPanel): RN-1255: Bulk error handling for importing survey questions #5641
Conversation
| @@ -10,7 +10,7 @@ import { verifyResponseStatus, stringifyQuery } from '@tupaia/utils'; | |||
|
|
|||
| import { logout } from '../authentication'; | |||
|
|
|||
| const FETCH_TIMEOUT = 45 * 1000; // 45 seconds in milliseconds | |||
| const FETCH_TIMEOUT = 120 * 1000; // 45 seconds in milliseconds | |||
There was a problem hiding this comment.
This is because a survey import could actually take more than 45 seconds now that it doesn't 'fail fast'
| message: { | ||
| paddingBlock: '0.5rem', | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Overriding styling in ui-components
| } catch (e) { | ||
| errors.push(e); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
I think I'd recommend extending this try/catch all the way down to the bottom of the rows loop. That way it's just a bit more guaranteed that we catch all errors, and it's more future-proof to new validation being added.
For instance one that isn't currently doesn't catch an error that might occur when we call processOptionSetName on line 213.
| @@ -20,7 +20,7 @@ const palette = { | |||
| }, | |||
| error: { | |||
| main: COLORS.RED, | |||
| light: COLORS.LIGHT_RED, | |||
| light: `${COLORS.LIGHT_RED}1A`, // 10% opacity | |||
| const constructImportValidationError = (message, field, { details }) => { | ||
| return { | ||
| message: new ImportValidationError(message, excelRowNumber, field).message, | ||
| details, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
| const constructImportValidationError = (message, field, { details }) => { | |
| return { | |
| message: new ImportValidationError(message, excelRowNumber, field).message, | |
| details, | |
| }; | |
| }; | |
| const constructImportValidationError = (message, field, { details }) => | |
| new ImportValidationError(message, excelRowNumber, field, undefined, details); |
I think we still want this function to be returning an error, as when we pass it into ObjectValidator that's what it's expected to do. We can convert the errors into the wrapped objects latter when we create the MultiValidationError
| if (errors.length > 0) { | ||
| throw new MultiValidationError('Errors occurred while importing questions', errors); | ||
| } |
There was a problem hiding this comment.
| if (errors.length > 0) { | |
| throw new MultiValidationError('Errors occurred while importing questions', errors); | |
| } | |
| if (errors.length > 0) { | |
| throw new MultiValidationError( | |
| 'Errors occurred while importing questions', | |
| errors.map(({ message, extraFields }) => ({ | |
| message, | |
| extraFields, | |
| })), | |
| ); | |
| } |
Assuming we've done the above comment to switch errors to be a list of Error instances
merge: update with latest dev before testing
Issue RN-1255: Bulk error handling for importing survey questions
Changes:
Screenshots: