-
Notifications
You must be signed in to change notification settings - Fork 285
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.
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
Connect users in tracked mail tasks #116
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution 👍
It looks good to me overall, I've requested a few changes.
Out of curiosity, are you using the mail tracker in production somewhere? If so, are you happy with the experience?
todo/mail/consumers/tracker.py
Outdated
@@ -65,7 +68,7 @@ def parse_references(task_list, references): | |||
if answer_thread is None: | |||
logger.info("no answer thread found in references") | |||
else: | |||
logger.info("found an answer thread: %d", answer_thread) | |||
logger.info(f"found an answer thread: {answer_thread}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using f-strings with the logger is bad practice, see https://klichx.dev/2019/11/09/f-strings-and-python-logging/
(the only reason why it hasn't been done above is that the default formatting mode doesn't support deferred dict access)
you're right about changing this line though, it should be a %s rather than a %d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That link is broken. I'd be interested to know the reasoning behind a preference against f-strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bernd-wechner The link works for me. The article makes a good point, though I have to say we use f-strings in logging at work and have never been bitten by that issue, which I consider an edge case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. that site doesn't use DNSEC and blocked here, so I used a proxy. The point it makes is valid for high volume logging. The rate at which todo is used here (or likely ever to be on a given site) makes the noted issues irrelevant beyond simply being better practice in general for logging (as some of it is high volume and frequency or logging things expensive to calculate). But all good, I now understand the concern.
todo/mail/consumers/tracker.py
Outdated
@@ -124,10 +127,18 @@ def insert_message(task_list, message, priority, task_title_format): | |||
|
|||
with transaction.atomic(): | |||
if best_task is None: | |||
user = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you move this code into a function and invert the condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely!
todo/mail/consumers/tracker.py
Outdated
best_task = Task.objects.create( | ||
priority=priority, | ||
title=format_task_title(task_title_format, message), | ||
task_list=task_list, | ||
created_by=user, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can also set the author of the comment, on line 145
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh yeah! Will do this.
README.md
Outdated
|
||
Settings: | ||
```python | ||
TODO_MATCH_USERS = True # Set to True if you would like to match users. If you do not have authentication setup, do not set this to True. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd change the option to TODO_MAIL_USER_MAPPER
(all other mail related options have the TODO_MAIL_
prefix), which would either be a function taking an email address and returning an user, or None
.
The default would be None, and the user would set it to something like get_django_user_by_email
to get the default email to user mapping.
Here's why: it's getting pretty common for authentication to be done outside of django. When a user logs in using some external tool, a django user is created if needed.
If somebody has such a setup, they might want to create users on the fly when an email is received from a never seen before user. Having this configurable callback enables just that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally hear you on the external authentication. However, I am a little unclear on what the setting should be when we want to use this function, could you expand on that? Where would get_django_users_by_email
need to be and what should it do?
Also, I will change the name of the option!
README.md
Outdated
@@ -261,6 +261,15 @@ TODO_MAIL_TRACKERS = { | |||
} | |||
``` | |||
|
|||
Optionally, you can match incoming users with their accounts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about:
Optionally, the email addresses of incoming emails can be mapped back to django users. If a user emails the test_tracker, and also is a registered User in your application, the user will show up as having created the task or comment. By default, only the email address will show up.
This isn't enabled by default, as some domains are misconfigured and do not prevent impersonation. If this option is enabled and your setup doesn't properly authenticate emails, malicious incoming emails might mistakenly be attributed to users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it, thank you!
No response from the OP. Does anyone want to see this PR continue? If so, speak up. Otherwise I'm going to close it without merging. |
@shacker Sorry for the delay, I kind of forgot about this. I wound up going back to work shortly after I made this PR... If you want to close that's fine with me, otherwise I am happy to take a look again in a few days and make those changes. |
@fross123 Definitely still interested in having the PR, if you feel like it's close and you're willing! |
Sorry about the delay in this folks. I've pushed some updates, let me know what you think. Wasn't too sure about the change requested to the setting, so I will update when we discuss that further. Thanks! |
def match_user(email): | ||
""" This function takes an email and checks for a registered user.""" | ||
|
||
if not settings.TODO_MAIL_USER_MAPPER: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice.
|
||
def test_tracker_email_match(todo_setup, django_user_model, settings): | ||
""" | ||
Ensure that a user is added to new lists when sent from registered email |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding a test!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, and apologies for the lengthy response delay. Approved and merging. Cheers!
I tweaked one of the logs for the tracker because it was failing a test. Hopefully it still logs what you need, I apologize if it doesn't.
Also, the feature discussed in #112. It works well in my pretty limited situation. I wound up adding a setting and only matching if settings.TODO_MATCH_USERS is true to make sure that current users would not be affected unnecessarily.
Thanks!