Smart Update Notification

Review Request #8738 — Created Feb. 10, 2017 and discarded

Information

Review Board
master

Reviewers

When viewing a review request, Review Board periodically checks to see if there have been any updates to the review request.Currently we only receive the last update regarding the type of change and who it was made by.
The goal of this project is to retrieve and display all sorts of updates since a specific timestamp with the type of change, who it was made by, and other useful details.

Manual tests in the browsers have been done on both UpdateBubble and TextBubble:
- No update: no update bubble was opened
- 1 update: update bubble was shown the 1 update with the update type and username
- Multiple updates: update bubble was shown with counts by username and update type. Please refer to attached file screenshot
- Hovering over the UpdateBubble will expand the bubble to display the text bubbles with some CSS transition
- All the texts are capped at 300 character per with a [...] indicator
- The text pane is vertically scrollable when necessary
- "Ship it!" label is displayed on middle top of text box when appliable
- For review/reply, text is either "body-top" content or first comment.
- For update, text is a short summary with some relevant description.
- Clicking on each text bubble will refresh the page and navigate to the specific location.

Python testing cases for review_request_last_update API include as following:
- New reviews/comments: pass
- New replies: pass
- New udpates (diffsets, field changes and file attachments): pass
- default case for no/improperly-formatted timestamp: pass
- Test case for no updates: pass

Jasmine JavaScript Unit tests have been added for:
- UpdateBubbleView: pass
- TextBubbleView: pass


Description From Last Updated

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 9 E303 too many blank lines (2)

reviewbotreviewbot

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

reviewbotreviewbot

Col: 80 E501 line too long (86 > 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 (105 > 79 characters)

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 5 E303 too many blank lines (3)

reviewbotreviewbot

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

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

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 (85 > 79 characters)

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 9 E303 too many blank lines (2)

reviewbotreviewbot

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

reviewbotreviewbot

Col: 80 E501 line too long (86 > 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 (105 > 79 characters)

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 5 E303 too many blank lines (3)

reviewbotreviewbot

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

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

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 (85 > 79 characters)

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

'timedelta' imported but unused

reviewbotreviewbot

'timezone' imported but unused

reviewbotreviewbot

'DiffSet' imported but unused

reviewbotreviewbot

'Review' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

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

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

Col: 63 W292 no newline at end of file

reviewbotreviewbot

Small nit, put a space after `<%='

KH khp

Just eyeing this comment, I think this comment can be one line possibly?

KH khp

I believe this line should be indented one more (see other comments below!) Also, "returned" instead of "return"

KH khp

Could do this in one line Capitalize "the" in the beginning

KH khp

Since this is an es6 file, this should probably be a const or use let.

mike_conleymike_conley

Why do we need to check if hasOwnProperty?

mike_conleymike_conley

Maybe I'm just getting old, but I'm having a hard time parsing this bit. Here's what I think is happening: …

mike_conleymike_conley

2 spaces between return and updateCountByUserName

KH khp

IMO using distinct strings for username and fullname could catch more bugs that involve the two values.

KH khp

If we're getting rid of the type argument, we should update this documentation.

mike_conleymike_conley

We probably don't want to have this change land in the tree.

mike_conleymike_conley

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

reviewbotreviewbot

Would something like this here and similar code below be preferable? new_reply = { 'id': review.pk, 'user': review.user, ...

KH khp

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

reviewbotreviewbot

I think you need a new line above the for

KH khp

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

reviewbotreviewbot

New line here too

KH khp

New line above the if I think

KH khp

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

reviewbotreviewbot

I'm not 100% sure but other comments seem to do this; """ and Retrieve might need to be on the …

KH khp

Missing periods at the end of some of these lines

KH khp

reiview -> review

KH khp

Might need to move this line up one like above

KH khp

Missing periods here "the updated" "the timestamp" "the new" needs to be capitalized

KH khp

New line here

KH khp

Periods

KH khp

Why is this commented out? Should it be removed instead?

mike_conleymike_conley

Col: 21 E122 continuation line missing indentation or outdented

reviewbotreviewbot

Col: 17 E122 continuation line missing indentation or outdented

reviewbotreviewbot

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

reviewbotreviewbot

Col: 9 E303 too many blank lines (2)

reviewbotreviewbot

Col: 21 E122 continuation line missing indentation or outdented

reviewbotreviewbot

Col: 25 E122 continuation line missing indentation or outdented

reviewbotreviewbot

I think this line should be indented as per the new documentation style being phased into Review Board. More information …

sharleenfishersharleenfisher

Also indented here.

sharleenfishersharleenfisher

Do we need the white-space on line 116? It looks like, from other code that I've seen, sometimes they put …

sharleenfishersharleenfisher

I think given the new documentation format, the type that 'username' is should be in brackets? For example, username (String):

sharleenfishersharleenfisher

Here too!

sharleenfishersharleenfisher

The type should be added in brackets here as well.

sharleenfishersharleenfisher

And here as well, I believe?

sharleenfishersharleenfisher

'_' imported but unused

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

I'm not sure these changes should be in this review request. They are already in https://reviews.reviewboard.org/r/8800/, no?

szhangszhang

Same with this test here.

szhangszhang

Col: 39 Missing semicolon.

reviewbotreviewbot

Col: 28 Missing semicolon.

reviewbotreviewbot

Col: 61 Missing semicolon.

reviewbotreviewbot

Col: 34 Missing semicolon.

reviewbotreviewbot

Col: 41 Missing semicolon.

reviewbotreviewbot

Col: 25 ['username'] is better written in dot notation.

reviewbotreviewbot

Col: 25 ['count'] is better written in dot notation.

reviewbotreviewbot

Col: 45 Missing semicolon.

reviewbotreviewbot

Col: 25 ['user'] is better written in dot notation.

reviewbotreviewbot

Col: 67 Missing semicolon.

reviewbotreviewbot

Col: 25 ['list'] is better written in dot notation.

reviewbotreviewbot

Col: 66 Missing semicolon.

reviewbotreviewbot

Col: 38 Missing semicolon.

reviewbotreviewbot

Col: 49 Missing semicolon.

reviewbotreviewbot

Col: 37 Missing semicolon.

reviewbotreviewbot

Col: 45 ['username'] is better written in dot notation.

reviewbotreviewbot

Col: 39 ['user'] is better written in dot notation.

reviewbotreviewbot

Col: 47 Missing semicolon.

reviewbotreviewbot

Col: 22 Missing semicolon.

reviewbotreviewbot

Col: 45 ['username'] is better written in dot notation.

reviewbotreviewbot

Col: 43 Missing semicolon.

reviewbotreviewbot

Col: 57 Missing semicolon.

reviewbotreviewbot

Col: 13 'info' is defined but never used.

reviewbotreviewbot

Col: 21 Expected '}' to match '{' from line 152 and instead saw 'user'.

reviewbotreviewbot

Col: 25 Expected ']' to match '[' from line 152 and instead saw ':'.

reviewbotreviewbot

Col: 27 Expected '}' to match '{' from line 130 and instead saw '{'.

reviewbotreviewbot

Col: 28 Missing semicolon.

reviewbotreviewbot

Col: 30 Label 'url' on /users/cherry statement.

reviewbotreviewbot

Col: 30 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 45 Missing semicolon.

reviewbotreviewbot

Col: 39 Missing semicolon.

reviewbotreviewbot

Col: 28 Missing semicolon.

reviewbotreviewbot

Col: 61 Missing semicolon.

reviewbotreviewbot

Col: 34 Missing semicolon.

reviewbotreviewbot

Col: 25 ['username'] is better written in dot notation.

reviewbotreviewbot

Col: 41 Missing semicolon.

reviewbotreviewbot

Col: 25 ['count'] is better written in dot notation.

reviewbotreviewbot

Col: 45 Missing semicolon.

reviewbotreviewbot

Col: 25 ['user'] is better written in dot notation.

reviewbotreviewbot

Col: 67 Missing semicolon.

reviewbotreviewbot

Col: 25 ['list'] is better written in dot notation.

reviewbotreviewbot

Col: 66 Missing semicolon.

reviewbotreviewbot

Col: 38 Missing semicolon.

reviewbotreviewbot

Col: 49 Missing semicolon.

reviewbotreviewbot

Col: 37 Missing semicolon.

reviewbotreviewbot

Col: 45 ['username'] is better written in dot notation.

reviewbotreviewbot

Col: 39 ['user'] is better written in dot notation.

reviewbotreviewbot

Col: 47 Missing semicolon.

reviewbotreviewbot

Col: 22 Missing semicolon.

reviewbotreviewbot

Col: 45 ['username'] is better written in dot notation.

reviewbotreviewbot

Col: 43 Missing semicolon.

reviewbotreviewbot

Col: 13 'info' is defined but never used.

reviewbotreviewbot

Col: 21 Expected '}' to match '{' from line 152 and instead saw 'user'.

reviewbotreviewbot

Col: 25 Expected ']' to match '[' from line 152 and instead saw ':'.

reviewbotreviewbot

Col: 27 Expected '}' to match '{' from line 130 and instead saw '{'.

reviewbotreviewbot

Col: 28 Missing semicolon.

reviewbotreviewbot

Col: 30 Label 'url' on /users/cherry statement.

reviewbotreviewbot

Col: 45 Missing semicolon.

reviewbotreviewbot

Col: 30 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 13 Expected ')' and instead saw '}'.

reviewbotreviewbot

Col: 10 Missing semicolon.

reviewbotreviewbot

Col: 20 Missing semicolon.

reviewbotreviewbot

Col: 23 Missing semicolon.

reviewbotreviewbot

Col: 10 Missing semicolon.

reviewbotreviewbot

Why the variable rename? server_timestamp makes it sound like the current time on the server.

chipx86chipx86

Blank line before if statements.

chipx86chipx86

And after. Any time there's a block level change.

chipx86chipx86

Can you add a comment explaining why we're doing the save_base? We'll probably forget down the road.

chipx86chipx86

All the HTML here should be using proper indentation to make the code more readable.

chipx86chipx86

Should use <%- ... %> for any user-controllable content (username, full name, etc.). This will escape the content so users …

chipx86chipx86

This function's iterating through everything but timestamps. I think we really just have a few types of entries in here …

chipx86chipx86

Blank line between code and new blocks.

chipx86chipx86

Here too.

chipx86chipx86

Here too.

chipx86chipx86

This can all be one statement.

chipx86chipx86

Here too. Also there's an alignment problem.

chipx86chipx86

Blank line here too.

chipx86chipx86

Blank line.

chipx86chipx86

Blank line.

chipx86chipx86

This template also needs indentation.

chipx86chipx86

Best to hide before adding the HTML, to reduce what needs to be done in the DOM.

chipx86chipx86

Col: 10 Missing semicolon.

reviewbotreviewbot

This can be one import.

chipx86chipx86

We have to maintain full backwards-compatibility. All the old fields must remain using the same behavior we used to have. …

chipx86chipx86

"server_timestamp" is the wrong name here. That sounds like the current time on the server.

chipx86chipx86

Docstrings don't belong in the middle of code blocks. You need to use comments.

chipx86chipx86

Blank line here.

chipx86chipx86

Blank line.

chipx86chipx86

This can be one statement.

chipx86chipx86

One statement.

chipx86chipx86

id is a reserved word in Python. This should be pk.

chipx86chipx86

Blank line.

chipx86chipx86
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request_last_update.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request_last_update.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
    
    
  2. Show all issues
    Col: 80
     E501 line too long (90 > 79 characters)
    
  3. Show all issues
    Col: 80
     E501 line too long (85 > 79 characters)
    
  4. Show all issues
    Col: 80
     E501 line too long (86 > 79 characters)
    
  5. Show all issues
    Col: 80
     E501 line too long (86 > 79 characters)
    
  6. Show all issues
    Col: 80
     E501 line too long (93 > 79 characters)
    
  7. Show all issues
    Col: 80
     E501 line too long (97 > 79 characters)
    
  8. Show all issues
    Col: 80
     E501 line too long (89 > 79 characters)
    
  9. Show all issues
    Col: 9
     E303 too many blank lines (2)
    
  10. Show all issues
    Col: 80
     E501 line too long (86 > 79 characters)
    
  11. Show all issues
    Col: 80
     E501 line too long (86 > 79 characters)
    
  12. Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  13. Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  14. Show all issues
    Col: 80
     E501 line too long (105 > 79 characters)
    
  15. Show all issues
    Col: 80
     E501 line too long (98 > 79 characters)
    
  16. Show all issues
    Col: 80
     E501 line too long (84 > 79 characters)
    
  17. Show all issues
    Col: 80
     E501 line too long (94 > 79 characters)
    
  18. Show all issues
    Col: 5
     E303 too many blank lines (3)
    
  19. Show all issues
    Col: 80
     E501 line too long (103 > 79 characters)
    
  20. Show all issues
    Col: 5
     E303 too many blank lines (2)
    
  21. Show all issues
    Col: 80
     E501 line too long (82 > 79 characters)
    
  22. Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  23. Show all issues
    Col: 80
     E501 line too long (85 > 79 characters)
    
  24. Show all issues
    Col: 5
     E303 too many blank lines (2)
    
  25. Show all issues
    Col: 5
     E303 too many blank lines (2)
    
  26. 
      
AN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/tests/test_review_request_last_update.py
        reviewboard/webapi/resources/review_request_last_update.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/tests/test_review_request_last_update.py
        reviewboard/webapi/resources/review_request_last_update.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
    
    
  2. Show all issues
    Col: 80
     E501 line too long (90 > 79 characters)
    
  3. Show all issues
    Col: 80
     E501 line too long (85 > 79 characters)
    
  4. Show all issues
    Col: 80
     E501 line too long (86 > 79 characters)
    
  5. Show all issues
    Col: 80
     E501 line too long (86 > 79 characters)
    
  6. Show all issues
    Col: 80
     E501 line too long (93 > 79 characters)
    
  7. Show all issues
    Col: 80
     E501 line too long (97 > 79 characters)
    
  8. Show all issues
    Col: 80
     E501 line too long (89 > 79 characters)
    
  9. Show all issues
    Col: 9
     E303 too many blank lines (2)
    
  10. Show all issues
    Col: 80
     E501 line too long (86 > 79 characters)
    
  11. Show all issues
    Col: 80
     E501 line too long (86 > 79 characters)
    
  12. Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  13. Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  14. Show all issues
    Col: 80
     E501 line too long (105 > 79 characters)
    
  15. Show all issues
    Col: 80
     E501 line too long (98 > 79 characters)
    
  16. Show all issues
    Col: 80
     E501 line too long (84 > 79 characters)
    
  17. Show all issues
    Col: 80
     E501 line too long (94 > 79 characters)
    
  18. Show all issues
    Col: 5
     E303 too many blank lines (3)
    
  19. Show all issues
    Col: 80
     E501 line too long (103 > 79 characters)
    
  20. Show all issues
    Col: 5
     E303 too many blank lines (2)
    
  21. Show all issues
    Col: 80
     E501 line too long (82 > 79 characters)
    
  22. Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  23. Show all issues
    Col: 80
     E501 line too long (85 > 79 characters)
    
  24. Show all issues
    Col: 5
     E303 too many blank lines (2)
    
  25. Show all issues
    Col: 5
     E303 too many blank lines (2)
    
  26. Show all issues
     'timedelta' imported but unused
    
  27. Show all issues
     'timezone' imported but unused
    
  28. Show all issues
     'DiffSet' imported but unused
    
  29. Show all issues
     'Review' imported but unused
    
  30. Show all issues
    Col: 80
     E501 line too long (82 > 79 characters)
    
  31. Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  32. Show all issues
    Col: 80
     E501 line too long (84 > 79 characters)
    
  33. Show all issues
    Col: 5
     E303 too many blank lines (2)
    
  34. Show all issues
    Col: 63
     W292 no newline at end of file
    
  35. 
      
AN
AN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/tests/test_review_request_last_update.py
        reviewboard/webapi/resources/review_request_last_update.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/tests/test_review_request_last_update.py
        reviewboard/webapi/resources/review_request_last_update.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
    
    
  2. Show all issues
    Col: 29
     E126 continuation line over-indented for hanging indent
    
  3. Show all issues
    Col: 41
     E126 continuation line over-indented for hanging indent
    
  4. Show all issues
    Col: 37
     E126 continuation line over-indented for hanging indent
    
  5. Show all issues
    Col: 37
     E126 continuation line over-indented for hanging indent
    
  6. 
      
KH
  1. Wow that's a lot of code! Mostly formatting nits and comments.

  2. Show all issues

    Small nit, put a space after `<%='

  3. Show all issues

    Just eyeing this comment, I think this comment can be one line possibly?

  4. Show all issues

    I believe this line should be indented one more (see other comments below!)

    Also, "returned" instead of "return"

  5. Show all issues

    Could do this in one line
    Capitalize "the" in the beginning

  6. Show all issues

    2 spaces between return and updateCountByUserName

  7. Show all issues

    IMO using distinct strings for username and fullname could catch more bugs that involve the two values.

  8. reviewboard/webapi/resources/review_request_last_update.py (Diff revision 3)
     
     
     
     
     
     
     
    Show all issues

    Would something like this here and similar code below be preferable?

    new_reply = {
        'id': review.pk,
        'user': review.user,
        ...
    
  9. Show all issues

    I think you need a new line above the for

  10. Show all issues

    New line here too

  11. Show all issues

    New line above the if I think

  12. Show all issues

    I'm not 100% sure but other comments seem to do this; """ and Retrieve might need to be on the same line, like:

    """Retrieve the new ...
    

    Missing a period at the end

  13. reviewboard/webapi/resources/review_request_last_update.py (Diff revision 3)
     
     
     
     
     
     
     
     
    Show all issues

    Missing periods at the end of some of these lines

  14. Show all issues

    reiview -> review

  15. Show all issues

    Might need to move this line up one like above

  16. reviewboard/webapi/resources/review_request_last_update.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Missing periods here
    "the updated" "the timestamp" "the new" needs to be capitalized

  17. Show all issues

    New line here

  18. reviewboard/webapi/resources/review_request_last_update.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
    Show all issues

    Periods

  19. 
      
mike_conley
  1. 
      
  2. Show all issues

    Since this is an es6 file, this should probably be a const or use let.

    1. Should I replace all the 'var' with 'let' in an es6 file unless I want it to be globally availabe?

    2. For any new code you write, you should be using const, unless you need the variable to be mutable - in which case, you should use let.

    3. Got it.

  3. Show all issues

    Why do we need to check if hasOwnProperty?

    1. I guess yes, we need to use it to exclude chain properties, only check "info" object's own properties. Otherwise countBy(info[key], 'username') will throw error

    2. Which chain properties are you referring to? What else in the info objects prototype chain is going to contain a 'username' property?

    3. It is recommended by some web samples to check if hasOwnProperty when traversing the JavaScript array to exclude some properties inherited from chain. But I did a test by removing hasOwnProperty check, the code works fine. I guess in the JSON object we don't need to do that. I have removed it.

  4. reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Maybe I'm just getting old, but I'm having a hard time parsing this bit.

    Here's what I think is happening:

    You're constructing a count of how many times a particular username has an update in the last-update object... That will look something like:

    {'<username>': 12}, in the event that some username has 12 updates.

    Then you're going through that returned object, and constructing a very similar object and putting it on result.

    Why is this inner loop necessary? Why do we need to check hasOwnProperty? Can we not just take the return value from _.countBy and assign it to result? If not, why not?

      • hasOwnProperty: we need to use it to exclude chain properties, only check "info" object's own properties. Otherwise countBy(info[key], 'username') will throw error.
      • Yes, you are right about the inner loop. As I am not familiar with Javascript, there might be better efficient way to do that. The return value from _.countBy is in the format of (key, value) pair, I don't know how to parse the pair results into the template. I googled the problem, it is suggested to convert (key, value) pair to the object format {'keyname': username, 'valuename': value}. That's the reason for the inner loop.
    1. The return value from _.countBy is in the format of (key, value) pair

      To be clear, the return value from .countBy is:

      {
        <key>: <count>,
        ...
      }
      

      The .template should take arguments in that format, too. For example, suppose I construct a template like this:

      let t = _.template("hello: <%= name %>");
      t({'name': 'anni'});
      // ^-- Returns "hello: anni"
      

      So I don't think an additional transformation is needed here.

    2. I've updated the final returned object updateCountbyUserName, which will be used to populate the template. The following is a sample of its format:
      { 'new_comments': [ {'count': 2, 'username': "mark", 'url': "/users/mark" } ] 'new-replies': [ {'count': 1, 'username': "alice", 'url': "/users/alice" } {'count': 2, 'username': "bob", 'url': "/users/bob" } ] 'new_diffsets': [] 'new_reviews': [] 'reviewrequest_updates': [] }

    3. In my updated template, first need to check if each type update is available, then display its info. Following is a sample of one type:

      '<% if (new_replies) { %>', '<% _.each(new_replies, function(reply) { %>', '<span class="reply" id="<%= reply.username %>">', '<%= reply.count %>', '<% if (reply.count == 1) { %>', ' new reply by ', '<% } else { %>', ' new replies by ', '<% } %>', '<a href="<%= reply.url %>" id="updates-bubble-user">', '<%= reply.username %>', '</a>', '</span>',
      I don't know how I can directly use the key value pair returned by .countBy

  5. Show all issues

    If we're getting rid of the type argument, we should update this documentation.

  6. Show all issues

    We probably don't want to have this change land in the tree.

  7. 
      
AN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/tests/test_review_request_last_update.py
        reviewboard/webapi/resources/review_request_last_update.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/tests/test_review_request_last_update.py
        reviewboard/webapi/resources/review_request_last_update.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
    
    
  2. 
      
mike_conley
  1. 
      
  2. Show all issues

    Why is this commented out? Should it be removed instead?

  3. 
      
AN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/tests/test_review_request_last_update.py
        reviewboard/webapi/resources/review_request_last_update.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/tests/test_review_request_last_update.py
        reviewboard/webapi/resources/review_request_last_update.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
    
    
  2. Show all issues
    Col: 21
     E122 continuation line missing indentation or outdented
    
  3. Show all issues
    Col: 17
     E122 continuation line missing indentation or outdented
    
  4. Show all issues
    Col: 80
     E501 line too long (83 > 79 characters)
    
  5. Show all issues
    Col: 9
     E303 too many blank lines (2)
    
  6. Show all issues
    Col: 21
     E122 continuation line missing indentation or outdented
    
  7. Show all issues
    Col: 25
     E122 continuation line missing indentation or outdented
    
  8. 
      
AN
AN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/tests/test_review_request_last_update.py
        reviewboard/webapi/resources/review_request_last_update.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/tests/test_review_request_last_update.py
        reviewboard/webapi/resources/review_request_last_update.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
    
    
  2. 
      
AN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/tests/test_review_request_last_update.py
        reviewboard/webapi/resources/review_request_last_update.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/tests/test_review_request_last_update.py
        reviewboard/webapi/resources/review_request_last_update.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
    
    
  2. 
      
sharleenfisher
  1. It looks like a lot of work has been done here already! Since this is just my first review, I'll probably just look over things stylistically.

    1. thank you very much for your following reviews :-)

  2. 
      
sharleenfisher
  1. Sorry about the previous review. Here are the formatting issues I found within the first file.

  2. Show all issues

    I think this line should be indented as per the new documentation style being phased into Review Board. More information can be found here.

    https://www.notion.so/reviewboard/Writing-Codebase-Documentation-e16312b5f061437cb73cbfa369ac3cb5

  3. Show all issues

    Also indented here.

  4. reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Do we need the white-space on line 116? It looks like, from other code that I've seen, sometimes they put a white line before and after a conditional or a for loop.

  5. Show all issues

    I think given the new documentation format, the type that 'username' is should be in brackets?

    For example,
    username (String):

  6. Show all issues

    Here too!

  7. Show all issues

    The type should be added in brackets here as well.

  8. Show all issues

    And here as well, I believe?

  9. 
      
AN
AN
AN
AN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/tests/test_review_request_last_update.py
        reviewboard/reviews/tests/test_review_request.py
        reviewboard/reviews/models/review_request.py
        reviewboard/webapi/resources/review_request_last_update.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/tests/test_review_request_last_update.py
        reviewboard/reviews/tests/test_review_request.py
        reviewboard/reviews/models/review_request.py
        reviewboard/webapi/resources/review_request_last_update.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
    
    
  2. Show all issues
     '_' imported but unused
    
  3. Show all issues
    Col: 5
     E303 too many blank lines (2)
    
  4. 
      
AN
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/tests/test_review_request_last_update.py
        reviewboard/reviews/tests/test_review_request.py
        reviewboard/reviews/models/review_request.py
        reviewboard/webapi/resources/review_request_last_update.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
        reviewboard/static/rb/css/pages/review-request.less
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/tests/test_review_request_last_update.py
        reviewboard/reviews/tests/test_review_request.py
        reviewboard/reviews/models/review_request.py
        reviewboard/webapi/resources/review_request_last_update.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
        reviewboard/static/rb/css/pages/review-request.less
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
    
    
  2. 
      
szhang
  1. 
      
  2. reviewboard/reviews/models/review_request.py (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    I'm not sure these changes should be in this review request. They are already in https://reviews.reviewboard.org/r/8800/, no?

    1. I need to call the get_last_activity() when implementing the last_update API backend. So need to include this temporary solution to make other codes working. Will sync this with /r/8800 once the solution to the get_last_activity() is finalized

  3. reviewboard/reviews/tests/test_review_request.py (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Same with this test here.

    1. Same as above explanation

  4. 
      
AN
Review request changed
Commit:
d601df2a1443284e666e06283cb5d8409e7478d2
a734c522ed39b7e8d46f3e8f5542a453011e1008

Checks run (1 failed, 1 succeeded, 1 failed with error)

JSHint failed.
PEP8 Style Checker internal error.
Pyflakes passed.

JSHint

AN
Review request changed
Commit:
a734c522ed39b7e8d46f3e8f5542a453011e1008
bb9f8f5b24108ed5a11c44782ac1e6169279efab

Checks run (1 failed, 1 succeeded, 1 failed with error)

JSHint failed.
PEP8 Style Checker internal error.
Pyflakes passed.

JSHint

AN
AN
Review request changed
Commit:
bb9f8f5b24108ed5a11c44782ac1e6169279efab
1cc94b5cab9fdf6d04c852f4fb1bf942b205d907

Checks run (1 failed, 1 succeeded, 1 failed with error)

JSHint failed.
PEP8 Style Checker internal error.
Pyflakes passed.

JSHint

AN
AN
AN
AN
AN
AN
AN
Review request changed
AN
Review request changed
Testing Done:
   

Manual tests in the browsers have been done on both UpdateBubble and TextBubble:

    - No update: no update bubble was opened
    - 1 update: update bubble was shown the 1 update with the update type and username
    - Multiple updates: update bubble was shown with counts by username and update type. Please refer to attached file screenshot
    - Hovering over the UpdateBubble will expand the bubble to display the text bubbles with some CSS transition
    - All the texts are capped at 300 character per with a [...] indicator
    - The text pane is vertically scrollable when necessary
    - "Ship it!" label is displayed on middle top of text box when appliable
    - For review/reply, text is either "body-top" content or first comment.
    - For update, text is a short summary with some relevant description.
    - Clicking on each text bubble will refresh the page and navigate to the specific location.

   
   

Python testing cases for review_request_last_update API include as following:

    - New reviews/comments: pass
    - New replies: pass
    - New udpates (diffsets, field changes and file attachments): pass
    - default case for no/improperly-formatted timestamp: pass
    - Test case for no updates: pass

   
~  

Jasmine testing for the UpdateBubbleView has been passed.

  ~

Jasmine JavaScript Unit tests have been added for:

  + - UpdateBubbleView: pass
  + - TextBubbleView: pass

Commit:
83ca097857c903acb0401dddacd8fe37ff5d6b4e
9cbbbc19d5d258e1265f59a42a9316097531bfab

Checks run (1 failed, 1 succeeded, 1 failed with error)

JSHint failed.
PEP8 Style Checker internal error.
Pyflakes passed.

JSHint

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

    Why the variable rename? server_timestamp makes it sound like the current time on the server.

    1. As I added a new parameter call timestamp for the function get(self, request, timestamp=None, *args, **kwargs), to differentiate these two timestamps, I changed the variable name. Not sure what name is appropriate in this case.

    2. last_updated_timestamp would be fine.

    3. OK, will change the variable name to last_updated_timestamp

  3. Show all issues

    Blank line before if statements.

  4. Show all issues

    And after. Any time there's a block level change.

  5. Show all issues

    Can you add a comment explaining why we're doing the save_base? We'll probably forget down the road.

  6. Show all issues

    All the HTML here should be using proper indentation to make the code more readable.

  7. Show all issues

    Should use <%- ... %> for any user-controllable content (username, full name, etc.). This will escape the content so users can't insert HTML.

  8. Show all issues

    This function's iterating through everything but timestamps. I think we really just have a few types of entries in here we care about, right? If so, can we handle those specifically?

    1. Can info.hasOwnProperty(key) handle that?

  9. Show all issues

    Blank line between code and new blocks.

  10. Show all issues

    Here too.

  11. Show all issues

    Here too.

  12. reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 14)
     
     
     
     
     
     
     
    Show all issues

    This can all be one statement.

  13. Show all issues

    Here too. Also there's an alignment problem.

  14. Show all issues

    Blank line here too.

  15. Show all issues

    Blank line.

  16. Show all issues

    Blank line.

  17. Show all issues

    This template also needs indentation.

  18. Show all issues

    Best to hide before adding the HTML, to reduce what needs to be done in the DOM.

    1. I am newbie on HTML things, can you please explain a bit more how I can hide before adding HTML?

  19. Show all issues

    This can be one import.

  20. reviewboard/webapi/resources/review_request_last_update.py (Diff revision 14)
     
     
     
     
     
     
     
    Show all issues

    We have to maintain full backwards-compatibility. All the old fields must remain using the same behavior we used to have. The new fields should just be new optional fields.

    So in other words, the older UI code using the newer API should behave the way it did before.

    1. As the change will take some time, I will work on it later next week.

  21. Show all issues

    "server_timestamp" is the wrong name here. That sounds like the current time on the server.

  22. reviewboard/webapi/resources/review_request_last_update.py (Diff revision 14)
     
     
     
     
     
     
     
    Show all issues

    Docstrings don't belong in the middle of code blocks. You need to use comments.

  23. Show all issues

    Blank line here.

  24. Show all issues

    Blank line.

  25. reviewboard/webapi/resources/review_request_last_update.py (Diff revision 14)
     
     
     
     
     
     
     
     
    Show all issues

    This can be one statement.

    1. Not sure how to do one statement correctly, could you please show me a sample?

  26. reviewboard/webapi/resources/review_request_last_update.py (Diff revision 14)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    One statement.

  27. Show all issues

    id is a reserved word in Python. This should be pk.

  28. Show all issues

    Blank line.

  29. 
      
AN
AN
david
Review request changed
Status:
Discarded