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

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

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

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

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

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

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

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

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

Hey Hiro,

Looking good! I have a few suggestions below - let me know if you have any questions on those.

Specifically, I think you could expand on a few areas (though the others might have other suggestions as well):

* We need to be able to handle multiple trophies per review request
* We need to move the trophy "type" logic out into the TrophyManager. TemplateTags, etc, should not care about palindromes or milestones. This adds a maintenance burden, and is fragile.
* We should try to make the types of Trophy's extendable

Thanks for your work,

-Mike

I can see this getting unwieldy as we add more and more trophies.

I wonder if it'd be cleaner to have a collection of trophy classes, where each trophy type has some kind of "qualifies" function to determine if the trophy type should be added.

Then, we just iterate through our list of trophy classes, and run the qualifies function on the review_request and user. If it returns true, we add a Trophy with the type specified by the class, and keep iterating.

If we allow this collection of Trophy classes to be accessed and written to, this would also make it easier for extensions to add new Trophy types.

Magic numbers like this make for fragile tests, so that if somebody were to change the fixtures, this test would break.

It looks like you're trying to test that the review request is saved and added to the database table. I don't think you need to test that in this file - I think that's more the responsibility of the tests in reviewboard/reviews/tests.py

So I think this test can be simplified somewhat - I would remove the parts that ensure that a new row was added. Just assume that if review_request1.save() doesn't throw an error that it successfully added the row.

What I *would* test though, is that there is an increase in the number of trophies after this test. Take a count of the trophies, and ensure that the number of trophies after the test is count + 1.

I'm curious - why are you cloning a review request instead of just creating a new one?

Only the id is checked for milestones and palindromes...why are you setting the local_site_id's and local_id to the same value?

You don't need the assertNotEqual if you just to assertEqual for "milestone". If the type == "milestone", this implies != "palindrome".

Same comments from above apply to this test as well. Remove the testing that checks if the review request was added, but add checks to ensure that a trophy was added. No need to check if the trophy_type is not a milestone.

And same comments here as well - no need to test the saving of the review request row. No need to deepcopy.

This tag obsoletes ifneatnumber. You should remove ifneatnumber and replace it with iftrophy.

You'll need to update the places ifneatnumber is called too.

I think this might be too fragile as we add more types of trophies.

So, it looks like this variable is being used inside trophy_box.html (templates/reviews/trophy_box.html) to decide which trophy icon to display.

Maybe instead, we should ask the TrophyManager to give us details for each trophy - for example, perhaps each trophy could return its own template to render.

trophy might be plural, so I'd call it trophies. Also, I'm pretty sure you can simplify this like so:

trophies = Trophy.objects.filter(review_request=rid)

You might need to do a little mucking about in trophy_box.html in order for it to handle multiple trophies.

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

Col: 41
 E126 continuation line over-indented for hanging indent

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

Col: 1
 E302 expected 2 blank lines, found 1

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

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

Col: 5
 E303 too many blank lines (2)

Col: 41
 E126 continuation line over-indented for hanging indent

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

Col: 1
 E302 expected 2 blank lines, found 1

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

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

Col: 5
 E303 too many blank lines (2)

Col: 41
 E126 continuation line over-indented for hanging indent

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

Col: 41
 E126 continuation line over-indented for hanging indent

This looks pretty good, Hiro! Some suggestions / notes below.

Thanks,

-Mike

Where is logging used?

No newline here.

Where is timezone used?

Hm - I wonder if we might want an additional parameter here for a unique-ID for the trophy.

The title is probably going to be seen by the user. It might have spaces and non-alphanumeric symbols in it, etc. We might want an ID field to use as the key in the dict instead of title.

Elsewhere in the codebase, we use self.description - let's be consistent and use the full word here too.

We generally use potholes for our methods - like is_qualified.

Please use a docstring like,

def isQualified(self, user, review_request):
    """Determines if the user qualifies for a trophy.

       [Longer explanation, including directive to override]
    """

Please include documentation on what each Trophy does and what is required to qualify.

The pattern we like to use to initialize via parent-class is:

super(MilestoneTrophy, self).__init__('rb/images/trophy.png',
               'milestone',
               'milestone trophy')

This should be a more descriptive description, and maybe should use the _ translation method that you see imported elsewhere (see https://github.com/reviewboard/reviewboard/blob/master/reviewboard/scmtools/svn.py#L388 for example. You must import the underscore like this: https://github.com/reviewboard/reviewboard/blob/master/reviewboard/scmtools/svn.py#L14)

Same requests as in MilestoneTrophy.

Can you elaborate on what TrophyManager is responsible for in this documentation?

This sort of initialization code should be put inside an __init__ function...

Although, now, come to think of it - I wonder if it just makes more sense to have trophy_dict exist *outside* of the TrophyManager - so, after class TrophyManager definition, maybe:

_all_trophies = [MilestoneTrophy, PalindromeTrophy]
_trophy_dict = {}
for trophy in _all_trophies:
  _trophy_dict[trophy.id] = trophy

What is calling this? If this isn't actually required, we should remove it...

Edit: I notice that you reached in and read trophy_dict directly. Maybe that's where you should use this function instead.

And call it something like get_trophy.

Nit - capitalize "computer".  Pluralize trophy, since more than one trophies might be awarded.

Maybe do this import at the top of the file, unless there's a good reason to constrain it to this function.

Nit - space after #

Swap these two lines - we like to do our imports in alphabetical order (diffviewer comes after accounts)

We really appreciate tests like this. Good work. :)

Here's where maybe we should use get_trophy instead of reaching in and indexing the dict directly.

Needs to be put after the reviewboard.attachments import, for alphabetical ordering.

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

Col: 67
 W291 trailing whitespace

Col: 41
 E126 continuation line over-indented for hanging indent

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

Col: 41
 E126 continuation line over-indented for hanging indent

This is looking great! There's some stylistic stuff to change, and a few better ways of doing things in Python and Django, but overall I'm very happy with this.

This isn't a manager, so this file is the wrong place for it. I'd say trophies.py.

Should be on the same line, like:

    """Determines if the user...

Same with other docstrings in your change.

This needs to be review_request.display_id.

Same with other trophies in your code.

Basically, "id" is the ID in the database, but that's not necessarily the ID people will see. If they're using Local Sites (a way to split a Review Board installation into several mini-installations), the real ID and the ID they'll see are different.

Blank line between these.

Blank line between these.

Blank line between these.

Blank line between these.

Blank line between these.

This is a manager, so this is the right place for it :) Just the trophy classes above should move.

This seems wrong.

Best to use itervalues()

What you want here is:

Trophy.objects.create(trophy_type=...,
                      review_request=...
                      user=...)

No need for this.

Should be in the list alphabetically (before reviewboard.reviews.models)

You can get rid of this blank line.

"review request"

The summary line should be on the line with """, then a blank line, then the rest of the description.

"in TrophyManager"

"review request"

This can just be written as:

    if not trophies:

There's a nice Pythonic way of doing this called "list comprehensions."

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

Only one blank line needed.

You can make this one line pretty easily.

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

Col: 41
 E126 continuation line over-indented for hanging indent

Just some documentation nits, but I'm really happy with this. With these fixed, I think I'm ready to give it my ship-it.

I'd replace this with:

This manager computes whether trophies should be created and associated with a review request.

Needs documentation, like "takes a trophy model and returns the abstract representation of the trophy".

boolean(True or False) is redundant. How about just:

Subclasses should override this function and return true if the user qualifies for the trophy for this review request.

How about:

The MilestoneTrophy is for review requests with interesting IDs.

Specifically, a user qualifies for this trophy when their review request has an ID >= 1000, and consists of a single digit followed by all zeros.

Ex: id=1000, id=2000, id=10000

12000? Are you sure?

It looks like the trophy only returns true iff the *first* number is non-zero and followed by all zeros, for numbers >= 1000. I don't think it qualifies if there are two non-zero numbers followed by all zeros.

The PalindromeTrophy is for review requests with interesting IDs.

Specifically, this trophy is awarded when a user opens a review request where the ID is a palindrome (the same forwards as it is backwards).

Ex: id=1221, id=1111, id=12321

Not true - the trophies are defined in reviewboard.accounts.trophies

"The contained contents are rendered" - not too clear. How about,

"an appropriate banner is displayed"

Let's put these in alphabetical ordering, so:

from reviewboard.accounts.models import LocalSiteProfile, Profile, Trophy

Maybe add a test here to make sure 12000 doesn't resolve to a milestone trophy.

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

Col: 41
 E126 continuation line over-indented for hanging indent

These are my last two suggestions, I'm almost positive.

It might be better to have trophies.py track this list. That way, TrophyManager doesn't have to know what trophies are available - it just acts as an interface to access them.

So something like:

from reviewboard.accounts.trophies import trophy_list

and then in trophies.py:

trophy_list = []

// define SomeTrophy

trophy_list.append(SomeTrophy())

// define AnotherTrophy

trophy_list.append(AnotherTrophy())

I don't think this comment tells us much that we didn't already know. You should say what a Trophy does / what it represents.

Something like,

The Trophy model keeps a record of trophies awarded to users.

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

Col: 41
 E126 continuation line over-indented for hanging indent

I'm quite happy with this. You have my ship-it, Hiroki! Great work!