Add time to promotion start and end date

Hello there,

This should be the solution.

This patch is supposed to fix tests and adds an post_update hook to update existing start and end dates.

Changing date formats everywhere to fix tests.

Fix one more test.

Investigating...

Thanks for your work in getting this started czigor!

UI looks good.

Feature Request:

When Inputing a specific end Date, the Time box should fill in by default to the end of that day:
Current:

Goal:

Error:

When a specific end time is selected, it saves as 12:00:00 pm. (or it is saved correctly, but presented in error)

To Do List:

1. End Time does not save correctly. Patch 10 saves all times as 12:00:00 pm
2. Add ajax validation so End Date/Time cannot be before Start Date/Time
3. Fix Promotion list formatting
   - Screenshot from patch 10:
4. End time should never be null once date has been set.

Proposed fix to todo #3 List Format. I understand that this is a UX decision and leaving it at the ISO 8601 date format (2004-02-12T15:19:21+00:00), though less readable, may be better as a default as Commerce is used in various countries.

Patch 10 changes the format to 'Y-m-d\TH:i:s' in Promotion.php line 338. Perhaps the list should be consistent with how the value is actually saved.

Testing is held back by this issue: https://www.drupal.org/project/commerce/issues/2998745

Progress:
getDefaultEndDate is in better shape.

Patch 17 still needs:

Tests written.
Make sure end date does not populate until checkbox for enddate option is clicked.

IGNORE!
My mistake, this patch had an old patch inside.

IGNORE!
My mistake, this patch had an old patch inside.

IGNORE!
My mistake, this patch had an old patch inside.

This does not have the test adjustment for the mysterious Sydney -> UTC timezone change.

+++ b/modules/promotion/tests/src/FunctionalJavascript/PromotionTest.php
@@ -69,66 +79,128 @@ class PromotionTest extends CommerceWebDriverTestBase {
+    // TODO This is a horrible fix!
+    // - 11 hours due to timezone diff of Syndey to GMT.
+    $end_date = gmdate('M j, Y', $timestamp + 604800 - 39600);
+    $end_time = gmdate('g:i:sa', $timestamp + 604800 - 39600);
+    $this->assertSession()->pageTextContains($end_date);
+    $this->assertSession()->pageTextContains($end_time);

This offsets what the test searches for by -11 hours.
Something is happening that switches the test's setup's timezone of Australia/Sydney into GMT.
To recreate issue, at admin/config/regional/settings, set timezone to Australia/Sydney and set ' Users may set their own time zone' to unchecked.

Timezone is set here: FunctionalTestSetupTrait::initConfig

Basically, DrupalDateTime is fundamentally flawed in regards to timezones. Commerce has been forced to use gmdate() to get around this, but its not ideal.

Aside from this error, which does not occur when a drupal setup is set to UTC time, the PromotionForm now has a validation that checks for the end date being before the start date.

This error could be helped greatly by a commerce store timezone field. See: #2883789: Create a $store->timezone field and use it for promotions and taxes
Also, this core issue could be of great help: #2632040: [PP-1] Add ability to select a timezone for datetime field

+/**
+ * Add second-level resolution to promotion start and end dates.
+ */
+function commerce_promotion_post_update_10(&$sandbox = NULL) {

+/**
+ * Add second-level resolution to promotion start and end dates.
+ */
+function commerce_promotion_post_update_11(&$sandbox = NULL) {

We seem to have the same update twice.

+    // 'U' is seconds since epoch.
+    if ($end_date->format('U') < $start_date->format('U')) {

I don't like the comment. Either we should assume people know the common date formats or look them up (we're making that assumption elsewhere where formats like 'M j, Y g:i:sa e' are used; they have no comments) or we should name the format like DateTimeItemInterface::DATETIME_STORAGE_FORMAT does.

-    $today = gmdate('Y-m-d', $this->time->getRequestTime());
+    $today = gmdate('Y-m-d\TH:i:s', $this->time->getRequestTime());

These should use DateTimeItemInterface::DATETIME_STORAGE_FORMAT instead.

+ // Check the 'Provide and end date' checkbox.
That should read "Provide an end date".

I'm not a fan of the format there. Timezone isn't displayed normally, is it? Can we show it in the user's or default timezone instead?

Do we need seconds? I suppose we could have this issue all over again to add seconds if someone wanted more granularity but I don't know there's a use case for it and it looks pretty cluttered.

Here's one of those same promotions. I'd created them before applying the patch and then ran the update, so the date looked right on the list page but it's wrong when I edit them: it should be 12:00 AM on the 26th and 30th.

It looks like there's time zone issues all around in this issue since the promotion list shows what I'd expect: starting November 26 at 12:00 AM America/Vancouver but the date is stored as 2018-11-26T00:00:00. Dates are stored in UTC so that's off by 8 hours (maybe causing the incorrect value in the form too).

If seconds are not shown, its not obvious that when an end time is set to 11:59pm, it ends at 11:59:00 or at 12:00am. Keeping the seconds granularity at least guarantees that 11:59:59 is the actual end time. Whether that is the start or end of the second, I'm not sure.
To get rid of seconds the popout menu would need to be altered as well as all the date formats. I think it would be more difficult to get rid of them than it would be worth.

Here is a new patch with those small points fixed.

I think the best solution to these timezone issues would be to give Commerce Store a timezone and reference everything in commerce to that. https://www.drupal.org/project/commerce/issues/2883789
Would you recommend this approach @Travis?

@nlz Why does the store need a timezone for this? The user can have their own time zone, the site has a default time zone, and it'll always be stored in UTC according to the date module.

In commerce we really should be using DrupalDateTime. Instead we have this which I believe is contributing to the timezone issues. (gmdate to be specific).

   $timestamp = \Drupal::time()->getRequestTime();
    $end_date = gmdate('m/d/Y', $timestamp + 604800);

I have tried using new DrupalDateTime in all this instances, but the problem is that if Users are allowed to set their own timezone, then the Promotions module creates a new time based on the User timezone instead of the default timezone. This issue can hopefully be fixed by replacing all gmdates with new DrupalDateTime( *specify default timezone* ). Because a timezone needs to be hardcoded, I think a store timezone would be more useful in the long run. Perhaps its better to let that be a separate issue?

I don't agree about using the store timezone. Promotions aren't tied to a store, are they? Definitely use the user's timezone if it's set since that's what they'll be thinking in when they write the time.

I have tried using new DrupalDateTime in all this instances, but the problem is that if Users are allowed to set their own timezone, then the Promotions module creates a new time based on the User timezone instead of the default timezone.

Isn't that what we want? The user who provides the time is going to write it in whichever zone they've said is theirs.

This issue can hopefully be fixed by replacing all gmdates with new DrupalDateTime( *specify default timezone* ).

If we've got to change that, let's go ahead. I'm not sure it's all use of gmdate though.

The update hook also needs to use a configured timezone instead of always UTC.

-      // The product_id setting has been removed and needs to be migrated to a condition.
+      // The product_id setting has been removed
+      // and needs to be migrated to a condition.

That should wrap as close to 80 characters as possible.

• TimeZone issues have been fixed, or mitigated, by setting setDefaultStartTime to returning a new DrupalDateTime('now', 'UTC').
• Rearranged Promotions so all time functions are next to each other.

+++ b/src/Plugin/Field/FieldWidget/EndDateWidget.php
@@ -54,17 +55,18 @@ class EndDateWidget extends DateTimeDefaultWidget {
-        // Adjust the date for storage.
-        datetime_date_default_time($date);
-        $date->setTimezone(new \DateTimezone(DATETIME_STORAGE_TIMEZONE));
-        $item['value'] = $date->format(DATETIME_DATE_STORAGE_FORMAT);

I believe this was an old fix to fight the User/Default timezone change. No longer needed.

The date I see at /admin/commerce/promotions doesn't match the date I see when I create or edit a promotion. For example, I added one and used the default time: 2018-11-27 16:01:18. When I view the list of promotions it says Nov 28, 2018 12:01:18am. My site's timezone is America/Vancouver.

Travis I don't think that the update hook will have any issue due to a timezone not being specified. Could you explain more?

If you're in America/Vancouver and you have a promotion starting on 2019-01-01 will it be 2019-01-01 08:00 UTC after the update or 2019-01-01 00:00 UTC (2018-12-31 16:00 Pacific)?

+++ b/modules/promotion/commerce_promotion.post_update.php
@@ -384,3 +385,48 @@ function commerce_promotion_post_update_9(&$sandbox = NULL) {
+function commerce_promotion_post_update_10(&$sandbox = NULL) {
+  $promotion_storage = \Drupal::entityTypeManager()->getStorage('commerce_promotion');
...
+  foreach ($promotions as $promotion) {
+    if ($promotion->start_date->value) {
+      $promotion->start_date = $promotion->start_date->value . 'T00:00:00';
+    }
+    if ($promotion->end_date->value) {
+      $promotion->end_date = $promotion->end_date->value . 'T00:00:00';
+    }

Because the update works directly with promotion storage, I don't think it will touch any of the timezone conversions in the Promotion class.

Before this patch there were no promotion dates stored with time information. The update should be changing stored values from 'YYYY-MM-DD' to 'YYYY-MM-DDT00:00:00'.

will it be 2019-01-01 08:00 UTC after the update or 2019-01-01 00:00 UTC (2018-12-31 16:00 Pacific)?

+++ b/modules/promotion/src/Entity/Promotion.php
@@ -327,16 +330,42 @@ class Promotion extends CommerceContentEntityBase implements PromotionInterface
+    // Change from storage timezone to current user's timezone.
+    $date = new DrupalDateTime($this->get('start_date')->value, DateTimeItemInterface::STORAGE_TIMEZONE);
+    $user_tz = new DateTimeZone(Drupal::currentUser()->getTimeZone() ?? DateTimeItemInterface::STORAGE_TIMEZONE);
+    $date = $date->setTimezone($user_tz);
+    return $date;

the getStartDate function in promotions will take the raw value of 'YYYY-MM-DDT00:00:00' and apply the users timezone to it. This happens when times are presented in promotionListBuilder and when editing a promotion.

Note to maintainer:

+++ b/modules/promotion/commerce_promotion.post_update.php
@@ -384,3 +387,60 @@ function commerce_promotion_post_update_9(&$sandbox = NULL) {
+function commerce_promotion_post_update_10(array &$sandbox = NULL) {
...
+      // This will pull timezone from User, then site default.
+      $date = new DrupalDateTime($promotion->start_date->value);
+      $date->setTimezone(new DateTimeZone(DateTimeItemInterface::STORAGE_TIMEZONE));
+      $promotion->start_date = $date->format(DateTimeItemInterface::DATETIME_STORAGE_FORMAT);

When running this update via the UI, old promotions will be updated via User timezone before being saved in UTC standard timezone.

However, when this update is run without a user, via drush updb, old promotion times will be updated via site default timezone, which may not be what the user intends. The updated timezones will be off by the # of hours between user's timezone and the site default timezone.

I'm glad PromotionAvailabilityTest failed as it showed I was missing logic in Promotion::available and getEndDate.

Patch tested and works for me.

+++ b/modules/promotion/tests/src/Kernel/PromotionAvailabilityTest.php
@@ -152,19 +154,36 @@ class PromotionAvailabilityTest extends CommerceKernelTestBase {
+    // TODO Remove try/catch and enable these for Drupal 8.7.
...
+    // TestCase::expectException(InvalidArgumentException::class);
+    // TestCase::expectExceptionMessage('End date cannot come before start date.');
+    try {
+      $promotion->setEndDate($date);
+    }
+    catch (InvalidArgumentException $e) {
+      if ($e->getMessage() != 'End date cannot come before start date.') {
+        self::fail('Promotion::setEndDate() did not throw proper error.');
+      }
+    }

Fix for php5 compatibility until Drupal 8.7.

From #23

This does not have the test adjustment for the mysterious Sydney -> UTC timezone change.

The testbot timezone is set to 'Australia/Sydney' in core/tests/bootstrap.php. Read the comment for more info.

I applied the patch and created a promotion. The displayed start date is correctly set to today but the time is not now, it is 13 hour in the future. The timezone of the test site is set to Auckland, running in a lando setup.

When viewing the promotion at /admin/commerce/promotions the displayed start time is formatted like this, Jan 24, 2019 9:50:05pm. I expected the 'pm' to be uppercase. Thinking I had control of that format I checked at admin/config/regional/date-time and do not see that format in the list. That is rather jarring. Maybe the user should not have control of that format, maybe they should, not for me to say. But I think this points to this needing a UX review.

This could use an IS update, the idea is simple but there are nuances about the UX and timezones that should be mentioned in the IS to help other reviews and the committers. It would be helpful to have before and after screenshots.

This does not include a code review.

This is a good size patch to review and was RTBC'ed in #48 after a manual testing. So, just cleaning up and getting ready for the committers, nice. This is a nice addition.

In general, the patch looks good and there are no coding standard issues. Well done nlz.

The challenges here with the review are that there are few interdiffs in the issue. It make is impossible to see the fixes applied for a given point, whether from a review point or view or from a learning point of view. If not seen yet, there is a handbook page for Creating an interdiff

There is also a lot of changes in the patch not related to the issue, mostly in the test files where assertions are changed. AFAICT it is just noise. If that is right, please move to a new issue, otherwise explain clearly why it is done in this patch.

1. +++ b/modules/order/tests/src/Kernel/Formatter/PriceCalculatedFormatterTest.php
   @@ -78,6 +79,9 @@ class PriceCalculatedFormatterTest extends CommerceKernelTestBase {
   +    // Set start date so it does not equal the time on line 88 of
   
   +++ b/modules/order/tests/src/Kernel/PriceCalculatorTest.php
   @@ -96,6 +96,9 @@ class PriceCalculatorTest extends CommerceKernelTestBase {
   +    // Set start date so it does not equal the time on line 88 of
   

   Surely the time on line 88 may not be on line 88 in the future. Should explain why the start date is set 1 second earlier than now.

2. +++ b/modules/promotion/commerce_promotion.post_update.php
   @@ -384,3 +387,60 @@ function commerce_promotion_post_update_9(&$sandbox = NULL) {
   + * Add second-level resolution to promotion start and end dates.
   

   What does 'second-level resolution' mean?

3. +++ b/modules/promotion/commerce_promotion.post_update.php
   @@ -384,3 +387,60 @@ function commerce_promotion_post_update_9(&$sandbox = NULL) {
   +      // This will pull timezone from User, then site default.
   

   Why is User capitalized? How is this getting timezone from the user entity?

   I've been pinged for having a comment after an 'if'. Maybe a comment above the for to explain what is being done here for start_date and end_date. Seems easier for the reader.

4. +++ b/modules/promotion/commerce_promotion.post_update.php
   @@ -384,3 +387,60 @@ function commerce_promotion_post_update_9(&$sandbox = NULL) {
   +      $promotion->start_date = $date->format(DateTimeItemInterface::DATETIME_STORAGE_FORMAT);
   ...
   +      $promotion->end_date = $date->format(DateTimeItemInterface::DATETIME_STORAGE_FORMAT);
   

   Can this be done with $promotion->set() ?

5. +++ b/modules/promotion/src/Entity/Promotion.php
   @@ -13,6 +13,9 @@ use Drupal\Core\Datetime\DrupalDateTime;
   +use Drupal\datetime\Plugin\Field\FieldType\DateTimeItemInterface;
   +use DateTimeZone;
   +use InvalidArgumentException;
   

   A nit, well this use list isn't alphabetized but at least follow the system being used. 'Tis easier to read.

6. +++ b/modules/promotion/src/Entity/Promotion.php
   @@ -327,16 +330,48 @@ class Promotion extends CommerceContentEntityBase implements PromotionInterface
   +    // Change from storage timezone to current user's timezone.
   

   Not familiar with 'storage timezone', can this change to explain what time is being changed. Is it the site timezone?

7. +++ b/modules/promotion/src/Entity/Promotion.php
   @@ -327,16 +330,48 @@ class Promotion extends CommerceContentEntityBase implements PromotionInterface
   +  public static function getDefaultStartDate() {
   
   @@ -345,16 +380,27 @@ class Promotion extends CommerceContentEntityBase implements PromotionInterface
   +  public static function getDefaultEndDate() {
   

   Why are these not on PromotionInterface?

8. +++ b/modules/promotion/src/Entity/Promotion.php
   @@ -418,15 +464,15 @@ class Promotion extends CommerceContentEntityBase implements PromotionInterface
   +    if (($usage_limit = $this->getUsageLimit())) {
   

   extra parens

9. +++ b/modules/promotion/src/Form/PromotionForm.php
   @@ -20,7 +21,7 @@ class PromotionForm extends ContentEntityForm {
   +        '#markup' => $this->t("Promotions can't be created until a store has been added. @link", ['@link' => $link->toString()]),
   

   Why is this changed in this patch. Usually in a separate one for standards.

10. +++ b/modules/promotion/src/Form/PromotionForm.php
    @@ -32,9 +33,6 @@ class PromotionForm extends ContentEntityForm {
    -    /* @var \Drupal\commerce_promotion\Entity\Promotion $promotion */
    -    $promotion = $this->entity;
    

    Why is this removed?

11. +++ b/modules/promotion/src/Form/PromotionForm.php
    @@ -119,6 +117,26 @@ class PromotionForm extends ContentEntityForm {
    +   * Checks that End Date comes after Start Date.
    

    Capitalization of start date and end date not needed.

12. +++ b/modules/promotion/src/Form/PromotionForm.php
    @@ -119,6 +117,26 @@ class PromotionForm extends ContentEntityForm {
    +        $form_state->setError($form, $this->t('End Date must come after Start Date.'));
    

    Should end date and start date be capitalized here?

13. +++ b/modules/promotion/src/PromotionStorage.php
    @@ -8,11 +8,13 @@ use Drupal\Component\Datetime\TimeInterface;
    +use Drupal\datetime\Plugin\Field\FieldType\DateTimeItemInterface;
    

    nit. move up, under Drupal\commerce_order ...

14. +++ b/modules/promotion/src/PromotionStorage.php
    @@ -83,7 +85,8 @@ class PromotionStorage extends CommerceContentEntityStorage implements Promotion
    +    $today = new DrupalDateTime('now', DateTimeItemInterface::STORAGE_TIMEZONE);
    

    This needs a comment explaining why DrupalDateTime and not anything else.

15. +++ b/modules/promotion/tests/src/FunctionalJavascript/PromotionTest.php
    @@ -4,11 +4,15 @@ namespace Drupal\Tests\commerce_promotion\FunctionalJavascript;
    +use Drupal;
    

    Remove

16. +++ b/modules/promotion/tests/src/FunctionalJavascript/PromotionTest.php
    @@ -4,11 +4,15 @@ namespace Drupal\Tests\commerce_promotion\FunctionalJavascript;
    + *
    + * @package Drupal\Tests\commerce_promotion\FunctionalJavascript
    

    Not needed.

17. +++ b/modules/promotion/tests/src/FunctionalJavascript/PromotionTest.php
    @@ -57,7 +68,7 @@ class PromotionTest extends CommerceWebDriverTestBase {
    -      $this->assertNotEmpty($tab_matches);
    +      self::assertNotEmpty($tab_matches);
    

    All these look unrelated to the issue. Are these needed for the tests to pass? if not, move to another issue.

Was talking to tbradbury about the changes here that replace gmdate with DrupalDateTime and how that may not be a good idea to have one part of a system using one date function than the rest of the system. He suggested asking bojanz or mglaman, for an opinion. So let's do that after the IS is updated.

Thanks for the thorough review!

This interdiff focuses on tests and your bullet points 1 and 17.

This interdiff focuses on commerce_promotion.post_update and your points 2, 3, and 4.

This interdiff is for Promotion.php. Also, I added getDefaultStartDate() & GetDefaultEndDate() to PromotionInterface.

Re: point #6:
I have changed my comments, but they are not much more clear. I could add a link to #2716891: DateTimeIso8601::setDateTime() vs. DateTimeItem::schema(): (at least) one of them is broken that explains the root of the problem. This also explains more on you concerns with replacing all the usages of gmdate() from your comment #55. It certainly is a big deal to change how commerce is handling time. I believe this patch uses DrupalDateTime to the best of its ability, which is the ideal, to use the tools that drupal provides instead of hard php. Hopefully DrupalDateTime will store timezone information in the future and this huge headache will be resolved.

Re: point #8:
The extra parentheses are because PhpStorm inspections suggested them when making an assignment in a condition. I don't think it technically changes things, it just lets the IDE know that it is intentional. However, upon further research, there is no set drupal standard for this, but #2907332: Disallow assignments in conditions seems to suggest that it is best practice to not make assignments within conditions, (mainly to avoid programmer errors).

This interdiff fixes the failing tests of patch #58. Also PromotionForm and PromotionStorage.

Re: point #10 - That was an unused variable, but you are correct that standards issues should not be part of this issue.

Re: point #14 - Previously the code used gmdate() as a way to force storage to use 'UTC' timezone. I chose to use DrupalDateTime combined with the DateTimeItemInterface constants because you can easily decipher all the necessary components; when, what timezone, and format.

@nlz, nice work! The IS explains what is going on here.

Two things about that though,

This patch standardizes the use of DrupalDateTime instead of gmdate() across Commerce.

This may need some clarification because gmdate was only used in Promotions. So, would it be correct to say that this patch brings Promotion in line with the rest of Commerce?

And let's add 'Remaining tasks' to the IS.

Still a few more items:

1. +++ b/modules/order/tests/src/Kernel/Formatter/PriceCalculatedFormatterTest.php
   @@ -78,6 +79,11 @@ class PriceCalculatedFormatterTest extends CommerceKernelTestBase {
   +    // The tests run so fast that the time set here (new DrupalDateTime()
   

   Much better comment but swap the two sentences so the most important piece is first. Actually, the first sentence isn't really needed and it is full of code.

2. +++ b/modules/order/tests/src/Kernel/PriceCalculatorTest.php
   @@ -96,6 +96,11 @@ class PriceCalculatorTest extends CommerceKernelTestBase {
   +    // The tests run so fast that the time set here (new DrupalDateTime()
   

   As above

3. +++ b/modules/promotion/commerce_promotion.post_update.php
   @@ -436,3 +438,61 @@ function commerce_promotion_post_update_10(&$sandbox = NULL) {
   + * Enables promotion start and end dates to store date *and time* values.
   

   I don't recall '*' in a comment.How are the '*' parsed for display?

4. +++ b/modules/promotion/src/Form/PromotionForm.php
   @@ -119,6 +120,26 @@ class PromotionForm extends ContentEntityForm {
   +   * Checks that end date comes after start date.
   

   Now that I see this I am not sure the 'comes after' is helpful for those where English is not their first language. How about 'Checks that the end data is later than the start date'.

5. +++ b/modules/promotion/src/Form/PromotionForm.php
   @@ -119,6 +120,26 @@ class PromotionForm extends ContentEntityForm {
   +        $form_state->setError($form, $this->t('End date must come after start date.'));
   

   And same for this string.

6. +++ b/modules/promotion/src/Entity/Promotion.php
   @@ -327,16 +330,50 @@ class Promotion extends CommerceContentEntityBase implements PromotionInterface
   +    // Convert standard storage timezone 'utc' to user's timezone.
   

   What is 'standard storage timezone'?

7. +++ b/modules/promotion/tests/src/FunctionalJavascript/PromotionTest.php
   @@ -2,7 +2,9 @@
   +use Drupal;
   

   Unused, can be removed.

Oh, the code standard fixes should go to another issue with links here. Or, whatever the resolution of that is.

Thanks @quiteone. This interdiff covers all of your points from comment #67.

For points 5 and 6 I just removed the comments because the code says what's happening just as well.

Getting better all the time ...

+++ b/modules/order/tests/src/Kernel/Formatter/PriceCalculatedFormatterTest.php
@@ -79,10 +79,9 @@ class PriceCalculatedFormatterTest extends CommerceKernelTestBase {
+    // Setting this promotion's start date to '-1 second' ensures the query
+    // in PromotionStorage ->condition('start_date', $today ('now'), '<=')
+    // will return this promotion.

Still not keen on code being in a comment, it should be expressed in words and I think it should be a present tense verb, Set not Setting.

How does this sound?

"Set this promotion's start date to '-1 second' so that the PromotionStorage query can load any promotion before 'now'."

Changed that comment to 'Start the promotion in the past to ensure it is active.'.

Fixed test that was failing due to the error message changing.

Much better and credit goes to tbradbury for the concise wording of the comment.

I think still todo here is to respond to these:

• #67. At the beginning of the comment there are 2 suggestions about the IS, related to the use of gmdate and to have a Remaining Task section in the IS.
• #68 Does Commerce have meta issues for such changes?

Changed test assertions from `self::` to `$this->` to be consistent with the rest of commerce/drupal.
See #3029358: Change tests calling static methods as '$this->' to 'static::' and #3029321: Change tests calling static methods as '$this->' to 'static::' for more details.

Fixed poor copy paste job from previous patch.

Removing UX Review based on comments in #80.
Removing Needs IS update, See #75
It looks like manual testing was done in #80

There are two followups to make, and fixes for some nits I just noticed. Settings NW for the nits. I'm currently thinking that once that is done this goes to RTBC to get feedback from the committers before making the followups.

1. +++ b/modules/promotion/tests/src/FunctionalJavascript/PromotionTest.php
   @@ -98,33 +99,111 @@ class PromotionTest extends CommerceWebDriverTestBase {
   +    // (1.) Test saving an end date & time.
   ...
   +    // (2.) Test for no End Date.
   ...
   +    // (3.) Test for End Date === Start Date.
   ...
   +    // (4.) Test for End Date before Start Date.
   

   remove the parenthesis, it it just noise.

2. +++ b/modules/promotion/tests/src/Kernel/PromotionStorageTest.php
   @@ -97,17 +98,19 @@ class PromotionStorageTest extends CommerceKernelTestBase {
   +    // (Promo 1)
   ...
   +    // (Promo 2)
   
   @@ -115,28 +118,37 @@ class PromotionStorageTest extends CommerceKernelTestBase {
   +    // (Promo 3)
   ...
   +    // (Promo 4)
   

   No need for parenthesis and should be Promotion. And can be on one line, that is, merged with the following one line comment.

@quietone.
For your point 1:
I would rather split each test into its own function, because they all have their own goals. However, this has a big performance cost. I have added comment blocks to give visual space to each different test. Because comment blocks are rare I have the feeling that you'll want them removed :-P

Point 2 is taken care of.

Patch works

Patch failed to apply for me. Here's a reroll.

+++ b/modules/order/tests/src/Kernel/Formatter/PriceCalculatedFormatterTest.php
@@ -78,6 +79,8 @@ class PriceCalculatedFormatterTest extends CommerceKernelTestBase {
     ]);

+++ b/modules/promotion/src/PromotionStorage.php
@@ -83,7 +85,8 @@ class PromotionStorage extends CommerceContentEntityStorage implements Promotion
+    $today = $today->format(DateTimeItemInterface::DATETIME_STORAGE_FORMAT);

Disclaimer: Timezones hurt my brain.

Don't we need to convert this to the intended timezone? And, more importantly, how do we decide that?

In the list builder we convert from UTC to the user's timezone but when actually checking to see if the promotion is available we don't...

@lisastreeter's reroll just had an extra use

Don't we need to convert this to the intended timezone? And, more importantly, how do we decide that?

In the list builder we convert from UTC to the user's timezone but when actually checking to see if the promotion is available we don't...

When saving a promotion:

• Your timezone is set to America/New_York and you enter a start time of 2019-10-01 07:00.
• Daylight savings is being observed so your time zone is UTC-4:00.
• 2019-10-01T07:00-4:00 is 2019-10-01 11:00 UTC.
• 2019-10-01T11:00Z is stored in the database.

When you look at the promotion again, 2019-10-01T11:00Z is loaded from the database and displayed to you as 2019-10-01T07:00-04:00.

To get active promotions, the query is looking for promotions that start before now, so now, formatted in UTC is the correct value to search by. You don't need to know the timezone of the user looking at products for that. If you wanted to show the user "this product is on sale until ______", then you'd translate it into their timezone when you print it.

+++ b/modules/promotion/commerce_promotion.post_update.php
@@ -436,3 +438,61 @@ function commerce_promotion_post_update_10(&$sandbox = NULL) {
+      $date = new DrupalDateTime($old_start_date);

Shouldn't this be $old_start_date->value? Same for $old_end_date

Thanks @tbradbury - I just had to step through the logic to see that for myself, didn't realize you already commented. This all makes sense to me now. From an admin/editor perspective there would just need to be some training, i.e. If you want this to kick off at 12 midnight eastern time and you're in central, you need to start it at 11m your time (basically exactly what you told me in slack).

The only other thing I noticed, and it's pretty trivial, is, for me, the default time is set to now() - 4 hours

One more comment and I'm done :D

I *think* we should be setting the default start date to storage timezone so the user with a timezone set doesn't see something unexpected and I think the post update should use ->value

Sorry to see that you've had so much trouble implementing this feature, the timezone question is a big PITA.

A bit of background from #2883789: Create a $store->timezone field and use it for promotions and taxes:

Because the date field stores no timezone, the promotion start/end dates are always specific to the customer's timezone. If you specified June 1st - June 10th assuming the EST timezone, and I'm in the GMT timezone, then the promotion will start earlier for me than for someone who's in EST. We could solve this by having a $store->timezone that's used for these things.

Another thing to note is that DateTimeItem (the field type) assumes that the date time is stored in UTC. The default widget, formatter and normalizer convert back and from UTC. Our approach in PromotionStorage is to take that stored date and just replace UTC with the current customer timezone, without converting the actual value ("2019-02-02 20:00" stays "2019-02-02 20:00").

There are 4 different possible timezones at play here:
1) Site timezone (admin/config/regional/settings with a fallback to PHP's default timezone)
2) Store timezone (only exists conceptually until #2883789: Create a $store->timezone field and use it for promotions and taxes lands)
3) Merchant timezone (the user editing the promotion)
4) Customer timezone (the user going through checkout)

The timezone needs to be used for both promotions and taxes.
Let's say France is increasing VAT from 20% to 21% on January 1st, from which perspective are we looking here? I would always say the perspective of the business, meaning site and store. Two customers from two different countries should not get the VAT increase at different times, therefore the customer timezone doesn't make sense. Same with the promotion timezone, you'd generally advertise a specific timezone "Offer is valid from Jan 1st to Jan 31st at midnight GMT+2". I would remove the merchant timezone from the same reason. If I'm editing a French store's promotion/tax type, the fact that I'm in the US should not be a factor. The difficulties the existing patch has with updating promotions show that pursuing the merchant timezone is a mistake.

So, with that in mind, our only two options are site timezone and store timezone. Let's discuss those two options:

1) Use the site timezone (as configured in the Drupal settings), both in the widget and when determining availability

This is super easy to implement (one function call to get the site timezone), and doesn't mess with the storage, we still convert to/from UTC. But it's not precise if the site has multiple stores which are not in the same timezone. For example, Sweden and Finland are one hour apart.

2. Break the storage assumption that the datetime is in UTC, and use the fact that there's no explicit timezone. Store the datetime as-is and assign the timezone of the current store when determining availability. $promotion->getStartDate($store_timezone = 'UTC'). This naturally requires a custom widget/formatter/normalizer. Stores get the correct behavior, at the cost of complexity.

Short example for how this works: Assume that UTC is 2:30 and GMT+1 (my site timezone) is 3:30.
With #1, I enter the value in the widget in GMT+1 (site timezone), it's shifted one hour behind, stored. Then on retrieval it's shifted one hour forward. With the scheme in #2 we don't shift, we just say "Your timezone was GMT+1 all along"

We have two large sites with an explicit requirement on #2, so that's the path we'll have to take.
I have done initial prep by committing #3093784: Add the concept of an order "calculation date". I am now focused on #2883789: Create a $store->timezone field and use it for promotions and taxes. That issue will have the majority of the timezone changes. The remaining patch here should be relatively simple.

Would be very happy to hear questions and comments.

#2883789: Create a $store->timezone field and use it for promotions and taxes has landed. Here's a new patch that builds upon it.

New pieces of logic:
- The promotion is no longer available on the end date (if you specify 2019-02-24 00:00:00 as the end date, the last second in which the promotion applies is 2019-02-23 23:59:59). This is known as a half-closed interval and matches the logic in Commerce Recurring.
- The update function bumps the end date by one day, so that it behaves close to how it behaved without the time (it was inclusive, so 2019-02-24 was valid until 2019-02-25).

And committed!

I have awarded the commit to nlz because they spent a lot of time pushing this issue forward. I know that in the end we ended up taking a slightly different road, and that's my fault for not figuring out the timezone details earlier, but what matters is that we made it to the end.

Feel free to add your feedback to this issue, and open followup issues for any missing features or encountered bugs.

Thanks, everyone!

yeeeeah that's a great new feature :))

I updated Drupal commerce to 8.x-2.17 and tried to run update.php.
But I get the error

Error: Call to a member function getPromotionId() on null in Drupal\commerce_promotion\Entity\Promotion->postSave() (line 560 of /var/www/html/hgsi/modules/contrib/commerce/modules/promotion/src/Entity/Promotion.php).

The update process was aborted prematurely while running update # in commerce_promotion_post_update_11.module.

@gurunathan: Perhaps the problematic promotion references coupons that were deleted but the reference remained... postSave() should probably call $this->getCoupons() instead.

Could you try applying the patch from #3117937: Promotion::postSave() should call the getter for getting coupons? And tun the update again?