Fix CatalogRule start date insert logic

[mmenozzi]

For the same reason the rule is inserted until 24 hours after the rule end date, we should also start inserting it 24 hours before the start date. Am I wrong?

[kiatng]

I think the code is correct. I made a test:

// Original code.
$fromTime = (int) strtotime('2020-01-01');
$toTime = (int) strtotime('2020-01-02');
$toTime = $toTime ? ($toTime + 86400 - 1) : 0;
$result = [date('r', $fromTime), date('r', $toTime)];

The result is

array(2) {
  [0] => string(31) "Wed, 01 Jan 2020 00:00:00 +0000"
  [1] => string(31) "Thu, 02 Jan 2020 23:59:59 +0000"
}

Now with your code:

// Changed code.
$fromTime = (int) strtotime('2020-01-01');
$fromTime = $fromTime ? ($fromTime - 86400 + 1) : 0;
$toTime = (int) strtotime('2020-01-02');
$toTime = $toTime ? ($toTime + 86400 - 1) : 0;
$result = [date('r', $fromTime), date('r', $toTime)];

The result is wrong:

array(2) {
  [0] => string(31) "Tue, 31 Dec 2019 00:00:01 +0000"
  [1] => string(31) "Thu, 02 Jan 2020 23:59:59 +0000"
}

[mmenozzi]

When we faced this issue it was a timezone issue. I remember that the day before the Black Friday we set a rule to star on the next day. We are in the Europe/Rome timezone but we have an Australian store view with Sidney timezone, so we expected to see the rule already applied in the AU store view because it was already the Black Friday day there.

I remember we debugged locally and the problem was that the rule wasn’t added at the moment we created it but it should because of the Sidney timezone. So we fixed with this change.

[kiatng]

Could it be that the Australian store timezone was not configured correctly in the backend?

[mmenozzi]

No it was configured correctly.

[seansan]

There have been some changes on this in the past also ... same problem ... I remember it was fixed (Amsterdam timezone) .... it has to do with time of running, core time of the (unix) server and the timezone.

[addison74]

We should move forward this PR. @kiatng - what is your opinion?

[fballiano]

I coulnd't really understand why this solutions fixes the problem but I think we should never use +-86400 seconds since it doesn't work the day of the daylight saving +-1 hour change.

[kiatng]

We should move forward this PR. @kiatng - what is your opinion?

I think we need to replicate the issue first.

[fballiano]

ok since nobody knows what this is about and how to replicate, I'm closing it.

[addison74]

I would leave it open. I will make time to replicate the issue. If it confirms I will go further to implement the PR and check what is happening.

[fballiano]

@addison74 I would close this, @kiatng tested the coded and seemed not work #1007 (comment) (that result is wrong for sure) but using SECONDS_IN_DAY is also not a super good practise since it doesn't work the day of daylightsaving change.

[fballiano]

maybe it could be converted to an issue but we need a full reproducible case

[mmenozzi]

Hi guys, I'll try to provide a way to reproduce the issue in the next days.

[mmenozzi]

Hi guys, the bug still exist. At least on version 19.4.21 of OpenMage.
Here the steps to reproduce it:

1. First of all make sure that the current time of the machine you'll use to do the test is after the 3 PM.
2. Install OpenMage 19.4.21 with sample data, paying attention to configure Europe/Rome as default timezone
3. Login to admin panel and create a website, called "Australian", with a store and a store view in it
4. Go in System -> Configuration -> General -> General, select the "Australian" website in the scope menu and configure the "AUS Eastern Standard Time (Australia/Sydney)" as timezone for the "Australian" website.
5. Go to Catalog -> Manage products, select all products, select the "Update attributes" mass action and add all products to the "Australian" website.
6. Now go in Promotions -> Catalog Price Rules and create a new active rule starting from tomorrow (for example if today is 2023-01-16, set the start date to 2023-01-17), which discounts all the products by 20%. Apply the rule, reindex everything and clear the cache.
7. Now go in the frontend, in the Australian store view. Search for the product "AVIATOR SUNGLASSES" and open its product page.

If the current time is 3 PM of the 2023-01-16 in Europe/Rome, it means that in the Australia/Sydney timezone the current time is 2 AM (or 1 AM depending on the daylight saving time) of the 2023-01-17. This means that on the "Australian" store view the promotion should be already applied and we should see the discounted price of 236 for the "AVIATOR SUNGLASSES"; but what happens is that we still see the full price of 295.

If you apply the patch provided in this PR and you save and apply the catalog price rule again you'll get the discounted price.

/cc @fballiano @addison74 @kiatng @seansan

[kiatng]

Note changing fromTime to one day earlier may cause the rule to be applied too early, although it works in the test case. Here, $fromTime is inserted in the table.

magento-lts/app/code/core/Mage/CatalogRule/Model/Resource/Rule.php

Lines 233 to 238 in 7c16d61

	->columns([
	new Zend_Db_Expr($store->getWebsiteId()),
	'cg.customer_group_id',
	'p.entity_id',
	new Zend_Db_Expr($rule->getId()),
	new Zend_Db_Expr($fromTime),

[edit]

To add the rule ahead of time, change line 209

magento-lts/app/code/core/Mage/CatalogRule/Model/Resource/Rule.php

Lines 208 to 213 in 7c16d61

	$timestamp = time();
	if ($fromTime > $timestamp
	|| ($toTime && $toTime < $timestamp)
	) {
	return;
	}

to

        if ($fromTime > $timestamp + self::SECONDS_IN_DAY

@mmenozzi Can you test line 209 without changing $fromTime and see if it works for you?

[sreichel]

Note changing fromTime to one day earlier may cause the rule to be applied too early

Also seen this .... the topic is slowly growing a beard ...

[luigifab]

I tested the test case with and without the PR, and this is not working, like @kiatng says the rule is applied to early.

australian website:

default website:

my rule 20% for tomorrow:

By applying rules and reindexing at Friday, 20 January 2023 14:13:08 Europe/Paris or Saturday, 21 January 2023 12:12:21 am Australia/Sydney, this does not change anything (with the PR - without the PR price are not discounted).

[kiatng]

Line 209:

        if ($fromTime > $timestamp
            || ($toTime && $toTime < $timestamp)
        ) {

The if statement is to restrict the number of rules to be added to the table. If the condition(s) is(are) true, it will skip the rule. The problem with this PR is that it changed the $fromTime, which is then added to the table resulting in the rule being applied too early.

Instead, we can change it to

        if ($fromTime > $timestamp + self::SECONDS_IN_DAY
            || ($toTime && $toTime < $timestamp)
        ) {

Note that the comparison for $toTime is correct as it is because the time is passed due date, the rule is skipped. However, the $fromTime is compared to 24 hours later; if $fromTime is not yet tomorrow, add the rule. Why this $timestamp + self::SECONDS_IN_DAY, has 24 hours added? It is because the cron runs only once a day. If the cron runs every 12 hours, then we do $timestamp + self::SECONDS_IN_DAY/2

@luigifab Can you test if this works.

[edit]

[luigifab]

With @kiatng suggestion, discount is not applied too early, BUT to display the discount, I had to "Save and apply" my rule at 26/01/23 13:08:49 utc / Friday, 27 January 2023 12:08:49 am Australia/Sydney.

Before applying the rule, I forgot to add the product to the cart to check the price. I will try again.

[luigifab]

It seems that the @kiatng solution works. @mmenozzi can you try?

I need to run catalogrule_apply_all (same as apply rules I think, because the cron job is started at night when my computer is off). Perhaps it is necessary to run this cron job more than once per day.

[fballiano]

so sorry guys, I've rebased the PR and it screwed everything... I've rebuilt it (adding my strtotime() suggestions), do you think it misses something?

[luigifab]

Not sure about changes between the version of the PR I tested and now.

[fballiano]

The 86400 second cannot be used, it does not work 2 days a year, that i am more than sure

[addison74]

This issue reminds me of another one that I discussed in another forum https://magento.stackexchange.com/questions/133405/issue-when-specialpricefromdate-is-equal-with-specialpricetodate/

What happens when price_from is perfectly equal to price_to? For example both having the value 2023-07-16 00:00. Naturally it should run for a day.

[luigifab]

Is it possible to deploy the PR on demo.openmage.org to test it again?

[sreichel]

so sorry guys, I've rebased the PR and it screwed everything... I've rebuilt it (adding my strtotime() suggestions), do you think it misses something?

@kiatng is the original PR still available?