-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Allow multiple conditions for expired quotes collection #24821
Allow multiple conditions for expired quotes collection #24821
Conversation
Hi @p-bystritsky. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
@p-bystritsky unfortunately, only members of the maintainers team are allowed to assign developers to the pull request |
@p-bystritsky can you please sign the CLA |
Hi @p-bystritsky, thank you for your contribution! |
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.
Hi @p-bystritsky,
Thank you for you review!
Lets discuss your solution under my comment in the code.
/** | ||
* Provides expire quotes filter fields. | ||
*/ | ||
class ExpireQuotesFilterFieldsProvider |
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.
Do you really believe that we need to have a separated class to collect the filters?
Or could we just add filters as array parameter to the app/code/Magento/Sales/Model/ResourceModel/Collection/ExpiredQuotesCollection.php
?
I can not come with real case when we need a new class.
Closing this PR due to creation of #24858. |
Hi @p-bystritsky, thank you for your contribution! |
Description (*)
This is a refactoring of #20777
With Magento\Sales\Cron\CleanExpiredQuotes::setExpireQuotesAdditionalFilterFields() assigning values directly to the attribute and getExpireQuotesAdditionalFilterFields() being protected isn't callable outside the class, hence unable to add more to the existing condition, rather they end up being replaced with the new ones.
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
EE code should also be adapted.
Contribution checklist (*)