Refactor e-mail generation and previewing

Review Request #8885 — Created April 6, 2017 and submitted

brennie
Review Board
release-3.0.x
8897
reviewboard

Previously e-mail generation and previewing used duplicated logic. The
preview_{foo}_email views would duplicate the logic used to generate
the 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 an EMailMessage but 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.

brenniebrennie

I don't think you meant to add reviewboard/notifications/original_cbs.py ?

daviddavid

'reviewboard.notifications.email.decorators.preview_email' imported but unused

reviewbotreviewbot

'reviewboard.notifications.email.message.prepare_password_changed_mail' imported but unused

reviewbotreviewbot

Might be slightly more intuitive as: if not (siteconfig.get('mail_send_review_close_mail') and review_request.public):

daviddavid

While here, can you remove the blank line and fix the ordering?

chipx86chipx86

"djblets"

chipx86chipx86

Let's reverse this check. It's quicker to check a boolean.

chipx86chipx86

No ending paren.

chipx86chipx86

No ending paren.

chipx86chipx86

Let's check trivial first.

chipx86chipx86

I think we have a :http: you can use for HTTP codes.

chipx86chipx86

This is :django:setting:

chipx86chipx86

raise

chipx86chipx86

Let's use .update here.

chipx86chipx86

We can use build_server_url().

chipx86chipx86

If .target_groups is already fetched, this is fine. If not, we should use .values_list(). (If the former, we should document ...

chipx86chipx86

This won't work. It's missing a second argument. Good indication that we're missing a test condition or tests aren't passing.

chipx86chipx86

Can this not be imported higher up?

chipx86chipx86

This could be combined.

chipx86chipx86

Reads kinda weird. Maybe just "Whether the review should only be sent to the submitter of the review request."

chipx86chipx86

Can be inline above in extra_context.

chipx86chipx86

Can now use Python 2.7 set syntax.

chipx86chipx86

Wrapped weird.

chipx86chipx86

Wrapping and paragraph break issues.

chipx86chipx86

Let's use .update({...})

chipx86chipx86

No need for the parens after for.

chipx86chipx86

This can probably be one statement.

chipx86chipx86

"updated"

chipx86chipx86

Capitalize the beginning of each.

chipx86chipx86

Let's put one per line. Helps a little with readability here, given the statement below.

chipx86chipx86

I doubt you want these. We should have a unit test covering this case.

chipx86chipx86

2 blank lines.

chipx86chipx86

Blank line should remain.

chipx86chipx86

Trailing comma.

chipx86chipx86

local variable 'e' is assigned to but never used

reviewbotreviewbot

Missing period.

chipx86chipx86

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot
brennie
brennie
brennie
brennie
brennie
brennie
Review request changed

Checks run (1 failed, 1 succeeded, 1 failed with error)

JSHint passed.
PEP8 Style Checker internal error.
Pyflakes failed.

Pyflakes

brennie
brennie
  1. 
      
  2. email/decorators.py should be in this diff.

  3. 
      
brennie
brennie
david
  1. 
      
  2. I don't think you meant to add reviewboard/notifications/original_cbs.py ?

  3. Might be slightly more intuitive as:

    if not (siteconfig.get('mail_send_review_close_mail') and
            review_request.public):
    
  4. 
      
brennie
david
  1. Ship It!
  2. 
      
brennie
chipx86
  1. Loving the e-mail changes :) Fantastic cleanup.

  2. reviewboard/notifications/email/__init__.py (Diff revision 8)
     
     
     
     

    While here, can you remove the blank line and fix the ordering?

    1. I think this was already fixed but didnt show up in the diff? :/ Its not in my local copy.

  3. Let's reverse this check. It's quicker to check a boolean.

  4. No ending paren.

  5. No ending paren.

  6. Let's check trivial first.

  7. I think we have a :http: you can use for HTTP codes.

  8. This is :django:setting:

  9. reviewboard/notifications/email/message.py (Diff revision 8)
     
     
     
     

    Let's use .update here.

  10. reviewboard/notifications/email/message.py (Diff revision 8)
     
     
     

    We can use build_server_url().

  11. reviewboard/notifications/email/message.py (Diff revision 8)
     
     
     
     
     

    If .target_groups is already fetched, this is fine. If not, we should use .values_list().

    (If the former, we should document that.)

  12. This won't work. It's missing a second argument.

    Good indication that we're missing a test condition or tests aren't passing.

  13. Can this not be imported higher up?

    1. Nope, ciruclar import. reviewboard.reviews.views imports email.messages.prepare_{} funcs.

  14. reviewboard/notifications/email/message.py (Diff revision 8)
     
     
     
     

    This could be combined.

    1. No they cannot. This requires extra_context.

  15. Reads kinda weird. Maybe just "Whether the review should only be sent to the submitter of the review request."

  16. reviewboard/notifications/email/message.py (Diff revision 8)
     
     
     
     

    Can be inline above in extra_context.

    1. Nope. Requires extra_context to build it.

  17. Can now use Python 2.7 set syntax.

  18. reviewboard/notifications/email/message.py (Diff revision 8)
     
     
     

    Wrapped weird.

  19. reviewboard/notifications/email/message.py (Diff revision 8)
     
     
     
     
     
     
     
     
     
     

    Wrapping and paragraph break issues.

    1. Reads fine to me, I don't know what you're talking about :P

  20. reviewboard/notifications/email/message.py (Diff revision 8)
     
     
     
     

    Let's use .update({...})

  21. No need for the parens after for.

  22. reviewboard/notifications/email/message.py (Diff revision 8)
     
     
     
     

    This can probably be one statement.

  23. reviewboard/notifications/email/utils.py (Diff revision 8)
     
     
     

    Capitalize the beginning of each.

  24. Let's put one per line. Helps a little with readability here, given the statement below.

  25. reviewboard/notifications/email/utils.py (Diff revision 8)
     
     
     

    I doubt you want these. We should have a unit test covering this case.

  26. reviewboard/reviews/views.py (Diff revision 8)
     
     
     
     
     

    2 blank lines.

  27. reviewboard/reviews/views.py (Diff revision 8)
     
     
     
     

    Blank line should remain.

  28. reviewboard/reviews/views.py (Diff revision 8)
     
     

    Trailing comma.

  29. 
      
brennie
Review request changed

Checks run (1 failed, 1 succeeded, 1 failed with error)

JSHint passed.
PEP8 Style Checker internal error.
Pyflakes failed.

Pyflakes

brennie
david
  1. Ship It!
  2. 
      
chipx86
  1. Great cleanup :)

  2. Missing period.

  3. 
      
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (4b6a38f)
Loading...