-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[CatalogPromotions] Take into account CP dates during processing #13189
[CatalogPromotions] Take into account CP dates during processing #13189
Conversation
34955d3
to
d328e8f
Compare
And I should see this product has no catalog promotion applied | ||
|
||
@api @ui | ||
Scenario: Not applying catalog promotion if its end date has alread passed |
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.
Scenario: Not applying catalog promotion if its end date has alread passed | |
Scenario: Not applying catalog promotion if its end date has already passed |
return $this->catalogPromotionRepository->findAll(); | ||
} | ||
|
||
return array_values($this->catalogPromotionRepository->findByCriteria($criteria)); |
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.
Why array_values()
?
public function provide(iterable $criteria = []): iterable | ||
{ | ||
if (empty($criteria)) { | ||
return $this->catalogPromotionRepository->findAll(); |
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.
Shouldn't this be just:
return $this->catalogPromotionRepository->findByCriteria($criteria);
Cause that repo method has foreach
so empty array will not add any where call etc. & in fact will do same as findAll()
.
/** | ||
* @param iterable|CriteriaInterface[] $criteria | ||
* | ||
* @return iterable|CatalogPromotionInterface[] |
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.
More accurate would be saying:
* @return iterable|CatalogPromotionInterface[] | |
* @return array<int, CatalogPromotionInterface> |
Thanks, Mateusz! 🎉 |
…PromotionsProvider (GSadee) This PR was merged into the 1.11-dev branch. Discussion ---------- | Q | A | --------------- | ----- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Related tickets | fixes after comments from #13189 | License | MIT <!-- - Bug fixes must be submitted against the 1.9 or 1.10 branch (the lowest possible) - Features and deprecations must be submitted against the master branch - Make sure that the correct base branch is set To be sure you are not breaking any Backward Compatibilities, check the documentation: https://docs.sylius.com/en/latest/book/organization/backward-compatibility-promise.html --> Commits ------- 5fb30cd [CatalogPromotion] Minor improvements in EligibleCatalogPromotionsProvider
Based on the work from this PR, we can start taking into account catalog promotion dates during their processing. Initially, I assumed that all
CatalogPromotionInterface $catalogPromotion
arguments passed toCatalogPromotionProcessor::process
will be already eligible (as we only use this processor in theAllCatalogPromotionsProcessor
service. If in the future, we will need filtration other than on CPs collection (e.g. on a single catalog promotion), we can extend the filter's functionality 🖖Note: I wanted to use a calendar service that is already used in Behat... but it's implemented in the ShippingBundle 💃 probably we would need to extract it somewhere to make it available for all other date-/time-based functionalities.