Trophy case 3,4

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

yuri_mingo
Review Board
master
reviewboard
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. Col: 80
     E501 line too long (101 > 79 characters)
    
  3. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    Col: 20
     E203 whitespace before '
  4. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    Col: 5
     E303 too many blank lines (2)
    
  5. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    Col: 13
     E201 whitespace after '('
    
  6. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    Col: 18
     E202 whitespace before ')'
    
  7. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    Col: 35
     E201 whitespace after '('
    
  8. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    Col: 68
     E202 whitespace before ')'
    
  9. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    Col: 22
     E201 whitespace after '('
    
  10. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    Col: 26
     E202 whitespace before ')'
    
  11. 
      
GR
  1. 
      
  2. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    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)
     
     
    You sure you want to name a function as `newfunc`? It is not a descriptive name. 
    1. No, of course not. 
  4. 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. Col: 69
     W291 trailing whitespace
    
  3. reviewboard/accounts/managers.py (Diff revision 2)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  4. reviewboard/accounts/managers.py (Diff revision 2)
     
     
    Col: 1
     W391 blank line at end of file
    
  5. reviewboard/accounts/managers.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  6. reviewboard/accounts/tests.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  7. reviewboard/accounts/tests.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  8. reviewboard/accounts/tests.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  9. reviewboard/accounts/tests.py (Diff revision 2)
     
     
    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. Col: 1
     W391 blank line at end of file
    
  3. reviewboard/accounts/managers.py (Diff revision 3)
     
     
    Col: 16
     E711 comparison to None should be 'if cond is None
  4. reviewboard/accounts/managers.py (Diff revision 3)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  5. reviewboard/accounts/managers.py (Diff revision 3)
     
     
    Col: 25
     E128 continuation line under-indented for visual indent
    
  6. reviewboard/accounts/managers.py (Diff revision 3)
     
     
    Col: 25
     E128 continuation line under-indented for visual indent
    
  7. reviewboard/accounts/tests.py (Diff revision 3)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  8. reviewboard/accounts/tests.py (Diff revision 3)
     
     
    Col: 5
     E303 too many blank lines (2)
    
  9. reviewboard/accounts/tests.py (Diff revision 3)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  10. reviewboard/accounts/tests.py (Diff revision 3)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  11. reviewboard/accounts/tests.py (Diff revision 3)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  12. reviewboard/accounts/tests.py (Diff revision 3)
     
     
    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)
     
     
    Col: 80
     E501 line too long (89 > 79 characters)
    
  3. reviewboard/accounts/managers.py (Diff revision 4)
     
     
    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)
     
     
    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)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  3. reviewboard/reviews/datagrids.py (Diff revision 12)
     
     
    Col: 9
     E221 multiple spaces before operator
    
  4. reviewboard/reviews/datagrids.py (Diff revision 12)
     
     
    Col: 28
     E127 continuation line over-indented for visual indent
    
  5. reviewboard/reviews/views.py (Diff revision 12)
     
     
    Col: 18
     E128 continuation line under-indented for visual indent
    
  6. reviewboard/reviews/views.py (Diff revision 12)
     
     
    Col: 18
     E128 continuation line under-indented for visual indent
    
  7. reviewboard/reviews/views.py (Diff revision 12)
     
     
    Col: 16
     E128 continuation line under-indented for visual indent
    
  8. reviewboard/reviews/views.py (Diff revision 12)
     
     
    Col: 16
     E128 continuation line under-indented for visual indent
    
  9. reviewboard/reviews/views.py (Diff revision 12)
     
     
    Col: 11
     E128 continuation line under-indented for visual indent
    
  10. reviewboard/reviews/views.py (Diff revision 12)
     
     
    Col: 11
     E128 continuation line under-indented for visual indent
    
  11. reviewboard/reviews/views.py (Diff revision 12)
     
     
    Col: 11
     E128 continuation line under-indented for visual indent
    
  12. reviewboard/reviews/views.py (Diff revision 12)
     
     
    Col: 23
     E203 whitespace before '
  13. reviewboard/urls.py (Diff revision 12)
     
     
    Col: 80
     E501 line too long (105 > 79 characters)
    
  14. reviewboard/urls.py (Diff revision 12)
     
     
    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)
     
     
    Col: 23
     E203 whitespace before '
  3. reviewboard/urls.py (Diff revision 13)
     
     
    Col: 12
     E225 missing whitespace around operator
    
  4. reviewboard/urls.py (Diff revision 13)
     
     
    Col: 20
     E226 missing optional whitespace around operator
    
  5. reviewboard/urls.py (Diff revision 13)
     
     
    Col: 24
     E225 missing whitespace around operator
    
  6. reviewboard/urls.py (Diff revision 13)
     
     
    Col: 35
     E225 missing whitespace around operator
    
  7. reviewboard/urls.py (Diff revision 13)
     
     
    Col: 38
     E226 missing optional whitespace around operator
    
  8. reviewboard/urls.py (Diff revision 13)
     
     
    Col: 41
     E226 missing optional whitespace around operator
    
  9. reviewboard/urls.py (Diff revision 13)
     
     
    Col: 44
     E226 missing optional whitespace around operator
    
  10. reviewboard/urls.py (Diff revision 13)
     
     
    Col: 53
     E226 missing optional whitespace around operator
    
  11. reviewboard/urls.py (Diff revision 13)
     
     
    Col: 55
     E226 missing optional whitespace around operator
    
  12. reviewboard/urls.py (Diff revision 13)
     
     
    Col: 69
     E225 missing whitespace around operator
    
  13. reviewboard/urls.py (Diff revision 13)
     
     
    Col: 12
     E225 missing whitespace around operator
    
  14. reviewboard/urls.py (Diff revision 13)
     
     
    Col: 21
     E225 missing whitespace around operator
    
  15. reviewboard/urls.py (Diff revision 13)
     
     
    Col: 24
     E226 missing optional whitespace around operator
    
  16. reviewboard/urls.py (Diff revision 13)
     
     
    Col: 27
     E226 missing optional whitespace around operator
    
  17. reviewboard/urls.py (Diff revision 13)
     
     
    Col: 30
     E226 missing optional whitespace around operator
    
  18. reviewboard/urls.py (Diff revision 13)
     
     
    Col: 39
     E226 missing optional whitespace around operator
    
  19. reviewboard/urls.py (Diff revision 13)
     
     
    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)
     
     
    Col: 23
     E203 whitespace before '
  3. reviewboard/urls.py (Diff revision 14)
     
     
    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)
     
     
    Col: 18
     E203 whitespace before '
  3. reviewboard/urls.py (Diff revision 15)
     
     
    Col: 80
     E501 line too long (105 > 79 characters)
    
  4. 
      
chipx86
  1. 
      
  2. reviewboard/urls.py (Diff revision 15)
     
     
    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)
     
     
    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)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  3. 
      
chipx86
  1. 
      
  2. reviewboard/accounts/managers.py (Diff revision 19)
     
     
    Call this "import_trophies" instead.
  3. reviewboard/accounts/managers.py (Diff revision 19)
     
     
     
    Blank line between these.
  4. reviewboard/accounts/managers.py (Diff revision 19)
     
     
     
    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)
     
     
     
    Blank line between these.
  6. reviewboard/accounts/managers.py (Diff revision 19)
     
     
    You're never actually saving the profile.
  7. reviewboard/accounts/tests.py (Diff revision 19)
     
     
    I don't think you want to do a copy. What's it for?
  8. reviewboard/reviews/views.py (Diff revision 19)
     
     
     
    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)
     
     
     
    Blank line between these.
  10. reviewboard/reviews/views.py (Diff revision 19)
     
     
     
     
     
     
    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. 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. CSS should never go in HTML directly.
  13. Indentation issue.
  14. Does it not work to pass this variable to "static" below. instead of using definevar?
  15. 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)
     
     
    I think this should just be "test_importing_trophies"
  3. reviewboard/reviews/views.py (Diff revision 20)
     
     
    Let's put these in alphabetical order, so Profile, ReviewRequestVisit, Trophy.
  4. reviewboard/reviews/views.py (Diff revision 20)
     
     
     
    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)
     
     
    Trailing whitespace
  6. reviewboard/static/rb/css/dashboard.less (Diff revision 20)
     
     
    Space before "none"
  7. Should be "Has gotten {{user_trophies}} troph{{ user_trophies|pluralize:"y,ies" }}.
  8. reviewboard/templates/reviews/user_page.html (Diff revision 20)
     
     
     
     
     
     
     
     
     
     
     
     
    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. The alt tag should be the trophy title
  11. 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)
     
     
     
     
    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)
     
     
    This should use Hiro's get_trophy, not getAbstractTrophy.
  4. 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)
     
     
    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)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    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)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    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)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    This file isn't being used anymore and can be removed.
  6. 
      
mike_conley
  1. 
      
  2. 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. 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)
     
     
    Clearer documentation would be,
    
    """Compute the trophies awarded to a user."""
  3. 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)
     
     
     
     
     
     
     
     
    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. 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. 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)
     
     
    This newline should go back.
  3. 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

Loading...