flake8
passed.
JSHint
passed.
Include site root in e-mail URLs once
Review Request #9448 — Created Dec. 18, 2017 and submitted
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 athttp://example.com/rb, the URL for
a review request would be generated ashttp://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 ofget_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_urlfor the/URL (which clobbers the site root) and
stripping the trailing/(so we don't get double/in URLs).
Previewed e-mails on a server where the site root was not
/and saw
that the generated URLs were correct.Ran unit tests.
| Description | From | Last Updated |
|---|---|---|
|
Typo in the description on the 3rd line: "exapmle" |
 chipx86
|
|
|
Bug 4612 |
 david
|
|
|
Unit tests are needed for the context variables for these paths, so we can be sure we don't regress again. … |
chipx86
|
|
|
Can we add a unit test for this? |
david
|
|
|
It took me a sec to realize what we were doing here. We're just ensuring we have a URL with … |
chipx86
|
|
|
Add another blank line here. |
david
|
|
|
Swap these. |
chipx86
|
|
|
Given the class name, I think this needs a docstring explaining bug 4612. Ideally, the class name would be more … |
chipx86
|
|
|
This must be in the format of:: ... _Bug 4612: https://.... |
chipx86
|
-
-
-
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.
-
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 fora 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()whichincludes 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 frombuild_server_urlfor the/URL (which clobbers the site root) andstripping the trailing /(so we don't get double/in URLs). - Bugs:
-
- Commit:
b1a08378b6833f3c2d824589a68b5d8425fbcc5fda4c5b41c0f3ccd5e910afd3195facee1775d7be- Diff:
Revision 2 (+368 -33)
Checks run (2 succeeded)
flake8 passed.JSHint passed.
- Commit:
-
da4c5b41c0f3ccd5e910afd3195facee1775d7be55ba541e517909596a69645ad5aee78cb9a59a75
- Diff:
-
Revision 3 (+369 -33)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
Addressed Christian's comments
- Commit:
-
55ba541e517909596a69645ad5aee78cb9a59a7538f8b534e3c3cd83bf4a03f6cedfb406938746cc
- Diff:
-
Revision 4 (+381 -33)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
Addressed Christian's comments
- Commit:
-
38f8b534e3c3cd83bf4a03f6cedfb406938746cc1b1ecabc0105ef7b300a6e633f1d8139e8a2bbae
- Diff:
-
Revision 5 (+381 -33)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.