-
-
Notifications
You must be signed in to change notification settings - Fork 392
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
OBPIH-4897 add autoincremental product code assignment when create products through import #3547
OBPIH-4897 add autoincremental product code assignment when create products through import #3547
Conversation
…oductTypes on product import
dd85ec7
to
6841865
Compare
grails-app/controllers/org/pih/warehouse/product/ProductController.groovy
Show resolved
Hide resolved
@@ -938,7 +938,7 @@ class ProductController { | |||
flash.message = "All ${command?.products?.size()} product(s) were imported successfully." | |||
redirect(controller: "product", action: "importAsCsv", params: [tag: tags[0]]) | |||
} catch (ValidationException e) { | |||
command.errors = e.errors | |||
command.errors.reject(e.message) |
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.
I don't know if this is what we want to do. Can you provide justification for this? The validation exception has an Errors object that includes an detailed error message for each invalid data element. Displaying just the message associated with the validation exception give us a generic error message without those details.
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.
for some reason, that I wasn't able to figure out, assigning e.errors
to command.errors
didn't work.
It didn't display any error messages on .gsp
page even thought error was was thrown.
I will try to do product.errors.reject("...")
inseatd of throwing an exception, I was just following an example where few line below we have
if (!product.save(flush: true)) {
hrow new ValidationException("Could not save product '" + product.name + "'", product.errors)
}
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.
Right, that is supposed to be the place where we detect whether product has errors and if so it throws the exception. We don't need to throw a validation exception everywhere. We just need to collect all the errors in the Errors object and throw the exception with that one Errors object.
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.
for some reason, that I wasn't able to figure out, assigning e.errors to command.errors didn't work.
It didn't display any error messages on .gsp page even thought error was was thrown.
What gsp is that?
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.
it was importAsCsv.gsp
https://github.com/openboxes/openboxes/blob/develop/grails-app/views/product/importAsCsv.gsp#L18
even thought it has appropriate logic for checking if command
has errors and siplay those errors but for some reason it didn't as if it lost those errors somewhere along the way
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.
Ok, well make the suggested change(s) and if you still can't get it to work I can take a look.
ProductType defaultProductType = ProductType.defaultProductType.list()?.first(); | ||
// Throw an error for product type with empty code and product identifier that is not a default product type | ||
if (product.productType?.id != defaultProductType?.id && !product.productType?.code && !product.productType?.productIdentifierFormat) { | ||
throw new ValidationException("Could not save product '" + product.name + "'", product.errors) |
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.
Instead of throwing an exception here you should just add to the product's errors object.
if (product.productType?.id != defaultProductType?.id && !product.productType?.code && !product.productType?.productIdentifierFormat) {
product.errors.reject("...")
}
The save below should fail due to the error and throw the exception. If for some reason it doesn't you can call validate on your own.
if (!product.validate() || !product.save(flush:true)) {
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.
Unfortunately doing product.errors.reject("...")
and after that calling product.validate()
clears out previously assigned errors. I wasn't able to find any useful information on this topic but I assume that by calling product.validate()
it just resets product.errors
and validates the object based on specified constrains in the class.
So instead I moved my validation logic into validateProducts(csv)
(where it probably should have been in the first place) and everything seems to work well there.
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.
I still see the exception being thrown here.
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.
Oh nevermind. I see what you did. But just for clarification ... validate should not clear product.errors so we should try to figure out how to do this properly at some point.
…er on product import
No description provided.