Use DateTimeImmutable for time calculations (#91)

* Use DateTimeImmutable for time calculations This avoids problems where times in the future are sometimes considered to be in the past. * Use DBDatetime for $now. Import DTI. Add test cov * Fix for broken unit tests Co-authored-by: Fred Condo <[email protected]> Co-authored-by: Chris Penny <[email protected]>
silverstripe-terraformers · Nov 24, 2021 · 2d93df0 · 2d93df0
1 parent 55c1398
commit 2d93df0
Show file tree

Hide file tree

Showing 5 changed files with 174 additions and 27 deletions.
diff --git a/src/Extension/EmbargoExpiryExtension.php b/src/Extension/EmbargoExpiryExtension.php
@@ -22,6 +22,7 @@
 use Symbiote\QueuedJobs\Services\QueuedJobService;
 use Terraformers\EmbargoExpiry\Job\PublishTargetJob;
 use Terraformers\EmbargoExpiry\Job\UnPublishTargetJob;
+use DateTimeImmutable;
 
 /**
  * Class WorkflowEmbargoExpiryExtension
@@ -120,7 +121,9 @@ public function validate(ValidationResult $validationResult): ValidationResult
             return $validationResult;
         }
 
-        if (strtotime($this->owner->DesiredPublishDate) > strtotime($unPublishDate)) {
+        $publishTime = new DateTimeImmutable($this->owner->DesiredPublishDate);
+        $unpublishTime = new DateTimeImmutable($unPublishDate);
+        if ($publishTime > $unpublishTime) {
             $validationResult->addFieldError(
                 'DesiredPublishDate',
                 _t(
@@ -901,23 +904,24 @@ public function addNoticeOrWarningFields(FieldList $fields): void
     public function getEmbargoExpiryNoticeFieldConditions(): array
     {
         $conditions = [];
+        $now = DBDatetime::now()->getTimestamp();
 
-        if ($this->getIsPublishScheduled()) {
-            $time = strtotime($this->owner->PublishOnDate);
+        if ($this->getPublishOnDateAsTimestamp()) {
+            $time = new DateTimeImmutable($this->owner->PublishOnDate);
 
             $conditions['embargo'] = [
-                'date' => $this->owner->PublishOnDate,
-                'warning' => ($time > 0 && $time < time()),
+                'date' => $time->format('Y-m-d H:i T'),
+                'warning' => ($time->getTimestamp() < $now),
                 'name' => _t(__CLASS__ . '.EMBARGO_NAME', 'embargo'),
             ];
         }
 
-        if ($this->getIsUnPublishScheduled()) {
-            $time = strtotime($this->owner->UnPublishOnDate);
+        if ($this->getUnPublishOnDateAsTimestamp()) {
+            $time = new DateTimeImmutable($this->owner->UnPublishOnDate);
 
             $conditions['expiry'] = [
-                'date' => $this->owner->UnPublishOnDate,
-                'warning' => ($time > 0 && $time < time()),
+                'date' => $time->format('Y-m-d H:i T'),
+                'warning' => ($time->getTimestamp() < $now),
                 'name' => _t(__CLASS__ . '.EXPIRY_NAME', 'expiry'),
             ];
         }

diff --git a/tests/php/Extension/EmbargoExpiryCMSMainExtensionTest.php b/tests/php/Extension/EmbargoExpiryCMSMainExtensionTest.php
@@ -81,6 +81,7 @@ public function testRemoveEmbargoAction(): void
         );
 
         // Refetch object from DB.
+        /** @var SiteTree|EmbargoExpiryExtension $page */
         $page = SiteTree::get()->byID($id);
 
         $this->assertFalse($page->getIsPublishScheduled());

diff --git a/tests/php/Extension/EmbargoExpiryCMSMainExtensionTest.yml b/tests/php/Extension/EmbargoExpiryCMSMainExtensionTest.yml
@@ -1,12 +1,10 @@
 SilverStripe\CMS\Model\SiteTree:
   home:
-    Title: Home Page
-    URLSegment: home
+    Title: Home
     PublishOnDate: 2014-01-07 12:00:00
     UnPublishOnDate: 2014-01-08 12:00:00
   contact:
-    Title: Home Page
-    URLSegment: home
+    Title: Contact us
     PublishOnDate: 2014-01-07 12:00:00
     UnPublishOnDate: 2014-01-08 12:00:00
 

diff --git a/tests/php/Extension/EmbargoExpiryExtensionTest.php b/tests/php/Extension/EmbargoExpiryExtensionTest.php
@@ -16,6 +16,7 @@
 use Symbiote\QueuedJobs\Services\QueuedJobService;
 use Terraformers\EmbargoExpiry\Extension\EmbargoExpiryExtension;
 use Terraformers\EmbargoExpiry\Tests\Fake\TestQueuedJobService;
+use DateTimeImmutable;
 
 /**
  * Class EmbargoExpiryExtensionTest
@@ -183,7 +184,7 @@ public function testUnPublishJobProcesses(): void
     {
         $service = $this->getService();
         /** @var SiteTree|EmbargoExpiryExtension $page */
-        $page = $this->objFromFixture(SiteTree::class, 'expiry1');
+        $page = $this->objFromFixture(SiteTree::class, 'expiryEmpty');
 
         // UnPublishDate is in the past, so it should be run immediately when we initialise it.
         $page->DesiredUnPublishDate = '2014-01-01 12:00:00';
@@ -233,9 +234,14 @@ public function testMessageConditionsCanEdit(): void
         $literalField = $fieldsArray[0];
         $content = $literalField->getContent();
 
+        $time = new DateTimeImmutable();
+
+        $expectedEmbargoMessage = sprintf('Embargo</strong>: 2014-01-07 12:00 %s', $time->getTimezone()->getName());
+        $expectedExpiryMessage = sprintf('Embargo</strong>: 2014-01-07 12:00 %s', $time->getTimezone()->getName());
+
         $this->assertNotContains('cannot currently be edited', $content);
-        $this->assertContains('Embargo</strong>: 2014-01-07 12:00:00', $content);
-        $this->assertContains('Expiry</strong>: 2014-01-08 12:00:00', $content);
+        $this->assertContains($expectedEmbargoMessage, $content);
+        $this->assertContains($expectedExpiryMessage, $content);
     }
 
     public function testMessageConditionsCannotEditGuest(): void
@@ -258,10 +264,15 @@ public function testMessageConditionsCannotEditGuest(): void
         $literalField = $fieldsArray[0];
         $content = $literalField->getContent();
 
+        $time = new DateTimeImmutable();
+
+        $expectedEmbargoMessage = sprintf('Embargo</strong>: 2014-01-07 12:00 %s', $time->getTimezone()->getName());
+        $expectedExpiryMessage = sprintf('Embargo</strong>: 2014-01-07 12:00 %s', $time->getTimezone()->getName());
+
         $this->assertContains('cannot currently be edited', $content);
         $this->assertContains('An administrator will need', $content);
-        $this->assertContains('Embargo</strong>: 2014-01-07 12:00:00', $content);
-        $this->assertContains('Expiry</strong>: 2014-01-08 12:00:00', $content);
+        $this->assertContains($expectedEmbargoMessage, $content);
+        $this->assertContains($expectedExpiryMessage, $content);
     }
 
     public function testMessageConditionsCannotEditAdmin(): void
@@ -284,10 +295,15 @@ public function testMessageConditionsCannotEditAdmin(): void
         $literalField = $fieldsArray[0];
         $content = $literalField->getContent();
 
+        $now = new DateTimeImmutable();
+
+        $expectedEmbargoMessage = sprintf('Embargo</strong>: 2014-01-07 12:00 %s', $now->getTimezone()->getName());
+        $expectedExpiryMessage = sprintf('Embargo</strong>: 2014-01-07 12:00 %s', $now->getTimezone()->getName());
+
         $this->assertContains('cannot currently be edited', $content);
         $this->assertContains('You will need to remove', $content);
-        $this->assertContains('Embargo</strong>: 2014-01-07 12:00:00', $content);
-        $this->assertContains('Expiry</strong>: 2014-01-08 12:00:00', $content);
+        $this->assertContains($expectedEmbargoMessage, $content);
+        $this->assertContains($expectedExpiryMessage, $content);
     }
 
     public function testMessageConditionsWarning(): void
@@ -297,7 +313,7 @@ public function testMessageConditionsWarning(): void
         $this->logInWithPermission('ADMIN');
 
         /** @var SiteTree|EmbargoExpiryExtension $page */
-        $page = $this->objFromFixture(SiteTree::class, 'messages1');
+        $page = $this->objFromFixture(SiteTree::class, 'messages2');
         $fields = new FieldList();
 
         $page->addNoticeOrWarningFields($fields);
@@ -310,9 +326,20 @@ public function testMessageConditionsWarning(): void
         $literalField = $fieldsArray[0];
         $content = $literalField->getContent();
 
+        $time = new DateTimeImmutable();
+
+        $expectedEmbargoMessage = sprintf(
+            'Embargo</strong>: 2014-01-03 12:00 %s<strong> (this date is in the',
+            $time->getTimezone()->getName()
+        );
+        $expectedExpiryMessage = sprintf(
+            'Expiry</strong>: 2014-01-04 12:00 %s<strong> (this date is in the',
+            $time->getTimezone()->getName()
+        );
+
         // Test that the two warning messages were added.
-        $this->assertContains('Embargo</strong>: 2014-01-07 12:00:00<strong> (this date is in the', $content);
-        $this->assertContains('Expiry</strong>: 2014-01-08 12:00:00<strong> (this date is in the', $content);
+        $this->assertContains($expectedEmbargoMessage, $content);
+        $this->assertContains($expectedExpiryMessage, $content);
     }
 
     public function testEmbargoExpiryFieldNoticeMessageNotEditable(): void
@@ -491,4 +518,100 @@ public function testValidateFailSetDate(): void
 
         $this->assertFalse($validationResult->isValid());
     }
+
+    public function testEmbargoMessagePassed(): void
+    {
+        /** @var SiteTree|EmbargoExpiryExtension $page */
+        $page = $this->objFromFixture(SiteTree::class, 'embargo1');
+
+        $time = new DateTimeImmutable();
+
+        $expectedConditions = [];
+        $expectedConditions['embargo'] = [
+            'date' => sprintf('2014-01-03 12:00 %s', $time->getTimezone()->getName()),
+            'warning' => true,
+            'name' => 'embargo',
+        ];
+
+        $this->assertEquals(
+            $expectedConditions,
+            $page->getEmbargoExpiryNoticeFieldConditions(),
+            '', // default value
+            0.0, // default value
+            10, // default value
+            true // sort both arrays so that the order of items doesn't matter
+        );
+    }
+
+    public function testEmbargoMessageFuture(): void
+    {
+        /** @var SiteTree|EmbargoExpiryExtension $page */
+        $page = $this->objFromFixture(SiteTree::class, 'embargo2');
+
+        $time = new DateTimeImmutable();
+
+        $expectedConditions = [];
+        $expectedConditions['embargo'] = [
+            'date' => sprintf('2014-01-08 12:00 %s', $time->getTimezone()->getName()),
+            'warning' => false,
+            'name' => 'embargo',
+        ];
+
+        $this->assertEquals(
+            $expectedConditions,
+            $page->getEmbargoExpiryNoticeFieldConditions(),
+            '', // default value
+            0.0, // default value
+            10, // default value
+            true // sort both arrays so that the order of items doesn't matter
+        );
+    }
+
+    public function testExpiryMessagePassed(): void
+    {
+        /** @var SiteTree|EmbargoExpiryExtension $page */
+        $page = $this->objFromFixture(SiteTree::class, 'expiry1');
+
+        $time = new DateTimeImmutable();
+
+        $expectedConditions = [];
+        $expectedConditions['expiry'] = [
+            'date' => sprintf('2014-01-03 12:00 %s', $time->getTimezone()->getName()),
+            'warning' => true,
+            'name' => 'expiry',
+        ];
+
+        $this->assertEquals(
+            $expectedConditions,
+            $page->getEmbargoExpiryNoticeFieldConditions(),
+            '', // default value
+            0.0, // default value
+            10, // default value
+            true // sort both arrays so that the order of items doesn't matter
+        );
+    }
+
+    public function testExpiryMessageFuture(): void
+    {
+        /** @var SiteTree|EmbargoExpiryExtension $page */
+        $page = $this->objFromFixture(SiteTree::class, 'expiry2');
+
+        $time = new DateTimeImmutable();
+
+        $expectedConditions = [];
+        $expectedConditions['expiry'] = [
+            'date' => sprintf('2014-01-08 12:00 %s', $time->getTimezone()->getName()),
+            'warning' => false,
+            'name' => 'expiry',
+        ];
+
+        $this->assertEquals(
+            $expectedConditions,
+            $page->getEmbargoExpiryNoticeFieldConditions(),
+            '', // default value
+            0.0, // default value
+            10, // default value
+            true // sort both arrays so that the order of items doesn't matter
+        );
+    }
 }
diff --git a/tests/php/Extension/EmbargoExpiryExtensionTest.yml b/tests/php/Extension/EmbargoExpiryExtensionTest.yml
@@ -2,10 +2,10 @@ SilverStripe\CMS\Model\SiteTree:
   home:
     Title: Home
     URLSegment: home
-  embargo1:
+  embargoEmpty:
     Title: Embargo1
     URLSegment: embargo1
-  expiry1:
+  expiryEmpty:
     Title: Expiry1
     URLSegment: expiry1
   idfields:
@@ -33,8 +33,8 @@ SilverStripe\CMS\Model\SiteTree:
     PublishOnDate: '2014-01-07 12:00:00'
     UnPublishOnDate: '2014-01-08 12:00:00'
   messages2:
-    Title: Messages Page 1
-    URLSegment: messages1
+    Title: Messages Page 2
+    URLSegment: messages2
     PublishOnDate: '2014-01-03 12:00:00'
     UnPublishOnDate: '2014-01-04 12:00:00'
   fields1:
@@ -57,3 +57,24 @@ SilverStripe\CMS\Model\SiteTree:
     URLSegment: validatepass
     DesiredPublishDate: '2014-01-08 12:00:00'
     UnPublishOnDate: '2014-01-07 12:00:00'
+  embargo1:
+    Title: Embargo1
+    PublishOnDate: 2014-01-03 12:00:00
+    UnPublishOnDate:
+  embargo2:
+    Title: Embargo2
+    PublishOnDate: 2014-01-08 12:00:00
+  expiry1:
+    Title: Expiry1
+    UnPublishOnDate: 2014-01-03 12:00:00
+  expiry2:
+    Title: Expiry2
+    UnPublishOnDate: 2014-01-08 12:00:00
+  both1:
+    Title: Both1
+    PublishOnDate: 2014-01-03 12:00:00
+    UnPublishOnDate: 2014-01-03 12:00:00
+  both2:
+    Title: Both2
+    PublishOnDate: 2014-01-08 12:00:00
+    UnPublishOnDate: 2014-01-08 12:00:00