Skip to content
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

Update default timezone handling for dates in CiviEntityStorage.php #461

Merged

Conversation

nickperkins
Copy link
Contributor

Overview

When handling date values, always use the DateTimeItemInterface::STORAGE_TIMEZONE when converting the data.

Before

When a view is taking a date field from a CIVICRM entity, it uses the site's default timezone when setting the value to be displayed. Given that this is a date field, it does not have a timezone.
image

image

After

We check to see if this is a date field or a datetime field. if it is a date field, use DateTimeItemInterface::STORAGE_TIMEZONE to ensure we don't accidentally change the date in the process.

image

Technical Details

The DateTimeItemInterface::STORAGE_TIMEZONE assumes that all datetime data is stored in UTC, so we must also ensure our default timezone is UTC when dealing with date fields.

Comments

I note that Drupal still shows a time of 12:00 no matter what, but that can be handled via formatting.

@civibot civibot bot added the 4.0.x label Jan 1, 2024
@jackrabbithanna
Copy link
Collaborator

This fix makes alot of sense. We'll be testing internally and likely to merge soon.

@pradpnayak
Copy link
Contributor

I tested this on our D10 version and the patch seems to have worked.

In Civi record
Screenshot 2024-03-15 at 15 36 26

Before patch in views:
Screenshot 2024-03-15 at 15 38 16

After patch
Screenshot 2024-03-15 at 15 36 32

@jackrabbithanna
Copy link
Collaborator

I reran the test in a separate PR, and then on my local .. there is a test fail.

1) Drupal\Tests\civicrm_entity\Kernel\CivicrmStorageGetTest::testLoadContact
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'1982/06/[27](https://github.com/eileenmcnaughton/civicrm_entity/actions/runs/8754115723/job/24025167226?pr=471#step:16:28)'
+'1982/06/28'

Error: /home/runner/drupal/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:94
/home/runner/work/civicrm_entity/civicrm_entity/tests/src/Kernel/CivicrmStorageGetTest.php:65
/home/runner/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:7[28](https://github.com/eileenmcnaughton/civicrm_entity/actions/runs/8754115723/job/24025167226?pr=471#step:16:29)

@jackrabbithanna
Copy link
Collaborator

The test: https://github.com/eileenmcnaughton/civicrm_entity/blob/4.0.x/tests/src/Kernel/CivicrmStorageGetTest.php#L65
$this->assertEquals('1982/06/27', $entity->get('birth_date')->date->format('Y/m/d'));
But the test data looks like https://github.com/eileenmcnaughton/civicrm_entity/blob/4.0.x/tests/src/Kernel/CivicrmEntityTestBase.php#L1696
'birth_date' => '1982-06-28',

Has the test been wrong all this time?

@jackrabbithanna
Copy link
Collaborator

With #472 and with this PR
Manual testing on local with custom date field, and contact date field I get right results.

I get passing tests against Drupal 10.2.5 and CiviCRM 5.69 on local:

jackrabbithanna@jackrabbithanna-HP-EliteBook-840-G1:/var/www/web/distro10.skvare.com$ sudo -u www-data -E ./vendor/bin/phpunit -v -c /var/www/web/distro10.skvare.com/phpunit.xml ./web/modules/contrib/civicrm_entity/tests/src/Kernel/CivicrmStorageGetTest.php
PHPUnit 9.6.18 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.1.14
Configuration: /var/www/web/distro10.skvare.com/phpunit.xml

Testing Drupal\Tests\civicrm_entity\Kernel\CivicrmStorageGetTest
......                                                              6 / 6 (100%)

Time: 10:24.006, Memory: 10.00 MB

OK (6 tests, 18 assertions)

Tests in #471 pass, I'll close that one and merge this one.

@jackrabbithanna jackrabbithanna merged commit 33f7df5 into eileenmcnaughton:4.0.x Apr 19, 2024
1 check failed
puresyntax71 added a commit to puresyntax71/civicrm_entity that referenced this pull request Apr 19, 2024
* origin/4.0.x:
  4.0.x field mappings (eileenmcnaughton#430)
  Adding a raw value function to return value from values array (eileenmcnaughton#465)
  Add filter for state province. (eileenmcnaughton#453)
  Update default timezone handling for dates in CiviEntityStorage.php (eileenmcnaughton#461)
  fix testLoadContact birthdate assertion (eileenmcnaughton#472)
  port to 4.0.x from 436: Include dblocale table names in list of civicrm entity info (eileenmcnaughton#438)
  Ignore convert to UTC if custom field is date only (eileenmcnaughton#469)
  update contact entity referece field on contact merge (eileenmcnaughton#456)
  update test versions of Drupal 10.2 and CiviCRM 5.69 (eileenmcnaughton#468)
  Clean value on custom field for Float (Number) and Money field type (eileenmcnaughton#463)
  From eileenmcnaughton#378 for 4.0.x (eileenmcnaughton#466)
  Add reset to the file URLs generated. (eileenmcnaughton#458)
  fix Views field custom date field output for year only (eileenmcnaughton#439)
puresyntax71 added a commit to puresyntax71/civicrm_entity that referenced this pull request Apr 27, 2024
* origin/4.0.x: (41 commits)
  patch from issue 3420771 (eileenmcnaughton#475)
  only check for data in enabled components on module uninstall (eileenmcnaughton#476)
  Adds is Empty/NULL operators (eileenmcnaughton#423)
  Pathauto (eileenmcnaughton#470)
  fix test to use string instead of integer for zip code (eileenmcnaughton#474)
  4.0.x field mappings (eileenmcnaughton#430)
  Adding a raw value function to return value from values array (eileenmcnaughton#465)
  Add filter for state province. (eileenmcnaughton#453)
  Update default timezone handling for dates in CiviEntityStorage.php (eileenmcnaughton#461)
  fix testLoadContact birthdate assertion (eileenmcnaughton#472)
  port to 4.0.x from 436: Include dblocale table names in list of civicrm entity info (eileenmcnaughton#438)
  Ignore convert to UTC if custom field is date only (eileenmcnaughton#469)
  update contact entity referece field on contact merge (eileenmcnaughton#456)
  update test versions of Drupal 10.2 and CiviCRM 5.69 (eileenmcnaughton#468)
  Clean value on custom field for Float (Number) and Money field type (eileenmcnaughton#463)
  From eileenmcnaughton#378 for 4.0.x (eileenmcnaughton#466)
  Add reset to the file URLs generated. (eileenmcnaughton#458)
  fix Views field custom date field output for year only (eileenmcnaughton#439)
  update Add Contact to Group Views Bulk operations action plugin access handler (eileenmcnaughton#464)
  Readme update (eileenmcnaughton#457)
  ...
puresyntax71 added a commit to puresyntax71/civicrm_entity that referenced this pull request Apr 30, 2024
* origin/4.0.x:
  patch from issue 3420771 (eileenmcnaughton#475)
  only check for data in enabled components on module uninstall (eileenmcnaughton#476)
  Adds is Empty/NULL operators (eileenmcnaughton#423)
  Pathauto (eileenmcnaughton#470)
  fix test to use string instead of integer for zip code (eileenmcnaughton#474)
  4.0.x field mappings (eileenmcnaughton#430)
  Adding a raw value function to return value from values array (eileenmcnaughton#465)
  Add filter for state province. (eileenmcnaughton#453)
  Update default timezone handling for dates in CiviEntityStorage.php (eileenmcnaughton#461)
  fix testLoadContact birthdate assertion (eileenmcnaughton#472)
  port to 4.0.x from 436: Include dblocale table names in list of civicrm entity info (eileenmcnaughton#438)
puresyntax71 pushed a commit to puresyntax71/civicrm_entity that referenced this pull request Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants