Summary

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

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

Information

Owner

helder

Repository

Review Board SVN (deprecated)

Branch

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

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?

Testing Done

All tests run.

Summary:

-	Rename review_request_draft_save() to review_request_draft_publish()
+	SoC branch

Description:

	+	I'll track my Summer of Code branch on this review request, keeping a changelog in the description field.
	+
	+	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?

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)

Show changes

	/trunk/reviewboard/reviews/email.py
	/trunk/reviewboard/reviews/tests.py
	/trunk/reviewboard/webapi/json.py
	/trunk/reviewboard/webapi/tests.py
	/trunk/reviewboard/webapi/urls.py

Description:

~		I'll track my Summer of Code branch on this review request, keeping a changelog in the description field.
	~	I'll track my Summer of Code branch on this review request. 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?

Change Summary:

Add mail test to the reviewrequests/reviews/draft/(save|publish) API

Diff:

Revision 3 (+81 -21)

Show changes

	/trunk/reviewboard/reviews/email.py
	/trunk/reviewboard/reviews/tests.py
	/trunk/reviewboard/webapi/json.py
	/trunk/reviewboard/webapi/tests.py
	/trunk/reviewboard/webapi/urls.py

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)

Show changes

	/trunk/reviewboard/reviews/email.py
	/trunk/reviewboard/reviews/tests.py
	/trunk/reviewboard/webapi/json.py
	/trunk/reviewboard/webapi/tests.py
	/trunk/reviewboard/webapi/urls.py

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

/trunk/reviewboard/reviews/email.py (Diff revision 4)
```
Alphabetize these.
```

/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)
```
Add another blank line here.
```

/trunk/reviewboard/reviews/email.py (Diff revision 4)

It seems like this should probably be in reviews.tests

helder July 6, 2009, 8:38 a.m.

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?

/trunk/reviewboard/webapi/tests.py (Diff revision 4)
```
Alphabetize these.
```
/trunk/reviewboard/webapi/tests.py (Diff revision 4)
```
These too.
```
/trunk/reviewboard/webapi/tests.py (Diff revision 4)
```
These two lines could be combined
```

Change Summary:

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

Formatting changes according to review.

Diff:

Revision 5 (+148 -37)

Show changes

	/trunk/reviewboard/reviews/email.py
	/trunk/reviewboard/reviews/models.py
	/trunk/reviewboard/reviews/signals.py
	/trunk/reviewboard/reviews/tests.py
	/trunk/reviewboard/reviews/views.py
	/trunk/reviewboard/webapi/json.py
	/trunk/reviewboard/webapi/tests.py
	/trunk/reviewboard/webapi/urls.py

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)
```
No blank line between these.
```

/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/reviews/views.py (Diff revision 5)
```
Where is this used?
```
/trunk/reviewboard/webapi/json.py (Diff revision 5)
```
Put after the send(
```
/trunk/reviewboard/webapi/json.py (Diff revision 5)
```
And here.
```
/trunk/reviewboard/webapi/json.py (Diff revision 5)
```
And here.
```
/trunk/reviewboard/webapi/tests.py (Diff revision 5)
```
No blank line here.
```
/trunk/reviewboard/webapi/tests.py (Diff revision 5)
```
What's this line for?
```

/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)

Show changes

	/trunk/reviewboard/reviews/email.py
	/trunk/reviewboard/reviews/models.py
	/trunk/reviewboard/reviews/signals.py
	/trunk/reviewboard/reviews/tests.py
	/trunk/reviewboard/webapi/json.py
	/trunk/reviewboard/webapi/tests.py
	/trunk/reviewboard/webapi/urls.py

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:

-	SoC branch
+	Send signals instead of emails. Listen to signals to send emails.

Description:

~		I'll track my Summer of Code branch on this review request. Diff 1 is described here; subsequent diffs will be described in the change description.
	~	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?

```
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.

/trunk/reviewboard/reviews/email.py (Diff revision 6)
```
Please add a docstring.
```
/trunk/reviewboard/reviews/email.py (Diff revision 6)
```
Please add a docstring.
```
/trunk/reviewboard/webapi/tests.py (Diff revision 6)
```
Add a blank line between these two.
```

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.

Diff:

Revision 7 (+469 -337)

Show changes

	/trunk/reviewboard/reviews/email.py
	/trunk/reviewboard/notifications/email.py
	/trunk/reviewboard/notifications/tests.py
	/trunk/reviewboard/reviews/models.py
	/trunk/reviewboard/reviews/signals.py
	/trunk/reviewboard/reviews/tests.py
	/trunk/reviewboard/webapi/json.py
	/trunk/reviewboard/webapi/tests.py
	1 more

Change Summary:

Previous diff had a stupid syntax error (rogue "p" at line ending in "()p").
Fixed now.

Diff:

Revision 8 (+469 -337)

Show changes

	/trunk/reviewboard/reviews/email.py
	/trunk/reviewboard/notifications/email.py
	/trunk/reviewboard/notifications/tests.py
	/trunk/reviewboard/reviews/models.py
	/trunk/reviewboard/reviews/signals.py
	/trunk/reviewboard/reviews/tests.py
	/trunk/reviewboard/webapi/json.py
	/trunk/reviewboard/webapi/tests.py
	1 more

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/email.py (Diff revision 8)
```
No blank line here.
```
/trunk/reviewboard/notifications/tests.py (Diff revision 8)
```
No blank line here.
```

/trunk/reviewboard/notifications/tests.py (Diff revision 8)

Everything should align with the get_email_address_for_user

/trunk/reviewboard/webapi/tests.py (Diff revision 8)
```
No blank line here.
```
/trunk/reviewboard/webapi/urls.py (Diff revision 8)
```
Blank line before the comment.
```

Change Summary:

Formatting fixes.

Diff:

Revision 9 (+467 -337)

Show changes

	/trunk/reviewboard/reviews/email.py
	/trunk/reviewboard/notifications/email.py
	/trunk/reviewboard/notifications/tests.py
	/trunk/reviewboard/reviews/models.py
	/trunk/reviewboard/reviews/signals.py
	/trunk/reviewboard/reviews/tests.py
	/trunk/reviewboard/webapi/json.py
	/trunk/reviewboard/webapi/tests.py
	1 more

```
Looks good to me.
```