-
Notifications
You must be signed in to change notification settings - Fork 9.4k
magento/magento2#39169: Special Price To Date is wrongly validated on applySpecialPrice #39690
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
base: 2.4-develop
Are you sure you want to change the base?
magento/magento2#39169: Special Price To Date is wrongly validated on applySpecialPrice #39690
Conversation
Hi @KrasnoshchokBohdan. Thank you for your contribution!
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
@magento run all tests |
@magento run all tests |
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.
Hello @KrasnoshchokBohdan,
Thanks for the contribution!
Please fix the failed static test and also please add an automated test in accordance to the DOD.
Thanks
…cialPrice - changing Copyright
@magento run all tests |
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.
@@ -274,7 +274,7 @@ public function isScopeDateInInterval($scope, $dateFrom = null, $dateTo = null) | |||
$scopeTimeStamp = $this->scopeTimeStamp($scope); | |||
$fromTimeStamp = strtotime($dateFrom); | |||
$toTimeStamp = strtotime($dateTo); | |||
if ($dateTo) { | |||
if ($dateTo && date('H:i:s', strtotime($dateTo)) === '00:00:00') { |
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.
We will have the same +1 day result if use '00:00:00' in the API query. I'm not sure that it's correct solution
pls provide more information if we update special price with
#39169 from description how I tested it https://app.magento.test/rest/default/V1/products/special-price (app.magento.test replace with your base_url) if we check with POST /rest/<store_code>/V1/products/special-price-information but we should triger \Magento\Framework\Stdlib\DateTime\Timezone::isScopeDateInInterval |
@KrasnoshchokBohdan I mean
After the fix I haven't checked the source code more deeply, but I don't rule out that somewhere the time value may be thrown off and date become |
but it was the same before fix fix is only cover
#39169 from description { sorry it seems I still don't understand the question |
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 I mentioned changes looks good for me.
Only one pointt that require attention is failed unit test.
@engcom-Hotel Do we need internal team approval for protected property?
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.
@KrasnoshchokBohdan Could you please check failed unit test
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.
/** | ||
* @var SpecialPriceService|null | ||
*/ | ||
protected ?SpecialPriceService $specialPriceService; |
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.
Hello @Den4ik,
Thanks for highlighting this point.
Hello @KrasnoshchokBohdan,
Is it possible to make it private
and if access in subclasses is needed, expose a protected getter as below:
protected function getSpecialPriceService(): ?SpecialPriceService
{
return $this->specialPriceService;
}
…cialPrice - adding getter for SpecialPriceService
@magento run all tests |
@Den4ik I can't find this test probably it's only for EE and B2B, sorry |
It seems that review comments has been taken care by @KrasnoshchokBohdan. Thank you @KrasnoshchokBohdan, moving it to review. |
@magento run Functional Tests B2B, Functional Tests CE, Functional Tests EE, Magento Health Index |
@magento run Static Tests |
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.
Hello @KrasnoshchokBohdan,
Thanks for making the changes. Just one small thing, static test is still failing. Request you to please fix that. Other failures seems flaky to me.
Thanks
@magento run Static Tests |
@magento run Database Compare, Functional Tests B2B, Functional Tests CE, Functional Tests EE, Integration Tests, Magento Health Index, Sample Data Tests B2B, Sample Data Tests CE, Sample Data Tests EE, Semantic Version Checker, Unit Tests, WebAPI Tests |
Description (*)
As the author of the issue mentioned, if we add a special price using POST /rest/<store_code>/V1/products/special-price specifying in the request body date like "price_to": "2017-07-16 23:59:59" i.e. time including hours, minutes and seconds, then as a result we will get that the special price will be valid for one day longer than the specified date
this happens in the method \Magento\Framework\Stdlib\DateTime\Timezone::isScopeDateInInterval where there is code that literally adds +1 day
according to the example from the documentation (where "price_to": "2017-07-16 23:59:59" including hours is specified) we get this execution

therefore in this pull request I propose to skip adding 1 day in case in "price_to" specifies the time including hours
Related Pull Requests
--
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)