-
Notifications
You must be signed in to change notification settings - Fork 1k
Add email notification on project/release removal #7071
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
Conversation
217a29b
to
9a22e56
Compare
Hi @MVrachev, is this still a WIP? I don't see any changes here that actually use these new templates. |
I consciously split the implementation of the issue #5714 into multiple pull requests. Here I wanted to discuss with you the text in the templates themselves because it's my first time using Jinjra2. There will be another pull request which will use those templates. |
Thanks for the explanation. Let's keep everything in one pull request, the maintainers have limited bandwidth and it's easier for us to review a single feature-complete PR. If you have any questions about the implementation, feel free to ask in #5714. |
9a22e56
to
f82fdec
Compare
@di I have updated my pull request with the code using it. |
f82fdec
to
db8ce96
Compare
I will fix the issues with the CI these days and you can have a look again. |
449418d
to
04c437e
Compare
@di I updated the unit tests in the pr. The tests I touched are not failing but some others do, but it seems they are failing in the master branch too. I think the pr is ready for a review. It's complete. |
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.
Hi @MVrachev! Thanks for your contribution.
Overall I think this looks great. The main theme is avoiding the word "package" in the code and templates in favor of "project" to maintain consistency with our terminology. I didn't point out every instance to keep from overwhelming the review.
04c437e
to
7a0ea1f
Compare
I addressed your feedback @ewdurbin. I was wondering what word should I use when there is a deletion of a particular release file in a project.
|
7a0ea1f
to
9d33268
Compare
Thanks for those changes @MVrachev! This question actually does stir up the fact that we're only notifying if a Project or Release as whole is removed. If notification for single file removal is added (which is probably a good idea), I'd suggest that we use something along the lines of:
I'm mostly sticking to the names that we use within the codebase/schema; see Project, Release, and File. Overall, package is just kind of a messy term. If anything this nudges me to consider working to shore up that FAQ entry :) |
e62d613
to
18637f1
Compare
Are there other changes required you want me to do @ewdurbin ?
|
It looks like there are some issues with test coverage and linting, once those are resolved this is good to go, thanks @MVrachev |
18637f1
to
d2666d6
Compare
6346caf
to
689a99d
Compare
@ewdurbin you were right. I have added multiple new unit tests in order to increase the code coverage and during that process I found some small bugs and problems. I think it requires a new look at the new changes. I made a new commit with them. |
521b2cf
to
4dd3334
Compare
I have changed the role of the submitter to "owner" for What I am suppose to do? |
Until now, where there are multiple contributors on a single the project, if one of them deletes a release or the whole project the other contributors don't get any notification, which is problematic. Connected with issue pypi#5714. Signed-off-by: Martin Vrachev <[email protected]>
Signed-off-by: Martin Vrachev <[email protected]>
2cc2f50
to
94851a9
Compare
Signed-off-by: Martin Vrachev <[email protected]>
94851a9
to
0fadd33
Compare
I fixed the translations issues. |
Thank you @MVrachev! |
Hi all,
Any help would be appreciated, thanks! |
Hi, emails are sent to user accounts on PyPI, not addresses listed in project metadata. Hope that helps! |
Thanks @di for the clarification. Are emails sent for removals only? Or can they also be sent for normal releases? (NPM does this and it's super useful) |
See #13234. |
thanks.
|
Until now, when there are multiple contributors on a single
the project, if one of them deletes a release or the whole
project the other contributors don't get any notification,
which is problematic.
Connected with issue #5714
Signed-off-by: Martin Vrachev [email protected]