Email notification when user's role is changed/removed. #3853
Conversation
|
Right now going through the code review comments of #3306 . Will address them (if they're missing). |
|
Thanks for the reviews @di I'll address them soon. |
waseem18
force-pushed
the
issue-3264
branch
2 times, most recently
from
114b27f to
0baa4d0
Compare
|
Addressed reviews @di |
There was a problem hiding this comment.
@waseem18 Looks like only some templates got updated. I've put comments everywhere there needs to be an update to be more clear.
Also, could you push new commits here instead of amending to the existing one? It makes it a lot easier for us to review.
|
@di Thanks for taking your time to review this PR. I've addressed the code reviews in a new commit. Thanks |
di
added
good first issue
|
Could someone at the sprints check this PR out and review it? |
- Send email to the user whose role has been changed/removed. - Send email to other owners of the project. - The person who made the change will not receive the email. Closes pypi#3264 Make linters happy Improve the tests and coverage Update email text. Notify all owners for any role changes. Update email subjects. Update tests. Since we're always sending email to all owners now when role changed, email recipients shouldn't ever be empty. The order matters? Reordering list ordering issue Minor changes Resolved a lint issue Resolved code reviews Used {%- -%} to strip additional new lines Resolved lint issue
Ran the command 'make reformat'
| Your role in the {{ project }} project has been changed by {{ submitter }}. | ||
|
|
||
| {% if role == 'Owner' -%} | ||
| You are now a project owner. This means that you may add other collaborators, |
There was a problem hiding this comment.
| You are now a project owner. This means that you may add other collaborators, | |
| You are now a project owner. This means that you can add other collaborators, |
| upload releases for the project, delete files and releases, or delete the entire | ||
| project. | ||
| {%- elif role == 'Maintainer' -%} | ||
| You are now a project maintainer. This means that you may upload releases for |
There was a problem hiding this comment.
| You are now a project maintainer. This means that you may upload releases for | |
| You are now a project maintainer. This means that you can upload releases for |
| by {{ submitter }}. | ||
|
|
||
| {% if role == 'Owner' -%} | ||
| You are now a project owner. This means that you may add other collaborators, |
There was a problem hiding this comment.
| You are now a project owner. This means that you may add other collaborators, | |
| You are now a project owner. This means that you can add other collaborators, |
There was a problem hiding this comment.
we should be consistent and use 'can' everywhere.
| upload releases for the project, delete files and releases, or delete the entire | ||
| project. | ||
| {%- elif role == 'Maintainer' -%} | ||
| You are now a project maintainer. This means that you may upload releases for |
There was a problem hiding this comment.
| You are now a project maintainer. This means that you may upload releases for | |
| You are now a project maintainer. This means that you can upload releases for |
| You are now a project maintainer. This means that you may upload releases for | ||
| the project. You cannot add collaborators, delete files, delete releases, or | ||
| delete the project. | ||
| {%- endif %} | ||
|
|
||
| Congratulations! |
There was a problem hiding this comment.
I'm not sure if "Congratulations!" is always appropriate for this email. WDYT @di?
|
|
||
| Congratulations! | ||
|
|
||
| If this was a mistake, you can reply to this email directly to communicate with | ||
| If this was a mistake, you can email [email protected] to communicate with |
There was a problem hiding this comment.
@di are you happy with this alternative?
|
Thanks for this PR @waseem18 and from addressing the feedback from @di @yeraydiazdiaz or @ewdurbin would either of you be able to complete an additional review here? |
|
Thanks for the review @nlhkabu. I'll address the feedback tonight |
|
Hi @waseem18 -- I'd love to help get this PR finished and merged! Would you be interested in addressing the existing reviews and rebasing? Thanks! |
|
Yes @brainwane I'll address the reviews tonight. |
|
@waseem18 Thanks for your work on this. To do this the right way, we should wait till we have #5863 implemented, so we can draw on the event logging and use it to trigger this notification. I'm sorry for the additional wait! Looking forward to getting help with some of the other open issues in the meantime! |
|
Note: |
|
@waseem18 Now that the audit log feature is available, we'd love you to take a fresh look at this pull request! |
|
@waseem18 or @rascalking would you like to follow up on this? |
|
@brainwane I may not be able to continue the work on this as of now. |
|
Yup, taking a look at this now (sorry for the delay). |
|
@rascalking Thanks for working on it! And no problem re: the delay. Hope you and yours are doing well. |
|
Fixed by #8328, thanks for your work here! |
PR which addresses issues on #3306
Closes #3264