Summary

Trophy case 3,4

Review Request #3878 — Created Feb. 17, 2013 and discarded Aug. 2, 2013, 7:17 p.m.

Information

Owner

yuri_mingo

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

https://hackpad.com/Trophy-Case-Design-MK30awhb3nL

Testing Done

(1)
unit test, named test_importing_trophies, do follow things.
1.Add User object named user1.
2.Add 2 reveiw requests associated with user1, display_id=1000 for Milestone Trophy and display_id=1221 for Palindrome Trophy.
3.Compute user1's all trophies.
And, this code passed test.

(2)
I made sure profile.extra_data works.
I set the pdb.set_trace in "if not extra_data.get('has_trophy_case', False)", then I tried to access twice and I checked that pdb is called only once.

Files

Issues

Description	From	Last Updated
Instead of display (id: 1), I think these should link to the review requests that caused the reward. Maybe something …	mike_conley	March 29, 2013, 9:28 p.m.
This is looking much, much better! Another suggestion - please remove the quotes, and use the trophy title instead of …	mike_conley	March 29, 2013, 10:04 p.m.
Col: 80 E501 line too long (101 > 79 characters)	reviewbot	Feb. 18, 2013, 5:21 p.m.
Col: 20 E203 whitespace before '	reviewbot	Feb. 18, 2013, 5:20 p.m.
Apparently, `written by Hiro` is not a good class description	GR gregwym	Feb. 22, 2013, 9:57 a.m.
Col: 5 E303 too many blank lines (2)	reviewbot	Feb. 18, 2013, 5:20 p.m.
Col: 13 E201 whitespace after '('	reviewbot	Feb. 18, 2013, 5:20 p.m.
Col: 18 E202 whitespace before ')'	reviewbot	Feb. 18, 2013, 5:20 p.m.
You sure you want to name a function as `newfunc`? It is not a descriptive name.	GR gregwym	Feb. 22, 2013, 9:57 a.m.
Col: 35 E201 whitespace after '('	reviewbot	Feb. 18, 2013, 5:20 p.m.
Col: 68 E202 whitespace before ')'	reviewbot	Feb. 18, 2013, 5:20 p.m.
Col: 22 E201 whitespace after '('	reviewbot	Feb. 18, 2013, 5:20 p.m.
Col: 26 E202 whitespace before ')'	reviewbot	Feb. 18, 2013, 5:21 p.m.
I think we use `` instead of ``. Besides, I wonder whether using is a good option. In my opinion, …	GR gregwym	Feb. 20, 2013, 7:34 a.m.
Col: 69 W291 trailing whitespace	reviewbot	Feb. 22, 2013, 3:08 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Feb. 22, 2013, 3:08 p.m.
Col: 1 W391 blank line at end of file	reviewbot	Feb. 22, 2013, 3:08 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Feb. 22, 2013, 3:08 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Feb. 22, 2013, 3:08 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Feb. 22, 2013, 3:08 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Feb. 22, 2013, 3:08 p.m.
Col: 1 W391 blank line at end of file	reviewbot	Feb. 22, 2013, 3:08 p.m.
Col: 1 W391 blank line at end of file	reviewbot	March 2, 2013, 11:59 a.m.
Col: 16 E711 comparison to None should be 'if cond is None	reviewbot	March 2, 2013, noon
Col: 1 W293 blank line contains whitespace	reviewbot	March 2, 2013, noon
Col: 25 E128 continuation line under-indented for visual indent	reviewbot	March 2, 2013, 12:06 p.m.
Col: 25 E128 continuation line under-indented for visual indent	reviewbot	March 2, 2013, 12:06 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	March 2, 2013, 12:06 p.m.
Col: 5 E303 too many blank lines (2)	reviewbot	March 2, 2013, 12:07 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	March 2, 2013, 12:07 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	March 2, 2013, 12:07 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	March 2, 2013, 12:07 p.m.
Col: 1 W391 blank line at end of file	reviewbot	March 2, 2013, 12:07 p.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	March 2, 2013, 12:13 p.m.
Col: 80 E501 line too long (92 > 79 characters)	reviewbot	March 2, 2013, 12:13 p.m.
Col: 43 W291 trailing whitespace	reviewbot	March 2, 2013, 12:13 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	March 21, 2013, 7:26 p.m.
Col: 9 E221 multiple spaces before operator	reviewbot	March 21, 2013, 7:26 p.m.
Col: 28 E127 continuation line over-indented for visual indent	reviewbot	March 21, 2013, 7:26 p.m.
Col: 18 E128 continuation line under-indented for visual indent	reviewbot	March 21, 2013, 7:27 p.m.
Col: 18 E128 continuation line under-indented for visual indent	reviewbot	March 21, 2013, 7:27 p.m.
Col: 16 E128 continuation line under-indented for visual indent	reviewbot	March 21, 2013, 7:27 p.m.
Col: 16 E128 continuation line under-indented for visual indent	reviewbot	March 21, 2013, 7:27 p.m.
Col: 11 E128 continuation line under-indented for visual indent	reviewbot	March 21, 2013, 7:28 p.m.
Col: 11 E128 continuation line under-indented for visual indent	reviewbot	March 21, 2013, 7:28 p.m.
Col: 11 E128 continuation line under-indented for visual indent	reviewbot	March 21, 2013, 7:28 p.m.
Col: 23 E203 whitespace before '	reviewbot	March 21, 2013, 7:29 p.m.
Col: 80 E501 line too long (105 > 79 characters)	reviewbot	March 21, 2013, 7:30 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	March 21, 2013, 7:30 p.m.
Col: 23 E203 whitespace before '	reviewbot	March 21, 2013, 7:31 p.m.
Col: 12 E225 missing whitespace around operator	reviewbot	March 21, 2013, 7:31 p.m.
Col: 20 E226 missing optional whitespace around operator	reviewbot	March 21, 2013, 7:31 p.m.
Col: 24 E225 missing whitespace around operator	reviewbot	March 21, 2013, 7:31 p.m.
Col: 35 E225 missing whitespace around operator	reviewbot	March 21, 2013, 7:31 p.m.
Col: 38 E226 missing optional whitespace around operator	reviewbot	March 21, 2013, 7:31 p.m.
Col: 41 E226 missing optional whitespace around operator	reviewbot	March 21, 2013, 7:31 p.m.
Col: 44 E226 missing optional whitespace around operator	reviewbot	March 21, 2013, 7:31 p.m.
Col: 53 E226 missing optional whitespace around operator	reviewbot	March 21, 2013, 7:31 p.m.
Col: 55 E226 missing optional whitespace around operator	reviewbot	March 21, 2013, 7:31 p.m.
Col: 69 E225 missing whitespace around operator	reviewbot	March 21, 2013, 7:31 p.m.
Col: 12 E225 missing whitespace around operator	reviewbot	March 21, 2013, 7:31 p.m.
Col: 21 E225 missing whitespace around operator	reviewbot	March 21, 2013, 7:31 p.m.
Col: 24 E226 missing optional whitespace around operator	reviewbot	March 21, 2013, 7:31 p.m.
Col: 27 E226 missing optional whitespace around operator	reviewbot	March 21, 2013, 7:31 p.m.
Col: 30 E226 missing optional whitespace around operator	reviewbot	March 21, 2013, 7:31 p.m.
Col: 39 E226 missing optional whitespace around operator	reviewbot	March 21, 2013, 7:31 p.m.
Col: 41 E225 missing whitespace around operator	reviewbot	March 21, 2013, 7:31 p.m.
Col: 23 E203 whitespace before '	reviewbot	March 21, 2013, 7:39 p.m.
Col: 80 E501 line too long (105 > 79 characters)	reviewbot	March 21, 2013, 7:44 p.m.
Col: 18 E203 whitespace before '	reviewbot	March 22, 2013, 6:41 p.m.
I don't see anything about this in the spec. Not sure we need or want a page showing all the …	chipx86	March 22, 2013, 6:37 p.m.
Col: 80 E501 line too long (105 > 79 characters)	reviewbot	March 22, 2013, 6:41 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	March 22, 2013, 8:14 p.m.
Call this "import_trophies" instead.	chipx86	March 24, 2013, 8:54 a.m.
Blank line between these.	chipx86	March 24, 2013, 8:54 a.m.
This is going to be very, very slow, because we're loading every single review request from the database that the …	chipx86	March 24, 2013, 9:51 a.m.
Blank line between these.	chipx86	March 24, 2013, 9:01 a.m.
You're never actually saving the profile.	chipx86	March 24, 2013, 9:02 a.m.
I don't think you want to do a copy. What's it for?	chipx86	March 24, 2013, 11:31 a.m.
Blank line before this. This can be simplified to: if not extra_data.get('has_trophy_case', False):	chipx86	March 24, 2013, 9:04 a.m.
Blank line between these.	chipx86	March 24, 2013, 9:04 a.m.
I'm not really sure what this is doing, but a better way to write it would be: trophy_list = [ …	chipx86	March 24, 2013, 10:02 a.m.
Col: 1 W293 blank line contains whitespace	reviewbot	March 24, 2013, 8:51 a.m.
This is going to look weird if it's only 1 trophy, since it will say: "Got 1 trophies". Instead, you …	chipx86	March 24, 2013, 9:13 a.m.
CSS should never go in HTML directly.	chipx86	March 24, 2013, 10:06 a.m.
Indentation issue.	chipx86	March 24, 2013, 10:06 a.m.
Does it not work to pass this variable to "static" below. instead of using definevar?	chipx86	March 24, 2013, 10:11 a.m.
This line is pretty long. I don't think we want this entire bit of logic either. If you have hundreds …	chipx86	March 24, 2013, 11:27 a.m.
I think this should just be "test_importing_trophies"	mike_conley	March 26, 2013, 7:31 a.m.
Let's put these in alphabetical order, so Profile, ReviewRequestVisit, Trophy.	mike_conley	March 26, 2013, 7:32 a.m.
You should use Trophy.objects.get_trophy(trophy.trophy_type) Callers shouldn't know (or care) that the manager holds the trophies in a trophy_dict.	mike_conley	March 26, 2013, 9:06 a.m.
Trailing whitespace	mike_conley	March 26, 2013, 7:30 a.m.
Space before "none"	mike_conley	March 26, 2013, 7:30 a.m.
Should be "Has gotten {{user_trophies}} troph{{ user_trophies\|pluralize:"y,ies" }}.	mike_conley	March 26, 2013, 7:28 a.m.
I'm confused - we appear to be showing a user's trophies twice - once in the header in a big …	mike_conley	March 26, 2013, 7:29 a.m.
The alt tag should be the trophy title	mike_conley	March 26, 2013, 7:26 a.m.
Remove this trailing whitespace.	mike_conley	March 26, 2013, 7:26 a.m.
This function is not up to date with what Hiro has in http://reviews.reviewboard.org/r/3905/ I think you need to merge in …	mike_conley	March 27, 2013, 10:25 a.m.
This should use Hiro's get_trophy, not getAbstractTrophy.	mike_conley	March 27, 2013, 10:25 a.m.
I'm confused, why is this still here? In my last review, I wrote: "...we appear to be showing a user's …	mike_conley	March 27, 2013, 10:31 a.m.
Is 'trophy' ever used? I don't think it is - if not, please remove it.	mike_conley	March 29, 2013, 5:48 p.m.
trophy-box is used by user_trophies.html, which is no longer being used. I think this set of rules can be removed.	mike_conley	March 29, 2013, 5:54 p.m.
The alignment of the HTML tags needs to be corrected. Also, I'd like you to post a screenshot showing what …	mike_conley	March 29, 2013, 9:27 p.m.
This file isn't being used anymore and can be removed.	mike_conley	March 29, 2013, 5:58 p.m.
Clearer documentation would be, """Compute the trophies awarded to a user."""	mike_conley	March 31, 2013, 12:03 a.m.
I think the alt tag should be the trophy description.	mike_conley	March 31, 2013, 12:02 a.m.
These need to be removed.	mike_conley	March 31, 2013, 1:40 a.m.
Hm, this shouldn't be here. You should increase the margin-bottom of the instead. Maybe something like: .trophy-info h1 { margin-bottom: …	mike_conley	March 31, 2013, 2:29 a.m.
This isn't the right way to link to a review request. It should be like this: {% blocktrans with trophy_data.trophy.review_request.display_id …	mike_conley	March 31, 2013, 2:29 a.m.
This newline should go back.	mike_conley	March 31, 2013, 2:56 a.m.
Break up this long line by putting the tag on the next line, and the {% endblocktrans %} on its …	mike_conley	March 31, 2013, 2:44 a.m.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/accounts/managers.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/accounts/evolutions/trophies.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/accounts/models.py
  Ignored Files:
    reviewboard/templates/reviews/user_page.html

reviewboard/accounts/evolutions/trophies.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (101 > 79 characters)
```
reviewboard/accounts/managers.py (Diff revision 1)
Show all issues
```
Col: 20
 E203 whitespace before '
```
reviewboard/accounts/managers.py (Diff revision 1)
Show all issues
```
Col: 5
 E303 too many blank lines (2)
```
reviewboard/accounts/managers.py (Diff revision 1)
Show all issues
```
Col: 13
 E201 whitespace after '('
```
reviewboard/accounts/managers.py (Diff revision 1)
Show all issues
```
Col: 18
 E202 whitespace before ')'
```
reviewboard/accounts/managers.py (Diff revision 1)
Show all issues
```
Col: 35
 E201 whitespace after '('
```
reviewboard/accounts/managers.py (Diff revision 1)
Show all issues
```
Col: 68
 E202 whitespace before ')'
```
reviewboard/accounts/managers.py (Diff revision 1)
Show all issues
```
Col: 22
 E201 whitespace after '('
```
reviewboard/accounts/managers.py (Diff revision 1)
Show all issues
```
Col: 26
 E202 whitespace before ')'
```

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

Show all issues

Apparently, `written by Hiro` is not a good class description

yuri_mingo Feb. 20, 2013, 7:34 a.m.

thanks for comments but it's not description. Note to self.
I'll fix later!

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

Show all issues

You sure you want to name a function as `newfunc`? It is not a descriptive name.

YU

yuri_mingo Feb. 20, 2013, 7:34 a.m.
```
No, of course not. 
```

reviewboard/templates/reviews/user_page.html (Diff revision 1)

Show all issues

I think we use `<br />` instead of `<br>`. 

Besides, I wonder whether using <br> is a good option. 
In my opinion, a `<ul>` with `list-style-type:none` works better than line-breaks.

YU

yuri_mingo Feb. 20, 2013, 7:34 a.m.
```
Great. Thanks.
```

Change Summary:

[accounts/managers.py]
    *Add compute_user_all_trophies(user) to TrophyManager class.
    *Hiro gonna create TrophyManager class and add compute_trophies(rid) func later.
    
[accounts/models.py]
    *Import TrophyManager and add it to Trophy class

[reviews/templatetags/reviewtags.py]
    *Import Trophy and create compute_user_all_trophies(user) func.
    *This func calls TrophyManager's function(see accounts/managers.py).
    
[templates/reviews/user_page.html]
    *add a template tag for call compute_user_all_trophies(user).
    *fix the list of trophies ... (<br>  -->  <ul> and <li>)

[accounts/tests.py]
    *add test_wip(self), which is assumed name.
    *test_wip(self) creates user, creates some review requests, publishes them and erases the
m.

Diff:

Revision 2 (+76 -2)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/trophies.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/templates/reviews/user_page.html

Fix it!

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/evolutions/trophies.py
    reviewboard/accounts/tests.py
  Ignored Files:
    reviewboard/templates/reviews/user_page.html

reviewboard/accounts/evolutions/trophies.py (Diff revision 2)
Show all issues
```
Col: 69
 W291 trailing whitespace
```
reviewboard/accounts/managers.py (Diff revision 2)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/accounts/managers.py (Diff revision 2)
Show all issues
```
Col: 1
 W391 blank line at end of file
```
reviewboard/accounts/managers.py (Diff revision 2)
Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
reviewboard/accounts/tests.py (Diff revision 2)
Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
reviewboard/accounts/tests.py (Diff revision 2)
Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
reviewboard/accounts/tests.py (Diff revision 2)
Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
reviewboard/accounts/tests.py (Diff revision 2)
Show all issues
```
Col: 1
 W391 blank line at end of file
```

Change Summary:

*add some code to user_page.html for show all trophies(review request ID) stored in the database.
*keep Hiroki's model up to date. 2013-02-27.
*call the compute_user_all_trophies, which is func of [accounts/managers.py], when user access user_page.html.(see view.py)
*move JSONField from LocalSiteProfile class to Profile class.

Diff:

Revision 3 (+151 -23)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/local_site_profile.py
	reviewboard/reviews/models.py
	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	2 more

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/models.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
    reviewboard/accounts/evolutions/local_site_profile.py
  Ignored Files:
    reviewboard/templates/reviews/user_page.html
    reviewboard/templates/reviews/trophy_box.html

reviewboard/accounts/evolutions/local_site_profile.py (Diff revision 3)
Show all issues
```
Col: 1
 W391 blank line at end of file
```

reviewboard/accounts/managers.py (Diff revision 3)

Show all issues

Col: 16
 E711 comparison to None should be 'if cond is None

reviewboard/accounts/managers.py (Diff revision 3)
Show all issues
```
Col: 1
 W293 blank line contains whitespace
```

reviewboard/accounts/managers.py (Diff revision 3)

Show all issues

Col: 25
 E128 continuation line under-indented for visual indent

reviewboard/accounts/managers.py (Diff revision 3)

Show all issues

Col: 25
 E128 continuation line under-indented for visual indent

reviewboard/accounts/tests.py (Diff revision 3)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/accounts/tests.py (Diff revision 3)
Show all issues
```
Col: 5
 E303 too many blank lines (2)
```
reviewboard/accounts/tests.py (Diff revision 3)
Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
reviewboard/accounts/tests.py (Diff revision 3)
Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
reviewboard/accounts/tests.py (Diff revision 3)
Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
reviewboard/accounts/tests.py (Diff revision 3)
Show all issues
```
Col: 1
 W391 blank line at end of file
```

Change Summary:

fix format

Diff:

Revision 4 (+148 -23)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/local_site_profile.py
	reviewboard/reviews/models.py
	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	2 more

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/models.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
    reviewboard/accounts/evolutions/local_site_profile.py
  Ignored Files:
    reviewboard/templates/reviews/user_page.html
    reviewboard/templates/reviews/trophy_box.html

reviewboard/accounts/managers.py (Diff revision 4)
Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```
reviewboard/accounts/managers.py (Diff revision 4)
Show all issues
```
Col: 80
 E501 line too long (92 > 79 characters)
```

Diff:

Revision 5 (+150 -23)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/local_site_profile.py
	reviewboard/reviews/models.py
	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	2 more

Fix it!

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/models.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
    reviewboard/accounts/evolutions/local_site_profile.py
  Ignored Files:
    reviewboard/templates/reviews/user_page.html
    reviewboard/templates/reviews/trophy_box.html

reviewboard/accounts/managers.py (Diff revision 5)
Show all issues
```
Col: 43
 W291 trailing whitespace
```

Diff:

Revision 6 (+150 -23)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/local_site_profile.py
	reviewboard/reviews/models.py
	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	2 more

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/models.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
    reviewboard/accounts/evolutions/local_site_profile.py
  Ignored Files:
    reviewboard/templates/reviews/user_page.html
    reviewboard/templates/reviews/trophy_box.html

Added Files:

Change Summary:

wip unittests for compute_user_all_trophies function.
(It doesn't work!)

Diff:

Revision 7 (+68 -23)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/local_site_profile.py
	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/templates/reviews/trophy_box.html
	1 more

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
    reviewboard/accounts/evolutions/local_site_profile.py
  Ignored Files:
    reviewboard/templates/reviews/user_page.html
    reviewboard/templates/reviews/trophy_box.html

Diff:

Revision 8 (+67 -22)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/local_site_profile.py
	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/templates/reviews/trophy_box.html
	1 more

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
    reviewboard/accounts/evolutions/local_site_profile.py
  Ignored Files:
    reviewboard/templates/reviews/user_page.html
    reviewboard/templates/reviews/trophy_box.html

Change Summary:

Fix tests.py (unittests for compute_user_all_trophies function).

Diff:

Revision 9 (+69 -22)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/accounts/evolutions/__init__.py
	reviewboard/accounts/evolutions/local_site_profile.py
	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/templates/reviews/trophy_box.html
	1 more

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/accounts/evolutions/__init__.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
    reviewboard/accounts/evolutions/local_site_profile.py
  Ignored Files:
    reviewboard/templates/reviews/user_page.html
    reviewboard/templates/reviews/trophy_box.html

Change Summary:

reimport hiroki's file

Diff:

Revision 10 (+52 -2)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/reviews/views.py
	reviewboard/templates/reviews/user_page.html

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
  Ignored Files:
    reviewboard/templates/reviews/user_page.html

Change Summary:

*fix old trophy model to new one.
*add user's trophies number to user_infobox.

Diff:

Revision 11 (+57 -3)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/templates/accounts/user_infobox.html
	reviewboard/templates/reviews/user_page.html

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
  Ignored Files:
    reviewboard/static/rb/css/common.less
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/reviews/user_page.html

Added Files:

Removed Files:
Added Files:

Change Summary:

add trophy_page, trophy_list and trophy_infobox

Diff:

Revision 12 (+304 -7)

Show changes

	reviewboard/urls.py
	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/reviews/datagrids.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/js/common.js
	5 more

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/datagrids.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/urls.py
    reviewboard/accounts/tests.py
  Ignored Files:
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/trophies/trophy_infobox.html
    reviewboard/templates/trophies/trophy_list.html
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/common.js
    reviewboard/templates/trophies/trophy.html
    reviewboard/templates/reviews/user_page.html

reviewboard/reviews/datagrids.py (Diff revision 12)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/reviews/datagrids.py (Diff revision 12)
Show all issues
```
Col: 9
 E221 multiple spaces before operator
```

reviewboard/reviews/datagrids.py (Diff revision 12)

Show all issues

Col: 28
 E127 continuation line over-indented for visual indent

reviewboard/reviews/views.py (Diff revision 12)

Show all issues

Col: 18
 E128 continuation line under-indented for visual indent

reviewboard/reviews/views.py (Diff revision 12)

Show all issues

Col: 18
 E128 continuation line under-indented for visual indent

reviewboard/reviews/views.py (Diff revision 12)

Show all issues

Col: 16
 E128 continuation line under-indented for visual indent

reviewboard/reviews/views.py (Diff revision 12)

Show all issues

Col: 16
 E128 continuation line under-indented for visual indent

reviewboard/reviews/views.py (Diff revision 12)

Show all issues

Col: 11
 E128 continuation line under-indented for visual indent

reviewboard/reviews/views.py (Diff revision 12)

Show all issues

Col: 11
 E128 continuation line under-indented for visual indent

reviewboard/reviews/views.py (Diff revision 12)

Show all issues

Col: 11
 E128 continuation line under-indented for visual indent

reviewboard/reviews/views.py (Diff revision 12)
Show all issues
```
Col: 23
 E203 whitespace before '
```

reviewboard/urls.py (Diff revision 12)

Show all issues

Col: 80
 E501 line too long (105 > 79 characters)

reviewboard/urls.py (Diff revision 12)

Show all issues

Col: 1
 W293 blank line contains whitespace

Change Summary:

fix coding style

Diff:

Revision 13 (+305 -6)

Show changes

	reviewboard/urls.py
	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/reviews/datagrids.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/js/common.js
	5 more

Fix it!

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/datagrids.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/urls.py
    reviewboard/accounts/tests.py
  Ignored Files:
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/trophies/trophy_infobox.html
    reviewboard/templates/trophies/trophy_list.html
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/common.js
    reviewboard/templates/trophies/trophy.html
    reviewboard/templates/reviews/user_page.html

reviewboard/reviews/views.py (Diff revision 13)
Show all issues
```
Col: 23
 E203 whitespace before '
```

reviewboard/urls.py (Diff revision 13)

Show all issues

Col: 12
 E225 missing whitespace around operator

reviewboard/urls.py (Diff revision 13)

Show all issues

Col: 20
 E226 missing optional whitespace around operator

reviewboard/urls.py (Diff revision 13)

Show all issues

Col: 24
 E225 missing whitespace around operator

reviewboard/urls.py (Diff revision 13)

Show all issues

Col: 35
 E225 missing whitespace around operator

reviewboard/urls.py (Diff revision 13)

Show all issues

Col: 38
 E226 missing optional whitespace around operator

reviewboard/urls.py (Diff revision 13)

Show all issues

Col: 41
 E226 missing optional whitespace around operator

reviewboard/urls.py (Diff revision 13)

Show all issues

Col: 44
 E226 missing optional whitespace around operator

reviewboard/urls.py (Diff revision 13)

Show all issues

Col: 53
 E226 missing optional whitespace around operator

reviewboard/urls.py (Diff revision 13)

Show all issues

Col: 55
 E226 missing optional whitespace around operator

reviewboard/urls.py (Diff revision 13)

Show all issues

Col: 69
 E225 missing whitespace around operator

reviewboard/urls.py (Diff revision 13)

Show all issues

Col: 12
 E225 missing whitespace around operator

reviewboard/urls.py (Diff revision 13)

Show all issues

Col: 21
 E225 missing whitespace around operator

reviewboard/urls.py (Diff revision 13)

Show all issues

Col: 24
 E226 missing optional whitespace around operator

reviewboard/urls.py (Diff revision 13)

Show all issues

Col: 27
 E226 missing optional whitespace around operator

reviewboard/urls.py (Diff revision 13)

Show all issues

Col: 30
 E226 missing optional whitespace around operator

reviewboard/urls.py (Diff revision 13)

Show all issues

Col: 39
 E226 missing optional whitespace around operator

reviewboard/urls.py (Diff revision 13)

Show all issues

Col: 41
 E225 missing whitespace around operator

Change Summary:

fix style

Diff:

Revision 14 (+304 -6)

Show changes

	reviewboard/urls.py
	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/reviews/datagrids.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/js/common.js
	5 more

Fix it!

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/datagrids.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/urls.py
    reviewboard/accounts/tests.py
  Ignored Files:
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/trophies/trophy_infobox.html
    reviewboard/templates/trophies/trophy_list.html
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/common.js
    reviewboard/templates/trophies/trophy.html
    reviewboard/templates/reviews/user_page.html

reviewboard/reviews/views.py (Diff revision 14)
Show all issues
```
Col: 23
 E203 whitespace before '
```

reviewboard/urls.py (Diff revision 14)

Show all issues

Col: 80
 E501 line too long (105 > 79 characters)

Change Summary:

fix trophy_infobox and trophy_page bugs

Diff:

Revision 15 (+307 -6)

Show changes

	reviewboard/urls.py
	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/reviews/datagrids.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/js/common.js
	5 more

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/datagrids.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/urls.py
    reviewboard/accounts/tests.py
  Ignored Files:
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/trophies/trophy_infobox.html
    reviewboard/templates/trophies/trophy_list.html
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/common.js
    reviewboard/templates/trophies/trophy.html
    reviewboard/templates/reviews/user_page.html

reviewboard/reviews/views.py (Diff revision 15)
Show all issues
```
Col: 18
 E203 whitespace before '
```

reviewboard/urls.py (Diff revision 15)

Show all issues

Col: 80
 E501 line too long (105 > 79 characters)

reviewboard/urls.py (Diff revision 15)

Show all issues

I don't see anything about this in the spec. Not sure we need or want a page showing all the types of trophies, since they're supposed to be more secret.

You should spend your time making a nice, clean list (not datagrid) of trophies a user has on their profile page. Something like a banner at the top of their profile page showing each type of trophy they have.

yuri_mingo March 22, 2013, 6:40 p.m.

I thought trophy_page was a good idea at the time.

Change Summary:

remove trophy_page, trophy_list and trophy_infobox. add trophy_box to top of user_page

Diff:

Revision 16 (+78 -3)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/templates/accounts/user_infobox.html
	reviewboard/templates/reviews/user_page.html
	reviewboard/templates/reviews/user_trophies.html

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
  Ignored Files:
    reviewboard/static/rb/css/common.less
    reviewboard/templates/reviews/user_trophies.html
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/reviews/user_page.html

Removed Files:

Change Summary:

fix display style of trophy_box

Diff:

Revision 17 (+74 -3)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/templates/accounts/user_infobox.html
	reviewboard/templates/reviews/user_page.html
	reviewboard/templates/reviews/user_trophies.html

Added Files:

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
  Ignored Files:
    reviewboard/static/rb/css/common.less
    reviewboard/templates/reviews/user_trophies.html
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/reviews/user_page.html

Change Summary:

add trophy_pic to trophy_box

Diff:

Revision 18 (+87 -3)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/templates/accounts/user_infobox.html
	reviewboard/templates/reviews/user_page.html
	reviewboard/templates/reviews/user_trophies.html

Removed Files:

Added Files:

Fix it!

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
  Ignored Files:
    reviewboard/static/rb/css/common.less
    reviewboard/templates/reviews/user_trophies.html
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/reviews/user_page.html

reviewboard/reviews/views.py (Diff revision 18)
Show all issues
```
Col: 1
 W293 blank line contains whitespace
```

Diff:

Revision 19 (+87 -3)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/templates/accounts/user_infobox.html
	reviewboard/templates/reviews/user_page.html
	reviewboard/templates/reviews/user_trophies.html

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
  Ignored Files:
    reviewboard/static/rb/css/common.less
    reviewboard/templates/reviews/user_trophies.html
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/reviews/user_page.html

reviewboard/reviews/views.py (Diff revision 19)
Show all issues
```
Col: 1
 W293 blank line contains whitespace
```

reviewboard/accounts/managers.py (Diff revision 19)
Show all issues
```
Call this "import_trophies" instead.
```
reviewboard/accounts/managers.py (Diff revision 19)
Show all issues
```
Blank line between these.
```

reviewboard/accounts/managers.py (Diff revision 19)

Show all issues

This is going to be very, very slow, because we're loading every single review request from the database that the user has ever made, and turning each one into a full object.

We know that every old trophy is one of two types: Milestone trophies (1000, 10000, etc.), and Fish trophies (1221, 1551, 12321, etc.). So, we know every one of them is an ID.

There are only two ID fields we care about: "id" itself, and "local_id" (used for Local Sites, which is a way to turn one RB installation into several mini-installations). So, we only need those fields, and "local_site" to know which one to use.

You can use Django's "only" method (https://docs.djangoproject.com/en/dev/ref/models/querysets/#only) for this.

You would need to do:

    q = user.review_requests.only('id', 'local_id', 'local_site')

    for review_request in q:
        ...

reviewboard/accounts/managers.py (Diff revision 19)
Show all issues
```
Blank line between these.
```
reviewboard/accounts/managers.py (Diff revision 19)
Show all issues
```
You're never actually saving the profile.
```
reviewboard/accounts/tests.py (Diff revision 19)
Show all issues
```
I don't think you want to do a copy. What's it for?
```

reviewboard/reviews/views.py (Diff revision 19)

Show all issues

Blank line before this.

This can be simplified to:

    if not extra_data.get('has_trophy_case', False):

reviewboard/reviews/views.py (Diff revision 19)
Show all issues
```
Blank line between these.
```

reviewboard/reviews/views.py (Diff revision 19)

Show all issues

I'm not really sure what this is doing, but a better way to write it would be:

    trophy_list = [
        (trophy, Trophy.objects.trophy_dict[...])
        for trophy in trophies
    ]


However, since this is being rendered by the template, it might be nicer to give these keys:

    trophy_list = [
        {
            'trophy': trophy,
            'trophy_cls': Trophy.objects...,
        }
        for trophy in trophies
    ]

Then the templates can use the key names instead of numbers.

reviewboard/templates/accounts/user_infobox.html (Diff revision 19)

Show all issues

This is going to look weird if it's only 1 trophy, since it will say: "Got 1 trophies". Instead, you want to use Django's "plural" template tag:

https://docs.djangoproject.com/en/dev/ref/templates/builtins/#pluralize

reviewboard/templates/reviews/user_page.html (Diff revision 19)
Show all issues
```
CSS should never go in HTML directly.
```
reviewboard/templates/reviews/user_page.html (Diff revision 19)
Show all issues
```
Indentation issue.
```

reviewboard/templates/reviews/user_trophies.html (Diff revision 19)

Show all issues

Does it not work to pass this variable to "static" below. instead of using definevar?

reviewboard/templates/reviews/user_trophies.html (Diff revision 19)

Show all issues

This line is pretty long. I don't think we want this entire bit of logic either. If you have hundreds of review requests, this will get long.

Instead, just a horizontal list of all their trophies that has the trophy image and then the review request ID number below.

Also, you're doing "review_request.id" in here, but it has to be "review_request.display_id"

Diff:

Revision 20 (+118 -3)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/dashboard.less
	reviewboard/templates/accounts/user_infobox.html
	reviewboard/templates/reviews/user_page.html
	1 more

Removed Files:

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
  Ignored Files:
    reviewboard/static/rb/css/dashboard.less
    reviewboard/static/rb/css/common.less
    reviewboard/templates/reviews/user_trophies.html
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/reviews/user_page.html

Change Summary:

fix coding style
change trophy-box style on the top of user_page.html

Summary:

-	[WIP] Trophy case 3,4
+	Trophy case 3,4

Added Files:

reviewboard/accounts/tests.py (Diff revision 20)
Show all issues
```
I think this should just be "test_importing_trophies"
```

reviewboard/reviews/views.py (Diff revision 20)

Show all issues

Let's put these in alphabetical order, so Profile, ReviewRequestVisit, Trophy.

reviewboard/reviews/views.py (Diff revision 20)

Show all issues

You should use Trophy.objects.get_trophy(trophy.trophy_type)

Callers shouldn't know (or care) that the manager holds the trophies in a trophy_dict.

reviewboard/static/rb/css/common.less (Diff revision 20)
Show all issues
```
Trailing whitespace
```
reviewboard/static/rb/css/dashboard.less (Diff revision 20)
Show all issues
```
Space before "none"
```

reviewboard/templates/accounts/user_infobox.html (Diff revision 20)

Show all issues

Should be "Has gotten {{user_trophies}} troph{{ user_trophies|pluralize:"y,ies" }}.

reviewboard/templates/reviews/user_page.html (Diff revision 20)

Show all issues

I'm confused - we appear to be showing a user's trophies twice - once in the header in a big yellow box, and once down the side.

We shouldn't duplicate this information.

The whole user page is probably going to undergo a redesign soon...I think I'd prefer, for now, to place the trophies just down the side, with their images.

reviewboard/templates/reviews/user_trophies.html (Diff revision 20)
```
What does this line do?
```
reviewboard/templates/reviews/user_trophies.html (Diff revision 20)
Show all issues
```
The alt tag should be the trophy title
```
reviewboard/templates/reviews/user_trophies.html (Diff revision 20)
Show all issues
```
Remove this trailing whitespace.
```

Change Summary:

fix coding style
fix importing trophy_cls

Diff:

Revision 21 (+106 -5)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/dashboard.less
	reviewboard/templates/accounts/user_infobox.html
	reviewboard/templates/reviews/user_page.html
	1 more

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/accounts/tests.py
    reviewboard/accounts/models.py
    reviewboard/accounts/managers.py
  Ignored Files:
    reviewboard/static/rb/css/dashboard.less
    reviewboard/static/rb/css/common.less
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/reviews/user_trophies.html
    reviewboard/templates/reviews/user_page.html

reviewboard/accounts/managers.py (Diff revision 21)

Show all issues

This function is not up to date with what Hiro has in http://reviews.reviewboard.org/r/3905/

I think you need to merge in his changes.

reviewboard/reviews/views.py (Diff revision 21)

Show all issues

This should use Hiro's get_trophy, not getAbstractTrophy.

reviewboard/templates/reviews/user_trophies.html (Diff revision 21)

Show all issues

I'm confused, why is this still here?

In my last review, I wrote: "...we appear to be showing a user's trophies twice - once in the header in a big yellow box, and once down the side.

We shouldn't duplicate this information.

The whole user page is probably going to undergo a redesign soon...*I think I'd prefer, for now, to place the trophies just down the side, with their images*."

So what you had down the left side was what I think we wanted, but we also wanted the images while you were at it.

Change Summary:

*remove trophy-box on top of user_page
*add trophy-list on side of user_page

Diff:

Revision 22 (+143 -3)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/dashboard.less
	reviewboard/templates/accounts/user_infobox.html
	reviewboard/templates/reviews/user_page.html
	1 more

Removed Files:

Added Files:

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
  Ignored Files:
    reviewboard/static/rb/css/dashboard.less
    reviewboard/static/rb/css/common.less
    reviewboard/templates/reviews/user_trophies.html
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/reviews/user_page.html

Diff:

Revision 23 (+143 -3)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/dashboard.less
	reviewboard/templates/accounts/user_infobox.html
	reviewboard/templates/reviews/user_page.html
	1 more

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
  Ignored Files:
    reviewboard/static/rb/css/dashboard.less
    reviewboard/static/rb/css/common.less
    reviewboard/templates/reviews/user_trophies.html
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/reviews/user_page.html

Hey Yuri - just a few things that I don't think are necessary anymore.

Also, I'd really like to see a screenshot showing what this looks like with the trophies in the sidebar. Thanks,

-Mike

reviewboard/reviews/views.py (Diff revision 23)

Show all issues

Is 'trophy' ever used? I don't think it is - if not, please remove it.

YU

yuri_mingo March 29, 2013, 6:45 p.m.
```
It use on line 58-62 of user_page.html.
```

reviewboard/static/rb/css/common.less (Diff revision 23)

Show all issues

trophy-box is used by user_trophies.html, which is no longer being used. I think this set of rules can be removed.

reviewboard/templates/reviews/user_page.html (Diff revision 23)

Show all issues

The alignment of the HTML tags needs to be corrected. Also, I'd like you to post a screenshot showing what this looks like.

yuri_mingo March 29, 2013, 6:45 p.m.

I don't know what the alignment of the HTML is, float of css is ok?

That screenshot is "Screenshot_from_2013-03-27_205707.png"

mike_conley March 29, 2013, 6:56 p.m.

The alignment of the HTML tags - <div class="trophy-set-sidebar">, for example, is not indented at all, when its parent <ul class="trophy-list"> is indented some number of spaces. <div class="trophy-set-sidebar"> should be indented the same number of spaces as <ul class="trophy-list">, plus one more space.

This rule should apply to every tag in this newly introduced block of HTML.

Thanks for pointing out the screenshot - I didn't notice you'd updated it. Like I mention in my screenshot comment, I think the trophies are too far to the right, and should be left-aligned.

reviewboard/templates/reviews/user_trophies.html (Diff revision 23)

Show all issues

This file isn't being used anymore and can be removed.

Screenshot_from_2013-03-27_205707.png

Instead of display (id: 1), I think these should link to the review requests that caused the reward.

Maybe something like

"[title of trophy]"
/r/15251

Where /r/15251 would link to review request 15251. Let's see what that looks like.

Also, why are these pushed so far to the right? Can you get rid of the margin to the left of the trophy images?

Show all issues

Instead of display (id: 1), I think these should link to the review requests that caused the reward.

Maybe something like

"[title of trophy]"
/r/15251

Where /r/15251 would link to review request 15251. Let's see what that looks like.

Also, why are these pushed so far to the right? Can you get rid of the margin to the left of the trophy images?

Change Summary:

*remove user_trophies.html completely
*fix position of trophy images.
*add new screenshot

Diff:

Revision 24 (+91 -4)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/dashboard.less
	reviewboard/templates/accounts/user_infobox.html
	reviewboard/templates/reviews/user_page.html

Removed Files:

Added Files:

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
  Ignored Files:
    reviewboard/static/rb/css/dashboard.less
    reviewboard/static/rb/css/common.less
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/reviews/user_page.html

Screenshot_from_2013-03-30_062900.png

Show all issues

This is looking much, much better!

Another suggestion - please remove the quotes, and use the trophy title instead of the trophy_type.

Change Summary:

*fix display style of trophy titles
*add new screenshot

Diff:

Revision 25 (+91 -4)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/dashboard.less
	reviewboard/templates/accounts/user_infobox.html
	reviewboard/templates/reviews/user_page.html

Removed Files:

Added Files:

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
  Ignored Files:
    reviewboard/static/rb/css/dashboard.less
    reviewboard/static/rb/css/common.less
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/reviews/user_page.html

Hey Yuri,

I'm pretty close to giving this a ship-it. Just two more nits below from me. Also, you need to fill out the "Testing Done" section - tell us what testing you did to make sure this all works. What steps did you follow to test? Describe what the unit test tests. That sort of thing.

-Mike

reviewboard/accounts/managers.py (Diff revision 25)

Show all issues

Clearer documentation would be,

"""Compute the trophies awarded to a user."""

reviewboard/templates/reviews/user_page.html (Diff revision 25)
Show all issues
```
I think the alt tag should be the trophy description.
```

Change Summary:

fix description

Diff:

Revision 26 (+95 -5)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/dashboard.less
	reviewboard/templates/accounts/user_infobox.html
	reviewboard/templates/reviews/user_page.html

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
  Ignored Files:
    reviewboard/static/rb/css/dashboard.less
    reviewboard/static/rb/css/common.less
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/reviews/user_page.html

1) Some print statements got introduced in this patch which need to be removed.
2) You still need to update the "Testing Done" section of the review request to include the testing you performed on this patch.

Thanks,

-Mike

reviewboard/accounts/managers.py (Diff revision 26)
Show all issues
```
These need to be removed.
```

Description:

~		[WIP]
	~	https://hackpad.com/Trophy-Case-Design-MK30awhb3nL
-		https://hackpad.com/Trophy-Case-Design-MK30awhb3nL

Testing Done:

	+	(1)
	+	unit test, named test_importing_trophies, do follow things.
	+	1.Add User object named user1.
	+	2.Add 2 reveiw requests associated with user1, display_id=1000 for Milestone Trophy and display_id=1221 for Palindrome Trophy.
	+	3.Compute user1's all trophies.
	+	And, this code passed test.
	+
	+	(2)
	+	I made sure profile.extra_data works.
	+	I set the pdb.set_trace in "if not extra_data.get('has_trophy_case', False)", then I tried to access twice and I checked that pdb is called only once.

Change Summary:

remove garbage codes.

Diff:

Revision 27 (+93 -5)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/dashboard.less
	reviewboard/templates/accounts/user_infobox.html
	reviewboard/templates/reviews/user_page.html

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
  Ignored Files:
    reviewboard/static/rb/css/dashboard.less
    reviewboard/static/rb/css/common.less
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/reviews/user_page.html

```
Gah, just noticed two more things.
```

reviewboard/templates/reviews/user_page.html (Diff revision 27)

Show all issues

Hm, this <br> shouldn't be here. You should increase the margin-bottom of the <h1> instead.

Maybe something like:

.trophy-info h1 {
  margin-bottom: 10px;
}

reviewboard/templates/reviews/user_page.html (Diff revision 27)

Show all issues

This isn't the right way to link to a review request. It should be like this:

{% blocktrans with trophy_data.trophy.review_request.display_id as id and review_request.get_absolute_url as review_request_url %}
<a href="{{review_request_url}}">/r/{{id}}</a>
{% endblocktrans %}

Please double check that this works.

YU

yuri_mingo March 31, 2013, 2:33 a.m.
```
np, get_absolute_url works.
```

Change Summary:

remove </ br> tag from user_page
use get_absolute_url() to link trophy's review_request pages.

Diff:

Revision 28 (+94 -5)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/dashboard.less
	reviewboard/templates/accounts/user_infobox.html
	reviewboard/templates/reviews/user_page.html

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
  Ignored Files:
    reviewboard/static/rb/css/dashboard.less
    reviewboard/static/rb/css/common.less
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/reviews/user_page.html

Fix it!

Last two fixes, and if I can't find anything else wrong, you have my ship-it.

reviewboard/accounts/managers.py (Diff revision 28)
Show all issues
```
This newline should go back.
```

reviewboard/templates/reviews/user_page.html (Diff revision 28)

Show all issues

Break up this long line by putting the <a> tag on the next line, and the {% endblocktrans %} on its own line.

Change Summary:

fix coding style

Diff:

Revision 29 (+96 -4)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/dashboard.less
	reviewboard/templates/accounts/user_infobox.html
	reviewboard/templates/reviews/user_page.html

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/accounts/managers.py
    reviewboard/accounts/models.py
    reviewboard/accounts/tests.py
  Ignored Files:
    reviewboard/static/rb/css/dashboard.less
    reviewboard/static/rb/css/common.less
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/reviews/user_page.html

Ship it!

```
The code looks good to me. Thanks Yuri!
```

Trophy case 3,4

Information

Reviewers

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Diff:

Diff:

Added Files:

Change Summary:

Diff:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Added Files:

Removed Files:

Added Files:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Removed Files:

Change Summary:

Diff:

Added Files:

Change Summary:

Diff:

Removed Files:

Added Files:

Diff:

Diff:

Removed Files:

Change Summary:

Summary:

Added Files:

Change Summary:

Diff:

Change Summary:

Diff:

Removed Files:

Added Files:

Diff:

Change Summary:

Diff:

Removed Files:

Added Files:

Change Summary:

Diff:

Removed Files:

Added Files:

Change Summary:

Diff:

Description:

Testing Done:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Status: Discarded