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

Segments - date relative local timezone #7244

Conversation

Projects
5 participants
@Maxell92
Copy link
Contributor

commented Feb 13, 2019

Q A
Bug fix? Y
New feature? N
Automated tests included? Y
Related user documentation PR URL
Related developer documentation PR URL
Issues addressed (#s or URLs)
BC breaks? N
Deprecations? Y

Description:

When we store datetime in db we always store it in utc so all comparisons are done in utc. The same thing doesn't work for date fields however. They are always stored as-is and thus we need to compare them in instance-local timezone instead of utc otherwise the comparison doesn't work correctly and the higher is the timezone offset the higher is the chance we make an error.

Steps to reproduce the bug:

  • Instance timezone is set to new york (utc-5)
  • It is 2018-01-01 22:00 localtime and I create a new contact with custom date field set to 2018-01-01 (today)
  • I create a segment that pulls contacts where the custom field is today
  • the contact I just created will not be part of the segment

Steps to test this PR:

  1. Load up this PR
  2. Repeat same process and contact will be added.

You can use Antarctica/McMurdo timezone for testing too (UTC+13)

List deprecations along with the new alternative:

  1. DateDecorator->getDefaultDate() > DateOptionParameters->getDefaultDate()

Maxell92 added some commits Feb 6, 2019

@npracht npracht added this to the 2.16.0 milestone Feb 18, 2019

@npracht npracht modified the milestones: 2.16.0, 2.15.2 Mar 28, 2019

@npracht npracht added this to Ready to Test (first time) in Mautic 2 Apr 4, 2019

@johbuch

johbuch approved these changes May 3, 2019

Copy link

left a comment

i have tested and it works
good job

@npracht

This comment has been minimized.

Copy link
Member

commented May 3, 2019

@kuzmany i think it would be a good idea to merge that, this is quite critical. Also it implies that you review #7122

@kuzmany

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

@npracht are you using it in production too?

@escopecz escopecz moved this from Ready to Test (first time) to Ready to Test (confirmation) in Mautic 2 May 6, 2019

@npracht

This comment has been minimized.

Copy link
Member

commented May 7, 2019

No, we've been testing it strongly but not merged. It is big, we wait that being merge before using it in production environment.

@AlbanL74fr

This comment has been minimized.

Copy link

commented May 12, 2019

@kuzmany It's working for me. Thanks!

Mautic 2 automation moved this from Ready to Test (confirmation) to Ready to Test (first time) May 12, 2019

@kuzmany kuzmany merged commit 991b5c7 into mautic:staging May 12, 2019

2 checks passed

Scrutinizer Analysis: 2 new issues, 5 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Mautic 2 automation moved this from Ready to Test (first time) to Merged May 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.