-
Notifications
You must be signed in to change notification settings - Fork 1
FRW-10501 Improve publish product logic based on timestamps. #96
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
FRW-10501 Improve publish product logic based on timestamps. #96
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ code/snyk check is complete. No issues have been found. (View Details) |
src/Spryker/Zed/EventBehavior/Business/Model/EventEntityTransferFilter.php
Outdated
Show resolved
Hide resolved
src/Spryker/Zed/EventBehavior/Business/Model/EventEntityTransferFilter.php
Outdated
Show resolved
Hide resolved
src/Spryker/Zed/EventBehavior/Business/Model/EventEntityTransferFilter.php
Show resolved
Hide resolved
src/Spryker/Shared/EventBehavior/Transfer/event_behavior.transfer.xml
Outdated
Show resolved
Hide resolved
src/Spryker/Zed/EventBehavior/Business/Model/EventEntityTransferFilter.php
Outdated
Show resolved
Hide resolved
src/Spryker/Zed/EventBehavior/Business/Model/EventEntityTransferFilter.php
Outdated
Show resolved
Hide resolved
src/Spryker/Shared/EventBehavior/Transfer/event_behavior.transfer.xml
Outdated
Show resolved
Hide resolved
src/Spryker/Shared/EventBehavior/Transfer/event_behavior.transfer.xml
Outdated
Show resolved
Hide resolved
src/Spryker/Zed/EventBehavior/Business/EventBehaviorFacadeInterface.php
Outdated
Show resolved
Hide resolved
…imestamp' of https://github.com/spryker/event-behavior into feature/frw-10501/master-p-and-s-improvement-based-on-timestamp
…imestamp' of https://github.com/spryker/event-behavior into feature/frw-10501/master-p-and-s-improvement-based-on-timestamp
|
| Dependency violations in Spryker.EventBehavior | |
src/Spryker/Zed/EventBehavior/Business/Model/EventEntityTransferFilter.php
Outdated
Show resolved
Hide resolved
src/Spryker/Zed/EventBehavior/Business/Model/EventEntityTransferFilter.php
Show resolved
Hide resolved
src/Spryker/Zed/EventBehavior/Business/Model/EventEntityTransferFilter.php
Outdated
Show resolved
Hide resolved
src/Spryker/Zed/EventBehavior/Business/Model/EventEntityTransferFilter.php
Outdated
Show resolved
Hide resolved
src/Spryker/Zed/EventBehavior/Business/Model/EventEntityTransferFilter.php
Outdated
Show resolved
Hide resolved
|
When CI is green, it's fine for me. |
| { | ||
| $ids = []; | ||
| foreach ($eventTransfers as $eventTransfer) { | ||
| $ids[(int)$eventTransfer->getId()] = (int)$eventTransfer->getTimestamp(); |
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.
so, if we go with the easiest flow that we can check. E.g. I've updating some big form on backoffice that have more than 1 entity in it. I put new data in it and it will trigger n+1 different events. There are not a 0 chance that those events will have the same id. With this approach we are going to loose some of the events.
The same goes for other similar methods
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.
As you see, we use the last one for a specific (unique) entity. It's part of the improvement
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. I still have my doubts, but please double check it on QA
src/Spryker/Zed/EventBehavior/Business/Model/EventEntityTransferFilter.php
Show resolved
Hide resolved
| return (new EventEntityTransfer())->setId($id); | ||
| return (new EventEntityTransfer()) | ||
| ->setId($id) | ||
| ->setTimestamp(time()); |
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.
so we just set time from now, but we need to compare it with the time from the message. This is not according AC
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.
are those changes mentioned in the changelogs?
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's enough here because we publish specific entities, we don't modify them.
gechetspr
left a comment
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.
Code is ok. Looks like are skipping older events on publish phase as stated in AC
Developer(s): @vol4onok
Ticket: https://spryker.atlassian.net/browse/FRW-10501
Release Group: https://release.spryker.com/release-groups/view/6017
merge: merge
Release Table
Module EventBehavior
Change log
Improvements
HydrateEventsRequesttransfer.HydrateEventsResponsetransfer.EventBehaviorFacadeInterface::hydrateEventDataTransfer()facade method to get event data with timestamp.