Render user display names in trophy messages

Review Request #9932 — Created May 11, 2018 and submitted

Information

Review Board
release-3.0.x
0146434...

Reviewers

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.

Ran unit tests.

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, …

chipx86chipx86

"favor"

chipx86chipx86

This is currently checking for its own existence. It should be display_format_str. We can also keep all the legacy support …

chipx86chipx86

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

chipx86chipx86

F401 'reviewboard.accounts.models.Trophy' imported but unused

reviewbotreviewbot

Needs to be in alphabetical order.

chipx86chipx86

F401 'reviewboard.accounts.trophies.MilestoneTrophy' imported but unused

reviewbotreviewbot

F841 local variable 'trophy' is assigned to but never used

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

F401 'warnings.catch_warnings' imported but unused

reviewbotreviewbot

F401 'warnings' imported but unused

reviewbotreviewbot

F401 'warnings.catch_warnings' imported but unused

reviewbotreviewbot
brennie
chipx86
  1. 
      
  2. reviewboard/accounts/trophies.py (Diff revision 1)
     
     
    Show all issues

    "favor"

    1. Respectfully disagree ;) but OK

  3. reviewboard/accounts/trophies.py (Diff revision 1)
     
     
     
     
     
     
    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)
    
    1. 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.

  4. reviewboard/accounts/trophies.py (Diff revision 1)
     
     
     
     
     
    Show all issues

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

  5. 
      
brennie
brennie
Review request changed
Change Summary:

Add more tests

Commit:
f711b850b2226b3db020113065f222045a2a17e6
b00fa01a5554f2cfe683b802a08d7c23789f6816

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
david
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. 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.

  3. reviewboard/accounts/tests.py (Diff revision 4)
     
     
     
    Show all issues

    Needs to be in alphabetical order.

  4. 
      
brennie
Review request changed
Change Summary:

Addressed feedback.

Commit:
541e5d5d9c9fe6915ff869753b4ce1918a4ea698
8326d1628f049b84f29f9bd07443ecba520e87e2

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
Review request changed
Change Summary:

Fix weirdo catch_warnings edge case

Commit:
e1e5c5cffc1c2ae16ee577d3b2dface1751bbe08
5aa4fb701384a5599b6f0ec66578cad27d467948

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
chipx86
  1. Ship It!
  2. 
      
brennie
Review request changed
Status:
Completed
Change Summary:
Pushed to release-3.0.x (d83bdd3)