----- Original Message ----- > From: "Daniel Axtens" <d...@axtens.net> > To: vkaba...@redhat.com, patchwork@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 <d...@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 <vkaba...@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@lists.ozlabs.org > > https://lists.ozlabs.org/listinfo/patchwork > _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork