Include repo name in review request emails
Review Request #3768 — Created Feb. 2, 2013 and submitted
Include repo name in review request emails For Issue 2483. The review request email notification template was altered to include the repository name along with the review group. This was requested since it would be useful for groups with multiple repositories. The template was altered so that if a repository is associated with the review request, then the name will appear in parentheses after the review group. If no repository was included, then nothing new will appear.
I set up emails to be sent through gmail smtp on my local test server and also created a repository to test with. I then created two review requests, one with the test repo and one with no repo. The emails for the test repo review request sent emails saying 'Review Request for Katkat (TestRepo) ' and the emails for the review request with no repo sent emails saying 'Review Request for Katkat'. This was the expected result.
| Description | From | Last Updated |
|---|---|---|
|
For HTML e-mails, we try to mimic the look of review request boxes. So we have some fields, like the … |
 chipx86
|
|
|
If there's a repository, it must have a name, so you don't need to check repository.name. You only need to … |
chipx86
|
|
|
You already have a margin-top style on the parent element. I don't think you need this additionally on the |
 david
|
|
|
This can be removed. It's really only useful when we have whole sections with lots of content below it, but … |
chipx86
|
SM
-
This looks fine, but I'm not sure if it's the best way to list the repository. Maybe a separate line that says something like: when there is no repo: "Repository: None" or when there is: "Repository: <repo_name>.
-
-
reviewboard/templates/notifications/review_request_email.html (Diff revision 1) For HTML e-mails, we try to mimic the look of review request boxes. So we have some fields, like the Bugs field. We should be adding one for the Repository. That means it'll end up looking different from the .txt template, but that's okay.
-
reviewboard/templates/notifications/review_request_email.txt (Diff revision 1) If there's a repository, it must have a name, so you don't need to check repository.name. You only need to do the outer check. I'm not sure that this is the right place to put it the repository name, though. This line can end up getting long, and there's no real context saying that the thing in parens is a repository name. I think what would work better is to have a separate line, if the repository exists, saying: "Repository: <name>"
-
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: Ignored Files: reviewboard/templates/notifications/review_request_email.html reviewboard/templates/notifications/review_request_email.txt
-
-
reviewboard/templates/notifications/review_request_email.html (Diff revision 2) You already have a margin-top style on the parent element. I don't think you need this additionally on the <b>
-
One tiny thing, and then we're good! :)
-
reviewboard/templates/notifications/review_request_email.txt (Diff revision 2) This can be removed. It's really only useful when we have whole sections with lots of content below it, but in this case I see it as more of a field under the previous section.
-
Would you be able to provide screenshots of both types of diffs? There's a handy URL you can use: http://<site>/r/<id>/preview-email/text/ And: http://<site>/r/<id>/preview-email/html/
KA
Change Summary:
Removed unnecessary lines in code and added screenshots to review request.
Diff: |
Revision 3 (+12) |
|---|---|
Added Files: |
-
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: Ignored Files: reviewboard/templates/notifications/review_request_email.html reviewboard/templates/notifications/review_request_email.txt
-
Looking at these screenshots....does anybody else feel like "Repository" should be the first thing? Before the review request description?
KA
Change Summary:
The 'Bugs' and 'Repository' sections of the email notification are now above the 'Description' section. I also changed the start of the bugs section in the text version to say 'Bugs: ' instead of 'This addresses bug ', which is something David suggested. I have also added new screenshots
Diff: |
Revision 4 (+30 -19) |
|---|---|
Added Files: |
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: Ignored Files: reviewboard/templates/notifications/review_request_email.html reviewboard/templates/notifications/review_request_email.txt