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.

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

Merged
merged 15 commits into from
Mar 14, 2018

Conversation

Mariatta
Copy link
Contributor

@Mariatta Mariatta commented Mar 7, 2018

  • send the email to the newly added collaborator
  • send the email to other owners
  • email not sent to the person who added the collaborator

Fixes #1000

project.name,
form.role_name.data,
email_recipients
)
Copy link
Member

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!

'email/collaborator-added.body.txt', fields, request=request
)

request.task(send_email).delay(body, email_recipients, subject)
Copy link
Member

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.


owners = request.db.query(Role).filter(Role.role_name == 'Owner',
Role.project == project)
email_recipients = [owner.user.email for owner in owners]
Copy link
Member

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 Users 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 }}'.
Copy link
Member

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.

@@ -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):
Copy link
Member

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,
)

Copy link
Contributor Author

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!
Copy link
Member

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.

project.name,
form.role_name.data,
user.email
)
Copy link
Member

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)

request.db.query(Role)
.join(User)
.filter(Role.role_name == 'Owner', Role.project == project)
)
Copy link
Member

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 and others added 9 commits March 10, 2018 11:39
- send the email to the newly added collaborator
- send the email to other owners
- email not sent to the person who added the collaborator

Closes pypi#1000
- Add footer to email
@brainwane
Copy link
Contributor

@Mariatta looks like tests are failing -- take a look?

@Mariatta
Copy link
Contributor Author

Thanks! The tests are passing now. Please re-review when you have the chance.

@lgh2
Copy link
Contributor

lgh2 commented Mar 13, 2018

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.

@Mariatta
Copy link
Contributor Author

Thanks @lgh2! With the latest commit (ca04e7c) the other owners should receive the email notification now.

@di di merged commit 4f6d3c9 into pypi:master Mar 14, 2018
@Mariatta Mariatta deleted the issue-1000 branch March 14, 2018 19:57
@di
Copy link
Member

di commented Mar 14, 2018

Thanks @Mariatta!

@Mariatta
Copy link
Contributor Author

Thanks for the patience in reviewing this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants