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