Connect users in tracked mail tasks #116
Conversation
fross123
changed the title
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.
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.
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.
@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.
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.
can you move this code into a function and invert the condition?
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.
you can also set the author of the comment, on line 145
There was a problem hiding this comment.
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.
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.
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.
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.
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: |
|
|
||
| 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 |
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!