[PATCH v3] Avoid timezone confusion
Veronika Kabatova
vkabatov at redhat.com
Thu Mar 8 02:20:40 AEDT 2018
----- Original Message -----
> From: "Daniel Axtens" <dja at axtens.net>
> To: vkabatov at redhat.com, patchwork at lists.ozlabs.org
> Sent: Wednesday, March 7, 2018 3:39:01 PM
> Subject: Re: [PATCH v3] Avoid timezone confusion
>
> Hi Veronika,
>
> Thank you for your patience.
>
> > Patchwork saves patches, comments etc with UTC timezone and reports
> > this time when opening the patch details. However, internally generated
> > processes such as events are reported with the instance's local time.
> > There's nothing wrong with that and making PW timezone-aware would add
> > useless complexity, but in a world-wide collaboration a lot of confusion
> > may arise as the timezone is not reported at all. Instance's local time
> > might be very different from the local time of CI integrating with PW,
> > which is different from the local time of person dealing with it etc.
> >
> > Use UTC everywhere by default instead of UTC for sumbissions and local
> > timezone for internally generated events (which is not reported).
>
> I found that Docker set up containers in the UTC timezone, which made
> things difficult to test. I patched the dockerfile to put the container
> in UTC+11, which should match the TZ value that Django uses.
>
> Without your patch, events were created with local time, stored in - as
> far as I can tell - timezone-unaware format in the database.
>
> I then applied your patch.
>
> With the patch, the events were created with UTC time, again stored
> AFAICT TZ-unaware.
>
> That all checks out - I was a bit worried Django was going to do
> something 'clever' and try to store in UTC and convert back and forth,
> and our conversion would lead to some sort of awful double-convert. But
> everything is in order so we can proceed :)
>
Glad it works for you! Having unified timezones saved us a lot of time
when debugging things internally so we wanted to help out.
> So I:
> - made the fixup to the migration number and dependency, as discussed.
> - made a minor edit to the doc fixup (s/sooner/earlier)
> - squashed your two patches
> - added
> Tested-by: Daniel Axtens <dja at axtens.net>
> - pushed to master at 8465e33c23310e4873d464fe2581842df2e9c6f8
>
Awesome, thanks! I think you missed Stephen's Reviewed-by [1] in the
commit. It doesn't matter too much to me, just wanted to bring it up
in case he'd like it there :)
[1] https://patchwork.ozlabs.org/patch/876744/#1864659
Thanks again,
Veronika
> Thanks again for your contributions to patchwork!
>
> Regards,
> Daniel
>
> >
> >
> > Signed-off-by: Veronika Kabatova <vkabatov at redhat.com>
> > ---
> > Changes: Implement Daniel's idea about using datetime.datetime.utcnow
> > ---
> > docs/api/rest.rst | 3 +-
> > patchwork/migrations/0024_timezone_unify.py | 46
> > ++++++++++++++++++++++
> > patchwork/models.py | 12 +++---
> > patchwork/notifications.py | 4 +-
> > patchwork/signals.py | 2 +-
> > patchwork/templates/patchwork/submission.html | 4 +-
> > patchwork/tests/test_checks.py | 10 ++---
> > patchwork/tests/test_expiry.py | 8 ++--
> > patchwork/tests/test_notifications.py | 2 +-
> > patchwork/tests/utils.py | 6 +--
> > .../notes/unify-timezones-0f7022f0c2a371be.yaml | 5 +++
> > 11 files changed, 77 insertions(+), 25 deletions(-)
> > create mode 100644 patchwork/migrations/0024_timezone_unify.py
> > create mode 100644
> > releasenotes/notes/unify-timezones-0f7022f0c2a371be.yaml
> >
> > diff --git a/docs/api/rest.rst b/docs/api/rest.rst
> > index 3d7292e..d526b27 100644
> > --- a/docs/api/rest.rst
> > +++ b/docs/api/rest.rst
> > @@ -107,7 +107,8 @@ Schema
> > ------
> >
> > Responses are returned as JSON. Blank fields are returned as ``null``,
> > rather
> > -than being omitted. Timestamps use the ISO 8601 format::
> > +than being omitted. Timestamps use the ISO 8601 format, times are by
> > default
> > +in UTC::
> >
> > YYYY-MM-DDTHH:MM:SSZ
> >
> > diff --git a/patchwork/migrations/0024_timezone_unify.py
> > b/patchwork/migrations/0024_timezone_unify.py
> > new file mode 100644
> > index 0000000..99f0642
> > --- /dev/null
> > +++ b/patchwork/migrations/0024_timezone_unify.py
> > @@ -0,0 +1,46 @@
> > +# -*- coding: utf-8 -*-
> > +# Generated by Django 1.10.8 on 2018-02-22 23:11
> > +from __future__ import unicode_literals
> > +
> > +import datetime
> > +from django.db import migrations, models
> > +
> > +
> > +class Migration(migrations.Migration):
> > +
> > + dependencies = [
> > + ('patchwork', '0023_submissiontag'),
> > + ]
> > +
> > + operations = [
> > + migrations.AlterField(
> > + model_name='check',
> > + name='date',
> > + field=models.DateTimeField(default=datetime.datetime.utcnow),
> > + ),
> > + migrations.AlterField(
> > + model_name='comment',
> > + name='date',
> > + field=models.DateTimeField(default=datetime.datetime.utcnow),
> > + ),
> > + migrations.AlterField(
> > + model_name='emailconfirmation',
> > + name='date',
> > + field=models.DateTimeField(default=datetime.datetime.utcnow),
> > + ),
> > + migrations.AlterField(
> > + model_name='event',
> > + name='date',
> > + field=models.DateTimeField(default=datetime.datetime.utcnow,
> > help_text=b'The time this event was created.'),
> > + ),
> > + migrations.AlterField(
> > + model_name='patchchangenotification',
> > + name='last_modified',
> > + field=models.DateTimeField(default=datetime.datetime.utcnow),
> > + ),
> > + migrations.AlterField(
> > + model_name='submission',
> > + name='date',
> > + field=models.DateTimeField(default=datetime.datetime.utcnow),
> > + ),
> > + ]
> > diff --git a/patchwork/models.py b/patchwork/models.py
> > index a8bb015..c5a2059 100644
> > --- a/patchwork/models.py
> > +++ b/patchwork/models.py
> > @@ -308,7 +308,7 @@ class EmailMixin(models.Model):
> > # email metadata
> >
> > msgid = models.CharField(max_length=255)
> > - date = models.DateTimeField(default=datetime.datetime.now)
> > + date = models.DateTimeField(default=datetime.datetime.utcnow)
> > headers = models.TextField(blank=True)
> >
> > # content
> > @@ -828,7 +828,7 @@ class Check(models.Model):
> >
> > patch = models.ForeignKey(Patch, on_delete=models.CASCADE)
> > user = models.ForeignKey(User, on_delete=models.CASCADE)
> > - date = models.DateTimeField(default=datetime.datetime.now)
> > + date = models.DateTimeField(default=datetime.datetime.utcnow)
> >
> > state = models.SmallIntegerField(
> > choices=STATE_CHOICES, default=STATE_PENDING,
> > @@ -900,7 +900,7 @@ class Event(models.Model):
> > db_index=True,
> > help_text='The category of the event.')
> > date = models.DateTimeField(
> > - default=datetime.datetime.now,
> > + default=datetime.datetime.utcnow,
> > help_text='The time this event was created.')
> >
> > # event object
> > @@ -965,7 +965,7 @@ class EmailConfirmation(models.Model):
> > email = models.CharField(max_length=200)
> > user = models.ForeignKey(User, null=True, on_delete=models.CASCADE)
> > key = HashField()
> > - date = models.DateTimeField(default=datetime.datetime.now)
> > + date = models.DateTimeField(default=datetime.datetime.utcnow)
> > active = models.BooleanField(default=True)
> >
> > def deactivate(self):
> > @@ -973,7 +973,7 @@ class EmailConfirmation(models.Model):
> > self.save()
> >
> > def is_valid(self):
> > - return self.date + self.validity > datetime.datetime.now()
> > + return self.date + self.validity > datetime.datetime.utcnow()
> >
> > def save(self, *args, **kwargs):
> > limit = 1 << 32
> > @@ -999,5 +999,5 @@ class EmailOptout(models.Model):
> > class PatchChangeNotification(models.Model):
> > patch = models.OneToOneField(Patch, primary_key=True,
> > on_delete=models.CASCADE)
> > - last_modified = models.DateTimeField(default=datetime.datetime.now)
> > + last_modified = models.DateTimeField(default=datetime.datetime.utcnow)
> > orig_state = models.ForeignKey(State, on_delete=models.CASCADE)
> > diff --git a/patchwork/notifications.py b/patchwork/notifications.py
> > index 88e9662..a5f6423 100644
> > --- a/patchwork/notifications.py
> > +++ b/patchwork/notifications.py
> > @@ -35,7 +35,7 @@ from patchwork.models import PatchChangeNotification
> >
> >
> > def send_notifications():
> > - date_limit = datetime.datetime.now() - datetime.timedelta(
> > + date_limit = datetime.datetime.utcnow() - datetime.timedelta(
> > minutes=settings.NOTIFICATION_DELAY_MINUTES)
> >
> > # We delay sending notifications to a user if they have other
> > @@ -104,7 +104,7 @@ def expire_notifications():
> > Users whose registration confirmation has expired are removed.
> > """
> > # expire any invalid confirmations
> > - q = (Q(date__lt=datetime.datetime.now() - EmailConfirmation.validity)
> > |
> > + q = (Q(date__lt=datetime.datetime.utcnow() -
> > EmailConfirmation.validity) |
> > Q(active=False))
> > EmailConfirmation.objects.filter(q).delete()
> >
> > diff --git a/patchwork/signals.py b/patchwork/signals.py
> > index e5e7370..f7b4f54 100644
> > --- a/patchwork/signals.py
> > +++ b/patchwork/signals.py
> > @@ -65,7 +65,7 @@ def patch_change_callback(sender, instance, raw,
> > **kwargs):
> > notification.delete()
> > return
> >
> > - notification.last_modified = dt.now()
> > + notification.last_modified = dt.utcnow()
> > notification.save()
> >
> >
> > diff --git a/patchwork/templates/patchwork/submission.html
> > b/patchwork/templates/patchwork/submission.html
> > index 6ed20c3..e817713 100644
> > --- a/patchwork/templates/patchwork/submission.html
> > +++ b/patchwork/templates/patchwork/submission.html
> > @@ -255,7 +255,7 @@ function toggle_div(link_id, headers_id)
> > <div class="comment">
> > <div class="meta">
> > <span>{{ submission.submitter|personify:project }}</span>
> > - <span class="pull-right">{{ submission.date }}</span>
> > + <span class="pull-right">{{ submission.date }} UTC</span>
> > </div>
> > <pre class="content">
> > {{ submission|commentsyntax }}
> > @@ -271,7 +271,7 @@ function toggle_div(link_id, headers_id)
> > <div class="comment">
> > <div class="meta">
> > <span>{{ item.submitter|personify:project }}</span>
> > - <span class="pull-right">{{ item.date }} | <a href="{% url
> > 'comment-redirect' comment_id=item.id %}"
> > + <span class="pull-right">{{ item.date }} UTC | <a href="{% url
> > 'comment-redirect' comment_id=item.id %}"
> > >#{{ forloop.counter }}</a></span>
> > </div>
> > <pre class="content">
> > diff --git a/patchwork/tests/test_checks.py
> > b/patchwork/tests/test_checks.py
> > index c0487f3..797390c 100644
> > --- a/patchwork/tests/test_checks.py
> > +++ b/patchwork/tests/test_checks.py
> > @@ -86,12 +86,12 @@ class PatchChecksTest(TransactionTestCase):
> > self.assertChecksEqual(self.patch, [check_a, check_b])
> >
> > def test_checks__duplicate_checks(self):
> > - self._create_check(date=(dt.now() - timedelta(days=1)))
> > + self._create_check(date=(dt.utcnow() - timedelta(days=1)))
> > check = self._create_check()
> > # this isn't a realistic scenario (dates shouldn't be set by user
> > so
> > # they will always increment), but it's useful to verify the
> > removal
> > # of older duplicates by the function
> > - self._create_check(date=(dt.now() - timedelta(days=2)))
> > + self._create_check(date=(dt.utcnow() - timedelta(days=2)))
> > self.assertChecksEqual(self.patch, [check])
> >
> > def test_checks__nultiple_users(self):
> > @@ -107,7 +107,7 @@ class PatchChecksTest(TransactionTestCase):
> > self.assertCheckCountEqual(self.patch, 1, {Check.STATE_SUCCESS:
> > 1})
> >
> > def test_check_count__multiple_checks(self):
> > - self._create_check(date=(dt.now() - timedelta(days=1)))
> > + self._create_check(date=(dt.utcnow() - timedelta(days=1)))
> > self._create_check(context='new/test1')
> > self.assertCheckCountEqual(self.patch, 2, {Check.STATE_SUCCESS:
> > 2})
> >
> > @@ -117,14 +117,14 @@ class PatchChecksTest(TransactionTestCase):
> > self.assertCheckCountEqual(self.patch, 2, {Check.STATE_SUCCESS:
> > 2})
> >
> > def test_check_count__duplicate_check_same_state(self):
> > - self._create_check(date=(dt.now() - timedelta(days=1)))
> > + self._create_check(date=(dt.utcnow() - timedelta(days=1)))
> > self.assertCheckCountEqual(self.patch, 1, {Check.STATE_SUCCESS:
> > 1})
> >
> > self._create_check()
> > self.assertCheckCountEqual(self.patch, 2, {Check.STATE_SUCCESS:
> > 1})
> >
> > def test_check_count__duplicate_check_new_state(self):
> > - self._create_check(date=(dt.now() - timedelta(days=1)))
> > + self._create_check(date=(dt.utcnow() - timedelta(days=1)))
> > self.assertCheckCountEqual(self.patch, 1, {Check.STATE_SUCCESS:
> > 1})
> >
> > self._create_check(state=Check.STATE_FAIL)
> > diff --git a/patchwork/tests/test_expiry.py
> > b/patchwork/tests/test_expiry.py
> > index 054d156..ce308bc 100644
> > --- a/patchwork/tests/test_expiry.py
> > +++ b/patchwork/tests/test_expiry.py
> > @@ -46,7 +46,7 @@ class TestRegistrationExpiry(TestCase):
> > return (user, conf)
> >
> > def test_old_registration_expiry(self):
> > - date = ((datetime.datetime.now() - EmailConfirmation.validity) -
> > + date = ((datetime.datetime.utcnow() - EmailConfirmation.validity)
> > -
> > datetime.timedelta(hours=1))
> > user, conf = self.register(date)
> >
> > @@ -57,7 +57,7 @@ class TestRegistrationExpiry(TestCase):
> > EmailConfirmation.objects.filter(pk=conf.pk).exists())
> >
> > def test_recent_registration_expiry(self):
> > - date = ((datetime.datetime.now() - EmailConfirmation.validity) +
> > + date = ((datetime.datetime.utcnow() - EmailConfirmation.validity)
> > +
> > datetime.timedelta(hours=1))
> > user, conf = self.register(date)
> >
> > @@ -68,7 +68,7 @@ class TestRegistrationExpiry(TestCase):
> > EmailConfirmation.objects.filter(pk=conf.pk).exists())
> >
> > def test_inactive_registration_expiry(self):
> > - user, conf = self.register(datetime.datetime.now())
> > + user, conf = self.register(datetime.datetime.utcnow())
> >
> > # confirm registration
> > conf.user.is_active = True
> > @@ -87,7 +87,7 @@ class TestRegistrationExpiry(TestCase):
> > submitter = patch.submitter
> >
> > # ... then starts registration...
> > - date = ((datetime.datetime.now() - EmailConfirmation.validity) -
> > + date = ((datetime.datetime.utcnow() - EmailConfirmation.validity)
> > -
> > datetime.timedelta(hours=1))
> > user = create_user(link_person=False, email=submitter.email)
> > user.is_active = False
> > diff --git a/patchwork/tests/test_notifications.py
> > b/patchwork/tests/test_notifications.py
> > index 6cd3200..6d902f8 100644
> > --- a/patchwork/tests/test_notifications.py
> > +++ b/patchwork/tests/test_notifications.py
> > @@ -120,7 +120,7 @@ class PatchNotificationEmailTest(TestCase):
> > self.project = create_project(send_notifications=True)
> >
> > def _expire_notifications(self, **kwargs):
> > - timestamp = datetime.datetime.now() - \
> > + timestamp = datetime.datetime.utcnow() - \
> > datetime.timedelta(minutes=settings.NOTIFICATION_DELAY_MINUTES
> > + 1)
> >
> > qs = PatchChangeNotification.objects.all()
> > diff --git a/patchwork/tests/utils.py b/patchwork/tests/utils.py
> > index 004c2ca..12b6d9e 100644
> > --- a/patchwork/tests/utils.py
> > +++ b/patchwork/tests/utils.py
> > @@ -214,7 +214,7 @@ def create_check(**kwargs):
> > values = {
> > 'patch': create_patch() if 'patch' not in kwargs else None,
> > 'user': create_user() if 'user' not in kwargs else None,
> > - 'date': dt.now(),
> > + 'date': dt.utcnow(),
> > 'state': Check.STATE_SUCCESS,
> > 'target_url': 'http://example.com/',
> > 'description': '',
> > @@ -229,7 +229,7 @@ def create_series(**kwargs):
> > """Create 'Series' object."""
> > values = {
> > 'project': create_project() if 'project' not in kwargs else None,
> > - 'date': dt.now(),
> > + 'date': dt.utcnow(),
> > 'submitter': create_person() if 'submitter' not in kwargs else
> > None,
> > 'total': 1,
> > }
> > @@ -275,7 +275,7 @@ def _create_submissions(create_func, count=1,
> > **kwargs):
> > 'submitter': create_person() if 'submitter' not in kwargs else
> > None,
> > }
> > values.update(kwargs)
> > - date = dt.now()
> > + date = dt.utcnow()
> >
> > objects = []
> > for i in range(0, count):
> > diff --git a/releasenotes/notes/unify-timezones-0f7022f0c2a371be.yaml
> > b/releasenotes/notes/unify-timezones-0f7022f0c2a371be.yaml
> > new file mode 100644
> > index 0000000..2513650
> > --- /dev/null
> > +++ b/releasenotes/notes/unify-timezones-0f7022f0c2a371be.yaml
> > @@ -0,0 +1,5 @@
> > +---
> > +other:
> > + - |
> > + Unify timezones used -- use UTC for both email submissions and
> > internal
> > + events.
> > --
> > 2.13.6
> >
> > _______________________________________________
> > Patchwork mailing list
> > Patchwork at lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/patchwork
>
More information about the Patchwork
mailing list