Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add timestamp to single message emails #287

Merged
merged 2 commits into from
Mar 31, 2018
Merged

Add timestamp to single message emails #287

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea86a4f
Add timestamp to single message emails
mattiaverga Mar 27, 2018
7e10dff
Add TZ information to message timestamps
mattiaverga Mar 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ Bug fixes
* Set digest content limits to 1k messages and 500k content length
(`#280 <https://github.com/fedora-infra/fmn/pull/280>`_).

* Add timestamp in single message emails
(`#284 <https://github.com/fedora-infra/fmn/pull/284>`_).


v2.0.2
======
Expand Down
6 changes: 4 additions & 2 deletions fmn/formatters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks!

try:
content = fedmsg.meta.msg2long_form(message, **config.app_conf) or u''
content += fedmsg.meta.msg2long_form(message, **config.app_conf) or u''
except Exception:
_log.exception('fedmsg.meta.msg2long_form failed to handle %r', message)
content = json.dumps(message, sort_keys=True, indent=4)
content += json.dumps(message, sort_keys=True, indent=4)

try:
link = fedmsg.meta.msg2link(message, **config.app_conf)
Expand Down
57 changes: 33 additions & 24 deletions fmn/tests/test_formatters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,8 +372,9 @@ def test_email(self):
'MIME-Version: 1.0\n'
'Content-Type: text/plain; charset="utf-8"\n'
'Content-Transfer-Encoding: base64\n\n'
"amNsaW5lIHVwZGF0ZWQgdGhlIHJ1bGVzIG9uIGEgZm1uIGVtYWlsIGZpbHRlcgoJaHR0cHM6Ly9h\n"
"cHBzLmZlZG9yYXByb2plY3Qub3JnL25vdGlmaWNhdGlvbnMv\n"
'RnJpIE9jdCAgNiAxNzoyNTozMCAyMDE3CmpjbGluZSB1cGRhdGVkIHRoZSBydWxlcyBvbiBhIGZt\n'
'biBlbWFpbCBmaWx0ZXIKCWh0dHBzOi8vYXBwcy5mZWRvcmFwcm9qZWN0Lm9yZy9ub3RpZmljYXRp\n'
'b25zLw==\n'
)

actual = formatters.email(self.message, self.recipient)
Expand All @@ -395,8 +396,9 @@ def test_subject_prefix(self):
'MIME-Version: 1.0\n'
'Content-Type: text/plain; charset="utf-8"\n'
'Content-Transfer-Encoding: base64\n\n'
"amNsaW5lIHVwZGF0ZWQgdGhlIHJ1bGVzIG9uIGEgZm1uIGVtYWlsIGZpbHRlcgoJaHR0cHM6Ly9h\n"
"cHBzLmZlZG9yYXByb2plY3Qub3JnL25vdGlmaWNhdGlvbnMv\n"
'RnJpIE9jdCAgNiAxNzoyNTozMCAyMDE3CmpjbGluZSB1cGRhdGVkIHRoZSBydWxlcyBvbiBhIGZt\n'
'biBlbWFpbCBmaWx0ZXIKCWh0dHBzOi8vYXBwcy5mZWRvcmFwcm9qZWN0Lm9yZy9ub3RpZmljYXRp\n'
'b25zLw==\n'
)

actual = formatters.email(self.message, self.recipient)
Expand All @@ -415,6 +417,7 @@ def test_unparsable_category(self):
'MIME-Version: 1.0\n'
'Content-Type: text/plain; charset="utf-8"\n'
'Content-Transfer-Encoding: base64\n\n'
'RnJpIE9jdCAgNiAxNzoyNTozMCAyMDE3Cg==\n'
)
self.message['topic'] = 'so.short'

Expand All @@ -437,8 +440,9 @@ def test_no_subtitle(self):
'MIME-Version: 1.0\n'
'Content-Type: text/plain; charset="utf-8"\n'
'Content-Transfer-Encoding: base64\n\n'
"amNsaW5lIHVwZGF0ZWQgdGhlIHJ1bGVzIG9uIGEgZm1uIGVtYWlsIGZpbHRlcgoJaHR0cHM6Ly9h\n"
"cHBzLmZlZG9yYXByb2plY3Qub3JnL25vdGlmaWNhdGlvbnMv\n"
'RnJpIE9jdCAgNiAxNzoyNTozMCAyMDE3CmpjbGluZSB1cGRhdGVkIHRoZSBydWxlcyBvbiBhIGZt\n'
'biBlbWFpbCBmaWx0ZXIKCWh0dHBzOi8vYXBwcy5mZWRvcmFwcm9qZWN0Lm9yZy9ub3RpZmljYXRp\n'
'b25zLw==\n'
)

actual = formatters.email(self.message, self.recipient)
Expand All @@ -459,8 +463,9 @@ def test_unparsable_usernames(self):
'MIME-Version: 1.0\n'
'Content-Type: text/plain; charset="utf-8"\n'
'Content-Transfer-Encoding: base64\n\n'
"amNsaW5lIHVwZGF0ZWQgdGhlIHJ1bGVzIG9uIGEgZm1uIGVtYWlsIGZpbHRlcgoJaHR0cHM6Ly9h\n"
"cHBzLmZlZG9yYXByb2plY3Qub3JnL25vdGlmaWNhdGlvbnMv\n"
'RnJpIE9jdCAgNiAxNzoyNTozMCAyMDE3CmpjbGluZSB1cGRhdGVkIHRoZSBydWxlcyBvbiBhIGZt\n'
'biBlbWFpbCBmaWx0ZXIKCWh0dHBzOi8vYXBwcy5mZWRvcmFwcm9qZWN0Lm9yZy9ub3RpZmljYXRp\n'
'b25zLw==\n'
)

actual = formatters.email(self.message, self.recipient)
Expand All @@ -483,8 +488,9 @@ def test_packages(self):
'MIME-Version: 1.0\n'
'Content-Type: text/plain; charset="utf-8"\n'
'Content-Transfer-Encoding: base64\n\n'
"amNsaW5lIHVwZGF0ZWQgdGhlIHJ1bGVzIG9uIGEgZm1uIGVtYWlsIGZpbHRlcgoJaHR0cHM6Ly9h\n"
"cHBzLmZlZG9yYXByb2plY3Qub3JnL25vdGlmaWNhdGlvbnMv\n"
'RnJpIE9jdCAgNiAxNzoyNTozMCAyMDE3CmpjbGluZSB1cGRhdGVkIHRoZSBydWxlcyBvbiBhIGZt\n'
'biBlbWFpbCBmaWx0ZXIKCWh0dHBzOi8vYXBwcy5mZWRvcmFwcm9qZWN0Lm9yZy9ub3RpZmljYXRp\n'
'b25zLw==\n'
)

actual = formatters.email(self.message, self.recipient)
Expand All @@ -506,8 +512,9 @@ def test_unparsable_packages(self):
'MIME-Version: 1.0\n'
'Content-Type: text/plain; charset="utf-8"\n'
'Content-Transfer-Encoding: base64\n\n'
"amNsaW5lIHVwZGF0ZWQgdGhlIHJ1bGVzIG9uIGEgZm1uIGVtYWlsIGZpbHRlcgoJaHR0cHM6Ly9h\n"
"cHBzLmZlZG9yYXByb2plY3Qub3JnL25vdGlmaWNhdGlvbnMv\n"
'RnJpIE9jdCAgNiAxNzoyNTozMCAyMDE3CmpjbGluZSB1cGRhdGVkIHRoZSBydWxlcyBvbiBhIGZt\n'
'biBlbWFpbCBmaWx0ZXIKCWh0dHBzOi8vYXBwcy5mZWRvcmFwcm9qZWN0Lm9yZy9ub3RpZmljYXRp\n'
'b25zLw==\n'
)

actual = formatters.email(self.message, self.recipient)
Expand All @@ -529,13 +536,13 @@ def test_unparsable_body(self):
'MIME-Version: 1.0\n'
'Content-Type: text/plain; charset="utf-8"\n'
'Content-Transfer-Encoding: base64\n\n'
'ewogICAgIm1zZyI6IHsKICAgICAgICAiY2hhbmdlZCI6ICJydWxlcyIsIAogICAgICAgICJjb250\n'
'ZXh0IjogImVtYWlsIiwgCiAgICAgICAgIm9wZW5pZCI6ICJqY2xpbmUuaWQuZmVkb3JhcHJvamVj\n'
'dC5vcmciCiAgICB9LCAKICAgICJtc2dfaWQiOiAiMjAxNy02YWE3MWQ1Yi1mYmU0LTQ5ZTctYWZk\n'
'ZC1hZmNmMGQyMjgwMmIiLCAKICAgICJ0aW1lc3RhbXAiOiAxNTA3MzEwNzMwLCAKICAgICJ0b3Bp\n'
'YyI6ICJvcmcuZmVkb3JhcHJvamVjdC5kZXYuZm1uLmZpbHRlci51cGRhdGUiLCAKICAgICJ1c2Vy\n'
'bmFtZSI6ICJ2YWdyYW50Igp9CglodHRwczovL2FwcHMuZmVkb3JhcHJvamVjdC5vcmcvbm90aWZp\n'
'Y2F0aW9ucy8=\n'
'RnJpIE9jdCAgNiAxNzoyNTozMCAyMDE3CnsKICAgICJtc2ciOiB7CiAgICAgICAgImNoYW5nZWQi\n'
'OiAicnVsZXMiLCAKICAgICAgICAiY29udGV4dCI6ICJlbWFpbCIsIAogICAgICAgICJvcGVuaWQi\n'
'OiAiamNsaW5lLmlkLmZlZG9yYXByb2plY3Qub3JnIgogICAgfSwgCiAgICAibXNnX2lkIjogIjIw\n'
'MTctNmFhNzFkNWItZmJlNC00OWU3LWFmZGQtYWZjZjBkMjI4MDJiIiwgCiAgICAidGltZXN0YW1w\n'
'IjogMTUwNzMxMDczMCwgCiAgICAidG9waWMiOiAib3JnLmZlZG9yYXByb2plY3QuZGV2LmZtbi5m\n'
'aWx0ZXIudXBkYXRlIiwgCiAgICAidXNlcm5hbWUiOiAidmFncmFudCIKfQoJaHR0cHM6Ly9hcHBz\n'
'LmZlZG9yYXByb2plY3Qub3JnL25vdGlmaWNhdGlvbnMv\n'
)

actual = formatters.email(self.message, self.recipient)
Expand All @@ -557,7 +564,8 @@ def test_unparsable_link(self):
'MIME-Version: 1.0\n'
'Content-Type: text/plain; charset="utf-8"\n'
'Content-Transfer-Encoding: base64\n\n'
'amNsaW5lIHVwZGF0ZWQgdGhlIHJ1bGVzIG9uIGEgZm1uIGVtYWlsIGZpbHRlcg==\n'
'RnJpIE9jdCAgNiAxNzoyNTozMCAyMDE3CmpjbGluZSB1cGRhdGVkIHRoZSBydWxlcyBvbiBhIGZt\n'
'biBlbWFpbCBmaWx0ZXI=\n'
)

actual = formatters.email(self.message, self.recipient)
Expand All @@ -578,10 +586,11 @@ def test_footer(self):
'MIME-Version: 1.0\n'
'Content-Type: text/plain; charset="utf-8"\n'
'Content-Transfer-Encoding: base64\n\n'
'amNsaW5lIHVwZGF0ZWQgdGhlIHJ1bGVzIG9uIGEgZm1uIGVtYWlsIGZpbHRlcgoJaHR0cHM6Ly9h\n'
'cHBzLmZlZG9yYXByb2plY3Qub3JnL25vdGlmaWNhdGlvbnMvCgotLQpZb3UgcmVjZWl2ZWQgdGhp\n'
'cyBtZXNzYWdlIGR1ZSB0byB5b3VyIHByZWZlcmVuY2Ugc2V0dGluZ3MgYXQgCmh0dHA6Ly9sb2Nh\n'
'bGhvc3Q6NTAwMC9qY2xpbmUuaWQuZmVkb3JhcHJvamVjdC5vcmcvZW1haWwvMTE=\n'
'RnJpIE9jdCAgNiAxNzoyNTozMCAyMDE3CmpjbGluZSB1cGRhdGVkIHRoZSBydWxlcyBvbiBhIGZt\n'
'biBlbWFpbCBmaWx0ZXIKCWh0dHBzOi8vYXBwcy5mZWRvcmFwcm9qZWN0Lm9yZy9ub3RpZmljYXRp\n'
'b25zLwoKLS0KWW91IHJlY2VpdmVkIHRoaXMgbWVzc2FnZSBkdWUgdG8geW91ciBwcmVmZXJlbmNl\n'
'IHNldHRpbmdzIGF0IApodHRwOi8vbG9jYWxob3N0OjUwMDAvamNsaW5lLmlkLmZlZG9yYXByb2pl\n'
'Y3Qub3JnL2VtYWlsLzEx\n'
)
self.recipient['triggered_by_links'] = True

Expand Down