Summary:

Smart Update Notification

Review Request #8738 — Created Feb. 10, 2017 and discarded Jan. 29, 2019, 1:46 p.m.

Information
Owner:	anni_cao
Repository:	Review Board
Branch:	master
Bugs:
Depends On:
Reviewers
Groups:	reviewboard, students
People:

Description

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.

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 JavaScript Unit tests have been added for:
- UpdateBubbleView: pass
- TextBubbleView: pass

Files

Issues

Description	From	Last Updated
Col: 80 E501 line too long (90 > 79 characters)	reviewbot	Feb. 17, 2017, 11:36 p.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	Feb. 17, 2017, 11:36 p.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	Feb. 17, 2017, 11:36 p.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	Feb. 17, 2017, 11:36 p.m.
Col: 80 E501 line too long (93 > 79 characters)	reviewbot	Feb. 17, 2017, 11:37 p.m.
Col: 80 E501 line too long (97 > 79 characters)	reviewbot	Feb. 17, 2017, 11:38 p.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	Feb. 17, 2017, 11:38 p.m.
Col: 9 E303 too many blank lines (2)	reviewbot	Feb. 17, 2017, 11:39 p.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	Feb. 17, 2017, 11:40 p.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	Feb. 17, 2017, 11:41 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Feb. 17, 2017, 11:41 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Feb. 17, 2017, 11:42 p.m.
Col: 80 E501 line too long (105 > 79 characters)	reviewbot	Feb. 17, 2017, 11:43 p.m.
Col: 80 E501 line too long (98 > 79 characters)	reviewbot	Feb. 17, 2017, 11:44 p.m.
Col: 80 E501 line too long (84 > 79 characters)	reviewbot	Feb. 17, 2017, 11:44 p.m.
Col: 80 E501 line too long (94 > 79 characters)	reviewbot	Feb. 17, 2017, 11:45 p.m.
Col: 5 E303 too many blank lines (3)	reviewbot	Feb. 17, 2017, 11:45 p.m.
Col: 80 E501 line too long (103 > 79 characters)	reviewbot	Feb. 17, 2017, 11:46 p.m.
Col: 5 E303 too many blank lines (2)	reviewbot	Feb. 17, 2017, 11:46 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Feb. 17, 2017, 11:46 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Feb. 17, 2017, 11:47 p.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	Feb. 17, 2017, 11:47 p.m.
Col: 5 E303 too many blank lines (2)	reviewbot	Feb. 17, 2017, 11:48 p.m.
Col: 5 E303 too many blank lines (2)	reviewbot	Feb. 17, 2017, 11:54 p.m.
Col: 80 E501 line too long (90 > 79 characters)	reviewbot	Feb. 17, 2017, 11:48 p.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	Feb. 17, 2017, 11:48 p.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	Feb. 17, 2017, 11:48 p.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	Feb. 17, 2017, 11:48 p.m.
Col: 80 E501 line too long (93 > 79 characters)	reviewbot	Feb. 17, 2017, 11:48 p.m.
Col: 80 E501 line too long (97 > 79 characters)	reviewbot	Feb. 17, 2017, 11:49 p.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	Feb. 17, 2017, 11:49 p.m.
Col: 9 E303 too many blank lines (2)	reviewbot	Feb. 17, 2017, 11:49 p.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	Feb. 17, 2017, 11:49 p.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	Feb. 17, 2017, 11:49 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Feb. 17, 2017, 11:49 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Feb. 17, 2017, 11:49 p.m.
Col: 80 E501 line too long (105 > 79 characters)	reviewbot	Feb. 17, 2017, 11:49 p.m.
Col: 80 E501 line too long (98 > 79 characters)	reviewbot	Feb. 17, 2017, 11:49 p.m.
Col: 80 E501 line too long (84 > 79 characters)	reviewbot	Feb. 17, 2017, 11:49 p.m.
Col: 80 E501 line too long (94 > 79 characters)	reviewbot	Feb. 17, 2017, 11:49 p.m.
Col: 5 E303 too many blank lines (3)	reviewbot	Feb. 17, 2017, 11:49 p.m.
Col: 80 E501 line too long (103 > 79 characters)	reviewbot	Feb. 17, 2017, 11:49 p.m.
Col: 5 E303 too many blank lines (2)	reviewbot	Feb. 17, 2017, 11:49 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Feb. 17, 2017, 11:49 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Feb. 17, 2017, 11:49 p.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	Feb. 17, 2017, 11:49 p.m.
Col: 5 E303 too many blank lines (2)	reviewbot	Feb. 17, 2017, 11:49 p.m.
Col: 5 E303 too many blank lines (2)	reviewbot	Feb. 17, 2017, 11:49 p.m.
'timedelta' imported but unused	reviewbot	Feb. 17, 2017, 11:49 p.m.
'timezone' imported but unused	reviewbot	Feb. 17, 2017, 11:50 p.m.
'DiffSet' imported but unused	reviewbot	Feb. 17, 2017, 11:50 p.m.
'Review' imported but unused	reviewbot	Feb. 17, 2017, 11:50 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Feb. 17, 2017, 11:51 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Feb. 17, 2017, 11:51 p.m.
Col: 80 E501 line too long (84 > 79 characters)	reviewbot	Feb. 17, 2017, 11:52 p.m.
Col: 5 E303 too many blank lines (2)	reviewbot	Feb. 17, 2017, 11:53 p.m.
Col: 63 W292 no newline at end of file	reviewbot	Feb. 17, 2017, 11:53 p.m.
Small nit, put a space after `<%='	KH khp	Feb. 25, 2017, 12:32 p.m.
Just eyeing this comment, I think this comment can be one line possibly?	KH khp	Feb. 25, 2017, 1:47 p.m.
I believe this line should be indented one more (see other comments below!) Also, "returned" instead of "return"	KH khp	Feb. 25, 2017, 1:34 p.m.
Could do this in one line Capitalize "the" in the beginning	KH khp	Feb. 25, 2017, 1:35 p.m.
Since this is an es6 file, this should probably be a const or use let.	mike_conley	Feb. 25, 2017, 7:49 p.m.
Why do we need to check if hasOwnProperty?	mike_conley	March 4, 2017, 11:45 a.m.
Maybe I'm just getting old, but I'm having a hard time parsing this bit. Here's what I think is happening: …	mike_conley	March 11, 2017, 6:39 p.m.
2 spaces between return and updateCountByUserName	KH khp	Feb. 25, 2017, 1:35 p.m.
IMO using distinct strings for username and fullname could catch more bugs that involve the two values.	KH khp	Feb. 25, 2017, 1:45 p.m.
If we're getting rid of the type argument, we should update this documentation.	mike_conley	Feb. 25, 2017, 11:45 a.m.
We probably don't want to have this change land in the tree.	mike_conley	Feb. 25, 2017, 11:45 a.m.
Col: 29 E126 continuation line over-indented for hanging indent	reviewbot	Feb. 18, 2017, 9:49 a.m.
Would something like this here and similar code below be preferable? new_reply = { 'id': review.pk, 'user': review.user, ...	KH khp	Feb. 25, 2017, 1:45 p.m.
Col: 41 E126 continuation line over-indented for hanging indent	reviewbot	Feb. 18, 2017, 9:49 a.m.
I think you need a new line above the for	KH khp	Feb. 25, 2017, 1:48 p.m.
Col: 37 E126 continuation line over-indented for hanging indent	reviewbot	Feb. 18, 2017, 9:49 a.m.
New line here too	KH khp	Feb. 25, 2017, 1:48 p.m.
New line above the if I think	KH khp	Feb. 25, 2017, 1:49 p.m.
Col: 37 E126 continuation line over-indented for hanging indent	reviewbot	Feb. 18, 2017, 9:50 a.m.
I'm not 100% sure but other comments seem to do this; """ and Retrieve might need to be on the …	KH khp	Feb. 25, 2017, 1:51 p.m.
Missing periods at the end of some of these lines	KH khp	Feb. 25, 2017, 1:56 p.m.
reiview -> review	KH khp	Feb. 25, 2017, 1:52 p.m.
Might need to move this line up one like above	KH khp	Feb. 25, 2017, 1:56 p.m.
Missing periods here "the updated" "the timestamp" "the new" needs to be capitalized	KH khp	Feb. 25, 2017, 1:55 p.m.
New line here	KH khp	Feb. 25, 2017, 1:55 p.m.
Periods	KH khp	Feb. 25, 2017, 1:55 p.m.
Why is this commented out? Should it be removed instead?	mike_conley	March 4, 2017, 12:09 p.m.
Col: 21 E122 continuation line missing indentation or outdented	reviewbot	March 4, 2017, 2:34 p.m.
Col: 17 E122 continuation line missing indentation or outdented	reviewbot	March 4, 2017, 2:34 p.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	March 4, 2017, 2:34 p.m.
Col: 9 E303 too many blank lines (2)	reviewbot	March 4, 2017, 2:35 p.m.
Col: 21 E122 continuation line missing indentation or outdented	reviewbot	March 4, 2017, 2:35 p.m.
Col: 25 E122 continuation line missing indentation or outdented	reviewbot	March 4, 2017, 2:35 p.m.
I think this line should be indented as per the new documentation style being phased into Review Board. More information …	sharleenfisher	March 7, 2017, 2:30 p.m.
Also indented here.	sharleenfisher	March 7, 2017, 2:30 p.m.
Do we need the white-space on line 116? It looks like, from other code that I've seen, sometimes they put …	sharleenfisher	March 7, 2017, 2:30 p.m.
I think given the new documentation format, the type that 'username' is should be in brackets? For example, username (String):	sharleenfisher	March 7, 2017, 2:33 p.m.
Here too!	sharleenfisher	March 7, 2017, 2:33 p.m.
The type should be added in brackets here as well.	sharleenfisher	March 7, 2017, 2:34 p.m.
And here as well, I believe?	sharleenfisher	March 7, 2017, 2:34 p.m.
'_' imported but unused	reviewbot	March 18, 2017, 9:43 p.m.
Col: 5 E303 too many blank lines (2)	reviewbot	March 18, 2017, 9:43 p.m.
I'm not sure these changes should be in this review request. They are already in https://reviews.reviewboard.org/r/8800/, no?	szhang	March 20, 2017, 6:42 p.m.
Same with this test here.	szhang	March 20, 2017, 6:42 p.m.
Col: 39 Missing semicolon.	reviewbot	March 26, 2017, 1:42 p.m.
Col: 28 Missing semicolon.	reviewbot	March 26, 2017, 1:34 p.m.
Col: 61 Missing semicolon.	reviewbot	March 26, 2017, 1:34 p.m.
Col: 34 Missing semicolon.	reviewbot	March 26, 2017, 1:34 p.m.
Col: 41 Missing semicolon.	reviewbot	March 26, 2017, 1:35 p.m.
Col: 25 ['username'] is better written in dot notation.	reviewbot	March 26, 2017, 1:35 p.m.
Col: 25 ['count'] is better written in dot notation.	reviewbot	March 26, 2017, 1:35 p.m.
Col: 45 Missing semicolon.	reviewbot	March 26, 2017, 1:35 p.m.
Col: 25 ['user'] is better written in dot notation.	reviewbot	March 26, 2017, 1:36 p.m.
Col: 67 Missing semicolon.	reviewbot	March 26, 2017, 1:36 p.m.
Col: 25 ['list'] is better written in dot notation.	reviewbot	March 26, 2017, 1:36 p.m.
Col: 66 Missing semicolon.	reviewbot	March 26, 2017, 1:36 p.m.
Col: 38 Missing semicolon.	reviewbot	March 26, 2017, 1:36 p.m.
Col: 49 Missing semicolon.	reviewbot	March 26, 2017, 1:36 p.m.
Col: 37 Missing semicolon.	reviewbot	March 26, 2017, 1:37 p.m.
Col: 45 ['username'] is better written in dot notation.	reviewbot	March 26, 2017, 1:37 p.m.
Col: 39 ['user'] is better written in dot notation.	reviewbot	March 26, 2017, 1:37 p.m.
Col: 47 Missing semicolon.	reviewbot	March 26, 2017, 1:37 p.m.
Col: 22 Missing semicolon.	reviewbot	March 26, 2017, 1:38 p.m.
Col: 45 ['username'] is better written in dot notation.	reviewbot	March 26, 2017, 1:38 p.m.
Col: 43 Missing semicolon.	reviewbot	March 26, 2017, 1:38 p.m.
Col: 57 Missing semicolon.	reviewbot	March 26, 2017, 1:40 p.m.
Col: 13 'info' is defined but never used.	reviewbot	March 26, 2017, 2:06 p.m.
Col: 21 Expected '}' to match '{' from line 152 and instead saw 'user'.	reviewbot	March 26, 2017, 2:06 p.m.
Col: 25 Expected ']' to match '[' from line 152 and instead saw ':'.	reviewbot	March 26, 2017, 2:06 p.m.
Col: 27 Expected '}' to match '{' from line 130 and instead saw '{'.	reviewbot	March 26, 2017, 2:06 p.m.
Col: 28 Missing semicolon.	reviewbot	March 26, 2017, 2:06 p.m.
Col: 30 Label 'url' on /users/cherry statement.	reviewbot	March 26, 2017, 2:06 p.m.
Col: 30 Expected an assignment or function call and instead saw an expression.	reviewbot	March 26, 2017, 2:07 p.m.
Col: 45 Missing semicolon.	reviewbot	March 26, 2017, 2:07 p.m.
Col: 39 Missing semicolon.	reviewbot	March 26, 2017, 1:42 p.m.
Col: 28 Missing semicolon.	reviewbot	March 26, 2017, 1:43 p.m.
Col: 61 Missing semicolon.	reviewbot	March 26, 2017, 1:43 p.m.
Col: 34 Missing semicolon.	reviewbot	March 26, 2017, 1:43 p.m.
Col: 25 ['username'] is better written in dot notation.	reviewbot	March 26, 2017, 1:43 p.m.
Col: 41 Missing semicolon.	reviewbot	March 26, 2017, 1:43 p.m.
Col: 25 ['count'] is better written in dot notation.	reviewbot	March 26, 2017, 1:43 p.m.
Col: 45 Missing semicolon.	reviewbot	March 26, 2017, 1:43 p.m.
Col: 25 ['user'] is better written in dot notation.	reviewbot	March 26, 2017, 1:43 p.m.
Col: 67 Missing semicolon.	reviewbot	March 26, 2017, 1:43 p.m.
Col: 25 ['list'] is better written in dot notation.	reviewbot	March 26, 2017, 1:43 p.m.
Col: 66 Missing semicolon.	reviewbot	March 26, 2017, 1:44 p.m.
Col: 38 Missing semicolon.	reviewbot	March 26, 2017, 1:44 p.m.
Col: 49 Missing semicolon.	reviewbot	March 26, 2017, 1:44 p.m.
Col: 37 Missing semicolon.	reviewbot	March 26, 2017, 1:44 p.m.
Col: 45 ['username'] is better written in dot notation.	reviewbot	March 26, 2017, 1:44 p.m.
Col: 39 ['user'] is better written in dot notation.	reviewbot	March 26, 2017, 1:44 p.m.
Col: 47 Missing semicolon.	reviewbot	March 26, 2017, 1:44 p.m.
Col: 22 Missing semicolon.	reviewbot	March 26, 2017, 1:44 p.m.
Col: 45 ['username'] is better written in dot notation.	reviewbot	March 26, 2017, 1:44 p.m.
Col: 43 Missing semicolon.	reviewbot	March 26, 2017, 1:44 p.m.
Col: 13 'info' is defined but never used.	reviewbot	March 26, 2017, 9:07 p.m.
Col: 21 Expected '}' to match '{' from line 152 and instead saw 'user'.	reviewbot	March 26, 2017, 9:04 p.m.
Col: 25 Expected ']' to match '[' from line 152 and instead saw ':'.	reviewbot	March 26, 2017, 9:04 p.m.
Col: 27 Expected '}' to match '{' from line 130 and instead saw '{'.	reviewbot	March 26, 2017, 9:05 p.m.
Col: 28 Missing semicolon.	reviewbot	March 26, 2017, 9:07 p.m.
Col: 30 Label 'url' on /users/cherry statement.	reviewbot	March 26, 2017, 9:05 p.m.
Col: 45 Missing semicolon.	reviewbot	March 26, 2017, 9:07 p.m.
Col: 30 Expected an assignment or function call and instead saw an expression.	reviewbot	March 26, 2017, 9:07 p.m.
Col: 13 Expected ')' and instead saw '}'.	reviewbot	March 26, 2017, 9:06 p.m.
Col: 10 Missing semicolon.	reviewbot	March 26, 2017, 9:09 p.m.
Col: 20 Missing semicolon.	reviewbot	March 26, 2017, 9:08 p.m.
Col: 23 Missing semicolon.	reviewbot	March 26, 2017, 9:08 p.m.
Col: 10 Missing semicolon.	reviewbot	April 1, 2017, 11:28 p.m.
Why the variable rename? server_timestamp makes it sound like the current time on the server.	chipx86	April 7, 2017, 6:54 p.m.
Blank line before if statements.	chipx86	April 7, 2017, 6:46 p.m.
And after. Any time there's a block level change.	chipx86	April 7, 2017, 6:47 p.m.
Can you add a comment explaining why we're doing the save_base? We'll probably forget down the road.	chipx86	April 7, 2017, 6:55 p.m.
All the HTML here should be using proper indentation to make the code more readable.	chipx86	April 7, 2017, 8:18 p.m.
Should use <%- ... %> for any user-controllable content (username, full name, etc.). This will escape the content so users …	chipx86	April 7, 2017, 8:18 p.m.
This function's iterating through everything but timestamps. I think we really just have a few types of entries in here …	chipx86	April 7, 2017, 6:36 p.m.
Blank line between code and new blocks.	chipx86	April 7, 2017, 6:57 p.m.
Here too.	chipx86	April 7, 2017, 6:58 p.m.
Here too.	chipx86	April 7, 2017, 6:58 p.m.
This can all be one statement.	chipx86	April 7, 2017, 6:36 p.m.
Here too. Also there's an alignment problem.	chipx86	April 7, 2017, 6:59 p.m.
Blank line here too.	chipx86	April 7, 2017, 6:59 p.m.
Blank line.	chipx86	April 7, 2017, 6:59 p.m.
Blank line.	chipx86	April 7, 2017, 6:59 p.m.
This template also needs indentation.	chipx86	April 7, 2017, 8:18 p.m.
Best to hide before adding the HTML, to reduce what needs to be done in the DOM.	chipx86	April 7, 2017, 6:36 p.m.
Col: 10 Missing semicolon.	reviewbot	April 1, 2017, 11:28 p.m.
This can be one import.	chipx86	April 7, 2017, 7:02 p.m.
We have to maintain full backwards-compatibility. All the old fields must remain using the same behavior we used to have. …	chipx86	April 7, 2017, 6:36 p.m.
"server_timestamp" is the wrong name here. That sounds like the current time on the server.	chipx86	April 7, 2017, 7:03 p.m.
Docstrings don't belong in the middle of code blocks. You need to use comments.	chipx86	April 7, 2017, 7:08 p.m.
Blank line here.	chipx86	April 7, 2017, 7:08 p.m.
Blank line.	chipx86	April 7, 2017, 7:08 p.m.
This can be one statement.	chipx86	April 7, 2017, 6:36 p.m.
One statement.	chipx86	April 7, 2017, 6:36 p.m.
id is a reserved word in Python. This should be pk.	chipx86	April 7, 2017, 8:49 p.m.
Blank line.	chipx86	April 7, 2017, 7:10 p.m.

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

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (90 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (86 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (86 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (93 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (97 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 1)
Show all issues
```
Col: 9
 E303 too many blank lines (2)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (86 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (86 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (105 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (98 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (84 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (94 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 1)
Show all issues
```
Col: 5
 E303 too many blank lines (3)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (103 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 1)
Show all issues
```
Col: 5
 E303 too many blank lines (2)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 1)
Show all issues
```
Col: 5
 E303 too many blank lines (2)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 1)
Show all issues
```
Col: 5
 E303 too many blank lines (2)
```

Commit:

-	f907035ea6901437eada2f44f24eb846707e28ef
+	96e714152e15283e9fb14eac16de7d01598fcb44

Diff:

Revision 2 (+635 -88)

Show changes

	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/review_request_last_update.py
	reviewboard/webapi/tests/mimetypes.py
	reviewboard/webapi/tests/test_review_request_last_update.py
	reviewboard/webapi/tests/urls.py

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

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (90 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (86 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (86 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (93 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (97 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 9
 E303 too many blank lines (2)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (86 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (86 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (105 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (98 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (84 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (94 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 5
 E303 too many blank lines (3)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (103 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 5
 E303 too many blank lines (2)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 5
 E303 too many blank lines (2)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 5
 E303 too many blank lines (2)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 2)
Show all issues
```
 'timedelta' imported but unused
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 2)
Show all issues
```
 'timezone' imported but unused
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 2)
Show all issues
```
 'DiffSet' imported but unused
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 2)
Show all issues
```
 'Review' imported but unused
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (84 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 5
 E303 too many blank lines (2)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 2)
Show all issues
```
Col: 63
 W292 no newline at end of file
```

Testing Done:

		Manual tests in the browsers have been done in following cases:
		- 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
	+
	+	Python testing cases for review_request_last_update API include as following:
	+	- New diffsets: pass
	+	- New reviews: pass
	+	- New replies: pass
	+	- New comments: pass
	+	- default case for no/improperly-formatted timestamp: fail
	+
	+	Javascript testing for updated UpdateBubbleView has been passed.

Commit:

-	96e714152e15283e9fb14eac16de7d01598fcb44
+	38c6cb98aab72406b8acfa27679b85f75bb88d1b

Diff:

Revision 3 (+640 -88)

Show changes

	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/review_request_last_update.py
	reviewboard/webapi/tests/mimetypes.py
	reviewboard/webapi/tests/test_review_request_last_update.py
	reviewboard/webapi/tests/urls.py

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

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 3)
Show all issues
```
Col: 29
 E126 continuation line over-indented for hanging indent
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 3)
Show all issues
```
Col: 41
 E126 continuation line over-indented for hanging indent
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 3)
Show all issues
```
Col: 37
 E126 continuation line over-indented for hanging indent
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 3)
Show all issues
```
Col: 37
 E126 continuation line over-indented for hanging indent
```

Wow that's a lot of code! Mostly formatting nits and comments.

reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 3)
Show all issues
```
Small nit, put a space after `<%='
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 3)
Show all issues
```
Just eyeing this comment, I think this comment can be one line possibly?
```

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

Show all issues

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

Also, "returned" instead of "return"

reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 3)
Show all issues
```
Could do this in one line

Capitalize "the" in the beginning
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 3)
Show all issues
```
2 spaces between return and updateCountByUserName
```

reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 3)

Show all issues

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

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

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 3)
Show all issues
```
I think you need a new line above the for
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 3)
Show all issues
```
New line here too
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 3)
Show all issues
```
New line above the if I think
```

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

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

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

Show all issues

Missing periods at the end of some of these lines

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 3)
Show all issues
```
reiview -> review
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 3)
Show all issues
```
Might need to move this line up one like above
```

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

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 3)
Show all issues
```
New line here
```

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

Show all issues

Periods

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

Show all issues

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

anni_cao Feb. 25, 2017, 12:12 p.m.

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

mike_conley March 4, 2017, 10:09 a.m.

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.

AN

anni_cao March 4, 2017, 11:28 a.m.
```
Got it.
```

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

Show all issues

Why do we need to check if hasOwnProperty?

anni_cao Feb. 25, 2017, 12:08 p.m.

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

mike_conley March 4, 2017, 10:09 a.m.

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

anni_cao March 4, 2017, 11:45 a.m.

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.

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?

anni_cao Feb. 25, 2017, 12:07 p.m.


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.

mike_conley March 4, 2017, 10:09 a.m.




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.

anni_cao March 4, 2017, 12:07 p.m.

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': []
    }

anni_cao March 4, 2017, 12:07 p.m.

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

reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js (Diff revision 3)
Show all issues
```
If we're getting rid of the type argument, we should update this documentation.
```
reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js (Diff revision 3)
Show all issues
```
We probably don't want to have this change land in the tree.
```

Commit:

-	38c6cb98aab72406b8acfa27679b85f75bb88d1b
+	6e8a75fb993863b4d4daa013b734f576278e6edd

Diff:

Revision 4 (+636 -89)

Show changes

	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/review_request_last_update.py
	reviewboard/webapi/tests/mimetypes.py
	reviewboard/webapi/tests/test_review_request_last_update.py
	reviewboard/webapi/tests/urls.py

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

reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 4)
Show all issues
```
Why is this commented out? Should it be removed instead?
```

Commit:

-	6e8a75fb993863b4d4daa013b734f576278e6edd
+	1d19e02ecf3f473e4788f9b805985679eab8806d

Diff:

Revision 5 (+767 -93)

Show changes

	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/review_request_last_update.py
	reviewboard/webapi/tests/mimetypes.py
	reviewboard/webapi/tests/test_review_request_last_update.py
	reviewboard/webapi/tests/urls.py

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

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 5)
Show all issues
```
Col: 21
 E122 continuation line missing indentation or outdented
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 5)
Show all issues
```
Col: 17
 E122 continuation line missing indentation or outdented
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 5)
Show all issues
```
Col: 80
 E501 line too long (83 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 5)
Show all issues
```
Col: 9
 E303 too many blank lines (2)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 5)
Show all issues
```
Col: 21
 E122 continuation line missing indentation or outdented
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 5)
Show all issues
```
Col: 25
 E122 continuation line missing indentation or outdented
```

Testing Done:

		Manual tests in the browsers have been done in following cases:
		- 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

		Python testing cases for review_request_last_update API include as following:
		- New diffsets: pass
		- New reviews: pass
		- New replies: pass
		- New comments: pass
~		- default case for no/improperly-formatted timestamp: fail
	~	- default case for no/improperly-formatted timestamp: pass
	+	- Test case for no updates: pass

		Javascript testing for updated UpdateBubbleView has been passed.

Removed Files:

Added Files:

Commit:

-	1d19e02ecf3f473e4788f9b805985679eab8806d
+	397d9a2434338e8391dbb928c3d1fdff92ca2894

Diff:

Revision 6 (+785 -93)

Show changes

	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/review_request_last_update.py
	reviewboard/webapi/tests/mimetypes.py
	reviewboard/webapi/tests/test_review_request_last_update.py
	reviewboard/webapi/tests/urls.py

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

Commit:

-	397d9a2434338e8391dbb928c3d1fdff92ca2894
+	a712413a59bf49799593c435ec9baac4ded67d9c

Diff:

Revision 7 (+775 -93)

Show changes

	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/review_request_last_update.py
	reviewboard/webapi/tests/mimetypes.py
	reviewboard/webapi/tests/test_review_request_last_update.py
	reviewboard/webapi/tests/urls.py

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

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.

anni_cao March 7, 2017, 2:28 p.m.

thank you very much for your following reviews :-)

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

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

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

reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 7)
Show all issues
```
Also indented here.
```

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.

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

Show all issues

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

For example,
username (String):

reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 7)
Show all issues
```
Here too!
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 7)
Show all issues
```
The type should be added in brackets here as well.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 7)
Show all issues
```
And here as well, I believe?
```

Removed Files:

Testing Done:

		Manual tests in the browsers have been done in following cases:
		- 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

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

		Javascript testing for updated UpdateBubbleView has been passed.

Added Files:

Commit:

-	a712413a59bf49799593c435ec9baac4ded67d9c
+	5828c98fa8c51f1fb8153c4f933f258389fbb9ab

Diff:

Revision 8 (+660 -102)

Show changes

	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/review_request_last_update.py
	reviewboard/webapi/tests/mimetypes.py
	2 more

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

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 8)
Show all issues
```
 '_' imported but unused
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 8)
Show all issues
```
Col: 5
 E303 too many blank lines (2)
```

Summary:

-	Smart Update Notification
+	Smart Update Notification [WIP]

Commit:

-	5828c98fa8c51f1fb8153c4f933f258389fbb9ab
+	d601df2a1443284e666e06283cb5d8409e7478d2

Diff:

Revision 9 (+981 -103)

Show changes

	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/static/rb/css/pages/review-request.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
	reviewboard/testing/testcase.py
	4 more

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

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?

anni_cao March 20, 2017, 6:42 p.m.

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

reviewboard/reviews/tests/test_review_request.py (Diff revision 9)

Show all issues

Same with this test here.

AN

anni_cao March 20, 2017, 6:42 p.m.
```
Same as above explanation
```

Commit:

-	d601df2a1443284e666e06283cb5d8409e7478d2
+	a734c522ed39b7e8d46f3e8f5542a453011e1008

Diff:

Revision 10 (+1222 -111)

Show changes

	reviewboard/reviews/models/review.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/static/rb/css/pages/review-request.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	6 more

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

JSHint failed.

PEP8 Style Checker internal error.

Pyflakes passed.

JSHint

```
Warning: Showing 30 of 44 failures.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 10)
Show all issues
```
Col: 39
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 10)
Show all issues
```
Col: 28
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 10)
Show all issues
```
Col: 61
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 10)
Show all issues
```
Col: 34
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 10)
Show all issues
```
Col: 41
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 10)
Show all issues
```
Col: 25
 ['username'] is better written in dot notation.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 10)
Show all issues
```
Col: 25
 ['count'] is better written in dot notation.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 10)
Show all issues
```
Col: 45
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 10)
Show all issues
```
Col: 25
 ['user'] is better written in dot notation.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 10)
Show all issues
```
Col: 67
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 10)
Show all issues
```
Col: 25
 ['list'] is better written in dot notation.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 10)
Show all issues
```
Col: 66
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 10)
Show all issues
```
Col: 38
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 10)
Show all issues
```
Col: 49
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 10)
Show all issues
```
Col: 37
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 10)
Show all issues
```
Col: 45
 ['username'] is better written in dot notation.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 10)
Show all issues
```
Col: 39
 ['user'] is better written in dot notation.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 10)
Show all issues
```
Col: 47
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 10)
Show all issues
```
Col: 22
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 10)
Show all issues
```
Col: 45
 ['username'] is better written in dot notation.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 10)
Show all issues
```
Col: 43
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 10)
Show all issues
```
Col: 57
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 10)
Show all issues
```
Col: 13
 'info' is defined but never used.
```
reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 10)
Show all issues
```
Col: 21
 Expected '}' to match '{' from line 152 and instead saw 'user'.
```
reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 10)
Show all issues
```
Col: 25
 Expected ']' to match '[' from line 152 and instead saw ':'.
```
reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 10)
Show all issues
```
Col: 27
 Expected '}' to match '{' from line 130 and instead saw '{'.
```
reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 10)
Show all issues
```
Col: 28
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 10)
Show all issues
```
Col: 30
 Label 'url' on /users/cherry statement.
```
reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 10)
Show all issues
```
Col: 30
 Expected an assignment or function call and instead saw an expression.
```
reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 10)
Show all issues
```
Col: 45
 Missing semicolon.
```

Commit:

-	a734c522ed39b7e8d46f3e8f5542a453011e1008
+	bb9f8f5b24108ed5a11c44782ac1e6169279efab

Diff:

Revision 11 (+1213 -111)

Show changes

	reviewboard/reviews/models/review.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/static/rb/css/pages/review-request.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	6 more

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

JSHint failed.

PEP8 Style Checker internal error.

Pyflakes passed.

JSHint

```
Warning: Showing 30 of 43 failures.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 11)
Show all issues
```
Col: 39
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 11)
Show all issues
```
Col: 28
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 11)
Show all issues
```
Col: 61
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 11)
Show all issues
```
Col: 34
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 11)
Show all issues
```
Col: 25
 ['username'] is better written in dot notation.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 11)
Show all issues
```
Col: 41
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 11)
Show all issues
```
Col: 25
 ['count'] is better written in dot notation.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 11)
Show all issues
```
Col: 45
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 11)
Show all issues
```
Col: 25
 ['user'] is better written in dot notation.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 11)
Show all issues
```
Col: 67
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 11)
Show all issues
```
Col: 25
 ['list'] is better written in dot notation.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 11)
Show all issues
```
Col: 66
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 11)
Show all issues
```
Col: 38
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 11)
Show all issues
```
Col: 49
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 11)
Show all issues
```
Col: 37
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 11)
Show all issues
```
Col: 45
 ['username'] is better written in dot notation.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 11)
Show all issues
```
Col: 39
 ['user'] is better written in dot notation.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 11)
Show all issues
```
Col: 47
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 11)
Show all issues
```
Col: 22
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 11)
Show all issues
```
Col: 45
 ['username'] is better written in dot notation.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 11)
Show all issues
```
Col: 43
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 11)
Show all issues
```
Col: 13
 'info' is defined but never used.
```
reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 11)
Show all issues
```
Col: 21
 Expected '}' to match '{' from line 152 and instead saw 'user'.
```
reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 11)
Show all issues
```
Col: 25
 Expected ']' to match '[' from line 152 and instead saw ':'.
```
reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 11)
Show all issues
```
Col: 27
 Expected '}' to match '{' from line 130 and instead saw '{'.
```
reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 11)
Show all issues
```
Col: 28
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 11)
Show all issues
```
Col: 30
 Label 'url' on /users/cherry statement.
```
reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 11)
Show all issues
```
Col: 45
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 11)
Show all issues
```
Col: 30
 Expected an assignment or function call and instead saw an expression.
```
reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 11)
Show all issues
```
Col: 13
 Expected ')' and instead saw '}'.
```

Added Files:

Commit:

-	bb9f8f5b24108ed5a11c44782ac1e6169279efab
+	1cc94b5cab9fdf6d04c852f4fb1bf942b205d907

Diff:

Revision 12 (+1248 -113)

Show changes

	reviewboard/reviews/models/review.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/static/rb/css/pages/review-request.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	6 more

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

JSHint failed.

PEP8 Style Checker internal error.

Pyflakes passed.

JSHint

reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 12)
Show all issues
```
Col: 10
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 12)
Show all issues
```
Col: 20
 Missing semicolon.
```
reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 12)
Show all issues
```
Col: 23
 Missing semicolon.
```

Summary:

-	Smart Update Notification [WIP]
+	Smart Update Notification

Testing Done:

~		Manual tests in the browsers have been done in following cases:
	~	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
	~	- 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.

		Python testing cases for review_request_last_update API include as following:
~		- New reviews: pass
	~	- 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

~		Javascript testing for updated UpdateBubbleView has been passed.
	~	Jasmine testing for the UpdateBubbleView has been passed.

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

Removed Files:
Added Files:

Added Files:

Added Files:

Commit:

-	1cc94b5cab9fdf6d04c852f4fb1bf942b205d907
+	83ca097857c903acb0401dddacd8fe37ff5d6b4e

Diff:

Revision 13 (+1248 -113)

Show changes

	reviewboard/reviews/models/review.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/static/rb/css/pages/review-request.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	6 more

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

JSHint failed.

PEP8 Style Checker internal error.

Pyflakes passed.

JSHint

reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 13)
Show all issues
```
Col: 10
 Missing semicolon.
```

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

	reviewboard/reviews/models/review.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/static/rb/css/pages/review-request.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	6 more

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

JSHint failed.

PEP8 Style Checker internal error.

Pyflakes passed.

JSHint

reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js (Diff revision 14)
Show all issues
```
Col: 10
 Missing semicolon.
```

Fix it!

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.

anni_cao April 7, 2017, 6:45 p.m.

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.

chipx86 April 7, 2017, 6:46 p.m.
```
last_updated_timestamp would be fine.
```

anni_cao April 7, 2017, 6:52 p.m.

OK, will change the variable name to last_updated_timestamp

reviewboard/reviews/models/review_request_draft.py (Diff revision 14)
Show all issues
```
Blank line before if statements.
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 14)
Show all issues
```
And after. Any time there's a block level change.
```

reviewboard/reviews/models/review_request_draft.py (Diff revision 14)

Show all issues

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

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

Show all issues

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

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

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.

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

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?

anni_cao April 7, 2017, 8:25 p.m.

Can info.hasOwnProperty(key) handle that?

reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 14)
Show all issues
```
Blank line between code and new blocks.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 14)
Show all issues
```
Here too.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 14)
Show all issues
```
Here too.
```

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

Show all issues

This can all be one statement.

reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 14)
Show all issues
```
Here too. Also there's an alignment problem.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 14)
Show all issues
```
Blank line here too.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 14)
Show all issues
```
Blank line.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 14)
Show all issues
```
Blank line.
```

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

Show all issues

This template also needs indentation.

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

Show all issues

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

anni_cao April 7, 2017, 8:43 p.m.

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

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 14)
Show all issues
```
This can be one import.
```

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.

anni_cao April 7, 2017, 8:44 p.m.

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

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

Show all issues

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

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.

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 14)
Show all issues
```
Blank line here.
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 14)
Show all issues
```
Blank line.
```

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

Show all issues

This can be one statement.

anni_cao April 7, 2017, 8:22 p.m.

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

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

Show all issues

One statement.

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 14)
Show all issues
```
id is a reserved word in Python. This should be pk.
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 14)
Show all issues
```
Blank line.
```

Commit:

-	9cbbbc19d5d258e1265f59a42a9316097531bfab
+	d22aabee1fec9611714c8245e1ab42edb9b9d3dd

Diff:

Revision 15 (+1329 -124)

Show changes

	reviewboard/reviews/models/review.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/static/rb/css/pages/review-request.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	6 more

Checks run (2 succeeded, 1 failed with error)

JSHint passed.

PEP8 Style Checker internal error.

Pyflakes passed.

Commit:

-	d22aabee1fec9611714c8245e1ab42edb9b9d3dd
+	50678fbd044e5dc67ddc7ff34a1f882bc8016ff1

Diff:

Revision 16 (+1350 -126)

Show changes

	reviewboard/reviews/models/review.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/tests/test_review_request.py
	reviewboard/static/rb/css/pages/review-request.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	6 more

Checks run (2 succeeded, 1 failed with error)

JSHint passed.

PEP8 Style Checker internal error.

Pyflakes passed.

Smart Update Notification

Commit:

Diff:

Testing Done:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Testing Done:

Removed Files:

Added Files:

Commit:

Diff:

Commit:

Diff:

Removed Files:

Testing Done:

Added Files:

Commit:

Diff:

Summary:

Commit:

Diff:

Commit:

Diff:

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

JSHint

Commit:

Diff:

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

JSHint

Added Files:

Commit:

Diff:

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

JSHint

Summary:

Testing Done:

Testing Done:

Removed Files:

Added Files:

Added Files:

Added Files:

Commit:

Diff:

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

JSHint

Testing Done:

Commit:

Diff:

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

JSHint

Commit:

Diff:

Checks run (2 succeeded, 1 failed with error)

Commit:

Diff:

Checks run (2 succeeded, 1 failed with error)

Status: Discarded