Summary

Render user display names in trophy messages

Review Request #9932 — Created May 11, 2018 and submitted May 23, 2018, 11:55 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

~~9924~~

Commit

0146434...

Reviewers

Groups

reviewboard

People

Description

Trophy message rendering has been reworked. Instead of a method that
every TrophyType subclass must implement, instead they must only add a
format string attribute. By default, the recipient's name is the only
provided parameter, but TrophyType.format_display_text can be
overridden to add extra context. All builtin trophies have been updated
to use this new behaviour.

Testing Done

Ran unit tests.

Issues

Description	From	Last Updated
I think there was a misunderstanding on what I was saying regarding legacy support. if a trophy only implements get_display_text, …	chipx86	May 23, 2018, 1:27 p.m.
"favor"	chipx86	May 15, 2018, 1:22 p.m.
This is currently checking for its own existence. It should be display_format_str. We can also keep all the legacy support …	chipx86	May 15, 2018, 1:24 p.m.
I think the review request ID should be a standard thing passed. This lets us simplify all subclasses.	chipx86	May 15, 2018, 1:25 p.m.
F401 'reviewboard.accounts.models.Trophy' imported but unused	reviewbot	May 15, 2018, 3:03 p.m.
Needs to be in alphabetical order.	chipx86	May 23, 2018, 12:38 p.m.
F401 'reviewboard.accounts.trophies.MilestoneTrophy' imported but unused	reviewbot	May 23, 2018, 3:35 p.m.
F841 local variable 'trophy' is assigned to but never used	reviewbot	May 23, 2018, 3:35 p.m.
E303 too many blank lines (2)	reviewbot	May 23, 2018, 3:35 p.m.
F401 'warnings.catch_warnings' imported but unused	reviewbot	May 23, 2018, 10:13 p.m.
F401 'warnings' imported but unused	reviewbot	May 23, 2018, 10:13 p.m.
F401 'warnings.catch_warnings' imported but unused	reviewbot	May 23, 2018, 10:13 p.m.

flake8 passed.

JSHint passed.

Description:

		Trophy message rendering has been reworked. Instead of a method that
		every `TrophyType` subclass must implement, instead they must only add a
		format string attribute. By default, the recipient's name is the only
		provided parameter, but `TrophyType.format_display_text` can be
		overridden to add extra context. All builtin trophies have been updated
		to use this new behaviour.
	+
	+	TODO: Add unit tests for future proofing the old-style trophies.

reviewboard/accounts/trophies.py (Diff revision 1)
The issue has been resolved. Show all issues
```
"favor"
```
1. brennie May 15, 2018, 11:44 a.m.
  Respectfully disagree ;) but OK

reviewboard/accounts/trophies.py (Diff revision 1)

The issue has been resolved. Show all issues

This is currently checking for its own existence. It should be display_format_str.
We can also keep all the legacy support confined here, reducing the work needed in the caller, by doing:

if self.display_format_str is None:
    warnings.warn(...)
    return self.get_display_text(trophy)

brennie May 15, 2018, 1:24 p.m.

Re: keeping the legacy support there, not having the attribute set (which may be due to a typo in the attr name) is different from calling the old function.

reviewboard/accounts/trophies.py (Diff revision 1)

The issue has been resolved. Show all issues

I think the review request ID should be a standard thing passed. This lets us simplify all subclasses.

Change Summary:

Addressed feedback.

Commit:

-	91831baf554be59df7a4bf0a91f9c3d178a87dae
+	f711b850b2226b3db020113065f222045a2a17e6

Diff:

Revision 2 (+57 -34)

Show changes

	reviewboard/accounts/trophies.py
	reviewboard/reviews/templatetags/reviewtags.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Add more tests

Commit:

-	f711b850b2226b3db020113065f222045a2a17e6
+	b00fa01a5554f2cfe683b802a08d7c23789f6816

Diff:

Revision 3 (+165 -34)

Show changes

	reviewboard/accounts/tests.py
	reviewboard/accounts/trophies.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_reviewtags.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/tests/test_reviewtags.py (Diff revision 3)
The issue has been resolved. Show all issues
```
F401 'reviewboard.accounts.models.Trophy' imported but unused
```

Description:

		Trophy message rendering has been reworked. Instead of a method that
		every `TrophyType` subclass must implement, instead they must only add a
		format string attribute. By default, the recipient's name is the only
		provided parameter, but `TrophyType.format_display_text` can be
		overridden to add extra context. All builtin trophies have been updated
		to use this new behaviour.
-
-		TODO: Add unit tests for future proofing the old-style trophies.

Change Summary:

Remove unused import.

Commit:

-	b00fa01a5554f2cfe683b802a08d7c23789f6816
+	541e5d5d9c9fe6915ff869753b4ce1918a4ea698

Diff:

Revision 4 (+164 -34)

Show changes

	reviewboard/accounts/tests.py
	reviewboard/accounts/trophies.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_reviewtags.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

The issue has been resolved. Show all issues

I think there was a misunderstanding on what I was saying regarding legacy support.

if a trophy only implements get_display_text, that should continue to work. This isn't about calling get_display_text and forwarding on to the new method. It's about the new method defaulting to calling the old one.

reviewboard/accounts/tests.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Needs to be in alphabetical order.
```

Change Summary:

Addressed feedback.

Commit:

-	541e5d5d9c9fe6915ff869753b4ce1918a4ea698
+	8326d1628f049b84f29f9bd07443ecba520e87e2

Diff:

Revision 5 (+175 -33)

Show changes

	reviewboard/accounts/tests.py
	reviewboard/accounts/trophies.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_reviewtags.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/accounts/tests.py (Diff revision 5)
The issue has been resolved. Show all issues
```
F401 'reviewboard.accounts.trophies.MilestoneTrophy' imported but unused
```
reviewboard/accounts/tests.py (Diff revision 5)
The issue has been resolved. Show all issues
```
F841 local variable 'trophy' is assigned to but never used
```
reviewboard/accounts/tests.py (Diff revision 5)
The issue has been resolved. Show all issues
```
E303 too many blank lines (2)
```

Change Summary:

Flake8 fixups

Commit:

-	8326d1628f049b84f29f9bd07443ecba520e87e2
+	e1e5c5cffc1c2ae16ee577d3b2dface1751bbe08

Diff:

Revision 6 (+174 -33)

Show changes

	reviewboard/accounts/tests.py
	reviewboard/accounts/trophies.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_reviewtags.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Fix weirdo catch_warnings edge case

Commit:

-	e1e5c5cffc1c2ae16ee577d3b2dface1751bbe08
+	5aa4fb701384a5599b6f0ec66578cad27d467948

Diff:

Revision 7 (+163 -33)

Show changes

	reviewboard/accounts/tests.py
	reviewboard/accounts/trophies.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_reviewtags.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/accounts/tests.py (Diff revision 7)
The issue has been resolved. Show all issues
```
F401 'warnings.catch_warnings' imported but unused
```
reviewboard/reviews/tests/test_reviewtags.py (Diff revision 7)
The issue has been resolved. Show all issues
```
F401 'warnings' imported but unused
```
reviewboard/reviews/tests/test_reviewtags.py (Diff revision 7)
The issue has been resolved. Show all issues
```
F401 'warnings.catch_warnings' imported but unused
```

Change Summary:

Flake8 fixups

Commit:

-	5aa4fb701384a5599b6f0ec66578cad27d467948
+	0146434a9b388396fc31ca54b2af0582fc2a3bac

Diff:

Revision 8 (+156 -33)

Show changes

	reviewboard/accounts/tests.py
	reviewboard/accounts/trophies.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_reviewtags.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (d83bdd3)