JSHint
passed.
PEP8 Style Checker
internal error.
Pyflakes
passed.
Refactor e-mail generation and previewing
Review Request #8885 — Created April 6, 2017 and submitted
Previously e-mail generation and previewing used duplicated logic. The
preview_{foo}_emailviews would duplicate the logic used to generate
the template context that themail_{foo}methods (that actually built
& sent the e-mails out).Now, we have utilities for building e-mail messages
(prepare_{foo}_mail) that generate anEMailMessagebut do not send
it and a generic e-mail sending & error-handling utility in
send_email. The logic for catching failed e-mails was present in each
method that sent an e-mail and it is now unified in a single place.Additionally, we have a decorator that generates e-mail preview views
from functions that can generate the necessary arguments to provide to a
prepare_{foo}_email. This way the view only needs to compute the state
necessary, which has resulted in a large amount of deduplication.Unit tests have been updated to reference the new methods.
Ran unit tests.
| Description | From | Last Updated |
|---|---|---|
|
email/decorators.py should be in this diff. |
 brennie
|
|
|
I don't think you meant to add reviewboard/notifications/original_cbs.py ? |
 david
|
|
|
'reviewboard.notifications.email.decorators.preview_email' imported but unused |
 reviewbot
|
|
|
'reviewboard.notifications.email.message.prepare_password_changed_mail' imported but unused |
reviewbot
|
|
|
Might be slightly more intuitive as: if not (siteconfig.get('mail_send_review_close_mail') and review_request.public): |
david
|
|
|
While here, can you remove the blank line and fix the ordering? |
 chipx86
|
|
|
"djblets" |
chipx86
|
|
|
Let's reverse this check. It's quicker to check a boolean. |
chipx86
|
|
|
No ending paren. |
chipx86
|
|
|
No ending paren. |
chipx86
|
|
|
Let's check trivial first. |
chipx86
|
|
|
I think we have a :http: you can use for HTTP codes. |
chipx86
|
|
|
This is :django:setting: |
chipx86
|
|
|
raise |
chipx86
|
|
|
Let's use .update here. |
chipx86
|
|
|
We can use build_server_url(). |
chipx86
|
|
|
If .target_groups is already fetched, this is fine. If not, we should use .values_list(). (If the former, we should document … |
chipx86
|
|
|
This won't work. It's missing a second argument. Good indication that we're missing a test condition or tests aren't passing. |
chipx86
|
|
|
Can this not be imported higher up? |
chipx86
|
|
|
This could be combined. |
chipx86
|
|
|
Reads kinda weird. Maybe just "Whether the review should only be sent to the submitter of the review request." |
chipx86
|
|
|
Can be inline above in extra_context. |
chipx86
|
|
|
Can now use Python 2.7 set syntax. |
chipx86
|
|
|
Wrapped weird. |
chipx86
|
|
|
Wrapping and paragraph break issues. |
chipx86
|
|
|
Let's use .update({...}) |
chipx86
|
|
|
No need for the parens after for. |
chipx86
|
|
|
This can probably be one statement. |
chipx86
|
|
|
"updated" |
chipx86
|
|
|
Capitalize the beginning of each. |
chipx86
|
|
|
Let's put one per line. Helps a little with readability here, given the statement below. |
chipx86
|
|
|
I doubt you want these. We should have a unit test covering this case. |
chipx86
|
|
|
2 blank lines. |
chipx86
|
|
|
Blank line should remain. |
chipx86
|
|
|
Trailing comma. |
chipx86
|
|
|
local variable 'e' is assigned to but never used |
reviewbot
|
|
|
Missing period. |
chipx86
|
|
|
E501 line too long (81 > 79 characters) |
reviewbot
|
|
|
E501 line too long (81 > 79 characters) |
reviewbot
|
|
|
E501 line too long (81 > 79 characters) |
reviewbot
|
|
|
E501 line too long (81 > 79 characters) |
reviewbot
|
|
|
E501 line too long (81 > 79 characters) |
reviewbot
|
|
|
E501 line too long (80 > 79 characters) |
reviewbot
|
- Description:
-
Previously e-mail generation and previewing used duplicated logic. The
preview_{foo}_emailviews would duplicate the logic used to generatethe template context that the mail_{foo}methods (that actually built& sent the e-mails out). Now, we have utilities for building e-mail messages
( prepare_{foo}_mail) that generate anEMailMessagebut do not sendit and a generic e-mail sending & error-handling utility in send_email. The logic for catching failed e-mails was present in eachmethod that sent an e-mail and it is now unified in a single place. Additionally, we have a decorator that generates e-mail preview views
from functions that can generate the necessary arguments to provide to a prepare_{foo}_email. This way the view only needs to compute the statenecessary, which has resulted in a large amount of deduplication. + While I was here I also updated the
ReviewPublishedEmailHookto take+ the to_submitter_onlykeyword argument, which it was missing before.+ Unit tests have been updated to reference the new methods.
- Description:
-
Previously e-mail generation and previewing used duplicated logic. The
preview_{foo}_emailviews would duplicate the logic used to generatethe template context that the mail_{foo}methods (that actually built& sent the e-mails out). Now, we have utilities for building e-mail messages
( prepare_{foo}_mail) that generate anEMailMessagebut do not sendit and a generic e-mail sending & error-handling utility in send_email. The logic for catching failed e-mails was present in eachmethod that sent an e-mail and it is now unified in a single place. Additionally, we have a decorator that generates e-mail preview views
from functions that can generate the necessary arguments to provide to a prepare_{foo}_email. This way the view only needs to compute the statenecessary, which has resulted in a large amount of deduplication. While I was here I also updated the
ReviewPublishedEmailHookto take~ the to_submitter_onlykeyword argument, which it was missing before.~ the to_submitter_onlykeyword argument, which it was missing before. I+ also replaced all the usage of + {{site_domain_method}}://{{site_domain}}in e-mail templates with the+ result of get_server_url.Unit tests have been updated to reference the new methods.
- Depends On:
-
- Diff:
Revision 2 (+954 -826)
Checks run (2 succeeded, 1 failed with error)
JSHint passed.PEP8 Style Checker internal error.Pyflakes passed.
- Change Summary:
-
Update description wrt to refactoring /r/8889 out.
- Description:
-
Previously e-mail generation and previewing used duplicated logic. The
preview_{foo}_emailviews would duplicate the logic used to generatethe template context that the mail_{foo}methods (that actually built& sent the e-mails out). Now, we have utilities for building e-mail messages
( prepare_{foo}_mail) that generate anEMailMessagebut do not sendit and a generic e-mail sending & error-handling utility in send_email. The logic for catching failed e-mails was present in eachmethod that sent an e-mail and it is now unified in a single place. Additionally, we have a decorator that generates e-mail preview views
from functions that can generate the necessary arguments to provide to a prepare_{foo}_email. This way the view only needs to compute the statenecessary, which has resulted in a large amount of deduplication. - While I was here I also updated the
ReviewPublishedEmailHookto take- the to_submitter_onlykeyword argument, which it was missing before. I- also replaced all the usage of - {{site_domain_method}}://{{site_domain}}in e-mail templates with the- result of get_server_url.- Unit tests have been updated to reference the new methods.
- Change Summary:
-
Refactored many changes out of this one.
- Depends On:
-
- Diff:
Revision 3 (+816 -802)
- Diff:
-
Revision 4 (+816 -802)
Checks run (2 succeeded, 1 failed with error)
JSHint
passed.
PEP8 Style Checker
internal error.
Pyflakes
passed.
- Diff:
-
Revision 5 (+890 -802)
Checks run (2 succeeded, 1 failed with error)
JSHint
passed.
PEP8 Style Checker
internal error.
Pyflakes
passed.
- Diff:
-
Revision 6 (+890 -803)
Checks run (2 succeeded, 1 failed with error)
JSHint
passed.
PEP8 Style Checker
internal error.
Pyflakes
passed.
- Change Summary:
-
Addressed David's issues.
- Diff:
-
Revision 7 (+890 -803)
Checks run (2 succeeded, 1 failed with error)
JSHint
passed.
PEP8 Style Checker
internal error.
Pyflakes
passed.
- Change Summary:
-
Force UTF8 charset in content type.
- Diff:
-
Revision 8 (+891 -803)
Checks run (2 succeeded, 1 failed with error)
JSHint
passed.
PEP8 Style Checker
internal error.
Pyflakes
passed.
-
Loving the e-mail changes :) Fantastic cleanup.
-
-
-
-
-
-
-
-
-
-
-
-
If
.target_groupsis already fetched, this is fine. If not, we should use.values_list().(If the former, we should document that.)
-
This won't work. It's missing a second argument.
Good indication that we're missing a test condition or tests aren't passing.
-
-
-
Reads kinda weird. Maybe just "Whether the review should only be sent to the submitter of the review request."
-
-
-
-
-
-
-
-
-
-
-
-
-
-
- Change Summary:
-
Addressed Christian's issues.
- Diff:
-
Revision 9 (+891 -799)
- Change Summary:
-
flake8
- Diff:
-
Revision 10 (+891 -799)
Checks run (2 succeeded, 1 failed with error)
JSHint
passed.
PEP8 Style Checker
internal error.
Pyflakes
passed.
- Diff:
-
Revision 11 (+891 -799)
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.