[PATCH v3] Avoid timezone confusion

Daniel Axtens dja at axtens.net
Sat Feb 24 12:58:56 AEDT 2018


Hi Veronika,

I am testing this now and I think I'm happy with it.

However, I set up a container in UTC+11, and while watching the events
api endpoint:
 - I added some patches without this patch
 - I applied this patch and migrated
 - I added some more patches
 - I noticed that the new events were no longer added to the front of
 the events list; they were now 11 hours back.

This is, on reflection, a straight-forward consequence of moving from
UTC+11 to UTC without attempting to migrate old data.

I still think migration is an incredibly difficult and error-prone waste
of time, so I still don't want to go down that road. What I do want is a
piece of documentation in the release notes to warn people that if you
are in UTC+n, your events feed will be out of order for n hours.

I don't know of any users of the event feed at the moment, so I'm not
worried about breaking anything: I just want a couple of sentences so
that any sysadmins/users who are especially observant don't freak out
that they're suddenly losing events.

You can do a respin of this, or just send a new patch on top of this and
I will apply both at the same time.

Regards,
Daniel

vkabatov at redhat.com writes:

> From: Veronika Kabatova <vkabatov at redhat.com>
>
> 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).
>
> 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