FRW-10501 Filter duplicate event entities. #94
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ code/snyk check is complete. No issues have been found. (View Details) |
…to bugfix/frw-10501/master-filter-duplicate-event-entities
gechetspr
left a comment
gechetspr
left a comment
There was a problem hiding this comment.
In the changelogs Propel must be with the capital first letter
| "spryker/event": "^2.4.0", | ||
| "spryker/event-dispatcher-extension": "^1.0.0", | ||
| "spryker/kernel": "^3.49.0", | ||
| "spryker/kernel": "^3.72.0", |
There was a problem hiding this comment.
why kernel version bump? not sure how changes in the module require newer version. Same goes for Propel module too
There was a problem hiding this comment.
versions outdated, previous CI didn't work
| "spryker/testify": "^3.42.0", | ||
| "spryker/transfer": "^3.25.0" | ||
| "spryker/testify": "^3.58.0", | ||
| "spryker/transfer": "^3.39.0" |
There was a problem hiding this comment.
same here. I don't see how improved array validation is relevant for the changes in the code
| } | ||
| $eventEntityTransfersByEvent[$data[EventBehavior::EVENT_CHANGE_NAME]][] = $eventEntityTransfer; | ||
| $keyId = is_int($id) || is_string($id) ? $id : spl_object_id($eventEntityTransfer); | ||
| $eventEntityTransfersByEvent[$data[EventBehavior::EVENT_CHANGE_NAME]][$keyId] = $eventEntityTransfer; |
There was a problem hiding this comment.
A few things with this code that need to be checked.
we have a triggeredRows var lower. Now we can have this number bigger than actual events that we trigger.
If we fix that, please check event records deletion code as it delete those if amount of triggered events in the same as amount of fetched events.
And one more thing that is not related to this code. Could you please check why we have triggerEvents method called and afterward we have a method that trigger events once again and remove them in the loop? Why to trigger events twice?
There was a problem hiding this comment.
- Not relevant for deleting. We can't delete the same entity twice.
- for bulk operations.
There was a problem hiding this comment.
but what about $triggeredRows var? Now it will lie to us as we trigger less events
There was a problem hiding this comment.
Good point. let me adjust code
There was a problem hiding this comment.
Adjusted logic. It's not related to these changes. triggeredRows always === count(events)
spryker-release-bot
deleted the
bugfix/frw-10501/master-filter-duplicate-event-entities
branch
Developer(s): @vol4onok
Ticket: https://spryker.atlassian.net/browse/FRW-10501
Release Group: https://release.spryker.com/release-groups/view/5921
PR Overview: https://release.spryker.com/release/pull-request/11609
merge: squash
Release Table
Module EventBehavior
Change log
Improvements
TriggerManagerto avoid duplicate event messages with the same IDs.EventBehaviorto use correctPrimaryKeyin propel module for event message if the table doesn't have primary key.Kernelversion dependency.Propelversion dependency.