Smart Update Notification

Review Request #8738 - Created Feb. 10, 2017 and updated

Anni Cao
Review Board
master
50678fb...
reviewboard, students

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

Loading file attachments...

  • 6
  • 0
  • 174
  • 13
  • 193
Description From Last Updated
This function's iterating through everything but timestamps. I think we really just have a few types of entries in here ... Christian Hammond Christian Hammond
This can all be one statement. Christian Hammond Christian Hammond
Best to hide before adding the HTML, to reduce what needs to be done in the DOM. Christian Hammond Christian Hammond
We have to maintain full backwards-compatibility. All the old fields must remain using the same behavior we used to have. ... Christian Hammond Christian Hammond
This can be one statement. Christian Hammond Christian Hammond
One statement. Christian Hammond Christian Hammond
Review Bot
  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. Col: 80
     E501 line too long (90 > 79 characters)
    
  3. Col: 80
     E501 line too long (85 > 79 characters)
    
  4. Col: 80
     E501 line too long (86 > 79 characters)
    
  5. Col: 80
     E501 line too long (86 > 79 characters)
    
  6. Col: 80
     E501 line too long (93 > 79 characters)
    
  7. Col: 80
     E501 line too long (97 > 79 characters)
    
  8. Col: 80
     E501 line too long (89 > 79 characters)
    
  9. Col: 9
     E303 too many blank lines (2)
    
  10. Col: 80
     E501 line too long (86 > 79 characters)
    
  11. Col: 80
     E501 line too long (86 > 79 characters)
    
  12. Col: 80
     E501 line too long (80 > 79 characters)
    
  13. Col: 80
     E501 line too long (80 > 79 characters)
    
  14. Col: 80
     E501 line too long (105 > 79 characters)
    
  15. Col: 80
     E501 line too long (98 > 79 characters)
    
  16. Col: 80
     E501 line too long (84 > 79 characters)
    
  17. Col: 80
     E501 line too long (94 > 79 characters)
    
  18. Col: 5
     E303 too many blank lines (3)
    
  19. Col: 80
     E501 line too long (103 > 79 characters)
    
  20. Col: 5
     E303 too many blank lines (2)
    
  21. Col: 80
     E501 line too long (82 > 79 characters)
    
  22. Col: 80
     E501 line too long (81 > 79 characters)
    
  23. Col: 80
     E501 line too long (85 > 79 characters)
    
  24. Col: 5
     E303 too many blank lines (2)
    
  25. Col: 5
     E303 too many blank lines (2)
    
  26. 
      
Anni Cao
Review Bot
  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. Col: 80
     E501 line too long (90 > 79 characters)
    
  3. Col: 80
     E501 line too long (85 > 79 characters)
    
  4. Col: 80
     E501 line too long (86 > 79 characters)
    
  5. Col: 80
     E501 line too long (86 > 79 characters)
    
  6. Col: 80
     E501 line too long (93 > 79 characters)
    
  7. Col: 80
     E501 line too long (97 > 79 characters)
    
  8. Col: 80
     E501 line too long (89 > 79 characters)
    
  9. Col: 9
     E303 too many blank lines (2)
    
  10. Col: 80
     E501 line too long (86 > 79 characters)
    
  11. Col: 80
     E501 line too long (86 > 79 characters)
    
  12. Col: 80
     E501 line too long (80 > 79 characters)
    
  13. Col: 80
     E501 line too long (80 > 79 characters)
    
  14. Col: 80
     E501 line too long (105 > 79 characters)
    
  15. Col: 80
     E501 line too long (98 > 79 characters)
    
  16. Col: 80
     E501 line too long (84 > 79 characters)
    
  17. Col: 80
     E501 line too long (94 > 79 characters)
    
  18. Col: 5
     E303 too many blank lines (3)
    
  19. Col: 80
     E501 line too long (103 > 79 characters)
    
  20. Col: 5
     E303 too many blank lines (2)
    
  21. Col: 80
     E501 line too long (82 > 79 characters)
    
  22. Col: 80
     E501 line too long (81 > 79 characters)
    
  23. Col: 80
     E501 line too long (85 > 79 characters)
    
  24. Col: 5
     E303 too many blank lines (2)
    
  25. Col: 5
     E303 too many blank lines (2)
    
  26.  'timedelta' imported but unused
    
  27.  'timezone' imported but unused
    
  28.  'DiffSet' imported but unused
    
  29.  'Review' imported but unused
    
  30. Col: 80
     E501 line too long (82 > 79 characters)
    
  31. Col: 1
     E302 expected 2 blank lines, found 1
    
  32. Col: 80
     E501 line too long (84 > 79 characters)
    
  33. Col: 5
     E303 too many blank lines (2)
    
  34. Col: 63
     W292 no newline at end of file
    
  35. 
      
Anni Cao
Anni Cao
Review Bot
  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. Col: 29
     E126 continuation line over-indented for hanging indent
    
  3. Col: 41
     E126 continuation line over-indented for hanging indent
    
  4. Col: 37
     E126 continuation line over-indented for hanging indent
    
  5. Col: 37
     E126 continuation line over-indented for hanging indent
    
  6. 
      
Kanghee Park
  1. Wow that's a lot of code! Mostly formatting nits and comments.

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

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

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

    Also, "returned" instead of "return"

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

  6. 2 spaces between return and updateCountByUserName

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

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

    new_reply = {
        'id': review.pk,
        'user': review.user,
        ...
    
  9. I think you need a new line above the for

  10. New line above the if I think

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

  12. reviewboard/webapi/resources/review_request_last_update.py (Diff revision 3)
     
     
     
     
     
     
     
     

    Missing periods at the end of some of these lines

  13. Might need to move this line up one like above

  14. reviewboard/webapi/resources/review_request_last_update.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     

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

  15. reviewboard/webapi/resources/review_request_last_update.py (Diff revision 3)
     
     
     
     
     
     
     
     
     

    Periods

  16. 
      
Mike Conley
  1. 
      
  2. 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. 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)
     
     
     
     
     
     
     
     
     
     

    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. If we're getting rid of the type argument, we should update this documentation.

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

  7. 
      
Anni Cao
Review Bot
  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. Why is this commented out? Should it be removed instead?

  3. 
      
Anni Cao
Review Bot
  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. Col: 21
     E122 continuation line missing indentation or outdented
    
  3. Col: 17
     E122 continuation line missing indentation or outdented
    
  4. Col: 80
     E501 line too long (83 > 79 characters)
    
  5. Col: 9
     E303 too many blank lines (2)
    
  6. Col: 21
     E122 continuation line missing indentation or outdented
    
  7. Col: 25
     E122 continuation line missing indentation or outdented
    
  8. 
      
Anni Cao
Anni Cao
Review Bot
  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. 
      
Anni Cao
Review Bot
  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. 
      
Sharleen Fisher
  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. 
      
Sharleen Fisher
  1. Sorry about the previous review. Here are the formatting issues I found within the first file.

  2. 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. reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     

    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.

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

    For example,
    username (String):

  5. The type should be added in brackets here as well.

  6. And here as well, I believe?

  7. 
      
Anni Cao
Anni Cao
Anni Cao
Anni Cao
Review Bot
  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.  '_' imported but unused
    
  3. Col: 5
     E303 too many blank lines (2)
    
  4. 
      
Anni Cao
Review Bot
  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. 
      
Simon Zhang
  1. 
      
  2. reviewboard/reviews/models/review_request.py (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    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)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Same with this test here.

    1. Same as above explanation

  4. 
      
Anni Cao
Review request changed

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

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

JSHint

Anni Cao
Review request changed

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

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

JSHint

Anni Cao
Anni Cao
Review request changed
Anni Cao
Anni Cao
Anni Cao
Anni Cao
Anni Cao
Anni Cao
Anni Cao
Review request changed
Anni Cao
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

Diff:

Revision 14 (+1308 -124)

Show changes

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

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

JSHint

Christian Hammond
  1. 
      
  2. reviewboard/reviews/models/review_request.py (Diff revision 14)
     
     
     

    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. Blank line before if statements.

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

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

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

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

  8. 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. Blank line between code and new blocks.

  10. reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 14)
     
     
     
     
     
     
     

    This can all be one statement.

  11. Here too. Also there's an alignment problem.

  12. Blank line here too.

  13. This template also needs indentation.

  14. 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?

  15. This can be one import.

  16. reviewboard/webapi/resources/review_request_last_update.py (Diff revision 14)
     
     
     
     
     
     
     

    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.

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

  18. reviewboard/webapi/resources/review_request_last_update.py (Diff revision 14)
     
     
     
     
     
     
     

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

  19. reviewboard/webapi/resources/review_request_last_update.py (Diff revision 14)
     
     
     
     
     
     
     
     

    This can be one statement.

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

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

    One statement.

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

  22. 
      
Anni Cao
Anni Cao
Review request changed
Loading...