-
Notifications
You must be signed in to change notification settings - Fork 4
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
[TDRD 87] Row validation error report download compatibility tweaks #4292
Conversation
annielh
commented
Nov 18, 2024
•
edited
Loading
edited
- Rename Field -> Column as requested by @TNAemmaPea
- Put row validation errors at the top of error report
- The asset id will now be the file path by default so we can use it directly for the identifier (Filepath) column.
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.
Looks good. Thanks. Just a couple of very minor comments.
@@ -134,15 +134,19 @@ class DraftMetadataChecksResultsController @Inject() ( | |||
reference <- consignmentService.getConsignmentRef(consignmentId, request.token.bearerAccessToken) | |||
errorReport <- draftMetadataService.getErrorReport(consignmentId) | |||
} yield { | |||
val orderingPrioritisingRowErrors: Ordering[Error] = |
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.
Could this be made a bit more configurable so the priory error type is not static? Something like this:
...
def downloadErrorReport(consignmentId: UUID, priorityErrorType: FileError = ROW_VALIDATION): Action[AnyContent] = standardUserAndTypeAction(consignmentId) { implicit request: Request[AnyContent] =>
for {
...
} yield {
val orderingPriorityErrors: Ordering[Error] =
Ordering.by[Error, (Boolean, String)](e => (!e.validationProcess.equals(s"$priorityErrorType"), e.validationProcess))
...
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.
Sorry just one very small additional comment
reference <- consignmentService.getConsignmentRef(consignmentId, request.token.bearerAccessToken) | ||
errorReport <- draftMetadataService.getErrorReport(consignmentId) | ||
} yield { | ||
val orderingPrioritisingRowErrors: Ordering[Error] = |
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.
Should rename this val to something less specific given it might not always be row errors with priority
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.
Looks good. Thanks.