Render user display names in trophy messages

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

brennie
Review Board
release-3.0.x
9924
0146434...
reviewboard

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)
     
     

    "favor"

    1. Respectfully disagree ;) but OK

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

    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)
     
     
     
     
     

    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

Diff:

Revision 3 (+165 -34)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

    Needs to be in alphabetical order.

  4. 
      
brennie
Review request changed

Change Summary:

Addressed feedback.

Commit:

-541e5d5d9c9fe6915ff869753b4ce1918a4ea698
+8326d1628f049b84f29f9bd07443ecba520e87e2

Diff:

Revision 5 (+175 -33)

Show changes

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

Diff:

Revision 7 (+163 -33)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
chipx86
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (d83bdd3)
Loading...