Summary

Render absolute links in review and review reply e-mails

Review Request #9856 — Created April 4, 2018 and submitted April 6, 2018, 3:36 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Commit

c3cb211...

Reviewers

Groups

reviewboard

People

Description

The commit that incorrectly refactored e-mail templtes to use
build_server_url() over {{domain_method}}://{{domain}} in the e-mail
templates regressed behaviour in the diff fragment template. It did not
actually pass the new site_url context variable to the template.
Additionally, we weren't actually using the context passed to
build_diff_comment_fragments to render the template.

Both issues are now resolved, resulting in correctly-rendered templates.

Testing Done

Viewed review and review reply preview e-mail views and saw absolute
links.

Issues

Description	From	Last Updated
Why not pass site_url to the template so it's consistent?	chipx86	April 4, 2018, 3:02 p.m.

flake8 passed.

JSHint passed.

The issue has been dropped. Show all issues

Why not pass site_url to the template so it's consistent?

brennie April 4, 2018, 2:46 p.m.

It requires modifying this template + the one that the reviews view uses and reviews view. This seemed simpler.

chipx86 April 4, 2018, 3:57 p.m.

Consistency is better. It's less simple to have different conventions for URL building across different e-mail templates.

We already pass extra context to build_diff_comment_fragments() as well.

Description:

		The commit that incorrectly refactored e-mail templtes to use
		`build_server_url()` over `{{domain_method}}://{{domain}}` in the e-mail
~		templates regressed behaviour in the diff fragment template. This
~		template is rendered by
~		`reviewboard.reviews.views.build_diff_comment_fragments`, which still
	~	templates regressed behaviour in the diff fragment template. It did not
	~	actually pass the new `site_url` context variable to the template. We now
	~	do so.
-		passes `domain_method` and `domain` instead of `site_root`.
-
-		The template has been reverted to use the original template variables
-		and e-mails now once again have absolute links for diff fragments.

Commit:

-	5e820c0401111b8b77e65164d56fe14b32dbeffa
+	e483f1b8117ed48d256be0338ae74048ab503cf3

Diff:

Revision 2 (+1)

Show changes

reviewboard/notifications/email/message.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Looks good, assuming you've re-run the testing.

Description:

		The commit that incorrectly refactored e-mail templtes to use
		`build_server_url()` over `{{domain_method}}://{{domain}}` in the e-mail
		templates regressed behaviour in the diff fragment template. It did not
~		actually pass the new `site_url` context variable to the template. We now
~		do so.
	~	actually pass the new `site_url` context variable to the template.
	~	Additionally, we weren't actually using the context passed to
	+	`build_diff_comment_fragments` to render the template.
	+
	+	Both issues are now resolved, resulting in correctly-rendered templates.

Testing Done:

~		Viewed test e-mails and saw absolute links.
	~	Viewed review and review reply preview e-mail views and saw absolute
	+	links.

Commit:

-	e483f1b8117ed48d256be0338ae74048ab503cf3
+	c3cb2116f2c57a29fb25e16f76eaba8681251593

Diff:

Revision 3 (+6 -2)

Show changes

	reviewboard/notifications/email/message.py
	reviewboard/reviews/views.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Summary:

-	Use domain and domain method in e-mail diff fragment template
+	Render absolute links in review and review reply e-mails

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (37ceb44)