Summary: |
|
|||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
Send signals instead of emails. Listen to signals to send emails.
Review Request #910 — Created July 2, 2009 and submitted
Information | |
---|---|
helder | |
Review Board SVN (deprecated) | |
Reviewers | |
reviewboard | |
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 better. 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.
Change Summary:
Test if mail is sent from the reviewrequests/draft/publish API. Also, extract assertValidRecipients out of reviews/tests.py to make it reusable by webapi/tests.py
Diff: |
Revision 2 (+73 -21) |
---|
Description: |
|
---|
Change Summary:
Add mail test to the reviewrequests/reviews/draft/(save|publish) API
Diff: |
Revision 3 (+81 -21) |
---|
Change Summary:
Test that reviewrequests/publish API sends email. This is the last test added to the existing code before changing it, so that we ensure it behaves the same afterwards.
Diff: |
Revision 4 (+98 -21) |
---|
-
-
-
/trunk/reviewboard/reviews/email.py (Diff revision 4) I think it would be better to import whatever else you need from mail instead of just importing mail.
-
-
/trunk/reviewboard/reviews/email.py (Diff revision 4) It seems like this should probably be in reviews.tests
-
-
-
Change Summary:
Send signals instead of emails. Listen to signals to send emails. Formatting changes according to review.
Diff: |
Revision 5 (+148 -37) |
---|
-
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.
-
-
/trunk/reviewboard/reviews/email.py (Diff revision 5) Should specifically import the models you need, instead of importing the entire module.
-
/trunk/reviewboard/reviews/email.py (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.
-
/trunk/reviewboard/reviews/email.py (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.
-
/trunk/reviewboard/reviews/models.py (Diff revision 5) Can you put these after send(, breaking across lines and aligning with the first parameter when necessary?
-
/trunk/reviewboard/reviews/signals.py (Diff revision 5) providing_args should be on the same line as Signal. The elements can be broken across lines.
-
/trunk/reviewboard/reviews/tests.py (Diff revision 5) Should be two lines between imports and classes.
-
-
-
-
-
-
-
/trunk/reviewboard/webapi/tests.py (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.
Change Summary:
Fix formatting and smaller details according to code review.
Diff: |
Revision 6 (+140 -35) |
---|
Change Summary:
This review request is now only related to the refactoring part and will not track the whole of my GSoC (won't include the webhooks part). This is self-contained and can be committed sepparetely.
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
-
This is looking pretty close to done.
-
/trunk/reviewboard/reviews/email.py (Diff revision 6) Wrap this to 80 characters. Please also add a docstring.
-
-
-
Change Summary:
Moved email-sending code and tests to /notifications, except for full-stack email tests which stayed in webapi/tests.py. Added docstrings to email-sending signal handlers. Formatting fixes.
Change Summary:
Previous diff had a stupid syntax error (rogue "p" at line ending in "()p"). Fixed now.
Diff: |
Revision 8 (+469 -337)
|
---|
-
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!
-
-
-
/trunk/reviewboard/notifications/tests.py (Diff revision 8) Everything should align with the get_email_address_for_user
-
-
Change Summary:
Formatting fixes.
Diff: |
Revision 9 (+467 -337)
|
---|