Skip to content

OBGM-517 Fix duplicating items on create putaway workflow#4159

Merged
awalkowiak merged 3 commits intofeature/upgrade-to-grails-3.3.10from
OBGM-517-items-are-duplicated-in-putaways
Jul 13, 2023
Merged

OBGM-517 Fix duplicating items on create putaway workflow#4159
awalkowiak merged 3 commits intofeature/upgrade-to-grails-3.3.10from
OBGM-517-items-are-duplicated-in-putaways

Conversation

@drodzewicz
Copy link
Collaborator

No description provided.

private Putaway bindPutawayData(Putaway putaway, User currentUser, Location currentLocation, JSONObject jsonObject) {
// Bind the putaway
bindData(putaway, jsonObject)
bindData(putaway, jsonObject, [exclude: ['putawayItems']])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in general lgtm, but I'm wondering what the difference in Grails 1 is, that it was working there? Did you check that and have the answer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not the first time I came across this issue.
Before in grails 1 it didn't bind deeply nested items, which is why later in the code we manually iterate through jsonObject.putawayItems and bind them ourselves.

jsonObject.putawayItems.each { putawayItemMap ->
PutawayItem putawayItem = new PutawayItem()
bindData(putawayItem, putawayItemMap)
// Bind the split items
putawayItemMap.splitItems.each { splitItemMap ->
PutawayItem splitItem = new PutawayItem()
bindData(splitItem, splitItemMap)
putawayItem.splitItems.add(splitItem)
}
putaway.putawayItems.add(putawayItem)
}

Grails 3 apparently does handle properly binding nested objects, hence why there appears a problem of duplicated items: first set is from implicit binding and the other is from manual binding.
So we have to choose one or the other.

I chose to exclude the implicit binding and stick to the manual to avoid unnecessary regression.

Copy link
Collaborator Author

@drodzewicz drodzewicz Jul 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I take back what I said.
In the end, I decided on using the grails data binding for the whole data object, because I didn't notice that the same problem was also on split items list of putaway item, and excluding the nested values and handling them manually is counterproductive.
I tested it to see if everything is bound properly and it seems to be working well.

@drodzewicz drodzewicz marked this pull request as draft July 11, 2023 13:13
@drodzewicz drodzewicz marked this pull request as ready for review July 11, 2023 15:54

order.save(failOnError: true)
order.save(failOnError: true, flush: true)
return order
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you try just to return the line that saves? so to do:

return order.save(failOnError: true)

calling save and then below returning the order (the not persisted one) feels weird to me
Try to do it without flush and let me know if it is working, as this flush should be unnecessary here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, it does not solve the problem.
the newly added split item is still not saved at that point and so when we return the putaway after updating, it does not contain the newly added split item.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should help is removing @transactional from the controller. Opening the transaction at controller level makes the transaction being commited not at the service level (so your changes made in service are not passed to the controller), but it is commited after you call render (so you pass not yet flushed data to the frontend).
What should be also visible is that you should see your changes "with a delay" - what I mean by that, is that when you try to do adding the splitItems twice, at the second attempt, you would suddenly see the changes from the first attempt.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @kchelstowski for the explanation, that solution you proposed did help.


putaway.putawayItems.add(putawayItem)
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you had made sure the binder handles that 2nd level nested (splitItemMap) cases, LGTM

@drodzewicz drodzewicz changed the title OBGM-517 Exclude putawayItems from implicit data binding OBGM-517 Fix duplicating items on create putaway workflow Jul 12, 2023
@awalkowiak awalkowiak merged commit 623cf99 into feature/upgrade-to-grails-3.3.10 Jul 13, 2023
@awalkowiak awalkowiak deleted the OBGM-517-items-are-duplicated-in-putaways branch July 13, 2023 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants