Add timestamp to single message emails #287
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #287 +/- ##
===========================================
+ Coverage 56.92% 56.95% +0.03%
===========================================
Files 70 70
Lines 3719 3722 +3
Branches 477 477
===========================================
+ Hits 2117 2120 +3
Misses 1536 1536
Partials 66 66
Continue to review full report at Codecov.
|
fmn/formatters.py
Outdated
| @@ -399,11 +399,13 @@ def email(message, recipient): | |||
| subject_prefix.strip(), subject.strip()) | |||
| email_message.add_header('Subject', subject.encode('utf-8')) | |||
|
|
|||
| timestamp = datetime.datetime.fromtimestamp(message['timestamp']) | |||
| content = timestamp.strftime('%c') + u'\n' | |||
There was a problem hiding this comment.
It's not documented anywhere AFAIK, but those timestamps are UTC so we should note that to avoid confusion. Maybe something like:
import pytz
utc_timestamp = pytz.utc.localize(message['timestamp']).strftime('%Y-%m-%d %H:%M:%S %Z')
content = u'Notification time stamped {}\n\n'.format(utc_timestamp)
Sorry, I know this is going to make you edit all those test results again :( - I should have been less lazy and added something to encode the expected results with base64 automatically.
There was a problem hiding this comment.
It would make more sense if the timezone is already set in the received message['timestamp'] (is that from fedmsg?).
Maybe we can ask fedmsg to implement that, in the meantime I can just modify here to set the timezone if the received message['timestamp'] is not already timezone aware.
And I think I should also modify the timestamps in digest creation in the same way.
What do you think?
There was a problem hiding this comment.
It would make more sense if the timezone is already set in the received message['timestamp'] (is that from fedmsg?) Maybe we can ask fedmsg to implement that, in the meantime I can just modify here to set the timezone if the received message['timestamp'] is not already timezone aware.
So looking at the fedmsg source, it's just time.time() which is seconds since the epoch (January 1, 1970, 00:00:00 (UTC)): https://github.com/fedora-infra/fedmsg/blob/a0547bb08372ade44adb088bc57de34f0f86e729/fedmsg/core.py#L290. This makes it easy for consumers to convert it to whatever timezone they desire, so I think it's best to leave that as it is.
I did realize my suggestion is wrong, though, since pytz.utc.localize wants datetime objects. An easier implementation is just datetime.datetime.fromtimestamp(message['timestamp'], tz=pytz.utc).
And I think I should also modify the timestamps in digest creation in the same way.
What do you think?
Yes, that's a good idea - you should just need to add that tz kwarg.
There was a problem hiding this comment.
Yes, I was confusing timestamps with datetime objects too... :-(
I'll work on it in the next days as soon as I have some free time.
|
It seems that the failure is not due to the PR, but to an internal error of Travis, doesn't it? |
|
Yeah, Travis is occasionally flaky. What I tend to do is just |
Fixes #285
This will add a timestamp as starting line for every single message email.
The other content (message or dump) remains unchanged.