Refactor e-mail generation and previewing

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

Barret Rennie
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.

  • 0
  • 0
  • 33
  • 10
  • 43
Description From Last Updated
Barret Rennie
Barret Rennie
Barret Rennie
Barret Rennie
Barret Rennie
Barret Rennie
Review request changed

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

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

Pyflakes

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

  3. 
      
Barret Rennie
Barret Rennie
David Trowbridge
  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. 
      
Barret Rennie
David Trowbridge
  1. Ship It!
  2. 
      
Barret Rennie
Christian Hammond
  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. 
      
Barret Rennie
Review request changed

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

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

Pyflakes

Barret Rennie
David Trowbridge
  1. Ship It!
  2. 
      
Christian Hammond
  1. Great cleanup :)

  2. Missing period.

  3. 
      
Barret Rennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Barret Rennie
Review request changed

Status: Closed (submitted)

Change Summary:

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