Send signals instead of emails. Listen to signals to send emails.

Review Request #910 — Created July 2, 2009 and submitted


Review Board SVN (deprecated)


This is the email refactoring part based on sending and listening to signals. All (core) code related to listening to signals and sending notifications (for now, only by email) will be kept under /notifications. This will pave the way for the introduction of webhooks. 

Diff 1 is described here; subsequent diffs will be described in the change description.

## Diff 1:
Rename review_request_draft_save() to review_request_draft_publish()

Saving is actually already done with review_request_draft_set(). What
this method really does is publishing, so the new name suits it

Question though: what is our deprecation policy? How long do we have to keep the old stuff around? Is there any particular marker in the code using for depracating stuff and notifying users when they use old stuff?
All tests run.
  1. I can't speak to the deprecation strategy because I don't think we've figured one out yet. We don't have a lot of API users, but we should maybe say that we'll mark things as deprecated for at least one major release and then kill them in the next. For our versioning, probably a 1.1 (or 1.5) with things marked and then get rid of it in 2.0
  2. /trunk/reviewboard/reviews/ (Diff revision 4)
    Alphabetize these.
  3. /trunk/reviewboard/reviews/ (Diff revision 4)
    I think it would be better to import whatever else you need from mail instead of just importing mail.
  4. /trunk/reviewboard/reviews/ (Diff revision 4)
    Add another blank line here.
  5. /trunk/reviewboard/reviews/ (Diff revision 4)
    It seems like this should probably be in reviews.tests
    1. Since emails aren't going to be necessarily tied to reviews anymore, I was thinking of putting all this code in a separate top-level emails directory. This way, other apps would only know about their signals, and send them when appropriate. Code in /emails would listen to them and act accordingly, and tests would be there too.
      Tests on the apps would only ensure signals are being sent, while tests on /emails would ensure the proper signals are being listened to and the right emails are being sent. And there could be integration tests somewhere (webapi/tests sounds the most blackbox-y way to go, right?).
      What do you think?
  6. /trunk/reviewboard/webapi/ (Diff revision 4)
    Alphabetize these.
  7. /trunk/reviewboard/webapi/ (Diff revision 4)
    These too.
  8. /trunk/reviewboard/webapi/ (Diff revision 4)
    These two lines could be combined
  1. Some changes, mostly stylistic.
    As for keeping all your changes in one review request, I'd actually rather prefer smaller diffs, each with their own review request. This helps us to keep track of each change, and the status of the change, and allows us to have more granualar commits.
  2. /trunk/reviewboard/reviews/ (Diff revision 5)
    No blank line between these.
  3. /trunk/reviewboard/reviews/ (Diff revision 5)
    Should specifically import the models you need, instead of importing the entire module.
  4. /trunk/reviewboard/reviews/ (Diff revision 5)
    Should be more like:
    def review_request_published_cb(sender, user, review_request, changedesc, **kwargs):
        mail_review_request(user, review_request, changedesc)
    Same with the other callbacks.
  5. /trunk/reviewboard/reviews/ (Diff revision 5)
    Should derive from 'object'.
    Also, this should be part of the test suite, instead of here. It's okay to reference classes in one test suite from another, in the case of webapi.
  6. /trunk/reviewboard/reviews/ (Diff revision 5)
    Can you put these after send(, breaking across lines and aligning with the first parameter when necessary?
  7. /trunk/reviewboard/reviews/ (Diff revision 5)
    providing_args should be on the same line as Signal. The elements can be broken across lines.
  8. /trunk/reviewboard/reviews/ (Diff revision 5)
    Should be two lines between imports and classes.
  9. /trunk/reviewboard/reviews/ (Diff revision 5)
    Where is this used?
  10. /trunk/reviewboard/webapi/ (Diff revision 5)
    Put after the send(
  11. /trunk/reviewboard/webapi/ (Diff revision 5)
    And here.
  12. /trunk/reviewboard/webapi/ (Diff revision 5)
    And here.
  13. /trunk/reviewboard/webapi/ (Diff revision 5)
    No blank line here.
  14. /trunk/reviewboard/webapi/ (Diff revision 5)
    What's this line for?
  15. /trunk/reviewboard/webapi/ (Diff revision 5)
    Store the id in one line, and then call apiPost in another. It'll make it easier to debug if the lookup fails.
  1. This is looking pretty close to done.
  2. /trunk/reviewboard/reviews/ (Diff revision 6)
    Wrap this to 80 characters.
    Please also add a docstring.
  3. /trunk/reviewboard/reviews/ (Diff revision 6)
    Please add a docstring.
  4. /trunk/reviewboard/reviews/ (Diff revision 6)
    Please add a docstring.
  5. /trunk/reviewboard/webapi/ (Diff revision 6)
    Add a blank line between these two.
  1. Almost done here. A couple small spacing changes, mostly. Then come talk to us and we'll get you set up to be able to commit!
  2. /trunk/reviewboard/notifications/ (Diff revision 8)
    No blank line here.
  3. /trunk/reviewboard/notifications/ (Diff revision 8)
    No blank line here.
  4. /trunk/reviewboard/notifications/ (Diff revision 8)
    Everything should align with the get_email_address_for_user
  5. /trunk/reviewboard/webapi/ (Diff revision 8)
    No blank line here.
  6. /trunk/reviewboard/webapi/ (Diff revision 8)
    Blank line before the comment.
Review request changed
  1. Looks good to me.