[Non-WIP] Add logic for computing and checking for trophies.

Review Request #4857 — Created Oct. 28, 2013 and submitted

Information

bzd
Review Board
master

Reviewers

Add functionality for trophy to be awarded to a user.

The trophy will be associated with the user, review request, and maybe a local site. It will be added to the database, and displayed on the review request page. Also added functionality for a subclass of TrophyType to be registered as a trophy. However, more work is needed to allow for importing trophies via extensions.

All unit tests pass. Checked with review requests 1000, 1001, and 3 and trophies were displayed correctly (or not at all). Also tested with a new database, filled with the fill-database script. Tested with review request ID's 1000, 10000, 1001, 2222, and 3. Appropriate trophies (or none at all) were awarded. The trophies' existence in the database was manually confirmed.

Description From Last Updated

This should be removed. We don't use these in Review Board, as a file will typically be modified and maybe …

chipx86chipx86

Blank line between these. Imports should be separated in upwards of 3 groups, as: Built-in Python modules. Third-party modules (those …

chipx86chipx86

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

chipx86chipx86

Unnecessary blank line.

ED edwlee

Docstrings should be in the form: """One-line summary.""" or: """One-line summary. Multi-line description. """

ED edwlee

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

chipx86chipx86

iftrophy shouldn't be the central point here for determining if there's a trophy for something. All logic around trophies should …

chipx86chipx86

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

chipx86chipx86

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

ED edwlee

Should be False, not 'false'. Also, you're setting this, but never saving the review request. When you save, you'll want …

chipx86chipx86

Too many blank lines.

ED edwlee

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'] = …

ED edwlee

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

chipx86chipx86

Blank line between these.

chipx86chipx86

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

chipx86chipx86

type is a reserved word.

chipx86chipx86

Imports should be sorted alphabetically.

ED edwlee

This doesn't seem right... You're overriding what extra_data is. I gather you're trying to create this field with a default …

chipx86chipx86

Too many blank lines.

ED edwlee

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

chipx86chipx86

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

chipx86chipx86

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

ED edwlee

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

ED edwlee

Same thing about docstrings as before.

ED edwlee

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

ED edwlee

Too many blank lines.

ED edwlee

This comment isn't particularly useful.

daviddavid

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

daviddavid

Trailing whitespace.

daviddavid

Please wrap this line to 80 columns.

daviddavid

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

daviddavid

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

daviddavid

Add a space between "#" and "Create"

daviddavid

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

daviddavid

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

daviddavid

Accessing review_request.submitter will give you the User object directly. What your code does is fetches the user object, calls its …

daviddavid

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)

daviddavid

Remove the space between """ and Testing

daviddavid

Same here.

daviddavid

You'll need to make an evolution for this, since you're adding a field to an existing model. We'll also probably …

daviddavid

Generally when using the 're' module, the entire module is imported, and then calls are made to re.match(). It's not …

daviddavid

Don't delete this line.

daviddavid

This code should move into the individual TrophyType subclasses, which should have a method that takes in a ReviewRequest, and …

daviddavid

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

daviddavid

It looks like you copied code from the scmtools (line 266 still says _scmtool_class), which works differently from the Review …

daviddavid

We should include the trophy category in the subclasses here, and add a class method to get the right subclass …

daviddavid

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

daviddavid

Leftover debug code.

daviddavid

Leftover debug code.

daviddavid

This could be shortened: review_request.extra_data['has_trophy_case'] = \ bool(calculated_trophy_types)

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

The trophy_id field isn't used anywhere, and if we ever have trophies that come from multiple people, it's going to …

daviddavid

Can we do localization here? Something like this: from django.utils.translation import ugettext_lazy _ ... def get_display_text(self, trophy): ... return _('%(user)s …

daviddavid

Can we wrap the title in _() ?

daviddavid

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

daviddavid

This line is too long. Can we wrap it? if (review_request.display_id >= 1000 and id_str == ''.join(reversed(id_str)):

daviddavid

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

daviddavid

Probably the next step here is to make the template tag iterate through the awarded trophies and have them insert …

daviddavid

We might as well let the database do the work here even in the empty case. You're also doing a …

daviddavid

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 …

daviddavid

You could use a list comprehension here, too: return [trophy_model.get_trophy_type()() for trophy_model in trophy_models]

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

This line is slightly misaligned.

daviddavid

You could combine these lines together.

daviddavid

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

daviddavid

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

daviddavid

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

chipx86chipx86

Blank line before this.

chipx86chipx86

Blank line before this.

chipx86chipx86

Preferably call this trophies and not trophy_models.

chipx86chipx86

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

chipx86chipx86

Blank line before this.

chipx86chipx86

Instead of creating and then saving, you can do: trophy = self.model.create(<params here>) Also, best to name this trophy and …

chipx86chipx86

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

chipx86chipx86

The point of the entry is to know whether or not we have to calculate again, so I think we …

chipx86chipx86

get_trophies I wouldn't use "model" everywhere. It's just not our convention. The "model" is the class definition and its schema, …

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

One-line summary, multi-line description. Cannot wrap or exceed a total of 79 characters for the line. A good way of …

chipx86chipx86

You can put review_request as a parameter to this, instead of pulling from kwargs. kwargs is just a representation of …

chipx86chipx86

Blank line before this.

chipx86chipx86

It isn't correct to call functions in a class definition. Any setup should happen in the setUp function for a …

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Two blank lines.

chipx86chipx86

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

chipx86chipx86

I'd maybe rename this for_category.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Same comment about wrapping.

chipx86chipx86

Should have a trailing comma.

chipx86chipx86

Two blank lines.

chipx86chipx86

We wrote @blocktag to prevent having to create these nodes. It does all this for you. Your entire template tag …

chipx86chipx86

"for a given" What do you mean by "instantiating existing and imported trophies?" I don't really see where it's doing …

chipx86chipx86

I think we want to unconditionally set this, since any review request without a trophy will otherwise have these calculations …

chipx86chipx86

This is duplicating the conditional above in compute_trophies. I think instead, we should either have compute_trophies not return existing trophies, …

chipx86chipx86

No blank line here.

chipx86chipx86

Nit - remove this blank line.

mike_conleymike_conley

In our other models, we put objects below the list of fields, with a blank line on either side.

chipx86chipx86

We should cache the result of this locally, like Tool.get_scmtool_class does.

chipx86chipx86

Should only call get_trophy_type once. Also, this function is missing a result when it can't find a valid TrophyType. However, …

chipx86chipx86

I'd just call this TrophyTests. There's not really a trophy "case" anywhere. We're just testing trophies.

chipx86chipx86

Likewise, we're awarding a trophy, not a trophy case.

chipx86chipx86

I'm really not understanding how these two tests are different. I assume when create_review_request is called, and the review requests …

mike_conleymike_conley

The """ should be all on one line for unit tests. Same with the others.

chipx86chipx86

calculated_trophies should be True already, since we published. You should instead be asserting that calculated_trophies is True.

chipx86chipx86

I'm really not sure why these calculated_trophies extra_data things are being set manually here... same below.

mike_conleymike_conley

Same as above. I don't see how these tests are different.

mike_conleymike_conley

I think we need from future import unicode_literals in every new file, right?

mike_conleymike_conley

This represents a type of a trophy, not an imported trophy.

chipx86chipx86

This should be image_url.

chipx86chipx86

To simplify this a bit, you can do: return _trophy_types.get(category, UnknownTrophy)

chipx86chipx86

"if the review request ID"

chipx86chipx86

It's registering a TrophyType subclass.

chipx86chipx86

"a type of trophy"

chipx86chipx86

_register_trophies itself should be safe-guarding here, like with HostingServices and stuff. Callers should be able to call _register_trophies multiple times …

chipx86chipx86

These aren't hosting services ;)

chipx86chipx86

"type of trophy"

chipx86chipx86

"TrophyType"

chipx86chipx86

"TrophyType"

chipx86chipx86

Missing the __future__ import discussed at the meeting. You can copy/paste it from other files in this directory.

chipx86chipx86

Blank line before this.

chipx86chipx86

This looks like the perfect use case for a template. Maybe I missed something, but why can't we use a …

mike_conleymike_conley

For readability, can you termiante strings after the \n? Actually, a better way to do things is to build a …

chipx86chipx86

If you use single quotes for the string, you won't need to escape the double quotes. It's also better to …

chipx86chipx86

Blank line before this.

chipx86chipx86
ED
  1. 
      
  2. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    Show all issues

    Unnecessary blank line.

  3. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    Show all issues

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

    or:

    """One-line summary.

    Multi-line description.
    """

  4. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    Show all issues

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

  5. reviewboard/accounts/managers.py (Diff revision 1)
     
     
     
    Show all issues

    Too many blank lines.

  6. reviewboard/accounts/managers.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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 ""
    
  7. reviewboard/reviews/models.py (Diff revision 1)
     
     
    Show all issues

    Imports should be sorted alphabetically.

  8. reviewboard/reviews/models.py (Diff revision 1)
     
     
     
    Show all issues

    Too many blank lines.

  9. reviewboard/reviews/models.py (Diff revision 1)
     
     
    Show all issues

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

  10. Show all issues

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

  11. Show all issues

    Same thing about docstrings as before.

  12. reviewboard/reviews/templatetags/reviewtags.py (Diff revision 1)
     
     
     
     
     
     
     
     
    Show all issues

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

  13. Show all issues

    Too many blank lines.

  14. 
      
BZ
chipx86
  1. 
      
  2. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    Show all issues

    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.

  3. reviewboard/accounts/managers.py (Diff revision 1)
     
     
     
    Show all issues

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

    1. Built-in Python modules.
    2. Third-party modules (those outside the project, like django)
    3. Modules within the project.
  4. reviewboard/accounts/managers.py (Diff revision 1)
     
     
     
     
    Show all issues

    Two blank lines here.

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

  5. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    Show all issues

    Comments should have sentence capitalization, and generally end with a period.

    Same with other comments in the file.

  6. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    Show all issues

    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.

  7. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    Show all issues

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

  8. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    Show all issues

    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.

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

    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.

  10. reviewboard/accounts/managers.py (Diff revision 1)
     
     
    Show all issues

    True, not 'true'.

    Same comment about saving and about the structure.

  11. reviewboard/accounts/managers.py (Diff revision 1)
     
     
     
    Show all issues

    Blank line between these.

  12. reviewboard/accounts/models.py (Diff revision 1)
     
     
    Show all issues

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

  13. reviewboard/accounts/models.py (Diff revision 1)
     
     
    Show all issues

    type is a reserved word.

  14. reviewboard/reviews/models.py (Diff revision 1)
     
     
     
    Show all issues

    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:
    
    1. Er, except the correct quoting style ('has_trophy_case'). I was doing Markdown quotes :P

  15. reviewboard/reviews/models.py (Diff revision 1)
     
     
     
    Show all issues

    Same comment about sentence capitalization.

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

  16. reviewboard/reviews/models.py (Diff revision 1)
     
     
     
    Show all issues

    What you want is:

    Trophy.objects.compute_trophies(self)
    
  17. 
      
BZ
BZ
BZ
BZ
david
  1. 
      
  2. reviewboard/accounts/managers.py (Diff revision 5)
     
     
    Show all issues

    This comment isn't particularly useful.

  3. reviewboard/accounts/managers.py (Diff revision 5)
     
     
    Show all issues

    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))
    
  4. reviewboard/accounts/managers.py (Diff revision 5)
     
     
    Show all issues

    Trailing whitespace.

  5. reviewboard/accounts/managers.py (Diff revision 5)
     
     
    Show all issues

    Please wrap this line to 80 columns.

  6. reviewboard/accounts/managers.py (Diff revision 5)
     
     
    Show all issues

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

  7. reviewboard/accounts/managers.py (Diff revision 5)
     
     
     
    Show all issues

    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
    
  8. reviewboard/accounts/managers.py (Diff revision 5)
     
     
    Show all issues

    Add a space between "#" and "Create"

  9. reviewboard/accounts/managers.py (Diff revision 5)
     
     
     
     
     
    Show all issues

    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

  10. reviewboard/accounts/managers.py (Diff revision 5)
     
     
    Show all issues

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

  11. reviewboard/accounts/managers.py (Diff revision 5)
     
     
    Show all issues

    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.

  12. reviewboard/accounts/managers.py (Diff revision 5)
     
     
    Show all issues

    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)
    
  13. reviewboard/accounts/tests.py (Diff revision 5)
     
     
    Show all issues

    Remove the space between """ and Testing

  14. reviewboard/accounts/tests.py (Diff revision 5)
     
     
    Show all issues

    Same here.

  15. reviewboard/reviews/models.py (Diff revision 5)
     
     
    Show all issues

    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.

  16. 
      
BZ
BZ
david
  1. This is looking pretty good! Just a couple trivial comments on code style.

  2. reviewboard/accounts/managers.py (Diff revision 6)
     
     
    Show all issues

    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.

  3. reviewboard/accounts/models.py (Diff revision 6)
     
     
    Show all issues

    Don't delete this line.

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

    1. I think my next step should be to make the code work with an abstract notion of trophy.

  2. 
      
BZ
BZ
BZ
david
  1. 
      
  2. reviewboard/accounts/managers.py (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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.

  3. reviewboard/accounts/managers.py (Diff revision 9)
     
     
    Show all issues

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

    1. By "trophy_bean_list", I meant to indictate that it will be a list of TrophyType subclasses (with string values) as opposed to a list of Trophy models.

    2. "bean" is 100% a java thing and has no meaning outside of java. Maybe say "trophy_types" instead?

  4. reviewboard/accounts/models.py (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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.

  5. reviewboard/accounts/trophies.py (Diff revision 9)
     
     
    Show all issues

    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):
            ...
    
    ...
    
  6. reviewboard/accounts/trophies.py (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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)
    
    1. Should this go in accounts/init.py similar to ui/init.py?

    2. I think keeping it in this same file is fine. reviews/ui/__init__.py is specific to Review UIs (which are split up into many other .py files), while accounts/__init__.py is for the accounts module in general.

  7. 
      
BZ
BZ
david
  1. 
      
  2. reviewboard/accounts/managers.py (Diff revision 12)
     
     
    Show all issues

    Leftover debug code.

  3. reviewboard/accounts/managers.py (Diff revision 12)
     
     
    Show all issues

    Leftover debug code.

  4. reviewboard/accounts/managers.py (Diff revision 12)
     
     
     
     
     
    Show all issues

    This could be shortened:

    review_request.extra_data['has_trophy_case'] = \
        bool(calculated_trophy_types)
    
  5. reviewboard/accounts/managers.py (Diff revision 12)
     
     
    Show all issues

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

  6. reviewboard/accounts/managers.py (Diff revision 12)
     
     
    Show all issues

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

  7. reviewboard/accounts/models.py (Diff revision 12)
     
     
     
     
     
     
    Show all issues

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

  8. reviewboard/accounts/trophies.py (Diff revision 12)
     
     
    Show all issues

    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.

  9. reviewboard/accounts/trophies.py (Diff revision 12)
     
     
    Show all issues

    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,
          }
    
  10. reviewboard/accounts/trophies.py (Diff revision 12)
     
     
    Show all issues

    Can we wrap the title in _() ?

  11. reviewboard/accounts/trophies.py (Diff revision 12)
     
     
    Show all issues

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

    1. How come we need to do this?

    2. Well, the purpose of the title field here is to be something that's presented to the user, right? If a user wants to use Review Board in chinese, we'd want to show the title of the trophy in chinese instead of english. the _() (which is an alias to ugettext because of the import I mentioned above), it will translate the string.

  12. reviewboard/accounts/trophies.py (Diff revision 12)
     
     
    Show all issues

    This line is too long. Can we wrap it?

    if (review_request.display_id >= 1000 and
        id_str == ''.join(reversed(id_str)):
    
  13. reviewboard/accounts/trophies.py (Diff revision 12)
     
     
    Show all issues

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

  14. reviewboard/templates/reviews/trophy_box.html (Diff revision 12)
     
     
     
     
     
     
     
     
    Show all issues

    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.

  15. 
      
BZ
BZ
BZ
BZ
BZ
BZ
david
  1. 
      
  2. reviewboard/accounts/managers.py (Diff revision 18)
     
     
     
     
     
     
     
     
    Show all issues

    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))
    
  3. reviewboard/accounts/managers.py (Diff revision 18)
     
     
     
     
     
     
    Show all issues

    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]
    
  4. reviewboard/accounts/managers.py (Diff revision 18)
     
     
     
     
     
     
     
    Show all issues

    You could use a list comprehension here, too:

    return [trophy_model.get_trophy_type()()
            for trophy_model in trophy_models]
    
  5. reviewboard/accounts/models.py (Diff revision 18)
     
     
    Show all issues

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

  6. reviewboard/accounts/trophies.py (Diff revision 18)
     
     
     
     
    Show all issues

    You could combine these lines:

    return (review_request.display_id >= 1000
            and re.match(r'^[1-9]0+$', id_str))
    
  7. reviewboard/accounts/trophies.py (Diff revision 18)
     
     
    Show all issues

    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.

  8. reviewboard/accounts/trophies.py (Diff revision 18)
     
     
    Show all issues

    This line is slightly misaligned.

  9. reviewboard/accounts/trophies.py (Diff revision 18)
     
     
     
     
    Show all issues

    You could combine these lines together.

    1. Do you mean writing it as the following? This would be longer than 80 characters across.

          if (review_request.display_id >= 1000 and id_str == ''.join(reversed(id_str))):
              return True
      
    2. I mean similar to the one above. Something like the following:

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

      Note that the conditional expression results in a bool. In general, it's nice to keep things as compact as possible, so we can skip the "if" and just return that bool directly.

  10. reviewboard/accounts/trophies.py (Diff revision 18)
     
     
    Show all issues

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

  11. reviewboard/accounts/trophies.py (Diff revision 18)
     
     
    Show all issues

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

  12. 
      
BZ
BZ
BZ
BZ
BZ
BZ
BZ
BZ
chipx86
  1. 
      
  2. reviewboard/accounts/managers.py (Diff revision 25)
     
     
     
     
     
    Show all issues

    Docstrings should be in the form of:

    """One-line summary.
    
    Multi-line description.
    """
    
  3. reviewboard/accounts/managers.py (Diff revision 25)
     
     
    Show all issues

    Blank line before this.

  4. reviewboard/accounts/managers.py (Diff revision 25)
     
     
    Show all issues

    Blank line before this.

  5. reviewboard/accounts/managers.py (Diff revision 25)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Preferably call this trophies and not trophy_models.

  6. reviewboard/accounts/managers.py (Diff revision 25)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    You can actually simplify this in a nice Pythonic way by doing:

    trophies = [
        self.model.create(...)
        for trophy_type in calculated_trophy_types
    ]
    
  7. reviewboard/accounts/managers.py (Diff revision 25)
     
     
    Show all issues

    Blank line before this.

  8. reviewboard/accounts/managers.py (Diff revision 25)
     
     
     
     
     
     
    Show all issues

    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.

  9. reviewboard/accounts/managers.py (Diff revision 25)
     
     
     
    Show all issues

    Blank line between these.

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

  10. reviewboard/accounts/managers.py (Diff revision 25)
     
     
    Show all issues

    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.

  11. reviewboard/accounts/managers.py (Diff revision 25)
     
     
    Show all issues

    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.

  12. reviewboard/accounts/managers.py (Diff revision 25)
     
     
    Show all issues

    "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."""
    
  13. reviewboard/accounts/managers.py (Diff revision 25)
     
     
     
     
    Show all issues

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

  14. reviewboard/accounts/models.py (Diff revision 25)
     
     
     
     
    Show all issues

    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.

  15. reviewboard/accounts/models.py (Diff revision 25)
     
     
     
    Show all issues

    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 _.

  16. reviewboard/accounts/models.py (Diff revision 25)
     
     
    Show all issues

    Blank line before this.

  17. reviewboard/accounts/tests.py (Diff revision 25)
     
     
     
    Show all issues

    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?

  18. reviewboard/accounts/tests.py (Diff revision 25)
     
     
     
     
    Show all issues

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

  19. reviewboard/accounts/tests.py (Diff revision 25)
     
     
    Show all issues

    No need to pop. Just compare with trophies[0] below.

    Same in other functions.

  20. reviewboard/accounts/trophies.py (Diff revision 25)
     
     
     
     
    Show all issues

    Two blank lines.

  21. reviewboard/accounts/trophies.py (Diff revision 25)
     
     
    Show all issues

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

  22. reviewboard/accounts/trophies.py (Diff revision 25)
     
     
    Show all issues

    I'd maybe rename this for_category.

    1. Rename the variable that?

    2. The method. So you'd call TrophyType.for_category(...)

  23. reviewboard/accounts/trophies.py (Diff revision 25)
     
     
     
     
    Show all issues

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

  24. reviewboard/accounts/trophies.py (Diff revision 25)
     
     
     
    Show all issues

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

  25. reviewboard/accounts/trophies.py (Diff revision 25)
     
     
     
    Show all issues

    Same comment about wrapping.

  26. Show all issues

    Should have a trailing comma.

  27. reviewboard/reviews/templatetags/reviewtags.py (Diff revision 25)
     
     
     
     
    Show all issues

    Two blank lines.

  28. Show all issues

    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.

  29. 
      
BZ
BZ
chipx86
  1. 
      
  2. reviewboard/accounts/managers.py (Diff revision 27)
     
     
    Show all issues

    "for a given"

    What do you mean by "instantiating existing and imported trophies?" I don't really see where it's doing that. It looks instead like it's computing trophies by looping through all registered trophy types and seeing if any apply to the review request.

  3. reviewboard/accounts/managers.py (Diff revision 27)
     
     
     
     
    Show all issues

    I think we want to unconditionally set this, since any review request without a trophy will otherwise have these calculations done every time. Having no trophies set is a valid result of calculating trophies.

  4. reviewboard/accounts/managers.py (Diff revision 27)
     
     
     
     
     
    Show all issues

    This is duplicating the conditional above in compute_trophies. I think instead, we should either have compute_trophies not return existing trophies, or we should have this just call compute_trophies internally. Probably the latter.

  5. reviewboard/accounts/models.py (Diff revision 27)
     
     
     
     
    Show all issues

    No blank line here.

  6. reviewboard/accounts/models.py (Diff revision 27)
     
     
    Show all issues

    In our other models, we put objects below the list of fields, with a blank line on either side.

  7. reviewboard/accounts/models.py (Diff revision 27)
     
     
    Show all issues

    We should cache the result of this locally, like Tool.get_scmtool_class does.

  8. reviewboard/accounts/models.py (Diff revision 27)
     
     
     
    Show all issues

    Should only call get_trophy_type once.

    Also, this function is missing a result when it can't find a valid TrophyType. However, I'm not sure if that's possible anymore, with UnknownTrophy. Maybe we should unconditionally call get_display_text now?

  9. reviewboard/accounts/tests.py (Diff revision 27)
     
     
     
    Show all issues

    I'd just call this TrophyTests.

    There's not really a trophy "case" anywhere. We're just testing trophies.

  10. reviewboard/accounts/tests.py (Diff revision 27)
     
     
    Show all issues

    Likewise, we're awarding a trophy, not a trophy case.

  11. reviewboard/accounts/tests.py (Diff revision 27)
     
     
     
    Show all issues

    The """ should be all on one line for unit tests.

    Same with the others.

  12. reviewboard/accounts/tests.py (Diff revision 27)
     
     
    Show all issues

    calculated_trophies should be True already, since we published. You should instead be asserting that calculated_trophies is True.

  13. reviewboard/accounts/trophies.py (Diff revision 27)
     
     
    Show all issues

    This represents a type of a trophy, not an imported trophy.

  14. reviewboard/accounts/trophies.py (Diff revision 27)
     
     
    Show all issues

    This should be image_url.

  15. reviewboard/accounts/trophies.py (Diff revision 27)
     
     
    Show all issues

    "if the review request ID"

  16. reviewboard/accounts/trophies.py (Diff revision 27)
     
     
    Show all issues

    It's registering a TrophyType subclass.

  17. reviewboard/accounts/trophies.py (Diff revision 27)
     
     
    Show all issues

    "a type of trophy"

  18. reviewboard/accounts/trophies.py (Diff revision 27)
     
     
     
    Show all issues

    _register_trophies itself should be safe-guarding here, like with HostingServices and stuff. Callers should be able to call _register_trophies multiple times without problems.

  19. reviewboard/accounts/trophies.py (Diff revision 27)
     
     
    Show all issues

    These aren't hosting services ;)

  20. reviewboard/accounts/trophies.py (Diff revision 27)
     
     
    Show all issues

    "type of trophy"

  21. reviewboard/accounts/trophies.py (Diff revision 27)
     
     
    Show all issues

    "TrophyType"

  22. reviewboard/accounts/trophies.py (Diff revision 27)
     
     
    Show all issues

    "TrophyType"

  23. Show all issues

    Missing the __future__ import discussed at the meeting. You can copy/paste it from other files in this directory.

  24. Show all issues

    Blank line before this.

  25. reviewboard/reviews/templatetags/reviewtags.py (Diff revision 27)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    For readability, can you termiante strings after the \n?

    Actually, a better way to do things is to build a list of string lines, instead of one big string. It's faster, and you can structure your code here in terms of lines.

    So:

    lines = []
    
    ...
    
    lines.append('<div class="...">')
    
    ...
    
    return '\n'.join(lines)
    
  26. Show all issues

    If you use single quotes for the string, you won't need to escape the double quotes.

    It's also better to put the indentation at the beginning of the line for that string, to help see the visual structure, instead of right after the \n

  27. Show all issues

    Blank line before this.

  28. 
      
mike_conley
  1. 
      
  2. reviewboard/accounts/models.py (Diff revision 27)
     
     
    Show all issues

    Nit - remove this blank line.

  3. reviewboard/accounts/tests.py (Diff revision 27)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    I'm really not understanding how these two tests are different. I assume when create_review_request is called, and the review requests are published, the _call_compute_trophies function is going to be called, which will set calculated_trophies once its done computing.

    So I don't really know what this "without initial trophy case" test is testing.

  4. reviewboard/accounts/tests.py (Diff revision 27)
     
     
    Show all issues

    I'm really not sure why these calculated_trophies extra_data things are being set manually here... same below.

  5. reviewboard/accounts/tests.py (Diff revision 27)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Same as above. I don't see how these tests are different.

  6. reviewboard/accounts/trophies.py (Diff revision 27)
     
     
    Show all issues

    I think we need from future import unicode_literals in every new file, right?

  7. reviewboard/reviews/templatetags/reviewtags.py (Diff revision 27)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    This looks like the perfect use case for a template. Maybe I missed something, but why can't we use a template?

    1. It was suggested that I do it like this and that it can be refactored later.

  8. 
      
BZ
chipx86
  1. This looks great! One small nifty trick you can do to save a couple lines.

  2. reviewboard/accounts/trophies.py (Diff revisions 27 - 28)
     
     
     
     
     
    Show all issues

    To simplify this a bit, you can do:

    return _trophy_types.get(category, UnknownTrophy)
    
  3. 
      
BZ
BZ
BZ
Review request changed
Status:
Completed
Change Summary:
Pushed to master (a91739c)