OBPIH-4896 Handle already existing product type with empty code and product identifier #3541
Conversation
…roduct identifier
| @@ -196,6 +196,14 @@ class ProductController { | |||
|
|
|||
| updateTags(productInstance, params) | |||
|
|
|||
| 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 (productInstance.productType != defaultProductType && !productInstance.productType?.code && !productInstance.productType?.productIdentifierFormat) { | |||
There was a problem hiding this comment.
I'm just wondering if you can compare productInstance.productType to defaultProductType using equality comparator - even if they are of the same class and have the same properties and keys, will the reference be the same, so you can compare objects?
I believe in JS it wouldn't work ({} === {} would return false), but does it work in Groovy and you were aware of that?
There was a problem hiding this comment.
yes it was intentional and it does work as intended.
There was a problem hiding this comment.
cool, then JS just sucks in this case :D
There was a problem hiding this comment.
This is a good point. And generally depends on the equals() implementation.
In general, we're probably ok. But I've seen cases where this doesn't do the right thing so it's better to compare primitives (productType.id) or implement equals.
We already have the "equals / hashCode" issue on our tech debt pile so I might do this to be on the safe side.
productInstance?.productType?.id != defaultProductType?.id
There was a problem hiding this comment.
yeah you guys have a good point.
Just to be on the safe lets compare primitive values to avoid potential bugs.
There was a problem hiding this comment.
@drodzewicz I'll merge it for now since we want to get these obdev, but please remember to make and test that change when you get a chance.
| @@ -196,6 +196,14 @@ class ProductController { | |||
|
|
|||
| updateTags(productInstance, params) | |||
|
|
|||
| 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 (productInstance.productType != defaultProductType && !productInstance.productType?.code && !productInstance.productType?.productIdentifierFormat) { | |||
There was a problem hiding this comment.
This is a good point. And generally depends on the equals() implementation.
In general, we're probably ok. But I've seen cases where this doesn't do the right thing so it's better to compare primitives (productType.id) or implement equals.
We already have the "equals / hashCode" issue on our tech debt pile so I might do this to be on the safe side.
productInstance?.productType?.id != defaultProductType?.id
jmiranda
deleted the
OBPIH-4896-handle-already-existing-product-type-with-empty-code-and-product-identifier
branch
No description provided.