• 
      

    User Infobox Improvements

    Review Request #7662 — Created Sept. 26, 2015 and discarded

    Information

    Review Board
    master

    Reviewers

    When hovering over a user's name, an info box appears showing their avatar/photo, name, e-mail address, and some other information. It's somewhat useful, but could be more useful.
    This change adds a couple new pieces of information:

    1) Their timezone and how many hours ahead/behind you they are (to help determine if they'd be available at the moment).
    2) Some review request stats (how many review requests they have been assigned to, and how many they have had out for review).

    A new UserInfoExtensionHook is also created to provide additional information, and associated entity tag data, for this box, should the need arise. That way, third-parties can integrate other services they use with this box. Documentation and example usage for the new hook are also provided.

    Moused over usernames to verify that the infobox appears with the new (and old) pieces of information displays correctly.
    Overrode get_extra_context to ensure that additional information added via the extension hook displays correctly.


    Description From Last Updated

    Just saying how far ahead/behind forces me to do math. Can we imitate what Slack does? They have: <clock symbol> …

    daviddavid

    There's a mismatch in styles here. We should aim for consistency, or separate them out more by whitespace. Also, the …

    chipx86chipx86

    Now that we're showing this using a time notation, we can probably skip the word "hours" ("4:00 hours" seems weird, …

    daviddavid

    'tzinfo' imported but unused

    reviewbotreviewbot

    'timedelta' imported but unused

    reviewbotreviewbot

    Col: 47 E231 missing whitespace after ','

    reviewbotreviewbot

    Col: 5 E303 too many blank lines (2)

    reviewbotreviewbot

    Col: 22 E261 at least two spaces before inline comment

    reviewbotreviewbot

    Col: 64 E226 missing whitespace around arithmetic operator

    reviewbotreviewbot

    Col: 31 E226 missing whitespace around arithmetic operator

    reviewbotreviewbot

    Col: 34 E226 missing whitespace around arithmetic operator

    reviewbotreviewbot

    Col: 37 E226 missing whitespace around arithmetic operator

    reviewbotreviewbot

    Col: 40 E226 missing whitespace around arithmetic operator

    reviewbotreviewbot

    Col: 60 E226 missing whitespace around arithmetic operator

    reviewbotreviewbot

    Col: 27 E226 missing whitespace around arithmetic operator

    reviewbotreviewbot

    Col: 30 E226 missing whitespace around arithmetic operator

    reviewbotreviewbot

    Col: 33 E226 missing whitespace around arithmetic operator

    reviewbotreviewbot

    Col: 36 E226 missing whitespace around arithmetic operator

    reviewbotreviewbot

    Col: 24 E261 at least two spaces before inline comment

    reviewbotreviewbot

    Col: 64 E226 missing whitespace around arithmetic operator

    reviewbotreviewbot

    Col: 26 E226 missing whitespace around arithmetic operator

    reviewbotreviewbot

    Col: 29 E226 missing whitespace around arithmetic operator

    reviewbotreviewbot

    Col: 32 E226 missing whitespace around arithmetic operator

    reviewbotreviewbot

    Col: 35 E226 missing whitespace around arithmetic operator

    reviewbotreviewbot

    Col: 60 E226 missing whitespace around arithmetic operator

    reviewbotreviewbot

    Col: 26 E226 missing whitespace around arithmetic operator

    reviewbotreviewbot

    Col: 29 E226 missing whitespace around arithmetic operator

    reviewbotreviewbot

    Col: 32 E226 missing whitespace around arithmetic operator

    reviewbotreviewbot

    Col: 35 E226 missing whitespace around arithmetic operator

    reviewbotreviewbot

    Col: 10 E261 at least two spaces before inline comment

    reviewbotreviewbot

    Col: 64 E226 missing whitespace around arithmetic operator

    reviewbotreviewbot

    Col: 75 E225 missing whitespace around operator

    reviewbotreviewbot

    Col: 13 E122 continuation line missing indentation or outdented

    reviewbotreviewbot

    Col: 60 E226 missing whitespace around arithmetic operator

    reviewbotreviewbot

    Col: 75 E225 missing whitespace around operator

    reviewbotreviewbot

    These imports need to be broken up. We organize our imports as following: from __future__ import unicode_literals # Standard library …

    brenniebrennie

    'django' imported but unused

    reviewbotreviewbot

    No blank line here.

    brenniebrennie

    Col: 32 E225 missing whitespace around operator

    reviewbotreviewbot

    Col: 28 E226 missing whitespace around arithmetic operator

    reviewbotreviewbot

    Col: 36 E225 missing whitespace around operator

    reviewbotreviewbot

    Col: 42 E226 missing whitespace around arithmetic operator

    reviewbotreviewbot

    This should be six.text_type (i.e., unicode) instead of str. Also This will have to use fomratting so we can localize …

    brenniebrennie

    Col: 36 E225 missing whitespace around operator

    reviewbotreviewbot

    Col: 5 E303 too many blank lines (3)

    reviewbotreviewbot

    Col: 57 E225 missing whitespace around operator

    reviewbotreviewbot

    I would break this up as: 'users_stats': { 'assigned': num_requests_assigned, 'posted': num_requests_posted, # ... } Then you can do the …

    brenniebrennie

    Col: 1 W391 blank line at end of file

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

    No need to sign your work here. :)

    mike_conleymike_conley

    This is essentially the same value as requsr_dttemp. Can't we re-use the same value for both, and just call it …

    mike_conleymike_conley

    Converting the UTC datetime into a string and then extracting things from the string and converting them back into integers …

    mike_conleymike_conley

    I think it'd maybe make more sense do put the tz and hours values into the context and pass them …

    mike_conleymike_conley

    We already have the user, no?

    mike_conleymike_conley

    It seems to me we should be able to narrow down the review_requests with a QuerySet to find the cases …

    mike_conleymike_conley

    I think we should be fine to pass these down to the template as ints.

    mike_conleymike_conley

    "Assigned:" and "Posted:" aren't going to be translated. Let's send down just the raw integers, and do the translation in …

    mike_conleymike_conley

    Col: 1 E302 expected 2 blank lines, found 1

    reviewbotreviewbot

    Col: 1 E302 expected 2 blank lines, found 1

    reviewbotreviewbot

    undefined name 'e'

    reviewbotreviewbot

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

    reviewbotreviewbot

    Col: 13 W292 no newline at end of file

    reviewbotreviewbot

    Col: 1 E302 expected 2 blank lines, found 1

    reviewbotreviewbot

    Col: 58 E251 unexpected spaces around keyword / parameter equals

    reviewbotreviewbot

    Col: 60 E251 unexpected spaces around keyword / parameter equals

    reviewbotreviewbot

    Col: 22 E225 missing whitespace around operator

    reviewbotreviewbot

    Col: 5 E303 too many blank lines (2)

    reviewbotreviewbot

    Col: 5 E265 block comment should start with '# '

    reviewbotreviewbot

    Col: 28 E226 missing whitespace around arithmetic operator

    reviewbotreviewbot

    "HTML"

    chipx86chipx86

    A couple things: 1) When showing examples like this, we want them in literal blocks. 2) I have no idea …

    chipx86chipx86

    This shouldn't be here.

    chipx86chipx86

    These docs, just like the ones above, will be generated and made available on our site. This needs to be …

    chipx86chipx86

    No blank line here.

    chipx86chipx86

    This shouldn't take a list of sections. That's going to lock us into a particular design, and it's going to …

    chipx86chipx86

    Missing a docstring. No blank line here.

    chipx86chipx86

    Two blank lines here.

    chipx86chipx86

    Col: 1 E302 expected 2 blank lines, found 1

    reviewbotreviewbot

    This is a third-party module.

    chipx86chipx86

    This must be inserted alphabetically.

    chipx86chipx86

    This blank line must remain.

    chipx86chipx86

    I'm going to want to see all this logic heavily documented, so every bit of it is clear.

    chipx86chipx86

    What's all this?

    chipx86chipx86

    <html> means the start of a document. We never want that, or we'll confuse browsers.

    chipx86chipx86

    On a large deployment, this may produce thousands or tens of thousands of review requests. Since we're just counting, we …

    chipx86chipx86

    Blank line between these.

    chipx86chipx86

    Missing a trailing comma.

    chipx86chipx86

    These should be one statemnet.

    chipx86chipx86

    Make sure all the content aligns properly with the current indentation level.

    chipx86chipx86

    'format_html' imported but unused

    reviewbotreviewbot

    'mark_safe' imported but unused

    reviewbotreviewbot

    Col: 39 E231 missing whitespace after ','

    reviewbotreviewbot

    Col: 9 E303 too many blank lines (2)

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

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

    reviewbotreviewbot

    Col: 13 E123 closing bracket does not match indentation of opening bracket's line

    reviewbotreviewbot

    Col: 43 E231 missing whitespace after ','

    reviewbotreviewbot

    Col: 51 E202 whitespace before ')'

    reviewbotreviewbot

    This should start with .. code-block:: python

    daviddavid

    Docstrings should start with a 1-line summary, then a blank line, then additional paragraphs of explanation.

    daviddavid

    We should check that template_name is a string, because otherwise we'll call render_to_string and pass it None. Alternatively, just get …

    daviddavid

    ExtensionHook doesn't define get_extra_context so the super call is superfluous and useless.

    daviddavid

    Remove this line.

    daviddavid

    We should just include the <span> in the template, instead of passing it in like this.

    daviddavid

    Please put this line back.

    daviddavid

    'format_html' imported but unused

    reviewbotreviewbot

    This should line up with the c in code-block.

    brenniebrennie

    This should line up with the c in code-block.

    brenniebrennie

    Can you reformat this as follows? raise ValueError( 'Invalid UserInfoBox section: %s; must be one of %s' % (section, ', …

    brenniebrennie

    Can you reformat this as: return render_to_string(self.template_name, RequestContext(request, context)) It's easier to read this way.

    brenniebrennie

    Remove this line.

    brenniebrennie

    This comment is unnecessary.

    brenniebrennie

    Single quotes. Also, this should be six.text_type, not str: from django.utils import six The reason we use this is that …

    brenniebrennie

    Can you put UserInfoBoxHook.render on one line? that way its clear there is not supposed to be a space between …

    brenniebrennie

    This should be '... extension: "', extension.id, e, exc_info=True) (I know some places use exc_info=1, but we aren't doing that …

    brenniebrennie

    This blank line isn't necessary when there's no docstring.

    daviddavid

    I'm not sure what "custom parameters..." means here, and it's not something that users can change (since render chooses what …

    daviddavid

    get_extra_context should return a dict, not a str.

    daviddavid

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

    reviewbotreviewbot

    There are fractional time zones that aren't 30 minutes (for example, Kathmandu, Nepal is GMT +5:45). I don't think this …

    daviddavid

    We should pass the raw numbers in to the template and put the text there.

    daviddavid

    As is, this won't include any of the arguments because the first argument doesn't have any format specifiers. You can …

    daviddavid

    Col: 30 E201 whitespace after '('

    reviewbotreviewbot

    local variable 'e' is assigned to but never used

    reviewbotreviewbot

    How about the following: # We round the timezones while minutes >= 10: if ahead: hr_diff += 0.25 else: hr_diff …

    brenniebrennie

    elif

    brenniebrennie

    When interpolating only one variable, you don't need to use dict interpolation.

    brenniebrennie

    So string additions are slow. How about: custom_info_parts = [] for hook in UserInfoBoxHooks.hooks: try: custom_info_parts.append(hook.render(user, request)) except Exception: # …

    brenniebrennie

    We want to make sure that this isn't going to be escaped incorrectly. We should be checking if the result …

    brenniebrennie

    extension.id on next line.

    brenniebrennie

    Can we move all of this computation to happen after the etag_if_none_match call? We don't want to do this if …

    daviddavid

    I suspect we should show something else (or not show this at all) if request.user is the same as user. …

    daviddavid

    Can we localize this? We also should be able to skip the six.text_type call. td_msg = _('Same local time as …

    daviddavid

    I know the existing text in here isn't localized, but we should mark these words (and the others in here) …

    daviddavid

    If you do: :py:class:`~reviewboard.extensions.hooks.UserInfoHook` it will still link to the right class, but it will render as UserInfoHook.

    brenniebrennie

    get_extra_context should return a rst :py:class:`dict`.

    brenniebrennie

    Double backquotes

    brenniebrennie

    Double backquotes

    brenniebrennie

    No blank line here.

    brenniebrennie

    six.text_type, not unicode. (from django.util import six)

    brenniebrennie

    How about: review_requests_sent = ( ReviewRequest.objects .public(user=request.user) .filter(submitter=user) .count() ) Does this include ALL sent rrs or only the open …

    brenniebrennie

    Why are you casting this to a string? Does this include ALL assigned rrs or only open?

    brenniebrennie

    Maybe we should we leave it up to the hook to return their strings as marked safe?

    brenniebrennie

    Col: 13 E131 continuation line unaligned for hanging indent

    reviewbotreviewbot

    "A :py:class:`~reviewboard.extensions.hooks.UserInfoBoxHook` can be used..."

    brenniebrennie

    Blank space after comma

    brenniebrennie

    "to return a :py:class:`dict`. The result of this function will be used, in addition to the request and user, for …

    brenniebrennie

    Should comment out the dots because its not valid python.

    brenniebrennie

    Provide an actual example.

    brenniebrennie

    backquotes around the strings, e.g. * ``'user-identity'`` * ``'extra-details'``

    brenniebrennie

    If required, :py:meth:`get_extra_context` may be overridden ...

    brenniebrennie

    If required, :py:meth:`get_extra_context` may be overridden ...

    brenniebrennie

    Can we call this SECTIONS instead?

    brenniebrennie

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

    reviewbotreviewbot

    All comments should be complete sentences and should end in periods. This goes for all comments in this method.

    brenniebrennie

    requser? How about target_user?

    brenniebrennie

    Instead of my_user, lets call it user or local_user

    brenniebrennie

    This isn't a very useful comment.

    brenniebrennie

    This is not a complete sentence.

    brenniebrennie

    We should probably wrap this in an {% if custom_info %}. Also class="custom-info"

    brenniebrennie

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

    reviewbotreviewbot

    We should call this SECTIONS

    daviddavid

    We should indent this as: return mark_safe( render_to_string(self.template_name, RequestContext(request, context)))

    daviddavid

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

    reviewbotreviewbot

    There's no need to cast this to a string.

    daviddavid

    We don't seem to be splitting this into the different sections.

    daviddavid

    The etag needs to take into account the time difference and stats, and probably the extension hook stuff as well. …

    brenniebrennie

    According to the raw diff, you removed a newline from the last line. Please add it back.

    brenniebrennie

    Unnecessary.

    brenniebrennie

    Can we just call this user_tz?

    brenniebrennie

    Unnecessary

    brenniebrennie

    This isn't the entity tag. You have to call encode_etag on the result.

    brenniebrennie

    Can you just put one, larger comment above the timezone calculation stuff? We don't need comments every few lines -- …

    brenniebrennie

    Unnecessary.

    brenniebrennie

    Unnecessary.

    brenniebrennie

    No blank line here.

    brenniebrennie

    Unnecessary.

    brenniebrennie

    Get.

    brenniebrennie

    Unnecessary.

    brenniebrennie

    "custom rendering context"

    brenniebrennie

    "In this case, the get_etag_data should be overridden to return data that will be used to generate the entity tag …

    brenniebrennie

    "Get"

    brenniebrennie

    Remove this.

    brenniebrennie
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/reviews/views.py
      
      Ignored Files:
          reviewboard/templates/accounts/user_infobox.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/reviews/views.py
      
      Ignored Files:
          reviewboard/templates/accounts/user_infobox.html
      
      
      WARNING: Number of comments exceeded maximum, showing 30 of 33.
    2. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
       'tzinfo' imported but unused
      
    3. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
       'timedelta' imported but unused
      
    4. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 47
       E231 missing whitespace after ','
      
    5. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 5
       E303 too many blank lines (2)
      
    6. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 22
       E261 at least two spaces before inline comment
      
    7. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 64
       E226 missing whitespace around arithmetic operator
      
    8. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 31
       E226 missing whitespace around arithmetic operator
      
    9. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 34
       E226 missing whitespace around arithmetic operator
      
    10. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 37
       E226 missing whitespace around arithmetic operator
      
    11. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 40
       E226 missing whitespace around arithmetic operator
      
    12. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 60
       E226 missing whitespace around arithmetic operator
      
    13. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 27
       E226 missing whitespace around arithmetic operator
      
    14. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 30
       E226 missing whitespace around arithmetic operator
      
    15. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 33
       E226 missing whitespace around arithmetic operator
      
    16. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 36
       E226 missing whitespace around arithmetic operator
      
    17. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 24
       E261 at least two spaces before inline comment
      
    18. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 64
       E226 missing whitespace around arithmetic operator
      
    19. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 26
       E226 missing whitespace around arithmetic operator
      
    20. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 29
       E226 missing whitespace around arithmetic operator
      
    21. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 32
       E226 missing whitespace around arithmetic operator
      
    22. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 35
       E226 missing whitespace around arithmetic operator
      
    23. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 60
       E226 missing whitespace around arithmetic operator
      
    24. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 26
       E226 missing whitespace around arithmetic operator
      
    25. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 29
       E226 missing whitespace around arithmetic operator
      
    26. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 32
       E226 missing whitespace around arithmetic operator
      
    27. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 35
       E226 missing whitespace around arithmetic operator
      
    28. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 10
       E261 at least two spaces before inline comment
      
    29. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 64
       E226 missing whitespace around arithmetic operator
      
    30. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 75
       E225 missing whitespace around operator
      
    31. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 13
       E122 continuation line missing indentation or outdented
      
    32. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 60
       E226 missing whitespace around arithmetic operator
      
    33. reviewboard/reviews/views.py (Diff revision 1)
       
       
      Show all issues
      Col: 75
       E225 missing whitespace around operator
      
    34. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/reviews/views.py
      
      Ignored Files:
          reviewboard/templates/accounts/user_infobox.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/reviews/views.py
      
      Ignored Files:
          reviewboard/templates/accounts/user_infobox.html
      
      
    2. reviewboard/reviews/views.py (Diff revision 2)
       
       
      Show all issues
       'django' imported but unused
      
    3. reviewboard/reviews/views.py (Diff revision 2)
       
       
      Show all issues
      Col: 32
       E225 missing whitespace around operator
      
    4. reviewboard/reviews/views.py (Diff revision 2)
       
       
      Show all issues
      Col: 28
       E226 missing whitespace around arithmetic operator
      
    5. reviewboard/reviews/views.py (Diff revision 2)
       
       
      Show all issues
      Col: 36
       E225 missing whitespace around operator
      
    6. reviewboard/reviews/views.py (Diff revision 2)
       
       
      Show all issues
      Col: 42
       E226 missing whitespace around arithmetic operator
      
    7. reviewboard/reviews/views.py (Diff revision 2)
       
       
      Show all issues
      Col: 36
       E225 missing whitespace around operator
      
    8. reviewboard/reviews/views.py (Diff revision 2)
       
       
      Show all issues
      Col: 5
       E303 too many blank lines (3)
      
    9. reviewboard/reviews/views.py (Diff revision 2)
       
       
      Show all issues
      Col: 57
       E225 missing whitespace around operator
      
    10. reviewboard/reviews/views.py (Diff revision 2)
       
       
      Show all issues
      Col: 1
       W391 blank line at end of file
      
    11. 
        
    brennie
    1. 
        
    2. reviewboard/reviews/views.py (Diff revision 2)
       
       
       
       
       
       
      Show all issues

      These imports need to be broken up. We organize our imports as following:

      from __future__ import unicode_literals
      
      # Standard library imports like logging, time, etc.
      
      # Third-party library imports like django, pytz, etc.
      
      # reviewboard.* imports
      
      1. It only uses pytz and datetime now. Is this a 'fix' or a 'drop'?

      2. It would be a fix.

    3. reviewboard/reviews/views.py (Diff revision 2)
       
       
      Show all issues

      No blank line here.

    4. reviewboard/reviews/views.py (Diff revision 2)
       
       
       
       
      Show all issues

      This should be six.text_type (i.e., unicode) instead of str.

      Also This will have to use fomratting so we can localize the strings, e.g.

      from django.utils.translation import ugettext as _
      
      # ...
      
      if hr_diff < 0:
          td_msg = (_('%(tz)s: %(hours)s hours ahead of you')
                    % {
                        'tz': requsr_tz,
                        'hours': -1 * hr_diff
                    })
      elif hr_diff > 0:
          # ...
      
      1. Missed this one. Will get on this asap.

    5. reviewboard/reviews/views.py (Diff revision 2)
       
       
      Show all issues

      I would break this up as:

      'users_stats': {
          'assigned': num_requests_assigned,
          'posted': num_requests_posted,
          # ...
      }
      

      Then you can do the formatting in the infobox with {% blocktrans %} .... {% endblocktrans %} so that it gets localized.

    6. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/reviews/views.py
      
      Ignored Files:
          reviewboard/templates/accounts/user_infobox.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/reviews/views.py
      
      Ignored Files:
          reviewboard/templates/accounts/user_infobox.html
      
      
    2. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/reviews/views.py
      
      Ignored Files:
          reviewboard/templates/accounts/user_infobox.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/reviews/views.py
      
      Ignored Files:
          reviewboard/templates/accounts/user_infobox.html
      
      
    2. reviewboard/reviews/views.py (Diff revision 4)
       
       
      Show all issues
      Col: 15
       E128 continuation line under-indented for visual indent
      
    3. reviewboard/reviews/views.py (Diff revision 4)
       
       
      Show all issues
      Col: 15
       E126 continuation line over-indented for hanging indent
      
    4. reviewboard/reviews/views.py (Diff revision 4)
       
       
      Show all issues
      Col: 15
       E128 continuation line under-indented for visual indent
      
    5. reviewboard/reviews/views.py (Diff revision 4)
       
       
      Show all issues
      Col: 15
       E126 continuation line over-indented for hanging indent
      
    6. reviewboard/reviews/views.py (Diff revision 4)
       
       
      Show all issues
      Col: 15
       E128 continuation line under-indented for visual indent
      
    7. reviewboard/reviews/views.py (Diff revision 4)
       
       
      Show all issues
      Col: 15
       E126 continuation line over-indented for hanging indent
      
    8. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/reviews/views.py
      
      Ignored Files:
          reviewboard/templates/accounts/user_infobox.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/reviews/views.py
      
      Ignored Files:
          reviewboard/templates/accounts/user_infobox.html
      
      
    2. reviewboard/reviews/views.py (Diff revision 5)
       
       
      Show all issues
      Col: 13
       E128 continuation line under-indented for visual indent
      
    3. reviewboard/reviews/views.py (Diff revision 5)
       
       
      Show all issues
      Col: 13
       E128 continuation line under-indented for visual indent
      
    4. reviewboard/reviews/views.py (Diff revision 5)
       
       
      Show all issues
      Col: 13
       E128 continuation line under-indented for visual indent
      
    5. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/reviews/views.py
      
      Ignored Files:
          reviewboard/templates/accounts/user_infobox.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/reviews/views.py
      
      Ignored Files:
          reviewboard/templates/accounts/user_infobox.html
      
      
    2. 
        
    CH
    david
    1. Can you please attach screenshots?

    2. 
        
    CH
    david
    1. I haven't really looked at the code yet, but I have one comment on the UI:

    2. Show all issues

      Just saying how far ahead/behind forces me to do math.

      Can we imitate what Slack does? They have:

      <clock symbol> 3:10PM local time / 3 hours ahead of you (EST)
      
    3. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          reviewboard/templates/accounts/user_infobox.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          reviewboard/templates/accounts/user_infobox.html
      
      
    2. reviewboard/extensions/hooks.py (Diff revision 7)
       
       
      Show all issues
      Col: 1
       E302 expected 2 blank lines, found 1
      
    3. Show all issues
      Col: 1
       E302 expected 2 blank lines, found 1
      
    4. Show all issues
       undefined name 'e'
      
    5. Show all issues
      Col: 13
       E128 continuation line under-indented for visual indent
      
    6. Show all issues
      Col: 13
       W292 no newline at end of file
      
    7. 
        
    mike_conley
    1. 
        
    2. reviewboard/reviews/views.py (Diff revision 6)
       
       
      Show all issues

      No need to sign your work here. :)

      1. Just making it easier to ctrl-f to my code :)

    3. reviewboard/reviews/views.py (Diff revision 6)
       
       
      Show all issues

      This is essentially the same value as requsr_dttemp.

      Can't we re-use the same value for both, and just call it "now" or "utc_now"?

      utc_now = datetime.datetime.now(pytz.utc)

    4. reviewboard/reviews/views.py (Diff revision 6)
       
       
       
       
       
      Show all issues

      Converting the UTC datetime into a string and then extracting things from the string and converting them back into integers to do math seems suboptimal.

      I'm reasonably certain that we can extract the values that we need from the return value of datetime.now(pytz.utc) and similar.

      Example:

      now = datetime.datetime.now(pytz.utc)
      
      other_user_tz = pytz.timezone(user.get_profile().timezone)
      other_user_time = now.astimezone(other_user_tz)
      
      my_user_tz = pytz.timezone(request.user.get_profile().timezone)
      my_user_time = now.astimezone(my_user_tz)
      

      Can we not then get the timedelta's that we want in seconds, and then convert to minutes / hours, to generate the values that we need?

      Also, you might find this interesting: http://stackoverflow.com/questions/14190045/how-to-convert-datetime-timedelta-to-minutes-hours-in-python

    5. reviewboard/reviews/views.py (Diff revision 6)
       
       
      Show all issues

      I think it'd maybe make more sense do put the tz and hours values into the context and pass them into the template, and then insert them with the translation strings in the template, instead of just sending down the translated strings from the view.

      1. The way the timezone and hours are calculated has undergone major changes since. Is this issue still applicable?

    6. reviewboard/reviews/views.py (Diff revision 6)
       
       
      Show all issues

      We already have the user, no?

    7. reviewboard/reviews/views.py (Diff revision 6)
       
       
       
       
       
      Show all issues

      It seems to me we should be able to narrow down the review_requests with a QuerySet to find the cases where rr.submitter == user. QuerySet docs: https://docs.djangoproject.com/en/1.6/topics/db/queries/

    8. reviewboard/reviews/views.py (Diff revision 6)
       
       
       
      Show all issues

      I think we should be fine to pass these down to the template as ints.

    9. reviewboard/reviews/views.py (Diff revision 6)
       
       
       
      Show all issues

      "Assigned:" and "Posted:" aren't going to be translated.

      Let's send down just the raw integers, and do the translation in the views.

    10. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/codebase/generating-documentation.rst~
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/templates/accounts/user_infobox.html
          docs/codebase/index.rst~
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/codebase/generating-documentation.rst~
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/templates/accounts/user_infobox.html
          docs/codebase/index.rst~
      
      
    2. reviewboard/extensions/hooks.py (Diff revision 8)
       
       
      Show all issues
      Col: 1
       E302 expected 2 blank lines, found 1
      
    3. reviewboard/extensions/hooks.py (Diff revision 8)
       
       
      Show all issues
      Col: 58
       E251 unexpected spaces around keyword / parameter equals
      
    4. reviewboard/extensions/hooks.py (Diff revision 8)
       
       
      Show all issues
      Col: 60
       E251 unexpected spaces around keyword / parameter equals
      
    5. reviewboard/extensions/hooks.py (Diff revision 8)
       
       
      Show all issues
      Col: 22
       E225 missing whitespace around operator
      
    6. reviewboard/extensions/hooks.py (Diff revision 8)
       
       
      Show all issues
      Col: 5
       E303 too many blank lines (2)
      
    7. reviewboard/reviews/views.py (Diff revision 8)
       
       
      Show all issues
      Col: 5
       E265 block comment should start with '# '
      
    8. reviewboard/reviews/views.py (Diff revision 8)
       
       
      Show all issues
      Col: 28
       E226 missing whitespace around arithmetic operator
      
    9. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/codebase/generating-documentation.rst~
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/templates/accounts/user_infobox.html
          docs/codebase/index.rst~
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/codebase/generating-documentation.rst~
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/templates/accounts/user_infobox.html
          docs/codebase/index.rst~
      
      
    2. reviewboard/extensions/hooks.py (Diff revision 9)
       
       
      Show all issues
      Col: 1
       E302 expected 2 blank lines, found 1
      
    3. 
        
    chipx86
    1. 
        
    2. Show all issues

      There's a mismatch in styles here. We should aim for consistency, or separate them out more by whitespace.

      Also, the lines are very close together. Let's add some spacing between all lines.

    3. 
        
    chipx86
    1. Looks like you have some files in here (the documentation ending with a ~) that you probably didn't mean to have.

    2. Show all issues

      "HTML"

    3. docs/manual/extending/extensions/hooks/user-infobox-hook.rst (Diff revision 9)
       
       
       
       
       
       
       
       
      Show all issues

      A couple things:

      1) When showing examples like this, we want them in literal blocks.
      2) I have no idea what this section is trying to say, so an extension author is going to be even more confused. Saying <html>...</html> suggests an entire HTML page will be in there.

      It's better to use more concrete examples and to explain the structure in words.

    4. reviewboard/extensions/hooks.py (Diff revision 9)
       
       
      Show all issues

      This shouldn't be here.

    5. reviewboard/extensions/hooks.py (Diff revision 9)
       
       
       
       
       
       
      Show all issues

      These docs, just like the ones above, will be generated and made available on our site. This needs to be more explanatory, like above.

    6. reviewboard/extensions/hooks.py (Diff revision 9)
       
       
       
       
      Show all issues

      No blank line here.

    7. reviewboard/extensions/hooks.py (Diff revision 9)
       
       
      Show all issues

      This shouldn't take a list of sections. That's going to lock us into a particular design, and it's going to be hard to adapt this to any future changes.

      I also don't think it's useful to take pre-defined strings. Infoboxes aren't going to show a single static string. They're going to need to compute things for the given user.

      I'd suggest a different design for the hook:

      1) Each UserInfoBoxHook should be about showing one single thing, not multiple things. Extensions can always instantiate more than one hook.
      2) The constructor should take only two things: 1) A section indicator (represented by a constant on this class indicating the section name -- this would be mapped to a string like "header", etc.), and 2) an optional template name.
      3) The render function below needs to take the user (since an extension will be basing info on the user), and should default to rendering the provided template with the request and user as context. However, the documentation should state that render can be overridden to provide custom rendering.

      This should not take is_enabled, as it makes no sense to instantiate a hook and just keep it disabled.

      We also need to rethink the sections. "header", "body", "footer" locks us into that model for the infobox, meaning we can't shuffle it later. For instance, say we want to make some of this appear side-by-side. "Footer" no longer makes sense there.

      Instead, let's go with "user-identity" (names, e-mail, possibly department, etc.), and "extra-details". I think going with a third doesn't make a lot of sense right now.

    8. reviewboard/extensions/hooks.py (Diff revision 9)
       
       
       
       
      Show all issues

      Missing a docstring.

      No blank line here.

    9. reviewboard/extensions/hooks.py (Diff revision 9)
       
       
       
       
      Show all issues

      Two blank lines here.

    10. reviewboard/reviews/views.py (Diff revision 9)
       
       
      Show all issues

      This is a third-party module.

      1. So we can't use pytz?

      2. No, it doesn't. However, we sort our imports into three groups:

        ```python
        from future import unicode_literals # Always.

        import os # Python standard library.

        import pytz # Third party modules

        import reviewboard # project imports.

      3. Ok got it. Thanks.

    11. reviewboard/reviews/views.py (Diff revision 9)
       
       
      Show all issues

      This must be inserted alphabetically.

    12. reviewboard/reviews/views.py (Diff revision 9)
       
       
       
       
       
      Show all issues

      This blank line must remain.

      1. Missed this one. Will fix it before the next post.

    13. reviewboard/reviews/views.py (Diff revision 9)
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      I'm going to want to see all this logic heavily documented, so every bit of it is clear.

    14. reviewboard/reviews/views.py (Diff revision 9)
       
       
       
       
      Show all issues

      What's all this?

      1. It's another method for calculating the time difference. Unlike the one above, it doesn't use divisions (which can be suboptimal), so I left it here commented out in case we want to use this method instead.

      2. What is suboptimal about division?

      3. I may be overthinking. Was always told in assembly language classes that division operations take more clock cycles to complete. Would the same principle apply here?

      4. It's true, a division instruction takes longer than, say, addition. However, we're operating at a much, much, much higher level here. Compared with the cost of querying a database that can live on a separate server, division is basically instantaneous. Even if you're talking about some small, tight, mathematically-intensive code, on modern CPUs it's probably not worth trying to optimize out your divisions (the long pipelines, vectorization, and speculative/out-of-order execution mean that the copiler and CPU microcode can usually do a better job than you can). It's only when writing code for simple, slow, basic microcontrollers where that kind of optimization makes sense.

        As Donald Knuth said, "premature optimization is the root of all evil." What that means is that you need to write simple code first, and then measure it to see where the bottlenecks are. If you start out trying to optimize, you'll end up writing complex, unmaintainable code that probably optimizes the wrong thing.

      5. Gotcha. It makes sense now that you've put it this way.

    15. reviewboard/reviews/views.py (Diff revision 9)
       
       
      Show all issues

      <html> means the start of a document. We never want that, or we'll confuse browsers.

    16. reviewboard/reviews/views.py (Diff revision 9)
       
       
       
       
       
       
       
      Show all issues

      On a large deployment, this may produce thousands or tens of thousands of review requests.

      Since we're just counting, we want to let the database handle this. We also don't need to re-check the user, since that's already part of the query. So:

      review_requests_sent = ReviewRequest.objects.public(user=user).count()
      
      1. I found that

        ReviewRequest.objects.public(user=user).count()

        returns the count of all review requests visible to the user, not the review requests posted by the user his/herself. This is why I have to go into the result and manually find the ones where the submitter is the user.

      2. We can still make this use the database, and should pass the request's user in to public so that we don't show them counts of things that aren't visible to them:

        review_requests_sent = (
            ReviewRequest.objects
            .public(user=request.user)
            .filter(submitter=user)
            .count())
        
      3. Ok, I'll try this. Thanks!

    17. reviewboard/reviews/views.py (Diff revision 9)
       
       
       
      Show all issues

      Blank line between these.

    18. reviewboard/reviews/views.py (Diff revision 9)
       
       
      Show all issues

      Missing a trailing comma.

    19. Show all issues

      These should be one statemnet.

    20. reviewboard/templates/accounts/user_infobox.html (Diff revision 9)
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      Make sure all the content aligns properly with the current indentation level.

    21. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          reviewboard/templates/accounts/user_infobox.html
          docs/codebase/generating-documentation.rst~
          docs/manual/extending/extensions/hooks/index.rst
          reviewboard/static/rb/css/ui/datagrids.less
          reviewboard/static/rb/css/common.less
          docs/codebase/index.rst~
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          reviewboard/templates/accounts/user_infobox.html
          docs/codebase/generating-documentation.rst~
          docs/manual/extending/extensions/hooks/index.rst
          reviewboard/static/rb/css/ui/datagrids.less
          reviewboard/static/rb/css/common.less
          docs/codebase/index.rst~
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
      
      
    2. reviewboard/extensions/hooks.py (Diff revision 10)
       
       
      Show all issues
       'format_html' imported but unused
      
    3. reviewboard/extensions/hooks.py (Diff revision 10)
       
       
      Show all issues
       'mark_safe' imported but unused
      
    4. reviewboard/extensions/hooks.py (Diff revision 10)
       
       
      Show all issues
      Col: 39
       E231 missing whitespace after ','
      
    5. reviewboard/extensions/hooks.py (Diff revision 10)
       
       
      Show all issues
      Col: 9
       E303 too many blank lines (2)
      
    6. reviewboard/extensions/hooks.py (Diff revision 10)
       
       
      Show all issues
      Col: 80
       E501 line too long (80 > 79 characters)
      
    7. reviewboard/extensions/hooks.py (Diff revision 10)
       
       
      Show all issues
      Col: 17
       E128 continuation line under-indented for visual indent
      
    8. reviewboard/reviews/views.py (Diff revision 10)
       
       
      Show all issues
      Col: 13
       E128 continuation line under-indented for visual indent
      
    9. reviewboard/reviews/views.py (Diff revision 10)
       
       
      Show all issues
      Col: 13
       E123 closing bracket does not match indentation of opening bracket's line
      
    10. reviewboard/reviews/views.py (Diff revision 10)
       
       
      Show all issues
      Col: 43
       E231 missing whitespace after ','
      
    11. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          reviewboard/templates/accounts/user_infobox.html
          docs/codebase/generating-documentation.rst~
          docs/manual/extending/extensions/hooks/index.rst
          reviewboard/static/rb/css/ui/datagrids.less
          reviewboard/static/rb/css/common.less
          docs/codebase/index.rst~
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          reviewboard/templates/accounts/user_infobox.html
          docs/codebase/generating-documentation.rst~
          docs/manual/extending/extensions/hooks/index.rst
          reviewboard/static/rb/css/ui/datagrids.less
          reviewboard/static/rb/css/common.less
          docs/codebase/index.rst~
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
      
      
    2. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          reviewboard/templates/accounts/user_infobox.html
          docs/codebase/generating-documentation.rst~
          docs/manual/extending/extensions/hooks/index.rst
          reviewboard/static/rb/css/ui/datagrids.less
          reviewboard/static/rb/css/common.less
          docs/codebase/index.rst~
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          reviewboard/templates/accounts/user_infobox.html
          docs/codebase/generating-documentation.rst~
          docs/manual/extending/extensions/hooks/index.rst
          reviewboard/static/rb/css/ui/datagrids.less
          reviewboard/static/rb/css/common.less
          docs/codebase/index.rst~
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
      
      
    2. reviewboard/reviews/views.py (Diff revision 12)
       
       
      Show all issues
      Col: 51
       E202 whitespace before ')'
      
    3. 
        
    CH
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          reviewboard/templates/accounts/user_infobox.html
          docs/codebase/generating-documentation.rst~
          docs/manual/extending/extensions/hooks/index.rst
          reviewboard/static/rb/css/ui/datagrids.less
          reviewboard/static/rb/css/common.less
          docs/codebase/index.rst~
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          reviewboard/templates/accounts/user_infobox.html
          docs/codebase/generating-documentation.rst~
          docs/manual/extending/extensions/hooks/index.rst
          reviewboard/static/rb/css/ui/datagrids.less
          reviewboard/static/rb/css/common.less
          docs/codebase/index.rst~
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
      
      
    2. 
        
    brennie
    1. To remove a file from your review request you can do:

      git rm <filename>
      git commit -m "Removed <filename>"
      rbt post -r 7662
      

      Also, you're going to want to add backup files to your global .gitignore or turn them off in your .vimrc. To do the former (a good idea), add the following to your ~/.gitconfig:

      [core]
          excludesfile = ~/.gitignore_global
      

      And then create a ~/.gitignore_global file with the following:

      *~
      
    2. 
        
    david
    1. 
        
    2. Show all issues

      This should start with .. code-block:: python

    3. reviewboard/extensions/hooks.py (Diff revision 13)
       
       
       
       
       
       
      Show all issues

      Docstrings should start with a 1-line summary, then a blank line, then additional paragraphs of explanation.

    4. reviewboard/extensions/hooks.py (Diff revision 13)
       
       
      Show all issues

      We should check that template_name is a string, because otherwise we'll call render_to_string and pass it None. Alternatively, just get rid of the default value for the parameter to force people to pass it in.

    5. reviewboard/extensions/hooks.py (Diff revision 13)
       
       
      Show all issues

      ExtensionHook doesn't define get_extra_context so the super call is superfluous and useless.

    6. reviewboard/extensions/hooks.py (Diff revision 13)
       
       
      Show all issues

      Remove this line.

    7. reviewboard/reviews/views.py (Diff revision 13)
       
       
      Show all issues

      We should just include the <span> in the template, instead of passing it in like this.

      1. So, what you mean is to do this:

        'clock_symbol': format_html("<span class='fa fa-clock-o'></span> ")

        instead of:

        clk_s = format_html("<span class='fa fa-clock-o'></span> ")
        ...
        'clocl_symbol': clk_s

        Is that correct?

      2. No, I mean in the template, instead of {{clock_symbol}}, use <span class='fa fa-clock-o'></span>. You can then remove it entirely from this file.

      3. Understood, thanks. I've fixed it.

    8. reviewboard/reviews/views.py (Diff revision 13)
       
       
      Show all issues

      Please put this line back.

    9. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          reviewboard/templates/accounts/user_infobox.html
          docs/codebase/generating-documentation.rst~
          docs/manual/extending/extensions/hooks/index.rst
          reviewboard/static/rb/css/common.less
          docs/codebase/index.rst~
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          reviewboard/templates/accounts/user_infobox.html
          docs/codebase/generating-documentation.rst~
          docs/manual/extending/extensions/hooks/index.rst
          reviewboard/static/rb/css/common.less
          docs/codebase/index.rst~
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
      
      
    2. reviewboard/reviews/views.py (Diff revision 14)
       
       
      Show all issues
       'format_html' imported but unused
      
    3. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          reviewboard/templates/accounts/user_infobox.html
          docs/codebase/generating-documentation.rst~
          docs/manual/extending/extensions/hooks/index.rst
          reviewboard/static/rb/css/common.less
          docs/codebase/index.rst~
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          reviewboard/templates/accounts/user_infobox.html
          docs/codebase/generating-documentation.rst~
          docs/manual/extending/extensions/hooks/index.rst
          reviewboard/static/rb/css/common.less
          docs/codebase/index.rst~
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
      
      
    2. 
        
    brennie
    1. 
        
    2. docs/manual/extending/extensions/hooks/user-infobox-hook.rst (Diff revision 15)
       
       
       
       
       
       
       
       
       
      Show all issues

      This should line up with the c in code-block.

    3. docs/manual/extending/extensions/hooks/user-infobox-hook.rst (Diff revision 15)
       
       
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      This should line up with the c in code-block.

    4. reviewboard/extensions/hooks.py (Diff revision 15)
       
       
       
       
      Show all issues

      Can you reformat this as follows?

              raise ValueError(
                  'Invalid UserInfoBox section: %s; must be one of %s'
                  % (section, ', '.join(UserInfoBoxHook.SECTION_OPTIONS))
      

      This avoids the text wrapping. We also prefer %-formatting parameters to appear on the next line when the string is long enough to warrant it.

    5. reviewboard/extensions/hooks.py (Diff revision 15)
       
       
       
      Show all issues

      Can you reformat this as:

      return render_to_string(self.template_name,
                              RequestContext(request, context))
      

      It's easier to read this way.

    6. reviewboard/reviews/views.py (Diff revision 15)
       
       
      Show all issues

      Remove this line.

    7. reviewboard/reviews/views.py (Diff revision 15)
       
       
      Show all issues

      This comment is unnecessary.

    8. reviewboard/reviews/views.py (Diff revision 15)
       
       
       
      Show all issues

      Single quotes. Also, this should be six.text_type, not str:

      from django.utils import six
      

      The reason we use this is that we use unicode literals everywhere and unicode != str. However, in Python 3, unicode was renamed to str and str was renamed to bytes. We use six.text_type so that when we eventually transition to Py3, we will have less headaches.

    9. reviewboard/reviews/views.py (Diff revision 15)
       
       
       
      Show all issues

      Can you put UserInfoBoxHook.render on one line? that way its clear there is not supposed to be a space between them.

    10. reviewboard/reviews/views.py (Diff revision 15)
       
       
       
      Show all issues

      This should be

      '... extension: "',
      extension.id, e, exc_info=True)
      

      (I know some places use exc_info=1, but we aren't doing that moving forward.)

      1. Is that double quote a typo?

        So should it look like below?

        logging.error('Error when running UserInfoBoxHook.render '
        'function in extension: ',
        extension.id, e, exc_info=True)

    11. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
    2. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
    2. reviewboard/extensions/hooks.py (Diff revision 17)
       
       
      Show all issues
      Col: 25
       E128 continuation line under-indented for visual indent
      
    3. 
        
    david
    1. 
        
    2. Show all issues

      This blank line isn't necessary when there's no docstring.

    3. Show all issues

      I'm not sure what "custom parameters..." means here, and it's not something that users can change (since render chooses what to pass in). We should list user, request.

    4. Show all issues

      get_extra_context should return a dict, not a str.

    5. reviewboard/reviews/views.py (Diff revision 17)
       
       
       
       
       
       
      Show all issues

      There are fractional time zones that aren't 30 minutes (for example, Kathmandu, Nepal is GMT +5:45). I don't think this will do the right thing for those.

    6. reviewboard/reviews/views.py (Diff revision 17)
       
       
       
       
      Show all issues

      We should pass the raw numbers in to the template and put the text there.

    7. reviewboard/reviews/views.py (Diff revision 17)
       
       
       
       
      Show all issues

      As is, this won't include any of the arguments because the first argument doesn't have any format specifiers. You can also simplify it using logging.exception:

      logging.exception('Error when running UserInfoBoxHook.render in '
                        'extension %s', extension.id)
      
    8. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
    2. reviewboard/reviews/views.py (Diff revision 18)
       
       
      Show all issues
      Col: 30
       E201 whitespace after '('
      
    3. reviewboard/reviews/views.py (Diff revision 18)
       
       
      Show all issues
       local variable 'e' is assigned to but never used
      
    4. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
    2. 
        
    brennie
    1. 
        
    2. reviewboard/reviews/views.py (Diff revision 19)
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      How about the following:

      # We round the timezones
      while minutes >= 10:
          if ahead:
              hr_diff += 0.25
          else:
              hr_diff -= 0.25
      
          minutes -= 15
      

      This will have the same effect, but be much shorter.

      Also, personally I'd much rather see 5:45 hours ahead of you instead of 5.75 hours ahead of you.

      1. If we're going with the '5:45 hours ahead of you' format, do we still want to round to the nearest 15 mins?

      2. Lets leave that out for now.

      3. Since time zones are essentially a political issue, I wouldn't put it past someone to create something even wackier. Let's show the computed number of minutes without rounding.

    3. reviewboard/reviews/views.py (Diff revision 19)
       
       
      Show all issues

      elif

    4. reviewboard/reviews/views.py (Diff revision 19)
       
       
       
       
       
       
      Show all issues

      When interpolating only one variable, you don't need to use dict interpolation.

    5. reviewboard/reviews/views.py (Diff revision 19)
       
       
       
       
       
       
      Show all issues

      So string additions are slow. How about:

      custom_info_parts = []
      
      for hook in UserInfoBoxHooks.hooks:
          try:
              custom_info_parts.append(hook.render(user, request))
          except Exception:
              # ...
      
      custom_info = ''.join(custom_info_parts)
      
    6. reviewboard/reviews/views.py (Diff revision 19)
       
       
      Show all issues

      We want to make sure that this isn't going to be escaped incorrectly. We should be checking if the result of hook.render is a SafeString or not. If not, it should be escaped so that we only deal with SafeStrings.

    7. reviewboard/reviews/views.py (Diff revision 19)
       
       
      Show all issues

      extension.id on next line.

    8. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
    2. 
        
    david
    1. 
        
    2. Show all issues

      Now that we're showing this using a time notation, we can probably skip the word "hours" ("4:00 hours" seems weird, and "4:30 hours" seems weirder)

    3. reviewboard/reviews/views.py (Diff revision 20)
       
       
       
       
       
       
       
      Show all issues

      Can we move all of this computation to happen after the etag_if_none_match call? We don't want to do this if the user's browser already has it cached.

      1. Just to clarify, so everything from line 1612 to line 1651?

      2. 1612 to 1675 (all your additions)

    4. reviewboard/reviews/views.py (Diff revision 20)
       
       
      Show all issues

      I suspect we should show something else (or not show this at all) if request.user is the same as user. I know I'm in my own timezone!

      1. Actually, the UI is not showing my own timezone. But I need it to calculate the time difference.

        So I'm using my_user_tz = pytz.timezone(request.user.get_profile().timezone) to get the datetime in
        the next line: my_user_dt = utc_now.astimezone(my_user_tz).replace(tzinfo=utc), so that I can
        get the timedelta between this datetime and the requested one: duration = requsr_dt - my_user_dt.

      2. OK, if it doesn't show it for yourself, that's cool.

    5. reviewboard/reviews/views.py (Diff revision 20)
       
       
      Show all issues

      Can we localize this? We also should be able to skip the six.text_type call.

      td_msg = _('Same local time as you (%s)') % requsr_tz
      
    6. Show all issues

      I know the existing text in here isn't localized, but we should mark these words (and the others in here) for translation. I think we should also put assigned/posted on separate lines.

      1. Is this correct:
        <p class="user-stats">{% trans "Assigned: " %}{{users_stats.assigned}}</p>
        <p class="user-stats">{% trans "Posted: " %}{{users_stats.posted}}</p>

      2. Yes, that'll do.

    7. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
    2. 
        
    brennie
    1. 
        
    2. Show all issues

      If you do:

      :py:class:`~reviewboard.extensions.hooks.UserInfoHook`
      

      it will still link to the right class, but it will render as UserInfoHook.

    3. Show all issues

      get_extra_context should return a
      rst :py:class:`dict`.

    4. reviewboard/extensions/hooks.py (Diff revision 21)
       
       
      Show all issues

      Double backquotes

    5. reviewboard/extensions/hooks.py (Diff revision 21)
       
       
      Show all issues

      Double backquotes

    6. reviewboard/reviews/views.py (Diff revision 21)
       
       
      Show all issues

      No blank line here.

    7. reviewboard/reviews/views.py (Diff revision 21)
       
       
      Show all issues

      six.text_type, not unicode.

      (from django.util import six)

    8. reviewboard/reviews/views.py (Diff revision 21)
       
       
       
      Show all issues

      How about:

      review_requests_sent = (
          ReviewRequest.objects
          .public(user=request.user)
          .filter(submitter=user)
          .count()
      )
      

      Does this include ALL sent rrs or only the open ones?

    9. reviewboard/reviews/views.py (Diff revision 21)
       
       
      Show all issues

      Why are you casting this to a string?

      Does this include ALL assigned rrs or only open?

    10. reviewboard/reviews/views.py (Diff revision 21)
       
       
      Show all issues

      Maybe we should we leave it up to the hook to return their strings as marked safe?

      1. We will potentially be calling mark_safe more than once if we do this. Would that be fine?

    11. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
    2. reviewboard/reviews/views.py (Diff revision 22)
       
       
      Show all issues
      Col: 13
       E131 continuation line unaligned for hanging indent
      
    3. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
    2. reviewboard/extensions/hooks.py (Diff revision 23)
       
       
      Show all issues
      Col: 33
       E128 continuation line under-indented for visual indent
      
    3. 
        
    brennie
    1. 
        
    2. Show all issues
      "A :py:class:`~reviewboard.extensions.hooks.UserInfoBoxHook` can be used..."
    3. Show all issues

      Blank space after comma

    4. Show all issues
      "to return a :py:class:`dict`. The result of this function will be used, in addition to the request and user, for the template's rendering context."
    5. Show all issues

      Should comment out the dots because its not valid python.

    6. Show all issues

      Provide an actual example.

    7. reviewboard/extensions/hooks.py (Diff revision 23)
       
       
       
      Show all issues

      backquotes around the strings, e.g.

      * ``'user-identity'``
      * ``'extra-details'``
      
    8. reviewboard/extensions/hooks.py (Diff revision 23)
       
       
       
      Show all issues

      If required, :py:meth:`get_extra_context` may be overridden ...
      
    9. reviewboard/extensions/hooks.py (Diff revision 23)
       
       
       
      Show all issues

      If required, :py:meth:`get_extra_context` may be overridden ...
      
    10. reviewboard/extensions/hooks.py (Diff revision 23)
       
       
      Show all issues

      Can we call this SECTIONS instead?

    11. reviewboard/reviews/views.py (Diff revision 23)
       
       
      Show all issues

      All comments should be complete sentences and should end in periods. This goes for all comments in this method.

    12. reviewboard/reviews/views.py (Diff revision 23)
       
       
      Show all issues

      requser? How about target_user?

    13. reviewboard/reviews/views.py (Diff revision 23)
       
       
      Show all issues

      Instead of my_user, lets call it user or local_user

    14. reviewboard/reviews/views.py (Diff revision 23)
       
       
      Show all issues

      This isn't a very useful comment.

    15. reviewboard/reviews/views.py (Diff revision 23)
       
       
      Show all issues

      This is not a complete sentence.

    16. Show all issues

      We should probably wrap this in an {% if custom_info %}.

      Also class="custom-info"

    17. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
    2. reviewboard/extensions/hooks.py (Diff revision 24)
       
       
      Show all issues
      Col: 13
       E128 continuation line under-indented for visual indent
      
    3. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
    2. reviewboard/extensions/hooks.py (Diff revision 25)
       
       
      Show all issues
      Col: 17
       E128 continuation line under-indented for visual indent
      
    3. 
        
    david
    1. 
        
    2. reviewboard/extensions/hooks.py (Diff revision 25)
       
       
      Show all issues

      We should call this SECTIONS

    3. reviewboard/extensions/hooks.py (Diff revision 25)
       
       
       
      Show all issues

      We should indent this as:

      return mark_safe(
          render_to_string(self.template_name,
                           RequestContext(request, context)))
      
    4. reviewboard/reviews/views.py (Diff revision 25)
       
       
      Show all issues

      There's no need to cast this to a string.

    5. reviewboard/reviews/views.py (Diff revision 25)
       
       
       
       
       
       
       
       
       
       
      Show all issues

      We don't seem to be splitting this into the different sections.

      1. At the moment, all custom info goes in between the user's email and login/joined info. Do we want the custom info appearing in two different places depending on sections, or just have this code splitting it into different sections regardless?

    6. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
    2. 
        
    brennie
    1. 
        
    2. reviewboard/reviews/views.py (Diff revision 26)
       
       
       
       
       
       
       
       
       
      Show all issues

      The etag needs to take into account the time difference and stats, and probably the extension hook stuff as well. You should talk to Christian about etags next week.

    3. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
    2. 
        
    brennie
    1. Some minor issues, and then I'm happy with this landing.

    2. Show all issues

      According to the raw diff, you removed a newline from the last line. Please add it back.

    3. reviewboard/reviews/views.py (Diff revision 27)
       
       
      Show all issues

      Unnecessary.

    4. reviewboard/reviews/views.py (Diff revision 27)
       
       
      Show all issues

      Can we just call this user_tz?

      1. Similarly, I have also changed local_user_dt to user_dt.

    5. reviewboard/reviews/views.py (Diff revision 27)
       
       
      Show all issues

      Unnecessary

    6. reviewboard/reviews/views.py (Diff revision 27)
       
       
      Show all issues

      This isn't the entity tag. You have to call encode_etag on the result.

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

      Can you just put one, larger comment above the timezone calculation stuff? We don't need comments every few lines -- the code is pretty self explanatory.

    8. reviewboard/reviews/views.py (Diff revision 27)
       
       
      Show all issues

      Unnecessary.

    9. reviewboard/reviews/views.py (Diff revision 27)
       
       
      Show all issues

      Unnecessary.

    10. reviewboard/reviews/views.py (Diff revision 27)
       
       
      Show all issues

      No blank line here.

    11. reviewboard/reviews/views.py (Diff revision 27)
       
       
      Show all issues

      Unnecessary.

    12. reviewboard/reviews/views.py (Diff revision 27)
       
       
      Show all issues

      Get.

    13. reviewboard/reviews/views.py (Diff revision 27)
       
       
      Show all issues

      Unnecessary.

    14. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/extensions/templatetags/rb_extensions.py
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
    2. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
    2. 
        
    brennie
    1. 
        
    2. Show all issues

      "custom rendering context"

    3. Show all issues

      "In this case, the get_etag_data should be overridden to return data that will be used to generate the entity tag for the infobox. This will help to ensure the infobox is cached correctly."

    4. reviewboard/reviews/views.py (Diff revision 29)
       
       
      Show all issues

      "Get"

    5. reviewboard/reviews/views.py (Diff revision 29)
       
       
      Show all issues

      Remove this.

    6. 
        
    CH
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/reviews/views.py
          reviewboard/extensions/hooks.py
      
      Ignored Files:
          docs/manual/extending/extensions/hooks/index.rst
          docs/manual/extending/extensions/hooks/user-infobox-hook.rst
          reviewboard/static/rb/css/common.less
          reviewboard/templates/accounts/user_infobox.html
      
      
    2. 
        
    CH
    Review request changed
    Status:
    Discarded