-
-
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-4790 implement product association import #3559
Conversation
grails-app/services/org/pih/warehouse/data/ProductAssociationDataService.groovy
Outdated
Show resolved
Hide resolved
return | ||
} | ||
|
||
Product product = Product.get(productAssociationInstance.product?.id) |
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.
If I'm not mistaken I believe the line
new ProductAssociation(params)
should handle setting all of the associations (including product and associatedProduct).
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.
yes it does but it also sets non existing products, for example if you pass a non existing product id, it resolves it into an "empty object" (object of ProductAssociation class with all the fields of value null) so if(thatEmptyObject)
results in true
.
So basically my problem was that I wanted to check if product exists in the DB and I wasn't sure how can I do this for
params['product.id'] = 'nonExistingId123'
ProductAssociation thatAssiosaction = new ProductAssociation(params)
if (thatAssiosaction.product) { ... }
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.
actually scratch that I just found a way.
I can check if product exists by using exists(...)
method
if (Product.exists(params['product.id'])) { ... }
associatedProduct : productAssociationInstance.associatedProduct, | ||
code : productAssociationInstance.code, | ||
]) | ||
if (foundProductAssociations && foundProductAssociations.size() > 0) { |
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.
Is there a case where we want to update a product association?
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.
No, as specified in the ticket product association import should not allow association updates, only creations
grails-app/services/org/pih/warehouse/data/ProductAssociationDataService.groovy
Outdated
Show resolved
Hide resolved
grails-app/services/org/pih/warehouse/data/ProductAssociationDataService.groovy
Outdated
Show resolved
Hide resolved
grails-app/services/org/pih/warehouse/data/ProductAssociationDataService.groovy
Outdated
Show resolved
Hide resolved
grails-app/services/org/pih/warehouse/data/ProductAssociationDataService.groovy
Outdated
Show resolved
Hide resolved
grails-app/services/org/pih/warehouse/data/ProductAssociationDataService.groovy
Show resolved
Hide resolved
log.info "Validate data " + command.filename | ||
|
||
command.data.eachWithIndex { params, index -> | ||
ProductAssociation productAssociationInstance = new ProductAssociation(params) |
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.
Now that I have replaced all instances where I used product.id
with product.productCode
and my params have
params: [
'product.prductCode': '16661',
....
]
instead of
params: [
'product.id': '16661',
....
]
now
ProductAssociation productAssociationInstance = new ProductAssociation(params)
does not populate product
and associatedProduct
fields instead I have to explicitly do
productAssociationInstance.product = Product.findByProductCode(params['product.productCode'])
productAssociationInstance.associatedProduct = Product.findByProductCode(params['associatedProduct.productCode'])
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'd say that's proper behavior. If there is a product code in the file, then you have to find it as you did, because IIRC it has to be an id in order to properly create object from params.
b13b370
to
0f5f363
Compare
ProductAssociation productAssociationInstance = new ProductAssociation(params) | ||
productAssociationInstance.product = Product.findByProductCode(params['product.productCode']) | ||
productAssociationInstance.associatedProduct = Product.findByProductCode(params['associatedProduct.productCode']) | ||
params['product.id'] = productAssociationInstance.product?.id |
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.
@drodzewicz is there a need to assign an id to params if you already have a product
and an associatedProduct
on the productAssociationInstance
?
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.
the reason for that is that I have productAssociationInstance.product
and productAssociationInstance.associatedProduct
inside a
Boolean validateData(ImportDataCommand command) { ... }
method and after validating we move on to importData(ImportDataCommand command) { ... }
method which is responsible for creating and saving those associations.
And so for
ProductAssociation firstProductAssociationInstance = new ProductAssociation(params)
to work properly
params has to have properties product.id
and associatedProduct.id
but instead it only has product.productCode
and associatedProduct.productCode
// - association.product = otherAssociation.associatedProduct | ||
// - association.associatedProduct = otherAssociation.product | ||
// - association.code = otherAssociation.code | ||
def otherMatchingTwoWayAssociation = command.data.find { |
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 not this be searched in all associations in the system instead in the file data only?
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 think this might be a topic for a tech huddle and at the moment we should focus only on associations provided in the imported file.
This also falls into a case of updating associations which is probably out of scope for this ticket
- raise error if the product is the same as associatedProduct - raise error if product association with given parameters already exists
No description provided.