-
Notifications
You must be signed in to change notification settings - Fork 964
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
Send email when a new collaborator has been added to the project. #3155
Conversation
warehouse/manage/views.py
Outdated
project.name, | ||
form.role_name.data, | ||
email_recipients | ||
) |
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.
Eventually we're going to be sending an invitation to the new collaborator instead of just a notification, so we might want to break that up into a separate email. That would allow us to make the notification to the new contributor slightly more personal, like:
You have been added as a {{ role }} to the PyPI project {{ project }} by {{ submitter }}.
Congratulations!
warehouse/email.py
Outdated
'email/collaborator-added.body.txt', fields, request=request | ||
) | ||
|
||
request.task(send_email).delay(body, email_recipients, subject) |
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.
As is, this will send the email to all the owners with all of their emails in theTO:
field. While this is probably not a big deal for most people, generally we try to keep user's email addresses private unless they make them public. I think we could modify send_email
to allow us to BCC multiple recipients instead, so the person receiving the email would just see their own address.
warehouse/manage/views.py
Outdated
|
||
owners = request.db.query(Role).filter(Role.role_name == 'Owner', | ||
Role.project == project) | ||
email_recipients = [owner.user.email for owner in owners] |
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.
We can reduce some of the memory required to do this by moving the .user
part into the DB with a join:
owners = (
request.db.query(Role)
.join(User)
.filter(Role.role_name == 'Owner', Role.project == project)
)
owner_emails = [owner.email for owner in owners]
That way we just User
s back from our query, not Roles
.
@@ -0,0 +1,5 @@ | |||
'{{ username }}' has been added as a collaborator with '{{ role }}' role to the PyPI | |||
project: '{{ project }}' by '{{ submitter }}'. |
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've been taking care to keep these emails at a fixed 80-character limit, which requires a bit of crafting of the template.
For this, we probably want to start out with the same "greeting" every time rather than the username as well.
So maybe something like:
A new collaborator has been added to a project you own on PyPI:
Username: {{ username }}
Role: {{ role }}
Collaborator for: {{ project }}
Added by: {{ submitter }}
If this was a mistake, you can reply to this email directly to communicate with
the PyPI administrators.
warehouse/email.py
Outdated
@@ -19,15 +19,18 @@ | |||
|
|||
|
|||
@tasks.task(bind=True, ignore_result=True, acks_late=True) | |||
def send_email(task, request, body, recipients, subject): | |||
def send_email(task, request, body, recipients, subject, send_to_bcc=False): |
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 think I would prefer if this more closely mimicked the arguments that Message
takes, so something like:
def send_email(task, request, body, subject, recipients=None, bcc=None)
(FYI, changing the order here might require updates elsewhere where we're passing this positional arguments)
And then since the default arguments for Message
are already None
, we can call it like:
message = Message(
body=body,
recipients=recipients,
bcc=bcc,
sender=...,
subject=subject,
)
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.
Ok it makes sense to do it that way. Thanks.
That'll be quite a big change, since this is being called everywhere as positional argument.
I will prepare a separate PR just to update recipients
into keyword only argument in send_email
method.
@@ -0,0 +1,3 @@ | |||
You have been added as {{ role }} to the PyPI project {{ project }} by {{ submitter }}. | |||
|
|||
Congratulations! |
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.
This needs the same "you can reply to the admins" message.
warehouse/manage/views.py
Outdated
project.name, | ||
form.role_name.data, | ||
user.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.
This is a nitpick, but could you make the indentation here consistent? (e.g. like the call to send_collaborator_added_email
above)
warehouse/manage/views.py
Outdated
request.db.query(Role) | ||
.join(User) | ||
.filter(Role.role_name == 'Owner', Role.project == project) | ||
) |
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.
Sorry, I gave you a bad example query before. This should be:
owners = (
request.db.query(User)
.join(Role.user)
.filter(Role.role_name == 'Owner', Role.project == project)
)
@Mariatta looks like tests are failing -- take a look? |
Thanks! The tests are passing now. Please re-review when you have the chance. |
Hi @Mariatta! I tested this PR and it looks like the emails are being sent to the new collaborator, but not to the other owners. |
Thanks @Mariatta! |
Thanks for the patience in reviewing this :) |
Fixes #1000