From f99ac7c7667853fa3a48e92821a07960c83b4082 Mon Sep 17 00:00:00 2001 From: Adam Williamson Date: Mar 02 2018 01:39:21 +0000 Subject: Adjust activity heatmap and logs for timezone (#1642) As discussed in #1642, there is a problem with the user activity log page in the web UI caused by a timezone mismatch. The library used to generate the heatmap assumes any date / time you feed it is in the browser timezone. Pagure ultimately feeds it the 'date' value of PagureLog instances, converted to a Unix timestamp. This timestamp reflects 00:00 on the date in question in the UTC timezone. If the browser timezone is behind UTC, the heatmap library treats the event as falling on the *previous* day, because in the browser timezone, that precise time falls on the previous day. It would be relatively easy to 'fix' this if all we wanted to do was have the heatmap reflect what date each event occurred on in the UTC timezone, as the activity log currently does. However, I don't think that's the best fix. I think the best behaviour here is for both the heatmap and the activity log to be relative to the browser timezone. As a Pagure user in a timezone several hours behind UTC, if I make a commit in the late afternoon of Wednesday, I'd expect the heatmap and the activity log to show that commit as occuring on Wednesday, not Thursday. So, this commit dumps the use of the 'date' value in this code, and works with the 'date_created' value instead, which is a precise UTC date and time (as opposed to just the date relative to UTC). We allow a timezone offset to be passed to the relevant methods, and figure out what date the events fell on *if that offset is applied*, rather than just what date they fell on in the UTC timezone. To facilitate this we add a custom method to the database model, `date_offset`, which gives the date when an arbitrary offset (in minutes, positive or negative) is applied. We tweak the relevant functions in pagure.lib to get all the necessary data from the database, then use the `date_offset` method to adjust and refine the results before returning. Fixes https://pagure.io/pagure/issue/1642 --- diff --git a/pagure/api/user.py b/pagure/api/user.py index 8256719..a30fc35 100644 --- a/pagure/api/user.py +++ b/pagure/api/user.py @@ -481,13 +481,15 @@ def api_view_user_activity_stats(username): """ date_format = flask.request.args.get('format', 'isoformat') + offset = flask.request.args.get('offset', 0) user = _get_user(username=username) stats = pagure.lib.get_yearly_stats_user( flask.g.session, user, - datetime.datetime.utcnow().date() + datetime.timedelta(days=1) + datetime.datetime.utcnow().date() + datetime.timedelta(days=1), + offset=offset ) def format_date(d): @@ -576,6 +578,7 @@ def api_view_user_activity_date(username, date): """ # noqa grouped = str(flask.request.args.get('grouped')).lower() in ['1', 'true'] + offset = flask.request.args.get('offset', 0) try: date = arrow.get(date) @@ -586,7 +589,9 @@ def api_view_user_activity_date(username, date): user = _get_user(username=username) - activities = pagure.lib.get_user_activity_day(flask.g.session, user, date) + activities = pagure.lib.get_user_activity_day( + flask.g.session, user, date, offset=offset + ) js_act = [] if grouped: commits = collections.defaultdict(list) diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index 012c718..2f437fc 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -33,6 +33,7 @@ import urlparse import uuid import markdown import werkzeug +from collections import Counter from math import ceil import copy @@ -4351,41 +4352,69 @@ def set_custom_key_value(session, issue, key, value): return 'Custom field %s reset (from %s)' % (key.name, old_value) -def get_yearly_stats_user(session, user, date): +def get_yearly_stats_user(session, user, date, offset=0): """ Return the activity of the specified user in the year preceding the - specified date. + specified date. 'offset' is intended to be a timezone offset from UTC, + in minutes: you can discover the offset for a timezone and pass that + in order for the results to be relative to that timezone. Note, offset + should be the amount of minutes that should be added to the UTC time to + produce the local time - so for timezones behind UTC the number should + be negative, and for timezones ahead of UTC the number should be + positive. This is the opposite of what Javascript getTimezoneOffset() + does, so you have to invert any value you get from that. """ start_date = datetime.datetime(date.year - 1, date.month, date.day) - query = session.query( - model.PagureLog.date, func.count(model.PagureLog.id) + events = session.query( + model.PagureLog ).filter( model.PagureLog.date_created.between(start_date, date) ).filter( model.PagureLog.user_id == user.id - ).group_by( - model.PagureLog.date - ).order_by( - model.PagureLog.date - ) - - return query.all() + ).all() + # Counter very handily does exactly what we want here: it gives + # us a dict with the dates as keys and the number of times each + # date occurs in the data as the values, we return its items as + # a list of tuples + return Counter([event.date_offset(offset) for event in events]).items() -def get_user_activity_day(session, user, date): +def get_user_activity_day(session, user, date, offset=0): """ Return the activity of the specified user on the specified date. + 'offset' is intended to be a timezone offset from UTC, in minutes: + you can discover the offset for a timezone and pass that, so this + will return activity that occurred on the specified date in the + desired timezone. Note, offset should be the amount of minutes + that should be added to the UTC time to produce the local time - + so for timezones behind UTC the number should be negative, and + for timezones ahead of UTC the number should be positive. This is + the opposite of what Javascript getTimezoneOffset() does, so you + have to invert any value you get from that. """ + dt = datetime.datetime.strptime(date, '%Y-%m-%d') + # if the offset is *negative* some of the events we want may be + # on the next day in UTC terms. if the offset is *positive* some + # of the events we want may be on the previous day in UTC terms. + # 'dt' will be at 00:00, so we subtract 1 day for prevday but add + # 2 days for nextday. e.g. 2018-02-15 00:00 - prevday will be + # 2018-02-14 00:00, nextday will be 2018-02-17 00:00. We'll get + # all events that occurred on 2018-02-14, 2018-02-15 or 2018-02-16 + # in UTC time. + prevday = dt - datetime.timedelta(days=1) + nextday = dt + datetime.timedelta(days=2) query = session.query( model.PagureLog ).filter( - model.PagureLog.date == date + model.PagureLog.date_created.between(prevday, nextday) ).filter( model.PagureLog.user_id == user.id ).order_by( model.PagureLog.id.asc() ) - - return query.all() + events = query.all() + # Now we filter down to the events that *really* occurred on the + # date we were asked for with the offset applied, and return + return [ev for ev in events if ev.date_offset(offset) == dt.date()] def log_action(session, action, obj, user_obj): diff --git a/pagure/lib/model.py b/pagure/lib/model.py index f95779b..4c1fe5d 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -2527,6 +2527,15 @@ class PagureLog(BASE): return desc % arg + def date_offset(self, offset): + '''Returns the date (as a datetime.date()) of this log entry + with a specified offset (in minutes) applied. Necessary if we + want to know what date this event occurred on in a particular + time zone. + ''' + offsetdt = self.date_created + datetime.timedelta(minutes=int(offset)) + return offsetdt.date() + class IssueWatcher(BASE): """ Stores the users watching issues. diff --git a/pagure/templates/_render_repo.html b/pagure/templates/_render_repo.html index 5906f19..ba89735 100644 --- a/pagure/templates/_render_repo.html +++ b/pagure/templates/_render_repo.html @@ -305,6 +305,8 @@ $('#user-activity').hide(); }); var cal = new CalHeatMap(); + var offset = new Date().getTimezoneOffset(); + offset = -offset; cal.init({ cellSize: 9, domain: "month", @@ -313,7 +315,7 @@ start: new Date(new Date().setMonth(new Date().getMonth() - 11)), data: "{{ url_for( 'api_ns.api_view_user_activity_stats', - username=username, format='timestamp') }}", + username=username, format='timestamp') }}" + '&offset=' + offset, dataType: "json", highlight: "now", onClick: function(date, nb) { @@ -323,7 +325,7 @@ type: 'GET', url: "{{ url_for( 'api_ns.api_view_user_activity_date', - username=username, date='') }}" + date + '?grouped=1', + username=username, date='') }}" + date + '?grouped=1&offset=' + offset, contentType: "application/json", dataType: 'json', success: function(data) { diff --git a/tests/test_pagure_flask_api_user.py b/tests/test_pagure_flask_api_user.py index 1651151..f0ed7b1 100644 --- a/tests/test_pagure_flask_api_user.py +++ b/tests/test_pagure_flask_api_user.py @@ -468,6 +468,132 @@ class PagureFlaskApiUSertests(tests.Modeltests): } self.assertEqual(data, exp) + @patch('pagure.lib.notify.send_email') + def test_api_view_user_activity_timezone_negative(self, mockemail): + """Test api_view_user_activity{_stats,_date} with a timezone + 5 hours behind UTC. The activities will occur on 2018-02-15 in + UTC, but on 2018-02-14 in local time. + """ + tests.create_projects(self.session) + repo = pagure.lib._get_project(self.session, 'test') + + dateobj = datetime.datetime(2018, 2, 15, 3, 30) + utcdate = '2018-02-15' + localdate = '2018-02-14' + # Create a single commit log + log = model.PagureLog( + user_id=1, + user_email='foo@bar.com', + project_id=1, + log_type='committed', + ref_id='githash', + date=dateobj.date(), + date_created=dateobj + ) + self.session.add(log) + self.session.commit() + + # Retrieve the user's stats with no offset + output = self.app.get('/api/0/user/pingou/activity/stats') + self.assertEqual(output.status_code, 200) + data = json.loads(output.data) + # date in output should be UTC date + self.assertDictEqual(data, {utcdate: 1}) + + # Retrieve the user's stats with correct offset + output = self.app.get('/api/0/user/pingou/activity/stats?offset=-300') + self.assertEqual(output.status_code, 200) + data = json.loads(output.data) + # date in output should be local date + self.assertDictEqual(data, {localdate: 1}) + + # Retrieve the user's logs for 2018-02-15 with no offset + output = self.app.get( + '/api/0/user/pingou/activity/%s?grouped=1' % utcdate) + self.assertEqual(output.status_code, 200) + data = json.loads(output.data) + exp = { + "activities": [ + { + "description_mk": "

pingou committed on test#githash

" + } + ], + "date": utcdate, + } + self.assertEqual(data, exp) + + # Now retrieve the user's logs for 2018-02-14 with correct + # offset applied + output = self.app.get( + '/api/0/user/pingou/activity/%s?grouped=1&offset=-300' % localdate) + self.assertEqual(output.status_code, 200) + data = json.loads(output.data) + exp['date'] = localdate + self.assertEqual(data, exp) + + @patch('pagure.lib.notify.send_email') + def test_api_view_user_activity_timezone_positive(self, mockemail): + """Test api_view_user_activity{_stats,_date} with a timezone + 4 hours ahead of UTC. The activities will occur on 2018-02-15 + in UTC, but on 2018-02-16 in local time. + """ + tests.create_projects(self.session) + repo = pagure.lib._get_project(self.session, 'test') + + dateobj = datetime.datetime(2018, 2, 15, 22, 30) + utcdate = '2018-02-15' + localdate = '2018-02-16' + # Create a single commit log + log = model.PagureLog( + user_id=1, + user_email='foo@bar.com', + project_id=1, + log_type='committed', + ref_id='githash', + date=dateobj.date(), + date_created=dateobj + ) + self.session.add(log) + self.session.commit() + + # Retrieve the user's stats with no offset + output = self.app.get('/api/0/user/pingou/activity/stats') + self.assertEqual(output.status_code, 200) + data = json.loads(output.data) + # date in output should be UTC date + self.assertDictEqual(data, {utcdate: 1}) + + # Retrieve the user's stats with correct offset + output = self.app.get('/api/0/user/pingou/activity/stats?offset=240') + self.assertEqual(output.status_code, 200) + data = json.loads(output.data) + # date in output should be local date + self.assertDictEqual(data, {localdate: 1}) + + # Retrieve the user's logs for 2018-02-15 with no offset + output = self.app.get( + '/api/0/user/pingou/activity/%s?grouped=1' % utcdate) + self.assertEqual(output.status_code, 200) + data = json.loads(output.data) + exp = { + "activities": [ + { + "description_mk": "

pingou committed on test#githash

" + } + ], + "date": utcdate, + } + self.assertEqual(data, exp) + + # Now retrieve the user's logs for 2018-02-16 with correct + # offset applied + output = self.app.get( + '/api/0/user/pingou/activity/%s?grouped=1&offset=240' % localdate) + self.assertEqual(output.status_code, 200) + data = json.loads(output.data) + exp['date'] = localdate + self.assertEqual(data, exp) + class PagureFlaskApiUsertestrequests(tests.Modeltests): """ Tests for the user requests endpoints """