Unnecessary blank line.

Docstrings should be in the form:

   """One-line summary."""
or:
"""One-line summary.
Multi-line description.

   """

You can do 'if not id or id < 1000'.

Too many blank lines.

You can combine these logic to something like:
if not interesting:
    if id_str == ''.join(reversed(id_str)):
        context['palindrome'] = True
        review_request.extra_data['has_trophy_case'] = 'true'
    else:
        context.pop()
        return ""

Imports should be sorted alphabetically.

Too many blank lines.

You don't need to pass in self as an argument.

Might be better to name this 'has_trophy' or 'has_trophy_assigned'.

Same thing about docstrings as before.

Comments shouldn't span across conditions. It's better if you combine and put them before the opening if statement.

Too many blank lines.

This should be removed. We don't use these in Review Board, as a file will typically be modified and maybe partially rewritten by many people over its lifetime.

Blank line between these. Imports should be separated in upwards of 3 groups, as:

Built-in Python modules.
Third-party modules (those outside the project, like django)
Modules within the project.

Two blank lines here.
Also, you wouldn't put parens around the import unless the imports flow over several lines.

Comments should have sentence capitalization, and generally end with a period.
Same with other comments in the file.

iftrophy shouldn't be the central point here for determining if there's a trophy for something. All logic around trophies should exist in this class, not in templatetags anywhere.
Since iftrophy is really trivial, and special-cased to this function's usage, that logic should just move directly into here.

Call this display_id or something. id is a reserved word in Python.

Should be False, not 'false'.
Also, you're setting this, but never saving the review request.
When you save, you'll want to do:
review_request.save(update_fields=['extra_data'])

That'll ensure that we write only what's necessary here, and won't risk stomping on other fields.
Also, this code should be structured o that we only ever set has_trophy_case once, to the computed value based on the logic in this function, and save it once.

From your pastie, I know that you've removing context and stuff from this function, which is great. That's exactly what you want to do.
This function should also, by the end of the project, not directly calculate these types of trophies. Instead, it should interface with pluggable trophy classes that calculate trophies for the review request, so that we can extend the trophy types via plugins. That can go in after this, though.

True, not 'true'.
Same comment about saving and about the structure.

Blank line between these.

This class is missing a docstring, and missing an objects = TrophyManager to associate with the manager.

type is a reserved word.

This doesn't seem right... You're overriding what extra_data is.
I gather you're trying to create this field with a default value in it. You shouldn't create default value in any extra_data. Instead, you should just check for the existence of a key in extra_data, and make decisions based on that. You can do so with:
if `has_trophy_case` in review_request.extra_data:

Same comment about sentence capitalization.
This comment is very narrow, width-wise. You can fit more on a line.

What you want is:
Trophy.objects.compute_trophies(self)

This comment isn't particularly useful.

Because this is in the manager, "self.model.objects" is the same as "self", so this can be simplified:

for trophy in self.filter(review_request=display_id):


You could even replace this with a list constructor:

existing_trophies = list(self.filter(review_request=display_id))

Trailing whitespace.

Please wrap this line to 80 columns.

To help with the unicode work I'm doing right now, can you change str to unicode here?

It might be nice to use a regular expression here instead of string multiplication + comparison. Something like this:

if re.match(r'^[1-9]0+$', id_str):
    # do stuff

Add a space between "#" and "Create"

You shouldn't need to do this. Because of the way foreign keys work in django, review_request.local_site will resolve to be either the LocalSite object or None

You already have the review request object (it was passed into your method).

Accessing review_request.submitter will give you the User object directly. What your code does is fetches the user object, calls its __str__ method (coercing to string), which returns the username, then queries for that username again.
Since you only use this once, might as well do it inline. Will show below.

With the above changes for foreign keys, this line might look like this:

trophy = self.model(category=trophy_category,
                    review_request=review_request,
                    local_site=review_request.local_site,
                    user=review_request.submitter)

Remove the space between """ and Testing

Same here.

You'll need to make an evolution for this, since you're adding a field to an existing model.
We'll also probably want to have an IRC discussion at some point about how this field should work with drafts.

This is looking pretty good! Just a couple trivial comments on code style.

Generally when using the 're' module, the entire module is imported, and then calls are made to re.match(). It's not incorrect to do it this way, but I think the other way is more conventional.

Don't delete this line.

I don't have any more comments about this WIP patch. Do you know what your next step is?

This code should move into the individual TrophyType subclasses, which should have a method that takes in a ReviewRequest, and we should iterate over the TrophyTypes to have them each run their own calculation.

Why did you rename this to "trophy_bean_list"??? What is a trophy bean?

It looks like you copied code from the scmtools (line 266 still says _scmtool_class), which works differently from the Review UI registration pattern.
If you make the changes I suggested below to the TrophyType code, this becomes very simple:


def get_trophy_type(self):
    trophy_type = TrophyType.get_type(self.category)


Note that it's possible that the trophy_type might be None, in which case we'd probably want to hide it from view.

We should include the trophy category in the subclasses here, and add a class method to get the right subclass based on a category name.
Something like this:

class TrophyType:
    ... (keep the stuff you have) ...

    @classmethod
    def get_type(cls, category):
        for trophy_type in _trophy_types:
            if trophy_type.category == category:
                return trophy_type


class MilestoneTrophy(TrophyType):
    category = 'milestone'

    def __init__(self, received_date):
        ...

...

This is a start, but you'll also need some code to register your built-in trophy types. Something like this:


from reviewboard.signals import initializing


def _register_trophies(**kwargs):
    register_trophy(MilestoneTrophy)
    register_trophy(PalindromeTrophy)


initializing.connect(_register_trophies)

Leftover debug code.

Leftover debug code.

This could be shortened:

review_request.extra_data['has_trophy_case'] = \
    bool(calculated_trophy_types)

You don't need to say "is True", you can just say if review_request['has_trophy_case']: and it will check for truthiness.

This should just be "else:" instead of elif.

TrophyType.get_type() already returns None if it doesn't find anything, so you can just return TrophyType.get_type(self.category) here.

The trophy_id field isn't used anywhere, and if we ever have trophies that come from multiple people, it's going to be hard to keep these unique. I think you should just remove it.

Can we do localization here? Something like this:

from django.utils.translation import ugettext_lazy _

...

   def get_display_text(self, trophy):
      ...
      return _('%(user)s got review request #%(id)!') % {
          'user': full_name,
          'id': review_request_id,
      }

Can we wrap the title in _() ?

We should change the title of this to be _('Fish Trophy')

This line is too long. Can we wrap it?

if (review_request.display_id >= 1000 and
    id_str == ''.join(reversed(id_str)):

Can we add a get_display_text method here that says "XXX got a fish trophy!"?

Probably the next step here is to make the template tag iterate through the awarded trophies and have them insert their own HTML instead of having the hard-coded stuff in here.

We might as well let the database do the work here even in the empty case. You're also doing a weird lookup with display_id in there when the foreign key will give you the related object directly:

if 'has_trophy_case' in review_request.extra_data:
    return list(self.filter(review_request=review_request))

The Trophy.get_display_text doesn't take in any additional parameters (the one "self" parameter is provided for you because it's a bound method).
This would also be a convenient place to use a list comprehension:

return [trophy_model.get_display_text()
        for trophy_model in trophy_models]

You could use a list comprehension here, too:

return [trophy_model.get_trophy_type()()
        for trophy_model in trophy_models]

Instead of using .count() == 0, you can use .empty(), which is a more efficient database query.

You could combine these lines:

return (review_request.display_id >= 1000
        and re.match(r'^[1-9]0+$', id_str))

Can we change the title here to "Fish Trophy"? A lot of our amusement from this comes from it being a bit of a mystery.

This line is slightly misaligned.

You could combine these lines together.

This line is indented too far. It should be either aligned exactly or indented 4 spaces compared to "review_request" on the previous line.

You could just move the call to trophy.user.get_full_name into the dict definition and make this 1 line shorter.

Docstrings should be in the form of:
"""One-line summary.

Multi-line description.
"""

Blank line before this.

Blank line before this.

Preferably call this trophies and not trophy_models.

You can actually simplify this in a nice Pythonic way by doing:
trophies = [
    self.model.create(...)
    for trophy_type in calculated_trophy_types
]

Blank line before this.

Instead of creating and then saving, you can do:
trophy = self.model.create(<params here>)

Also, best to name this trophy and not trophy_model.

Blank line between these.
It's good being able to see where block statements begin/end when they cause indentation changes.

The point of the entry is to know whether or not we have to calculate again, so I think we want this to always be True.
I don't think has_trophy_case properly conveys what this is. calculated_trophies is more correct.
Also, this function should simply return right away if this is already True, so we don't repeat all this.

get_trophies
I wouldn't use "model" everywhere. It's just not our convention. The "model" is the class definition and its schema, but things interacting with it are just dealing with it as what it is. A representation of a trophy, not a DB model.

"any possible" is not clear, and "This function" isn't really useful. I'd say something like:
"""Returns a list of trophies fo a review request."""

This function seems pointless. Callers should just call the right thing on the trophy itself.

One-line summary, multi-line description. Cannot wrap or exceed a total of 79 characters for the line.
A good way of approaching these is not to think about what it's technically doing (like here, what it's attached to), but by what it is.
"A trophy represents an achievement given to the user.", and then go into details in the description.

You can put review_request as a parameter to this, instead of pulling from kwargs. kwargs is just a representation of all parameters passed to a function by keyword.
Also, given that this function is internal, it's best to prefix it with a _.

Blank line before this.

It isn't correct to call functions in a class definition. Any setup should happen in the setUp function for a test suite.
You shouldn't even need to register these. It should happen automatically when you fetch trophy types, right?

Please be sure to use the same format for these test docstrings that we use elsewhere in the codebase.

No need to pop. Just compare with trophies[0] below.
Same in other functions.

Two blank lines.

This and all other classes in this file are missing a docstring.

I'd maybe rename this for_category.

You should explicitly return None at the end if nothing matches, and then be sure to handle that in all call sites appropriately.

This looks to be exceeding 79 characters. Wrap the parameters to the following line after __init__.

Same comment about wrapping.

Should have a trailing comma.

Two blank lines.

We wrote @blocktag to prevent having to create these nodes. It does all this for you. Your entire template tag should be very similar to ifneatnumber.
You also shouldn't hard-code HTML in a template tag. You should be doing exactly what ifneatnumber is doing. Passing in context, and letting the template render it how it chooses.