Trophy case 3,4

Review Request #3878 — Created Feb. 16, 2013 and discarded

Information

Review Board
master

Reviewers

https://hackpad.com/Trophy-Case-Design-MK30awhb3nL
(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.

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_conleymike_conley

This is looking much, much better! Another suggestion - please remove the quotes, and use the trophy title instead of …

mike_conleymike_conley

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

reviewbotreviewbot

Col: 20 E203 whitespace before '

reviewbotreviewbot

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

GR gregwym

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

Col: 13 E201 whitespace after '('

reviewbotreviewbot

Col: 18 E202 whitespace before ')'

reviewbotreviewbot

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

GR gregwym

Col: 35 E201 whitespace after '('

reviewbotreviewbot

Col: 68 E202 whitespace before ')'

reviewbotreviewbot

Col: 22 E201 whitespace after '('

reviewbotreviewbot

Col: 26 E202 whitespace before ')'

reviewbotreviewbot

I think we use `` instead of ``. Besides, I wonder whether using is a good option. In my opinion, …

GR gregwym

Col: 69 W291 trailing whitespace

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 W391 blank line at end of file

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W391 blank line at end of file

reviewbotreviewbot

Col: 1 W391 blank line at end of file

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W391 blank line at end of file

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 43 W291 trailing whitespace

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 9 E221 multiple spaces before operator

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 23 E203 whitespace before '

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 23 E203 whitespace before '

reviewbotreviewbot

Col: 12 E225 missing whitespace around operator

reviewbotreviewbot

Col: 20 E226 missing optional whitespace around operator

reviewbotreviewbot

Col: 24 E225 missing whitespace around operator

reviewbotreviewbot

Col: 35 E225 missing whitespace around operator

reviewbotreviewbot

Col: 38 E226 missing optional whitespace around operator

reviewbotreviewbot

Col: 41 E226 missing optional whitespace around operator

reviewbotreviewbot

Col: 44 E226 missing optional whitespace around operator

reviewbotreviewbot

Col: 53 E226 missing optional whitespace around operator

reviewbotreviewbot

Col: 55 E226 missing optional whitespace around operator

reviewbotreviewbot

Col: 69 E225 missing whitespace around operator

reviewbotreviewbot

Col: 12 E225 missing whitespace around operator

reviewbotreviewbot

Col: 21 E225 missing whitespace around operator

reviewbotreviewbot

Col: 24 E226 missing optional whitespace around operator

reviewbotreviewbot

Col: 27 E226 missing optional whitespace around operator

reviewbotreviewbot

Col: 30 E226 missing optional whitespace around operator

reviewbotreviewbot

Col: 39 E226 missing optional whitespace around operator

reviewbotreviewbot

Col: 41 E225 missing whitespace around operator

reviewbotreviewbot

Col: 23 E203 whitespace before '

reviewbotreviewbot

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

reviewbotreviewbot

Col: 18 E203 whitespace before '

reviewbotreviewbot

I don't see anything about this in the spec. Not sure we need or want a page showing all the …

chipx86chipx86

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Call this "import_trophies" instead.

chipx86chipx86

Blank line between these.

chipx86chipx86

This is going to be very, very slow, because we're loading every single review request from the database that the …

chipx86chipx86

Blank line between these.

chipx86chipx86

You're never actually saving the profile.

chipx86chipx86

I don't think you want to do a copy. What's it for?

chipx86chipx86

Blank line before this. This can be simplified to: if not extra_data.get('has_trophy_case', False):

chipx86chipx86

Blank line between these.

chipx86chipx86

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

chipx86chipx86

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

This is going to look weird if it's only 1 trophy, since it will say: "Got 1 trophies". Instead, you …

chipx86chipx86

CSS should never go in HTML directly.

chipx86chipx86

Indentation issue.

chipx86chipx86

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

chipx86chipx86

This line is pretty long. I don't think we want this entire bit of logic either. If you have hundreds …

chipx86chipx86

I think this should just be "test_importing_trophies"

mike_conleymike_conley

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

mike_conleymike_conley

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_conleymike_conley

Trailing whitespace

mike_conleymike_conley

Space before "none"

mike_conleymike_conley

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

mike_conleymike_conley

I'm confused - we appear to be showing a user's trophies twice - once in the header in a big …

mike_conleymike_conley

The alt tag should be the trophy title

mike_conleymike_conley

Remove this trailing whitespace.

mike_conleymike_conley

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_conleymike_conley

This should use Hiro's get_trophy, not getAbstractTrophy.

mike_conleymike_conley

I'm confused, why is this still here? In my last review, I wrote: "...we appear to be showing a user's …

mike_conleymike_conley

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

mike_conleymike_conley

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

mike_conleymike_conley

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

mike_conleymike_conley

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

mike_conleymike_conley

Clearer documentation would be, """Compute the trophies awarded to a user."""

mike_conleymike_conley

I think the alt tag should be the trophy description.

mike_conleymike_conley

These need to be removed.

mike_conleymike_conley

Hm, this shouldn't be here. You should increase the margin-bottom of the instead. Maybe something like: .trophy-info h1 { margin-bottom: …

mike_conleymike_conley

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_conleymike_conley

This newline should go back.

mike_conleymike_conley

Break up this long line by putting the tag on the next line, and the {% endblocktrans %} on its …

mike_conleymike_conley
reviewbot
  1. 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
    
    
  2. Show all issues
    Col: 80
     E501 line too long (101 > 79 characters)
    
  3. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    Show all issues
    Col: 20
     E203 whitespace before '
  4. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    Show all issues
    Col: 5
     E303 too many blank lines (2)
    
  5. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    Show all issues
    Col: 13
     E201 whitespace after '('
    
  6. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    Show all issues
    Col: 18
     E202 whitespace before ')'
    
  7. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    Show all issues
    Col: 35
     E201 whitespace after '('
    
  8. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    Show all issues
    Col: 68
     E202 whitespace before ')'
    
  9. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    Show all issues
    Col: 22
     E201 whitespace after '('
    
  10. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    Show all issues
    Col: 26
     E202 whitespace before ')'
    
  11. 
      
GR
  1. 
      
  2. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    Show all issues
    Apparently, `written by Hiro` is not a good class description
    1. thanks for comments but it's not description. Note to self.
      I'll fix later!
  3. 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. 
    1. No, of course not. 
  4. 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. 
  5. 
      
YU
reviewbot
  1. 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
    
    
  2. Show all issues
    Col: 69
     W291 trailing whitespace
    
  3. reviewboard/accounts/managers.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  4. reviewboard/accounts/managers.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     W391 blank line at end of file
    
  5. reviewboard/accounts/managers.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  6. reviewboard/accounts/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  7. reviewboard/accounts/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  8. reviewboard/accounts/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  9. reviewboard/accounts/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     W391 blank line at end of file
    
  10. 
      
YU
reviewbot
  1. 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
    
    
  2. Show all issues
    Col: 1
     W391 blank line at end of file
    
  3. reviewboard/accounts/managers.py (Diff revision 3)
     
     
    Show all issues
    Col: 16
     E711 comparison to None should be 'if cond is None
  4. reviewboard/accounts/managers.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  5. reviewboard/accounts/managers.py (Diff revision 3)
     
     
    Show all issues
    Col: 25
     E128 continuation line under-indented for visual indent
    
  6. reviewboard/accounts/managers.py (Diff revision 3)
     
     
    Show all issues
    Col: 25
     E128 continuation line under-indented for visual indent
    
  7. reviewboard/accounts/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  8. reviewboard/accounts/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 5
     E303 too many blank lines (2)
    
  9. reviewboard/accounts/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  10. reviewboard/accounts/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  11. reviewboard/accounts/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  12. reviewboard/accounts/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W391 blank line at end of file
    
  13. 
      
YU
reviewbot
  1. 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
    
    
  2. reviewboard/accounts/managers.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (89 > 79 characters)
    
  3. reviewboard/accounts/managers.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (92 > 79 characters)
    
  4. 
      
YU
reviewbot
  1. 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
    
    
  2. reviewboard/accounts/managers.py (Diff revision 5)
     
     
    Show all issues
    Col: 43
     W291 trailing whitespace
    
  3. 
      
YU
reviewbot
  1. 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
    
    
  2. 
      
YU
YU
reviewbot
  1. 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
    
    
  2. 
      
YU
reviewbot
  1. 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
    
    
  2. 
      
YU
reviewbot
  1. 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
    
    
  2. 
      
YU
reviewbot
  1. 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
    
    
  2. 
      
YU
reviewbot
  1. 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
    
    
  2. 
      
YU
YU
YU
reviewbot
  1. 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
    
    
  2. reviewboard/reviews/datagrids.py (Diff revision 12)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  3. reviewboard/reviews/datagrids.py (Diff revision 12)
     
     
    Show all issues
    Col: 9
     E221 multiple spaces before operator
    
  4. reviewboard/reviews/datagrids.py (Diff revision 12)
     
     
    Show all issues
    Col: 28
     E127 continuation line over-indented for visual indent
    
  5. reviewboard/reviews/views.py (Diff revision 12)
     
     
    Show all issues
    Col: 18
     E128 continuation line under-indented for visual indent
    
  6. reviewboard/reviews/views.py (Diff revision 12)
     
     
    Show all issues
    Col: 18
     E128 continuation line under-indented for visual indent
    
  7. reviewboard/reviews/views.py (Diff revision 12)
     
     
    Show all issues
    Col: 16
     E128 continuation line under-indented for visual indent
    
  8. reviewboard/reviews/views.py (Diff revision 12)
     
     
    Show all issues
    Col: 16
     E128 continuation line under-indented for visual indent
    
  9. reviewboard/reviews/views.py (Diff revision 12)
     
     
    Show all issues
    Col: 11
     E128 continuation line under-indented for visual indent
    
  10. reviewboard/reviews/views.py (Diff revision 12)
     
     
    Show all issues
    Col: 11
     E128 continuation line under-indented for visual indent
    
  11. reviewboard/reviews/views.py (Diff revision 12)
     
     
    Show all issues
    Col: 11
     E128 continuation line under-indented for visual indent
    
  12. reviewboard/reviews/views.py (Diff revision 12)
     
     
    Show all issues
    Col: 23
     E203 whitespace before '
  13. reviewboard/urls.py (Diff revision 12)
     
     
    Show all issues
    Col: 80
     E501 line too long (105 > 79 characters)
    
  14. reviewboard/urls.py (Diff revision 12)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  15. 
      
YU
reviewbot
  1. 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
    
    
  2. reviewboard/reviews/views.py (Diff revision 13)
     
     
    Show all issues
    Col: 23
     E203 whitespace before '
  3. reviewboard/urls.py (Diff revision 13)
     
     
    Show all issues
    Col: 12
     E225 missing whitespace around operator
    
  4. reviewboard/urls.py (Diff revision 13)
     
     
    Show all issues
    Col: 20
     E226 missing optional whitespace around operator
    
  5. reviewboard/urls.py (Diff revision 13)
     
     
    Show all issues
    Col: 24
     E225 missing whitespace around operator
    
  6. reviewboard/urls.py (Diff revision 13)
     
     
    Show all issues
    Col: 35
     E225 missing whitespace around operator
    
  7. reviewboard/urls.py (Diff revision 13)
     
     
    Show all issues
    Col: 38
     E226 missing optional whitespace around operator
    
  8. reviewboard/urls.py (Diff revision 13)
     
     
    Show all issues
    Col: 41
     E226 missing optional whitespace around operator
    
  9. reviewboard/urls.py (Diff revision 13)
     
     
    Show all issues
    Col: 44
     E226 missing optional whitespace around operator
    
  10. reviewboard/urls.py (Diff revision 13)
     
     
    Show all issues
    Col: 53
     E226 missing optional whitespace around operator
    
  11. reviewboard/urls.py (Diff revision 13)
     
     
    Show all issues
    Col: 55
     E226 missing optional whitespace around operator
    
  12. reviewboard/urls.py (Diff revision 13)
     
     
    Show all issues
    Col: 69
     E225 missing whitespace around operator
    
  13. reviewboard/urls.py (Diff revision 13)
     
     
    Show all issues
    Col: 12
     E225 missing whitespace around operator
    
  14. reviewboard/urls.py (Diff revision 13)
     
     
    Show all issues
    Col: 21
     E225 missing whitespace around operator
    
  15. reviewboard/urls.py (Diff revision 13)
     
     
    Show all issues
    Col: 24
     E226 missing optional whitespace around operator
    
  16. reviewboard/urls.py (Diff revision 13)
     
     
    Show all issues
    Col: 27
     E226 missing optional whitespace around operator
    
  17. reviewboard/urls.py (Diff revision 13)
     
     
    Show all issues
    Col: 30
     E226 missing optional whitespace around operator
    
  18. reviewboard/urls.py (Diff revision 13)
     
     
    Show all issues
    Col: 39
     E226 missing optional whitespace around operator
    
  19. reviewboard/urls.py (Diff revision 13)
     
     
    Show all issues
    Col: 41
     E225 missing whitespace around operator
    
  20. 
      
YU
reviewbot
  1. 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
    
    
  2. reviewboard/reviews/views.py (Diff revision 14)
     
     
    Show all issues
    Col: 23
     E203 whitespace before '
  3. reviewboard/urls.py (Diff revision 14)
     
     
    Show all issues
    Col: 80
     E501 line too long (105 > 79 characters)
    
  4. 
      
YU
reviewbot
  1. 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
    
    
  2. reviewboard/reviews/views.py (Diff revision 15)
     
     
    Show all issues
    Col: 18
     E203 whitespace before '
  3. reviewboard/urls.py (Diff revision 15)
     
     
    Show all issues
    Col: 80
     E501 line too long (105 > 79 characters)
    
  4. 
      
chipx86
  1. 
      
  2. 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.
    1. I thought trophy_page was a good idea at the time.
  3. 
      
YU
reviewbot
  1. 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
    
    
  2. 
      
YU
YU
reviewbot
  1. 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
    
    
  2. 
      
YU
reviewbot
  1. 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
    
    
  2. reviewboard/reviews/views.py (Diff revision 18)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  3. 
      
YU
reviewbot
  1. 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
    
    
  2. reviewboard/reviews/views.py (Diff revision 19)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  3. 
      
chipx86
  1. 
      
  2. reviewboard/accounts/managers.py (Diff revision 19)
     
     
    Show all issues
    Call this "import_trophies" instead.
  3. reviewboard/accounts/managers.py (Diff revision 19)
     
     
     
    Show all issues
    Blank line between these.
  4. 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:
            ...
  5. reviewboard/accounts/managers.py (Diff revision 19)
     
     
     
    Show all issues
    Blank line between these.
  6. reviewboard/accounts/managers.py (Diff revision 19)
     
     
    Show all issues
    You're never actually saving the profile.
  7. reviewboard/accounts/tests.py (Diff revision 19)
     
     
    Show all issues
    I don't think you want to do a copy. What's it for?
  8. 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):
  9. reviewboard/reviews/views.py (Diff revision 19)
     
     
     
    Show all issues
    Blank line between these.
  10. 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.
  11. 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
  12. Show all issues
    CSS should never go in HTML directly.
  13. Show all issues
    Indentation issue.
  14. Show all issues
    Does it not work to pass this variable to "static" below. instead of using definevar?
  15. 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"
  16. 
      
YU
reviewbot
  1. 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
    
    
  2. 
      
YU
mike_conley
  1. 
      
  2. reviewboard/accounts/tests.py (Diff revision 20)
     
     
    Show all issues
    I think this should just be "test_importing_trophies"
  3. reviewboard/reviews/views.py (Diff revision 20)
     
     
    Show all issues
    Let's put these in alphabetical order, so Profile, ReviewRequestVisit, Trophy.
  4. 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.
  5. reviewboard/static/rb/css/common.less (Diff revision 20)
     
     
    Show all issues
    Trailing whitespace
  6. reviewboard/static/rb/css/dashboard.less (Diff revision 20)
     
     
    Show all issues
    Space before "none"
  7. Show all issues
    Should be "Has gotten {{user_trophies}} troph{{ user_trophies|pluralize:"y,ies" }}.
  8. 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.
  9. What does this line do?
  10. Show all issues
    The alt tag should be the trophy title
  11. Show all issues
    Remove this trailing whitespace.
  12. 
      
YU
reviewbot
  1. 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
    
    
  2. 
      
mike_conley
  1. 
      
  2. 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.
  3. reviewboard/reviews/views.py (Diff revision 21)
     
     
    Show all issues
    This should use Hiro's get_trophy, not getAbstractTrophy.
  4. 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.
  5. 
      
YU
reviewbot
  1. 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
    
    
  2. 
      
YU
reviewbot
  1. 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
    
    
  2. 
      
mike_conley
  1. 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
  2. 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.
    1. It use on line 58-62 of user_page.html.
  3. 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.
  4. 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.
    1. 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"
    2. 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.
  5. reviewboard/templates/reviews/user_trophies.html (Diff revision 23)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    This file isn't being used anymore and can be removed.
  6. 
      
mike_conley
  1. 
      
  2. 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?
    
    
  3. 
      
YU
reviewbot
  1. 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
    
    
  2. 
      
mike_conley
  1. 
      
  2. 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.
  3. 
      
YU
reviewbot
  1. 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
    
    
  2. 
      
mike_conley
  1. 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
  2. reviewboard/accounts/managers.py (Diff revision 25)
     
     
    Show all issues
    Clearer documentation would be,
    
    """Compute the trophies awarded to a user."""
  3. Show all issues
    I think the alt tag should be the trophy description.
  4. 
      
YU
reviewbot
  1. 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
    
    
  2. 
      
mike_conley
  1. 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
  2. reviewboard/accounts/managers.py (Diff revision 26)
     
     
     
     
     
     
     
     
    Show all issues
    These need to be removed.
  3. 
      
YU
YU
reviewbot
  1. 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
    
    
  2. 
      
mike_conley
  1. Gah, just noticed two more things.
  2. 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;
    }
  3. 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.
    1. np, get_absolute_url works.
  4. 
      
YU
reviewbot
  1. 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
    
    
  2. 
      
mike_conley
  1. Last two fixes, and if I can't find anything else wrong, you have my ship-it.
  2. reviewboard/accounts/managers.py (Diff revision 28)
     
     
    Show all issues
    This newline should go back.
  3. Show all issues
    Break up this long line by putting the <a> tag on the next line, and the {% endblocktrans %} on its own line.
  4. 
      
YU
reviewbot
  1. 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
    
    
  2. 
      
mike_conley
  1. The code looks good to me. Thanks Yuri!
  2. 
      
YU
Review request changed
Status:
Discarded