Summary

Include site root in e-mail URLs once

Review Request #9448 — Created Dec. 18, 2017 and submitted Jan. 8, 2018, 7:25 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-3.0.x

Bugs

4612

Depends On

Commit

1b1ecab...

Reviewers

Groups

reviewboard

People

Description

The refactoring of e-mails in Review Board 3.0 ended up causing a
regression where URLs in e-mails would have the site root applied twice.
For example, for a server hosted at http://example.com/rb, the URL for
a review request would be generated as http://example.com/rb/rb/r/1.
This was due to no longer computing the site URL (which was the scheme +
domain only), but instead using the result of get_server_url() which
includes the site root. This regression wasn't apparent due to a lack of
testing on configurations where the site root was anything other than
/.

Now, we pass in the correct value for site_url, computing it from
build_server_url for the / URL (which clobbers the site root) and
stripping the trailing / (so we don't get double / in URLs).

Testing Done

Previewed e-mails on a server where the site root was not / and saw
that the generated URLs were correct.

Ran unit tests.

Issues

Description	From	Last Updated
Typo in the description on the 3rd line: "exapmle"	chipx86	Dec. 18, 2017, 6:05 p.m.
Bug 4612	david	Dec. 18, 2017, 6:06 p.m.
Unit tests are needed for the context variables for these paths, so we can be sure we don't regress again. …	chipx86	Dec. 19, 2017, 3:06 p.m.
Can we add a unit test for this?	david	Dec. 19, 2017, 3:06 p.m.
It took me a sec to realize what we were doing here. We're just ensuring we have a URL with …	chipx86	Dec. 19, 2017, 3:06 p.m.
Add another blank line here.	david	Dec. 20, 2017, 4:44 p.m.
Swap these.	chipx86	Dec. 21, 2017, 2:52 p.m.
Given the class name, I think this needs a docstring explaining bug 4612. Ideally, the class name would be more …	chipx86	Dec. 21, 2017, 2:52 p.m.
This must be in the format of:: ... _Bug 4612: https://....	chipx86	Jan. 8, 2018, 7:15 p.m.

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues
```
Bug 4612
```
The issue has been resolved. Show all issues
```
Can we add a unit test for this?
```

The issue has been resolved. Show all issues

Typo in the description on the 3rd line: "exapmle"

The issue has been resolved. Show all issues

Unit tests are needed for the context variables for these paths, so we can be sure we don't regress again. They should test with/without site roots and with/without local sites.

reviewboard/notifications/email/message.py (Diff revision 1)

The issue has been resolved. Show all issues

It took me a sec to realize what we were doing here. We're just ensuring we have a URL with a / at the end, and then stripping that away, right?
Can you make this a utility function in this file so that we don't repeat the logic, and so that there's a docstring that describes exactly what we're doing?

Change Summary:

Added unit tests.

Description:

		The refactoring of e-mails in Review Board 3.0 ended up causing a
		regression where URLs in e-mails would have the site root applied twice.
~		For exapmle, for a server hosted at `http://example.com/rb`, the URL for
	~	For example, for a server hosted at `http://example.com/rb`, the URL for
		a review request would be generated as `http://example.com/rb/rb/r/1`.
		This was due to no longer computing the site URL (which was the scheme +
		domain only), but instead using the result of `get_server_url()` which
		includes the site root. This regression wasn't apparent due to a lack of
		testing on configurations where the site root was anything other than
		`/`.

		Now, we pass in the correct value for `site_url`, computing it from
		`build_server_url` for the `/` URL (which clobbers the site root) and
		stripping the trailing `/` (so we don't get double `/` in URLs).

Bugs:

4612

Commit:

b1a08378b6833f3c2d824589a68b5d8425fbcc5f

da4c5b41c0f3ccd5e910afd3195facee1775d7be

Diff:

Revision 2 (+368 -33)

Show changes

	reviewboard/notifications/tests.py
	reviewboard/notifications/email/message.py
	reviewboard/templates/notifications/api_token_created.html
	reviewboard/templates/notifications/api_token_deleted.html
	reviewboard/templates/notifications/api_token_updated.html
	reviewboard/templates/notifications/review_email.txt

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Testing Done:: Previewed e-mails on a server where the site root was not / and saw
that the generated URLs were correct.
+
+
Ran unit tests.

reviewboard/notifications/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Add another blank line here.
```

Commit:

da4c5b41c0f3ccd5e910afd3195facee1775d7be

55ba541e517909596a69645ad5aee78cb9a59a75

Diff:

Revision 3 (+369 -33)

Show changes

	reviewboard/notifications/tests.py
	reviewboard/notifications/email/message.py
	reviewboard/templates/notifications/api_token_created.html
	reviewboard/templates/notifications/api_token_deleted.html
	reviewboard/templates/notifications/api_token_updated.html
	reviewboard/templates/notifications/review_email.txt

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/notifications/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Swap these.
```

reviewboard/notifications/tests.py (Diff revision 3)

The issue has been resolved. Show all issues

Given the class name, I think this needs a docstring explaining bug 4612. Ideally, the class name would be more self-describing.
Same in other places where this is mentioned.

Change Summary:

Addressed Christian's comments

Commit:

55ba541e517909596a69645ad5aee78cb9a59a75

38f8b534e3c3cd83bf4a03f6cedfb406938746cc

Diff:

Revision 4 (+381 -33)

Show changes

	reviewboard/notifications/tests.py
	reviewboard/notifications/email/message.py
	reviewboard/templates/notifications/api_token_created.html
	reviewboard/templates/notifications/api_token_deleted.html
	reviewboard/templates/notifications/api_token_updated.html
	reviewboard/templates/notifications/review_email.txt

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

reviewboard/notifications/tests.py (Diff revision 4)

The issue has been resolved. Show all issues

This must be in the format of::

... _Bug 4612: https://....

Change Summary:

Addressed Christian's comments

Commit:

38f8b534e3c3cd83bf4a03f6cedfb406938746cc

1b1ecabc0105ef7b300a6e633f1d8139e8a2bbae

Diff:

Revision 5 (+381 -33)

Show changes

	reviewboard/notifications/tests.py
	reviewboard/notifications/email/message.py
	reviewboard/templates/notifications/api_token_created.html
	reviewboard/templates/notifications/api_token_deleted.html
	reviewboard/templates/notifications/api_token_updated.html
	reviewboard/templates/notifications/review_email.txt

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Status:: Completed
Change Summary:: Pushed to release-3.0.x (cbc43f9)

		Previewed e-mails on a server where the site root was not `/` and saw
		that the generated URLs were correct.
	+
	+	Ran unit tests.