Collapsing Old Reviews on Review Requests

Review Request #8489 — Created Oct. 25, 2016 and discarded

Connor-Y
Review Board
master
8536
reviewboard, students

We want to promote discussion and foster ease of use for the site.
On active or old review requests it can be hard to track whats happening.
This can be helped by collapsing inactive reviews.
This makes it easier to browse and see whats new.

This is done by implementing logic such that:
1. If the review hasn't been seen (by this user), show it
2. If the review is new, show it
3. If there is a new reply, show the review
3. If the user has a draft for that review, show it
4. If there are open issues for the review, show it
5. Otherwise, hide it (via collapsing)

Tested by hand to make sure everything works
Unit tests written:
1. Test old review
2. Test new review
3. Test review with old reply
4. Test review with new reply
5. Test review with draft for user
6. Test review with draft for another user
7. Test review with open issue
8. Test all of the above when logged out
9. Test all of the above with user who hasn't visited before
10. Repeat test with same user now that they've visited

Description From Last Updated

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 23 E261 at least two spaces before inline comment

reviewbotreviewbot

Col: 24 E262 inline comment should start with '# '

reviewbotreviewbot

Col: 22 E261 at least two spaces before inline comment

reviewbotreviewbot

Col: 23 E262 inline comment should start with '# '

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

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

reviewbotreviewbot

Col: 26 E261 at least two spaces before inline comment

reviewbotreviewbot

Col: 27 E262 inline comment should start with '# '

reviewbotreviewbot

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

reviewbotreviewbot

Col: 21 W503 line break before binary operator

reviewbotreviewbot

Col: 17 W503 line break before binary operator

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

undefined name 'timestamp'

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

undefined name 'datetime'

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 82 W291 trailing whitespace

reviewbotreviewbot

Col: 22 W503 line break before binary operator

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 80 W291 trailing whitespace

reviewbotreviewbot

Col: 22 W503 line break before binary operator

reviewbotreviewbot

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

reviewbotreviewbot

Col: 22 W503 line break before binary operator

reviewbotreviewbot

'datetime' imported but unused

reviewbotreviewbot

'ReviewRequest' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

local variable 'comment_text_1' is assigned to but never used

reviewbotreviewbot

local variable 'comment_text_2' is assigned to but never used

reviewbotreviewbot

local variable 'comment_text_3' is assigned to but never used

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

local variable 'filediff' is assigned to but never used

reviewbotreviewbot

local variable 'user2' is assigned to but never used

reviewbotreviewbot

local variable 'review_one' is assigned to but never used

reviewbotreviewbot

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

reviewbotreviewbot

local variable 'review_two' is assigned to but never used

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

local variable 'reply_for_three' is assigned to but never used

reviewbotreviewbot

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

reviewbotreviewbot

local variable 'reply_for_four' is assigned to but never used

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

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

reviewbotreviewbot

timestamps.

brenniebrennie

Instead of keeping track of i manually, you can do: for i, review in enumerate(review_request.reviews.all()): Review.objects.filter(pk=review.pk).update(timestamp=timestamp[i])

brenniebrennie

Col: 79 W291 trailing whitespace

reviewbotreviewbot

Col: 9 E303 too many blank lines (2)

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 9 E303 too many blank lines (2)

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

No print() in unit tests. YOu can add a message to self.assertEqual as the third argument.

brenniebrennie

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

These should be together.

brenniebrennie

undo this.

brenniebrennie

Missing a period.

brenniebrennie

No blank line here.

brenniebrennie

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

isinstance(entry, ReviewEntry)

brenniebrennie

These are only used in one place, so just put them inline.

brenniebrennie

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

reviewbotreviewbot

This should be if isinstance(latest_reply, basestring))

brenniebrennie

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

reviewbotreviewbot

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

reviewbotreviewbot

This should be if last_visited != 0 and isinstance(last_visited, basestring))

brenniebrennie

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 80 W291 trailing whitespace

reviewbotreviewbot

This should be formatted as: collapsed = ( review.timestmap < oldest_allowed_time and (latest_reply is None or (latest_reply < oldest_allowed_time and ...

brenniebrennie

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

reviewbotreviewbot

Col: 25 W503 line break before binary operator

reviewbotreviewbot

Col: 50 W291 trailing whitespace

reviewbotreviewbot

Col: 30 W503 line break before binary operator

reviewbotreviewbot

Col: 68 W291 trailing whitespace

reviewbotreviewbot

Col: 34 W503 line break before binary operator

reviewbotreviewbot

Undo this.

brenniebrennie

from datetime import datetime, timedelta

brenniebrennie

'datetime' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

'ReviewRequest' imported but unused

reviewbotreviewbot

Undo this.

brenniebrennie

Col: 19 E222 multiple spaces after operator

reviewbotreviewbot

local variable 'diffset' is assigned to but never used

reviewbotreviewbot

local variable 'user2' is assigned to but never used

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

These should all be in one group. Imports go as follows: from __future import unicode_literals python stdlib imports 3rdpart imports ...

brenniebrennie

isinstance(i, cls) returns a bool.

brenniebrennie

latest_reply_timestamp

brenniebrennie

If it is not a string, what is it? If it is string or None, then you can just do ...

brenniebrennie

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

reviewbotreviewbot

latest_reply_time

brenniebrennie

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

reviewbotreviewbot

W\hy would it be 0 or a string?

brenniebrennie

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

reviewbotreviewbot

last_visited_time

brenniebrennie

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

reviewbotreviewbot

Col: 9 E303 too many blank lines (2)

reviewbotreviewbot

Col: 69 E261 at least two spaces before inline comment

reviewbotreviewbot

Col: 70 E262 inline comment should start with '# '

reviewbotreviewbot

Col: 69 E261 at least two spaces before inline comment

reviewbotreviewbot

Col: 70 E262 inline comment should start with '# '

reviewbotreviewbot

Col: 78 E261 at least two spaces before inline comment

reviewbotreviewbot

Col: 79 E262 inline comment should start with '# '

reviewbotreviewbot

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

reviewbotreviewbot

Col: 78 E261 at least two spaces before inline comment

reviewbotreviewbot

Col: 79 E262 inline comment should start with '# '

reviewbotreviewbot

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

reviewbotreviewbot

Col: 78 E261 at least two spaces before inline comment

reviewbotreviewbot

Col: 79 E262 inline comment should start with '# '

reviewbotreviewbot

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

reviewbotreviewbot

Col: 78 E261 at least two spaces before inline comment

reviewbotreviewbot

Col: 79 E262 inline comment should start with '# '

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

local variable 'last_visited' is assigned to but never used

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Rename latest_reply_timestamp variable name

MB mbalogh

I will say use review_timestamp since you declared it.

LA larmiej

This comment doesn't make sense to me. Can you flesh it out?

daviddavid

Comments should include periods at the end. Several other instances in this same method.

daviddavid

This can use zip. You're also not saving any queries by using .update() in this way. How about: for review, ...

daviddavid

This could be: self.assertEqual( [entry.collapsed for entry in entries], expected_values)

daviddavid

It doesn't make sense to use indices here. for entry in entries: self.assertEqual(entry.collapsed, False)

daviddavid

Same comment as above re: direct iteration.

daviddavid

Same comment as above re: the expected_values check.

daviddavid

This is adding a new query for something which we already are fetching below. Instead of adding this, how about ...

daviddavid

One major problem that this introduces is that a ChangeEntry or InitialStatusUpdatesEntry which contains multiple reviews (those attached to a ...

daviddavid

What is "allowed" time?

daviddavid

In this case, it's probably cleaner to format this as: latest_reply_timestamp = \ data.latest_timestamps_by_review_id.get(review.pk)

daviddavid

This should all not be necessary anymore, right?

daviddavid

Draft reviews aren't shown in the review list. This can go away.

daviddavid

Indentation is a little off here. Should be: if (last_visited_time <= review_timestamp or (latest_reply_timestamp is not None and last_visited_time <= ...

daviddavid

Too many blank lines here.

daviddavid

Why this change?

daviddavid

Something's weird with your change and what its parent commit is. This shouldn't be showing up in your diff.

daviddavid

'ReviewRequestDraft' imported but unused

reviewbotreviewbot

local variable 'filediff' is assigned to but never used

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 19 E222 multiple spaces after operator

reviewbotreviewbot

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

reviewbotreviewbot

This shouldn't be possible. Only top-level reviews get their own entry.

daviddavid

This will do the wrong thing for entries that are not ReviewEntry. We should just move this assignment into the ...

daviddavid

You shouldn't need any special cases for these--they show up in reviews_entry_map.

daviddavid

Col: 13 E303 too many blank lines (2)

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 12 E114 indentation is not a multiple of four (comment)

reviewbotreviewbot

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

reviewbotreviewbot

Col: 17 E116 unexpected indentation (comment)

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 17 E116 unexpected indentation (comment)

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 13 E303 too many blank lines (2)

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 13 E303 too many blank lines (3)

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

'StatusUpdate' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

You might want to split this up into multiple tests since it does a lot.

brenniebrennie

No period for test docstrings (the test runner adds periods).

brenniebrennie

No blank line here.

brenniebrennie

Comments should almost always be complete sentences and they should begin with a capital.

brenniebrennie

Why are there more than six entries here?

brenniebrennie

Blank line between these.

brenniebrennie

You should be building the URL using reverse(). Same elsewhere.

brenniebrennie

Missing docstring. This test is gigantic and should be split up into multiple tests.

brenniebrennie

datetime is a python stdlib import.

brenniebrennie

Move the comment into the docstring.

brenniebrennie

Blank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 89 E226 missing whitespace around arithmetic operator

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 5 E303 too many blank lines (3)

reviewbotreviewbot
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    
  2. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 5
     E265 block comment should start with '# '
    
  3. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  4. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (96 > 79 characters)
    
  5. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (86 > 79 characters)
    
  6. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 5
     E265 block comment should start with '# '
    
  7. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 23
     E261 at least two spaces before inline comment
    
  8. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 24
     E262 inline comment should start with '# '
    
  9. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 22
     E261 at least two spaces before inline comment
    
  10. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 23
     E262 inline comment should start with '# '
    
  11. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 5
     E265 block comment should start with '# '
    
  12. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  13. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 9
     E265 block comment should start with '# '
    
  14. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 26
     E261 at least two spaces before inline comment
    
  15. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 27
     E262 inline comment should start with '# '
    
  16. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (93 > 79 characters)
    
  17. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 21
     W503 line break before binary operator
    
  18. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 17
     W503 line break before binary operator
    
  19. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 9
     E265 block comment should start with '# '
    
  20. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 9
     E265 block comment should start with '# '
    
  21. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (105 > 79 characters)
    
  22. reviewboard/reviews/views.py (Diff revision 1)
     
     
     undefined name 'timestamp'
    
  23. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 9
     E265 block comment should start with '# '
    
  24. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  25. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  26. 
      
CO
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    
  2. reviewboard/reviews/views.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (96 > 79 characters)
    
  3. reviewboard/reviews/views.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (86 > 79 characters)
    
  4. reviewboard/reviews/views.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (95 > 79 characters)
    
  5. reviewboard/reviews/views.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (137 > 79 characters)
    
  6. reviewboard/reviews/views.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (86 > 79 characters)
    
  7. reviewboard/reviews/views.py (Diff revision 2)
     
     
     undefined name 'datetime'
    
  8. reviewboard/reviews/views.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  9. reviewboard/reviews/views.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  10. reviewboard/reviews/views.py (Diff revision 2)
     
     
    Col: 82
     W291 trailing whitespace
    
  11. reviewboard/reviews/views.py (Diff revision 2)
     
     
    Col: 22
     W503 line break before binary operator
    
  12. reviewboard/reviews/views.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (91 > 79 characters)
    
  13. reviewboard/reviews/views.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (97 > 79 characters)
    
  14. reviewboard/reviews/views.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  15. reviewboard/reviews/views.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  16. 
      
CO
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    
  2. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Col: 9
     E265 block comment should start with '# '
    
  3. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Col: 9
     E265 block comment should start with '# '
    
  4. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Col: 9
     E265 block comment should start with '# '
    
  5. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Col: 9
     E265 block comment should start with '# '
    
  6. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Col: 9
     E265 block comment should start with '# '
    
  7. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (86 > 79 characters)
    
  8. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  9. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  10. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  11. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  12. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Col: 80
     W291 trailing whitespace
    
  13. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Col: 22
     W503 line break before binary operator
    
  14. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (85 > 79 characters)
    
  15. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Col: 22
     W503 line break before binary operator
    
  16. 
      
CO
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/reviews/tests/test_views.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/reviews/tests/test_views.py
    
    
    WARNING: Number of comments exceeded maximum, showing 30 of 44.
  2. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
     'datetime' imported but unused
    
  3. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
     'ReviewRequest' imported but unused
    
  4. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (85 > 79 characters)
    
  5. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
     local variable 'comment_text_1' is assigned to but never used
    
  6. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
     local variable 'comment_text_2' is assigned to but never used
    
  7. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
     local variable 'comment_text_3' is assigned to but never used
    
  8. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  9. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
     local variable 'filediff' is assigned to but never used
    
  10. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
     local variable 'user2' is assigned to but never used
    
  11. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
     local variable 'review_one' is assigned to but never used
    
  12. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  13. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
     local variable 'review_two' is assigned to but never used
    
  14. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  15. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  16. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
     local variable 'reply_for_three' is assigned to but never used
    
  17. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  18. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
     local variable 'reply_for_four' is assigned to but never used
    
  19. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  20. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (86 > 79 characters)
    
  21. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
    Col: 79
     W291 trailing whitespace
    
  22. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
    Col: 9
     E303 too many blank lines (2)
    
  23. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  24. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  25. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
    Col: 9
     E303 too many blank lines (2)
    
  26. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  27. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (91 > 79 characters)
    
  28. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
    Col: 9
     E265 block comment should start with '# '
    
  29. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  30. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  31. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (86 > 79 characters)
    
  32. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  33. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  34. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  35. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  36. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Col: 80
     W291 trailing whitespace
    
  37. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Col: 25
     E127 continuation line over-indented for visual indent
    
  38. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Col: 25
     W503 line break before binary operator
    
  39. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Col: 50
     W291 trailing whitespace
    
  40. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Col: 30
     W503 line break before binary operator
    
  41. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Col: 68
     W291 trailing whitespace
    
  42. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Col: 34
     W503 line break before binary operator
    
  43. 
      
brennie
  1. 
      
  2. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     

    timestamps.

  3. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
     
     
     

    Instead of keeping track of i manually, you can do:

    for i, review in enumerate(review_request.reviews.all()):
        Review.objects.filter(pk=review.pk).update(timestamp=timestamp[i])
    
  4. reviewboard/reviews/tests/test_views.py (Diff revision 4)
     
     
     

    No print() in unit tests. YOu can add a message to self.assertEqual as the third argument.

  5. reviewboard/reviews/views.py (Diff revision 4)
     
     
     
     
     
     

    These should be together.

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

    undo this.

  7. reviewboard/reviews/views.py (Diff revision 4)
     
     

    Missing a period.

  8. reviewboard/reviews/views.py (Diff revision 4)
     
     

    No blank line here.

  9. reviewboard/reviews/views.py (Diff revision 4)
     
     
     

    isinstance(entry, ReviewEntry)

  10. reviewboard/reviews/views.py (Diff revision 4)
     
     
     
     

    These are only used in one place, so just put them inline.

  11. reviewboard/reviews/views.py (Diff revision 4)
     
     

    This should be if isinstance(latest_reply, basestring))

  12. reviewboard/reviews/views.py (Diff revision 4)
     
     

    This should be if last_visited != 0 and isinstance(last_visited, basestring))

  13. reviewboard/reviews/views.py (Diff revision 4)
     
     
     
     
     

    This should be formatted as:

    collapsed = (
        review.timestmap < oldest_allowed_time and
        (latest_reply is None or
         (latest_reply < oldest_allowed_time and
          last_visited != 0 and latest_reply < last_visited))
    )
    
  14. reviewboard/testing/testcase.py (Diff revision 4)
     
     

    Undo this.

  15. 
      
CO
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
    
    
  2. reviewboard/reviews/tests/test_views.py (Diff revision 5)
     
     
     'datetime' imported but unused
    
  3. reviewboard/reviews/tests/test_views.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (85 > 79 characters)
    
  4. reviewboard/reviews/tests/test_views.py (Diff revision 5)
     
     
     'ReviewRequest' imported but unused
    
  5. reviewboard/reviews/tests/test_views.py (Diff revision 5)
     
     
    Col: 19
     E222 multiple spaces after operator
    
  6. reviewboard/reviews/tests/test_views.py (Diff revision 5)
     
     
     local variable 'diffset' is assigned to but never used
    
  7. reviewboard/reviews/tests/test_views.py (Diff revision 5)
     
     
     local variable 'user2' is assigned to but never used
    
  8. reviewboard/reviews/tests/test_views.py (Diff revision 5)
     
     
    Col: 22
     E128 continuation line under-indented for visual indent
    
  9. reviewboard/reviews/tests/test_views.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (91 > 79 characters)
    
  10. reviewboard/reviews/tests/test_views.py (Diff revision 5)
     
     
    Col: 9
     E265 block comment should start with '# '
    
  11. reviewboard/reviews/views.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  12. reviewboard/reviews/views.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  13. reviewboard/reviews/views.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  14. reviewboard/reviews/views.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  15. reviewboard/reviews/views.py (Diff revision 5)
     
     
    Col: 9
     E303 too many blank lines (2)
    
  16. 
      
brennie
  1. 
      
  2. reviewboard/reviews/tests/test_views.py (Diff revision 5)
     
     
     

    from datetime import datetime, timedelta

  3. reviewboard/reviews/tests/test_views.py (Diff revision 5)
     
     

    Undo this.

  4. reviewboard/reviews/views.py (Diff revision 5)
     
     
     
     
     
     

    These should all be in one group.

    Imports go as follows:

    from __future import unicode_literals
    
    python stdlib imports
    
    3rdpart imports
    
    project imports
    

    Imports of the form from foo import bar come after import foo. Both should be in alphabetical order.

  5. reviewboard/reviews/views.py (Diff revision 5)
     
     

    isinstance(i, cls) returns a bool.

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

    latest_reply_timestamp

  7. reviewboard/reviews/views.py (Diff revision 5)
     
     

    If it is not a string, what is it?

    If it is string or None, then you can just do if latest_reply:

  8. reviewboard/reviews/views.py (Diff revision 5)
     
     

    latest_reply_time

  9. reviewboard/reviews/views.py (Diff revision 5)
     
     

    W\hy would it be 0 or a string?

  10. reviewboard/reviews/views.py (Diff revision 5)
     
     

    last_visited_time

  11. 
      
CO
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
    
    
  2. reviewboard/reviews/tests/test_views.py (Diff revision 6)
     
     
    Col: 69
     E261 at least two spaces before inline comment
    
  3. reviewboard/reviews/tests/test_views.py (Diff revision 6)
     
     
    Col: 70
     E262 inline comment should start with '# '
    
  4. reviewboard/reviews/tests/test_views.py (Diff revision 6)
     
     
    Col: 69
     E261 at least two spaces before inline comment
    
  5. reviewboard/reviews/tests/test_views.py (Diff revision 6)
     
     
    Col: 70
     E262 inline comment should start with '# '
    
  6. reviewboard/reviews/tests/test_views.py (Diff revision 6)
     
     
    Col: 78
     E261 at least two spaces before inline comment
    
  7. reviewboard/reviews/tests/test_views.py (Diff revision 6)
     
     
    Col: 79
     E262 inline comment should start with '# '
    
  8. reviewboard/reviews/tests/test_views.py (Diff revision 6)
     
     
    Col: 80
     E501 line too long (88 > 79 characters)
    
  9. reviewboard/reviews/tests/test_views.py (Diff revision 6)
     
     
    Col: 78
     E261 at least two spaces before inline comment
    
  10. reviewboard/reviews/tests/test_views.py (Diff revision 6)
     
     
    Col: 79
     E262 inline comment should start with '# '
    
  11. reviewboard/reviews/tests/test_views.py (Diff revision 6)
     
     
    Col: 80
     E501 line too long (88 > 79 characters)
    
  12. reviewboard/reviews/tests/test_views.py (Diff revision 6)
     
     
    Col: 78
     E261 at least two spaces before inline comment
    
  13. reviewboard/reviews/tests/test_views.py (Diff revision 6)
     
     
    Col: 79
     E262 inline comment should start with '# '
    
  14. reviewboard/reviews/tests/test_views.py (Diff revision 6)
     
     
    Col: 80
     E501 line too long (90 > 79 characters)
    
  15. reviewboard/reviews/tests/test_views.py (Diff revision 6)
     
     
    Col: 78
     E261 at least two spaces before inline comment
    
  16. reviewboard/reviews/tests/test_views.py (Diff revision 6)
     
     
    Col: 79
     E262 inline comment should start with '# '
    
  17. reviewboard/reviews/tests/test_views.py (Diff revision 6)
     
     
    Col: 80
     E501 line too long (89 > 79 characters)
    
  18. reviewboard/reviews/tests/test_views.py (Diff revision 6)
     
     
    Col: 80
     E501 line too long (98 > 79 characters)
    
  19. reviewboard/reviews/views.py (Diff revision 6)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  20. reviewboard/reviews/views.py (Diff revision 6)
     
     
     local variable 'last_visited' is assigned to but never used
    
  21. reviewboard/reviews/views.py (Diff revision 6)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  22. reviewboard/reviews/views.py (Diff revision 6)
     
     
    Col: 29
     E128 continuation line under-indented for visual indent
    
  23. reviewboard/reviews/views.py (Diff revision 6)
     
     
    Col: 29
     E128 continuation line under-indented for visual indent
    
  24. reviewboard/reviews/views.py (Diff revision 6)
     
     
    Col: 26
     E127 continuation line over-indented for visual indent
    
  25. 
      
CO
CO
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
    
    
  2. 
      
CO
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
    
    
  2. 
      
MB
  1. 
      
  2. reviewboard/reviews/views.py (Diff revisions 7 - 8)
     
     

    Basestring can have a default value

  3. reviewboard/reviews/views.py (Diff revisions 7 - 8)
     
     

    Rename latest_reply_timestamp variable name

  4. 
      
MB
  1. Lets try this

    1. Okay, you tried it

    2. Please use demo.reviewboard.org for testing. This server is used for production work, and your tests are interfering with that. Thanks!

  2. And add a comment

  3. Footers are great

MB
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 8)
     
     
     
     
     
     
     
     
     
    Why deleted these lines?
  3. 
      
FI
  1. 
      
  2. reviewboard/reviews/tests/test_views.py (Diff revision 8)
     
     

    Might be missing something here, but doesn't this need to have a name like test_review_detail_collapse?

    1. Thats probably a good idea, thanks.

  3. 
      
LA
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 8)
     
     

    I will say use review_timestamp since you declared it.

  3. 
      
CO
CO
david
  1. 
      
  2. reviewboard/reviews/tests/test_views.py (Diff revision 10)
     
     

    This comment doesn't make sense to me. Can you flesh it out?

  3. reviewboard/reviews/tests/test_views.py (Diff revision 10)
     
     

    Comments should include periods at the end. Several other instances in this same method.

  4. reviewboard/reviews/tests/test_views.py (Diff revision 10)
     
     
     

    This can use zip. You're also not saving any queries by using .update() in this way. How about:

    for review, timestamp in zip(review_request.reviews.all(),
                                 timestamps):
        review.timestamp = timestamp
        review.save(update_fields=['timestamp'])
    
    1. This doesn't seem to properly order the reviews. The order in review_request.reviews.all() isn't necessarily sequential. I need the filter call filer(pk=review.pk) to make sure the right review gets the right timestamp.
      I did switch the call to zip, but I don't think thats any faster than using enumerate.

    2. You're not doing anything different with the order. enumerate will iterate through everything in the queryset in whatever order they come back in, just like zip.

      Calling Review.objects.filter(pk=review.pk) is just creating a queryset that finds the object you already have.

      If you want things in a guaranteed order (perhaps PK?), use review_request.reviews.order_by('pk').

    3. Oops I made a mistake. The issue isn't the ordering, its actually the call to the save() function. save() automatically sets the timestamp to timezone.now() which is overwriting my change to the timestamp. Thats why I was updating the database directly.

  5. reviewboard/reviews/tests/test_views.py (Diff revision 10)
     
     
     

    This could be:

    self.assertEqual(
        [entry.collapsed for entry in entries],
        expected_values)
    
  6. reviewboard/reviews/tests/test_views.py (Diff revision 10)
     
     
     

    It doesn't make sense to use indices here.

    for entry in entries:
        self.assertEqual(entry.collapsed, False)
    
  7. reviewboard/reviews/tests/test_views.py (Diff revision 10)
     
     
     

    Same comment as above re: direct iteration.

  8. reviewboard/reviews/tests/test_views.py (Diff revision 10)
     
     
     

    Same comment as above re: the expected_values check.

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

    This is adding a new query for something which we already are fetching below.

    Instead of adding this, how about changing it so the default value of last_visited is datetime.min, and then conditionalize the new assignment of last_visited on line 358 to only happen if visited_is_new is False?

  10. reviewboard/reviews/views.py (Diff revision 10)
     
     

    One major problem that this introduces is that a ChangeEntry or InitialStatusUpdatesEntry which contains multiple reviews (those attached to a StatusUpdate) will no longer get expanded in cases where there are new replies or draft replies. Instead of looping over entries and only making changes to ReviewEntry subclasses, perhaps we can loop over data.reviews, bailing early if review.base_reply_to_id is not None.

    We'd then potentially have the situation there where one review in an entry would want it expanded but another would want it collapsed. Perhaps the right solution is to start with review entries collapsed, and then conditionally expand them if there's something interesting.

  11. reviewboard/reviews/views.py (Diff revision 10)
     
     

    What is "allowed" time?

  12. reviewboard/reviews/views.py (Diff revision 10)
     
     
     

    In this case, it's probably cleaner to format this as:

    latest_reply_timestamp = \
        data.latest_timestamps_by_review_id.get(review.pk)
    
  13. reviewboard/reviews/views.py (Diff revision 10)
     
     
     
     
     
     
     

    This should all not be necessary anymore, right?

  14. reviewboard/reviews/views.py (Diff revision 10)
     
     
     
     

    Draft reviews aren't shown in the review list. This can go away.

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

    Indentation is a little off here. Should be:

    if (last_visited_time <= review_timestamp or
        (latest_reply_timestamp is not None and
         last_visited_time <= latest_reply_timestamp)):
        collapsed = False
    

    It also seems like this could be combined with the "oldest allowed time" check above in some way--the code is very similar.

  16. reviewboard/reviews/views.py (Diff revision 10)
     
     
     

    Too many blank lines here.

  17. reviewboard/reviews/views.py (Diff revision 10)
     
     

    Why this change?

    1. This reverses the order in which reviews appear. Currently, the oldest review appears at the top of the page while the newer ones appear at the bottom. This change makes it so that the newest review appears at the top and the oldest appears at the bottom. The point is to cut down on the amount of scrolling a user has to do to reach the new (and more likely important) content.

    2. Whether we should do that at all is highly debatable (a big change like that is bound to piss off a lot of users, whether or not it's actually an improvement). Even if we decided to do it, we shouldn't include it in this change (which is specifically addressing the open/close state of entries).

  18. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
    
    
  2. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/detail.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/detail.py
    
    
  2. 
      
CO
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/detail.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/detail.py
    
    
  2. reviewboard/reviews/tests/test_views.py (Diff revision 11)
     
     
     'ReviewRequestDraft' imported but unused
    
  3. reviewboard/reviews/tests/test_views.py (Diff revision 11)
     
     
     local variable 'filediff' is assigned to but never used
    
  4. reviewboard/reviews/tests/test_views.py (Diff revision 11)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  5. reviewboard/reviews/tests/test_views.py (Diff revision 11)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  6. reviewboard/reviews/views.py (Diff revision 11)
     
     
    Col: 19
     E222 multiple spaces after operator
    
  7. reviewboard/reviews/views.py (Diff revision 11)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  8. 
      
david
  1. Looking at your logic, I think it's a bit overcomplicated and fragile, the way that you're special casing each type of entry.

    Instead, how about:

    1. Default every entry which contains one or more reviews to collapsed.
    2. Iterate over every review in reviews_entry_map. If the review has a reason to not be collapsed, expand the relevant entry.
  2. reviewboard/reviews/detail.py (Diff revision 11)
     
     

    Something's weird with your change and what its parent commit is. This shouldn't be showing up in your diff.

  3. reviewboard/reviews/views.py (Diff revision 11)
     
     

    This shouldn't be possible. Only top-level reviews get their own entry.

  4. reviewboard/reviews/views.py (Diff revision 11)
     
     

    This will do the wrong thing for entries that are not ReviewEntry. We should just move this assignment into the if statement.

  5. reviewboard/reviews/views.py (Diff revision 11)
     
     
     
     
     
     
     
     
     
     

    You shouldn't need any special cases for these--they show up in reviews_entry_map.

  6. 
      
CO
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/detail.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/detail.py
    
    
    WARNING: Number of comments exceeded maximum, showing 30 of 35.
  2. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 13
     E303 too many blank lines (2)
    
  3. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 13
     E265 block comment should start with '# '
    
  4. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 13
     E265 block comment should start with '# '
    
  5. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 13
     E265 block comment should start with '# '
    
  6. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 12
     E114 indentation is not a multiple of four (comment)
    
  7. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 80
     E501 line too long (91 > 79 characters)
    
  8. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 17
     E116 unexpected indentation (comment)
    
  9. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 17
     E265 block comment should start with '# '
    
  10. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 80
     E501 line too long (85 > 79 characters)
    
  11. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 17
     E116 unexpected indentation (comment)
    
  12. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 17
     E265 block comment should start with '# '
    
  13. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 13
     E265 block comment should start with '# '
    
  14. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 13
     E303 too many blank lines (2)
    
  15. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  16. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 80
     E501 line too long (84 > 79 characters)
    
  17. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  18. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 13
     E265 block comment should start with '# '
    
  19. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  20. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 13
     E265 block comment should start with '# '
    
  21. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 13
     E265 block comment should start with '# '
    
  22. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  23. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 13
     E265 block comment should start with '# '
    
  24. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 13
     E265 block comment should start with '# '
    
  25. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  26. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 13
     E265 block comment should start with '# '
    
  27. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 13
     E303 too many blank lines (3)
    
  28. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 80
     E501 line too long (84 > 79 characters)
    
  29. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 80
     E501 line too long (84 > 79 characters)
    
  30. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  31. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  32. 
      
CO
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/detail.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/detail.py
    
    
  2. reviewboard/reviews/tests/test_views.py (Diff revision 13)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  3. reviewboard/reviews/tests/test_views.py (Diff revision 13)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  4. reviewboard/reviews/tests/test_views.py (Diff revision 13)
     
     
    Col: 80
     E501 line too long (84 > 79 characters)
    
  5. reviewboard/reviews/tests/test_views.py (Diff revision 13)
     
     
    Col: 50
     E127 continuation line over-indented for visual indent
    
  6. reviewboard/reviews/tests/test_views.py (Diff revision 13)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  7. reviewboard/reviews/tests/test_views.py (Diff revision 13)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  8. reviewboard/reviews/tests/test_views.py (Diff revision 13)
     
     
    Col: 50
     E127 continuation line over-indented for visual indent
    
  9. reviewboard/reviews/tests/test_views.py (Diff revision 13)
     
     
    Col: 40
     E128 continuation line under-indented for visual indent
    
  10. reviewboard/reviews/tests/test_views.py (Diff revision 13)
     
     
    Col: 50
     E127 continuation line over-indented for visual indent
    
  11. reviewboard/reviews/tests/test_views.py (Diff revision 13)
     
     
    Col: 40
     E128 continuation line under-indented for visual indent
    
  12. reviewboard/reviews/tests/test_views.py (Diff revision 13)
     
     
    Col: 41
     E127 continuation line over-indented for visual indent
    
  13. reviewboard/reviews/tests/test_views.py (Diff revision 13)
     
     
    Col: 51
     E127 continuation line over-indented for visual indent
    
  14. reviewboard/reviews/tests/test_views.py (Diff revision 13)
     
     
    Col: 41
     E127 continuation line over-indented for visual indent
    
  15. reviewboard/reviews/tests/test_views.py (Diff revision 13)
     
     
    Col: 50
     E127 continuation line over-indented for visual indent
    
  16. reviewboard/reviews/views.py (Diff revision 13)
     
     
     'StatusUpdate' imported but unused
    
  17. reviewboard/reviews/views.py (Diff revision 13)
     
     
    Col: 21
     E127 continuation line over-indented for visual indent
    
  18. reviewboard/reviews/views.py (Diff revision 13)
     
     
    Col: 21
     E127 continuation line over-indented for visual indent
    
  19. 
      
CO
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/reviews/tests/test_views.py (Diff revision 14)
     
     

    You might want to split this up into multiple tests since it does a lot.

  3. reviewboard/reviews/tests/test_views.py (Diff revision 14)
     
     

    No period for test docstrings (the test runner adds periods).

  4. reviewboard/reviews/tests/test_views.py (Diff revision 14)
     
     

    No blank line here.

  5. reviewboard/reviews/tests/test_views.py (Diff revision 14)
     
     

    Comments should almost always be complete sentences and they should begin with a capital.

  6. reviewboard/reviews/tests/test_views.py (Diff revision 14)
     
     
     
     

    Why are there more than six entries here?

    1. We also need to set the timestamp of the replies for each review.

  7. reviewboard/reviews/tests/test_views.py (Diff revision 14)
     
     
     

    Blank line between these.

  8. reviewboard/reviews/tests/test_views.py (Diff revision 14)
     
     

    You should be building the URL using reverse(). Same elsewhere.

    1. The rest of the test suite seems to use this command to get the entire review request.
      Also, I'm not quite sure how to use reverse() or local_site_reverse() to get the entire review request.

    2. rsp = self.client.get(
          local_site_reverse(
              'review-request-detail',
              kwargs={
                  'review_request_id': review_request.pk,
              }
          ))
      
  9. reviewboard/reviews/tests/test_views.py (Diff revision 14)
     
     

    Missing docstring.

    This test is gigantic and should be split up into multiple tests.

  10. reviewboard/reviews/views.py (Diff revision 14)
     
     
     
     
     

    datetime is a python stdlib import.

    1. I actually want the datetime from the datetime library. They both have the same name and do similar things so its kinda confusing.

    2. Yes, which is standard library. This should look like:

      import logging
      import time
      from datetime import datetime, timedelta
      
      # 3rd party imports
      
      # reviewboard.* imports
      
  11. reviewboard/reviews/views.py (Diff revision 14)
     
     
     

    Move the comment into the docstring.

  12. reviewboard/reviews/views.py (Diff revision 14)
     
     
     

    Blank line between these.

  13. reviewboard/reviews/views.py (Diff revision 14)
     
     
     

    Blank line between these.

  14. 
      
CO
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
    
    
  2. reviewboard/reviews/tests/test_views.py (Diff revision 15)
     
     
    Col: 80
     E501 line too long (89 > 79 characters)
    
  3. reviewboard/reviews/tests/test_views.py (Diff revision 15)
     
     
    Col: 80
     E501 line too long (89 > 79 characters)
    
  4. reviewboard/reviews/tests/test_views.py (Diff revision 15)
     
     
    Col: 80
     E501 line too long (89 > 79 characters)
    
  5. reviewboard/reviews/tests/test_views.py (Diff revision 15)
     
     
    Col: 80
     E501 line too long (92 > 79 characters)
    
  6. reviewboard/reviews/tests/test_views.py (Diff revision 15)
     
     
    Col: 89
     E226 missing whitespace around arithmetic operator
    
  7. 
      
CO
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
    
    
  2. reviewboard/reviews/tests/test_views.py (Diff revision 16)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
CO
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
    
    
  2. reviewboard/reviews/tests/test_views.py (Diff revision 17)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. reviewboard/reviews/tests/test_views.py (Diff revision 17)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  4. reviewboard/reviews/tests/test_views.py (Diff revision 17)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  5. reviewboard/reviews/tests/test_views.py (Diff revision 17)
     
     
    Col: 5
     E303 too many blank lines (3)
    
  6. 
      
CO
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
    
    
  2. 
      
chipx86
Review request changed

Status: Discarded

Change Summary:

Rewrote for the new entry work as /r/9281/

Loading...