OBPIH-6022 Product Sources List Actions (improvements after QA) #4479
Conversation
alannadolny
force-pushed
the
OBPIH-6022-2
branch
from
5110470
to
029a18d
Compare
| } | ||
|
|
||
| Throwable root = ExceptionUtils.getRootCause(request.getAttribute('exception')) | ||
| render([errorCode: 500, errorsMessages: [root.getMessage()]]) |
There was a problem hiding this comment.
you have a typo here - errorsMessages instead of errorMessages.
By the way if we have a single message, I'd consider using errorMessage.
There was a problem hiding this comment.
you have a typo here - errorsMessages instead of errorMessages.
Good catch.
By the way if we have a single message, I'd consider using errorMessage.
Can you elaborate on that? I assume our convention is to use the "errorMessages" key, so why would it be better to have a duplicate key "errorMessage" that requires the API client to check for values under both keys?
There was a problem hiding this comment.
@jmiranda we already have errorMessage - e.g. look at handleValidationErrors - we pass both errorMessage and errorMessages. Frontend side (error handler) is already prepared for both options, so we wouldn't have to introduce anything new. It's just that if we have a single message (like in this), not an array of messages (like for validation errors), we should pass errorMessage in my opinion, because in the end frontend would still destruct this array and make it a single message.
| id: 'react.productSupplier.deleted.label', | ||
| defaultMessage: `Product Source ${productSupplierId} deleted`, | ||
| data: { | ||
| id: productSupplierId, |
There was a problem hiding this comment.
wow, I've never seen this. 👏
There was a problem hiding this comment.
Can you elaborate on that? What does "this" refer to?
(that is a question related to @kchelstowski comment in case it wasn't clear)
There was a problem hiding this comment.
@jmiranda "this" refers to using data property of translate method. I haven't known we could pass custom params for translation on the react side and then use it in the messages.properties like he did:
react.productSupplier.deleted.label=Product Source ${id} deleted
| @@ -854,6 +856,7 @@ class UrlMappings { | |||
| "500"(controller: "errors", action: "handleNotFound", exception: ObjectNotFoundException) | |||
| "500"(controller: "errors", action: "handleValidationErrors", exception: ValidationException) | |||
| "500"(controller: "errors", action: "handleUnauthorized", exception: AuthenticationException) | |||
| "500"(controller: "errors", action: "handleSqlIntegrityConstraintViolationException", exception: SQLIntegrityConstraintViolationException) | |||
There was a problem hiding this comment.
This is minor, but let's just call this handleConstraintViolation to avoid readability issues.
| } | ||
|
|
||
| Throwable root = ExceptionUtils.getRootCause(request.getAttribute('exception')) | ||
| render([errorCode: 500, errorsMessages: [root.getMessage()]]) |
There was a problem hiding this comment.
you have a typo here - errorsMessages instead of errorMessages.
Good catch.
By the way if we have a single message, I'd consider using errorMessage.
Can you elaborate on that? I assume our convention is to use the "errorMessages" key, so why would it be better to have a duplicate key "errorMessage" that requires the API client to check for values under both keys?
| id: 'react.productSupplier.deleted.label', | ||
| defaultMessage: `Product Source ${productSupplierId} deleted`, | ||
| data: { | ||
| id: productSupplierId, |
There was a problem hiding this comment.
Can you elaborate on that? What does "this" refer to?
(that is a question related to @kchelstowski comment in case it wasn't clear)
alannadolny
force-pushed
the
OBPIH-6022-2
branch
from
a79e5a8
to
f24823d
Compare
alannadolny
force-pushed
the
OBPIH-6022-2
branch
from
f24823d
to
aa04f63
Compare
* OBPIH-6022 Add new translations * OBPIH-6022 Add success message after deleting product supplier * OBPIH-6022 Add handling sql integrity constraint violation exception * OBPIH-6022 Hide redirects to product source edit page * OBPIH-6022 Fixes after review
No description provided.