Summary

Adding smarter update notifications

Review Request #8519 — Created Nov. 3, 2016 and discarded Dec. 28, 2017, 2:38 p.m.

Information

Owner

rswanson

Repository

Review Board

Branch

release-3.0.x

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 updates regarding the type of change
and who it was made by. The goal of this project is to improve
on the detail provided.

Testing Done

Added unit tests for backend code - all pass.
Manually verified API response locally.

Issues

Description	From	Last Updated
Col: 80 E501 line too long (84 > 79 characters)	reviewbot	Nov. 17, 2016, 12:58 p.m.
undefined name 'DiffSet'	reviewbot	Nov. 18, 2016, 10:10 a.m.
Col: 80 E501 line too long (97 > 79 characters)	reviewbot	Nov. 18, 2016, 10:12 a.m.
The way you are currently doing this is going to build 7 lists. Each call to list() will create one …	brennie	Nov. 18, 2016, 10:21 a.m.
Col: 80 E501 line too long (97 > 79 characters)	reviewbot	Nov. 18, 2016, 10:12 a.m.
This can throw if someone does ?timestamp=0 or some other invalid format.	brennie	Nov. 19, 2016, 10:13 a.m.
utc_timestamp will always be truthy. If we fail to parse the timestamp, we will get an exception.	brennie	Nov. 19, 2016, 10:14 a.m.
So there are 4 comment types (comments, general, file attachment, and screenshot). That means for every review we will be …	brennie	Nov. 19, 2016, 12:18 p.m.
We will probably want to get reviews and replies in two separate lists (may want to confer with christian on …	brennie	Nov. 19, 2016, 12:18 p.m.
This isn't being used anymore.	brennie	Nov. 24, 2016, 9:28 a.m.
Timestamps on a review are all set to the same value when published. This whole function wouldn't be doing anything …	chipx86	Nov. 24, 2016, 9:56 a.m.
Make sure to follow the conventions for docstrings, specifically "Args" and "Returns". https://www.reviewboard.org/docs/codebase/dev/docs/writing-codebase-docs/	chipx86	Nov. 24, 2016, 9:58 a.m.
I don't think we should be adding new methods that just produce different variations of a query here. This query …	chipx86	Nov. 24, 2016, 12:49 p.m.
We should always say "review request" instead of "request" (which tends to be the term for HTTP Request).	chipx86	Nov. 24, 2016, 10:09 a.m.
This should not return DiffSets associated with review request drafts, i.e. review_request_draft=None.	brennie	Nov. 24, 2016, 9:29 a.m.
Same here as above.	chipx86	Nov. 24, 2016, 12:49 p.m.
This should make sure that the reviews aren't drafts, i.e., public=True.	brennie	Nov. 24, 2016, 9:30 a.m.
timestamp doesn't need to be in quotes. Also can you change this to: data: { timestamp: this._lastUpdateTimestamp, },	brennie	Nov. 24, 2016, 9:31 a.m.
Make sure this doesn't stay :)	brennie	Nov. 24, 2016, 9:31 a.m.
Instead of importing pytz, you should do from django.utils.timezone import utc.	brennie	Nov. 24, 2016, 9:33 a.m.
Single import statement, one name per line.	brennie	Nov. 24, 2016, 9:35 a.m.
This should be the WebAPI resource and not the model, i.e. [reviewboard.webapi.resources.diff.DiffResource]	brennie	Nov. 24, 2016, 9:36 a.m.
This shouldn't be a string. It should be a reference to the resource model. Same below.	chipx86	Nov. 24, 2016, 1 p.m.
This should be the WebAPI resource and not the model, i.e. [reviewboard.webapi.resources.review.ReviewResource]	brennie	Nov. 24, 2016, 9:36 a.m.
This should be the WebAPI resource and not the model, i.e. [reviewboard.webapi.resources.base_comment.BaseCommentResource] (I think?) You will want to build the …	brennie	Nov. 19, 2016, 3:08 p.m.
"out-of-date"	chipx86	Nov. 24, 2016, 10:10 a.m.
No blank line here.	chipx86	Nov. 24, 2016, 10:10 a.m.
This comment isn't really clear. I think this is meant to be an example of the format, but if so, …	chipx86	Nov. 24, 2016, 10:13 a.m.
Since you never use timestamp again, just reassign to timestamp.	brennie	Nov. 24, 2016, 9:43 a.m.
After you remove the pytz import you can convert to a aware time with (timezone.get_default_timezone() .localize(datetime .strptime(timestamp, '%Y-%m-%dT%H:%M:%SZ')))	CO Connor-Y	Nov. 24, 2016, 9:55 a.m.
Comments should be proper sentences, periods and all.	chipx86	Nov. 24, 2016, 10:13 a.m.
Just call this reviews.	brennie	Nov. 24, 2016, 9:44 a.m.
Blank line between code and new comments.	chipx86	Nov. 24, 2016, 10:14 a.m.
This isn't too gross :). However, instead of iterating over this twice, we can iterate once: new_reviews = [] new_replies …	brennie	Nov. 24, 2016, 9:48 a.m.
Col: 80 E501 line too long (100 > 79 characters)	reviewbot	Nov. 24, 2016, 1:05 p.m.
Col: 80 E501 line too long (101 > 79 characters)	reviewbot	Nov. 24, 2016, 1:01 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Nov. 24, 2016, 1:01 p.m.
This isn't too helpful to the caller. There should be suitable error information. However, we're also not actually working with …	chipx86	Nov. 24, 2016, 3:47 p.m.
Docstring etc. I would rename new_reviews to just reviews.	brennie	Nov. 24, 2016, 1:05 p.m.
Can you make this a constant on the class?	brennie	Nov. 24, 2016, 9:51 a.m.
This needs to be review_id__in or review__pk__in. Also, one per line.	brennie	Nov. 24, 2016, 9:53 a.m.
from django.utils import six	brennie	Nov. 24, 2016, 9:54 a.m.
This file is missing a: from __future__ import unicode_literals Which must be at the top.	chipx86	Nov. 24, 2016, 10:15 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Nov. 24, 2016, 1:02 p.m.
Missing a docstring.	chipx86	Nov. 24, 2016, 10:24 a.m.
Blank line between these.	chipx86	Nov. 24, 2016, 10:16 a.m.
Blank line between these.	chipx86	Nov. 24, 2016, 10:16 a.m.
'INVALID_FORM_DATA' imported but unused	reviewbot	Nov. 24, 2016, 3:37 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Nov. 24, 2016, 3:38 p.m.
Col: 9 E303 too many blank lines (2)	reviewbot	Nov. 24, 2016, 3:38 p.m.
Col: 13 E116 unexpected indentation (comment)	reviewbot	Nov. 24, 2016, 3:38 p.m.
Col: 13 E265 block comment should start with '# '	reviewbot	Nov. 24, 2016, 3:38 p.m.
Col: 13 E116 unexpected indentation (comment)	reviewbot	Nov. 24, 2016, 3:38 p.m.
Col: 13 E116 unexpected indentation (comment)	reviewbot	Nov. 24, 2016, 3:38 p.m.
Col: 63 E127 continuation line over-indented for visual indent	reviewbot	Nov. 24, 2016, 3:39 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Nov. 24, 2016, 3:39 p.m.
undefined name 'DOES_NOT_EXIST'	reviewbot	Nov. 25, 2016, 3:07 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Nov. 25, 2016, 3:07 p.m.
These can probably all be condensed into one test.	brennie	Nov. 28, 2016, 6:33 p.m.
undefined name 'DOES_NOT_EXIST'	reviewbot	Nov. 25, 2016, 3:04 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Nov. 25, 2016, 3:05 p.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	Nov. 25, 2016, 3:05 p.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	Nov. 25, 2016, 3:06 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Nov. 25, 2016, 3:06 p.m.
undefined name 'review_request'	reviewbot	Nov. 25, 2016, 3:07 p.m.
undefined name 'DOES_NOT_EXIST'	reviewbot	Nov. 28, 2016, 10:18 a.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Nov. 28, 2016, 10:18 a.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	Nov. 28, 2016, 10:18 a.m.
undefined name 'review'	reviewbot	Nov. 28, 2016, 10:17 a.m.
undefined name 'review'	reviewbot	Nov. 28, 2016, 10:17 a.m.
Col: 47 E128 continuation line under-indented for visual indent	reviewbot	Nov. 28, 2016, 10:17 a.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	Nov. 28, 2016, 10:17 a.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	Nov. 28, 2016, 10:17 a.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Nov. 28, 2016, 10:17 a.m.
undefined name 'DOES_NOT_EXIST'	reviewbot	Nov. 28, 2016, 10:18 a.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Nov. 28, 2016, 10:18 a.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	Nov. 28, 2016, 10:18 a.m.
Col: 47 E128 continuation line under-indented for visual indent	reviewbot	Nov. 28, 2016, 10:18 a.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	Nov. 28, 2016, 10:18 a.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	Nov. 28, 2016, 10:18 a.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Nov. 28, 2016, 10:18 a.m.
Col: 80 E501 line too long (88 > 79 characters)	reviewbot	Nov. 28, 2016, 10:17 a.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Nov. 28, 2016, 10:17 a.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	Nov. 28, 2016, 10:17 a.m.
Col: 47 E128 continuation line under-indented for visual indent	reviewbot	Nov. 28, 2016, 10:17 a.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	Nov. 28, 2016, 10:17 a.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	Nov. 28, 2016, 10:17 a.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Nov. 28, 2016, 10:18 a.m.
Col: 80 E501 line too long (88 > 79 characters)	reviewbot	Nov. 28, 2016, 10:14 a.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Nov. 28, 2016, 10:16 a.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	Nov. 28, 2016, 10:16 a.m.
Col: 47 E128 continuation line under-indented for visual indent	reviewbot	Nov. 28, 2016, 10:12 a.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	Nov. 28, 2016, 10:13 a.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	Nov. 28, 2016, 10:14 a.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Nov. 30, 2016, 3:04 p.m.
'datetime' imported but unused	reviewbot	Nov. 30, 2016, 3:03 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Nov. 30, 2016, 3:03 p.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	Nov. 30, 2016, 3:03 p.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	Nov. 30, 2016, 3:03 p.m.
Col: 37 E128 continuation line under-indented for visual indent	reviewbot	Nov. 30, 2016, 3:03 p.m.
Col: 42 E128 continuation line under-indented for visual indent	reviewbot	Nov. 30, 2016, 3:03 p.m.
Col: 27 E201 whitespace after '{'	reviewbot	Nov. 30, 2016, 3:03 p.m.
Col: 32 E127 continuation line over-indented for visual indent	reviewbot	Nov. 30, 2016, 3:03 p.m.
Col: 33 E201 whitespace after '{'	reviewbot	Nov. 30, 2016, 3:03 p.m.
Col: 27 E201 whitespace after '{'	reviewbot	Nov. 30, 2016, 3:03 p.m.
Col: 32 E127 continuation line over-indented for visual indent	reviewbot	Nov. 30, 2016, 3:03 p.m.
Col: 33 E201 whitespace after '{'	reviewbot	Nov. 30, 2016, 3:03 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Nov. 30, 2016, 3:03 p.m.
'datetime' imported but unused	reviewbot	Nov. 29, 2016, 5:48 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Nov. 29, 2016, 5:49 p.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	Nov. 29, 2016, 5:49 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Nov. 29, 2016, 5:49 p.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	Nov. 29, 2016, 5:55 p.m.
Col: 26 E126 continuation line over-indented for hanging indent	reviewbot	Nov. 29, 2016, 5:55 p.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	Nov. 29, 2016, 5:58 p.m.
Col: 37 E128 continuation line under-indented for visual indent	reviewbot	Nov. 29, 2016, 6:01 p.m.
Col: 42 E128 continuation line under-indented for visual indent	reviewbot	Nov. 29, 2016, 6:01 p.m.
Col: 27 E201 whitespace after '{'	reviewbot	Nov. 29, 2016, 6:01 p.m.
Col: 32 E127 continuation line over-indented for visual indent	reviewbot	Nov. 29, 2016, 6:01 p.m.
Col: 33 E201 whitespace after '{'	reviewbot	Nov. 29, 2016, 6:01 p.m.
Col: 27 E201 whitespace after '{'	reviewbot	Nov. 29, 2016, 6:01 p.m.
Col: 32 E127 continuation line over-indented for visual indent	reviewbot	Nov. 29, 2016, 6:01 p.m.
Col: 33 E201 whitespace after '{'	reviewbot	Nov. 29, 2016, 6:01 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Nov. 29, 2016, 6:02 p.m.
Undo this.	brennie	Dec. 2, 2016, 2:11 p.m.
This won't (or shouldn't, anyway) result with the timestamp saved to the review request in the DB. Instead of setting …	brennie	Dec. 4, 2016, 2:29 p.m.
This can eventually lead to fun to debug circular reference issues. Instead, when you want to refer to ReviewResource you …	brennie	Dec. 2, 2016, 3:34 p.m.
This should actually be: ['reviewboard.webapi.resources.diff.DiffResource']	brennie	Dec. 2, 2016, 2:13 p.m.
Same goes for here with the appropriate resource	brennie	Dec. 2, 2016, 2:18 p.m.
Same goes for here with the appropriate resource	brennie	Dec. 2, 2016, 2:18 p.m.
Same goes for here with the appropriate resource	brennie	Dec. 2, 2016, 2:18 p.m.
Same goes for here with the appropriate resource	brennie	Dec. 2, 2016, 2:18 p.m.
Constant names should be ALL_CAPS. Also, can you format this as: COMMENT_TYPES = ( Comment, # ... )	brennie	Dec. 2, 2016, 2:25 p.m.
Don't use exceptions like this for flow control. Instead: if not timestamp: # old behaviour return ... try: timestamp = …	brennie	Dec. 4, 2016, 2:44 p.m.
This documentation needs to go into the docs for the timestamp field.	brennie	Dec. 2, 2016, 2:33 p.m.
No blank line here.	brennie	Dec. 2, 2016, 2:34 p.m.
When the timestamp is provided and invalid, we want to give an INVALID_FORM_DATA response.	brennie	Dec. 2, 2016, 7:23 p.m.
missing docstring.	brennie	Dec. 2, 2016, 3:17 p.m.
Needs file docstring.	brennie	Dec. 2, 2016, 3:21 p.m.
Blank line between these.	brennie	Dec. 2, 2016, 3:23 p.m.
Blank line between these.	brennie	Dec. 2, 2016, 3:23 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Dec. 2, 2016, 10:07 a.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	Dec. 2, 2016, 10:07 a.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	Dec. 2, 2016, 10:07 a.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	Dec. 4, 2016, 2:31 p.m.
Col: 26 E126 continuation line over-indented for hanging indent	reviewbot	Dec. 4, 2016, 2:30 p.m.
Col: 26 E126 continuation line over-indented for hanging indent	reviewbot	Dec. 4, 2016, 2:30 p.m.
Docstring.	brennie	Dec. 2, 2016, 3:33 p.m.
Let's leave the comment, please.	david	Dec. 10, 2016, 5:26 p.m.
Even though the line would go over 80 columns, let's keep this in one piece.	david	Dec. 10, 2016, 5:27 p.m.
Col: 21 E128 continuation line under-indented for visual indent	reviewbot	Dec. 4, 2016, 2:47 p.m.
This is only used once in an internal method, so let's just have it inline there.	david	Dec. 10, 2016, 5:28 p.m.
Col: 5 E124 closing bracket does not match visual indentation	reviewbot	Dec. 4, 2016, 2:47 p.m.
We only use this in the one place, so let's avoid assigning the extra variable (or constructing the list): for …	david	Dec. 10, 2016, 5:30 p.m.
We shouldn't need to convert to list here.	david	Dec. 10, 2016, 5:31 p.m.
Keep this on one line, even though it's long. Should also be list of reviewboard.webapi.resource.review.ReviewResource	david	Dec. 10, 2016, 5:31 p.m.
Same here re list of .... This also doesn't need the parens.	david	Dec. 10, 2016, 5:32 p.m.
It looks like some of your other change has snuck in here. Do you have one branch dependent on the …	david	Dec. 10, 2016, 5:32 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Dec. 4, 2016, 2:47 p.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	Dec. 4, 2016, 2:47 p.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	Dec. 4, 2016, 2:47 p.m.
Col: 26 E126 continuation line over-indented for hanging indent	reviewbot	Dec. 4, 2016, 2:47 p.m.
Col: 9 E124 closing bracket does not match visual indentation	reviewbot	Dec. 4, 2016, 2:47 p.m.
Col: 26 E126 continuation line over-indented for hanging indent	reviewbot	Dec. 4, 2016, 2:47 p.m.
Col: 9 E124 closing bracket does not match visual indentation	reviewbot	Dec. 4, 2016, 2:47 p.m.
This line should be empty	FI finaiized	Dec. 5, 2016, 8 p.m.
Blank line in between here.	david	Dec. 10, 2016, 5:33 p.m.
Should be formatted as: Returns: unicode: The resulting absolute URL to teh item resource.	david	Dec. 10, 2016, 5:33 p.m.
Blank line between these.	brennie	Dec. 11, 2016, 2:44 a.m.
This should be the resource, e.g. ['reviewboard.webapi.resources.diff.DiffResource'].	brennie	Dec. 11, 2016, 2:44 a.m.
This should be the resource, e.g. ['reviewboard.webapi.resources.review.ReviewResource'].	brennie	Dec. 11, 2016, 2:44 a.m.
This should be the resource, e.g. ['reviewboard.webapi.resources.review_reply.ReviewReplyResource'].	brennie	Dec. 11, 2016, 2:44 a.m.
This should be the resource, e.g. ['reviewboard.webapi.resources.base_comment.BaseCommentResource'].	brennie	Dec. 11, 2016, 2:44 a.m.
This should be the resource, e.g. ['reviewboard.webapi.resources.change.ChangeResource'].	brennie	Dec. 11, 2016, 2:44 a.m.
You can refactor this to remove code duplication. try: timestamp = datetime.strptime(timestamp, ...).replace(tzinfo=utc) except ValueError: timestamp = None if timestamp …	brennie	Dec. 11, 2016, 2:44 a.m.
Col: 46 E127 continuation line over-indented for visual indent	reviewbot	Dec. 10, 2016, 9:44 p.m.
Col: 46 E127 continuation line over-indented for visual indent	reviewbot	Dec. 10, 2016, 9:44 p.m.
These keys are common between all responses, so you can further refactor the code to do: response_keys = { 'timestamp': …	brennie	Dec. 11, 2016, 2:44 a.m.
local variable 'comment_types' is assigned to but never used	reviewbot	Dec. 10, 2016, 9:44 p.m.
Make this a class level constant, e.g., self.COMMENT_TYPES.	brennie	Dec. 11, 2016, 2:44 a.m.
Col: 9 E124 closing bracket does not match visual indentation	reviewbot	Dec. 10, 2016, 9:44 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Dec. 10, 2016, 9:44 p.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	Dec. 10, 2016, 9:44 p.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	Dec. 10, 2016, 9:44 p.m.
Col: 26 E126 continuation line over-indented for hanging indent	reviewbot	Dec. 10, 2016, 9:44 p.m.
Col: 9 E124 closing bracket does not match visual indentation	reviewbot	Dec. 10, 2016, 9:44 p.m.
Col: 26 E126 continuation line over-indented for hanging indent	reviewbot	Dec. 10, 2016, 9:44 p.m.
Col: 9 E124 closing bracket does not match visual indentation	reviewbot	Dec. 10, 2016, 9:44 p.m.

Status:: Discarded

Status:: Re-opened
Summary:: Integrating Extension mixin in to RB root resource
[WIP] Adding smarter update notifications
Description:: -
[WIP] Adding smarter update notifications
-

When viewing a review request, ReviewBoard periodically checks
to see if there have been any updates to the review request.
Currently we only receive updates regarding the type of change
and who it was made by. The goal of this project is to improve
on the detail provided.
Commit:: 1e4c2f3758debd08feecedd1f0817102674d5282
Diff:: Revision 1 (+23 -6)

reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
reviewboard/webapi/resources/review_request_last_update.py

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/review_request_last_update.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js



Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/review_request_last_update.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js

Commit:

1e4c2f3758debd08feecedd1f0817102674d5282

e333a0c827db2cf462d82c7bf50e71e45fbd8151

Diff:

Revision 2 (+84 -6)

Show changes

	reviewboard/reviews/models/review_request.py
	reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
	reviewboard/webapi/resources/review_request_last_update.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/review_request_last_update.py
    reviewboard/reviews/models/review_request.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/review_request_last_update.py
    reviewboard/reviews/models/review_request.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js

reviewboard/reviews/models/review_request.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (84 > 79 characters)
```

Description:: ~
When viewing a review request, ReviewBoard periodically checks
~
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 updates regarding the type of change
and who it was made by. The goal of this project is to improve
on the detail provided.

Commit:

e333a0c827db2cf462d82c7bf50e71e45fbd8151

69b688b25b753d56ae999e0b6a08eead14bff08f

Diff:

Revision 3 (+122 -5)

Show changes

	reviewboard/reviews/models/review.py
	reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
	reviewboard/webapi/resources/review_request.py
	reviewboard/webapi/resources/review_request_last_update.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/review_request_last_update.py
    reviewboard/reviews/models/review.py
    reviewboard/webapi/resources/review_request.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/review_request_last_update.py
    reviewboard/reviews/models/review.py
    reviewboard/webapi/resources/review_request.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js

reviewboard/webapi/resources/review_request.py (Diff revision 3)
The issue has been dropped. Show all issues
```
 undefined name 'DiffSet'
```

Commit:

69b688b25b753d56ae999e0b6a08eead14bff08f

79c982145400d7ff4b59facbe8055ee46a21f6e1

Diff:

Revision 4 (+127 -5)

Show changes

	reviewboard/reviews/models/review.py
	reviewboard/reviews/models/review_request.py
	reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
	reviewboard/webapi/resources/review_request_last_update.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/review_request_last_update.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/review_request.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/review_request_last_update.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/review_request.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js

reviewboard/reviews/models/review_request.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (97 > 79 characters)
```

Commit:

79c982145400d7ff4b59facbe8055ee46a21f6e1

9d9bd3a3fb4d52322c6d3b958be9b8e39dcc4532

Diff:

Revision 5 (+128 -5)

Show changes

	reviewboard/reviews/models/review.py
	reviewboard/reviews/models/review_request.py
	reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
	reviewboard/webapi/resources/review_request_last_update.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/review_request_last_update.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/review_request.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/review_request_last_update.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/review_request.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js

reviewboard/reviews/models/review_request.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (97 > 79 characters)
```

reviewboard/reviews/models/review.py (Diff revision 5)

The issue has been resolved. Show all issues

The way you are currently doing this is going to build 7 lists. Each call to list() will create one (so thats 4) and each addition will create a new intermediate list (and there are 3 of those). that is quite expensive in terms of memory allocation. Ideally we want to use one list and use list.extend:

comment_types = (
    self.comments,
    self.screenshot_comments,
    self.file_attachment_comments,
    self.general_comments,
)
comments = []

for comment_type in comment_types:
    comments.extend(comment_type.filter(timestamp__gt=timestamp))

RS

rswanson Nov. 19, 2016, 12:18 p.m.
```
Thanks for the suggestion!
```

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 5)
The issue has been resolved. Show all issues
```
This can throw if someone does ?timestamp=0 or some other invalid format.
```
1. RS
  
  rswanson Nov. 19, 2016, 12:18 p.m.
  I'll handle this with a try/except.
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 5)
The issue has been resolved. Show all issues
```
utc_timestamp will always be truthy. If we fail to parse the timestamp, we will get an exception.
```
1. RS
  
  rswanson Nov. 19, 2016, 12:18 p.m.
  Removed this

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

The issue has been resolved. Show all issues

So there are 4 comment types (comments, general, file attachment, and screenshot). That means for every review we will be doing four queries, so we're going to be doing 4 * r queries. We really don't want to be doing this.

If we had a large review request, with say 100 reviews, someone could query the api with ?timestamp=1970-01-01T00:00:00Z and get every review and we'd have to do 400 database queries. This can lead to denial of service attacks.

We really want to do the minimum number of queries here, which will be four. Instead of going through each individual review, you'll have to query through each comment class, e.g.:

review_pks = [review.pk for review in new_reviews]
new_comments = []

comment_types = (Comment, FileAttachmentComment, GeneralComment, ScreenshotComment)

for comment_cls in comment_types:
    new_comments.extend(comment_cls.objects.filter(
        review_id__in=review_pks, timestamp__gt=timestamp))

If we need to parse out comments to their respective reviews afterwards, we can post-process the dataset.

rswanson Nov. 19, 2016, 12:18 p.m.

That's a really good point that I hadn't considered. Thanks for the suggestion!

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

Replies are not comment updates. Replies are actually a Review object with base_reply_to having a non-null value.

rswanson Nov. 19, 2016, 12:18 p.m.

Good to know. So far I'm querying for diffsets, reviews, and comments. In my original planning I thought that would be sufficient information to gather for the new update notifications, but now I'm not so sure. Should I be querying for replies as well?

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

The issue has been resolved. Show all issues

We will probably want to get reviews and replies in two separate lists (may want to confer with christian on that). However, you can do one query for each type of review + reply and then filter them out in python land.

rswanson Nov. 19, 2016, 12:18 p.m.

Thanks for the heads up. I've changed the code to include these objects separately for now. I'll be sure to follow up with Christian.

Change Summary:

Addressing review comments. Broke up reviews and replies from being returned in the same list. Started developing unit tests in line with other API classes.

Commit:

9d9bd3a3fb4d52322c6d3b958be9b8e39dcc4532

c1fd6e792305a399623769e098705f917977de35

Diff:

Revision 6 (+184 -6)

Show changes

	reviewboard/reviews/models/review.py
	reviewboard/reviews/models/review_request.py
	reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
	reviewboard/webapi/resources/review_request_last_update.py
	reviewboard/webapi/tests/test_review_request_last_update.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/tests/test_review_request_last_update.py
    reviewboard/webapi/resources/review_request_last_update.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/review_request.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/tests/test_review_request_last_update.py
    reviewboard/webapi/resources/review_request_last_update.py
    reviewboard/reviews/models/review.py
    reviewboard/reviews/models/review_request.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (100 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (101 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 6)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Fix it!

reviewboard/reviews/models/review.py (Diff revision 6)
The issue has been resolved. Show all issues
```
This isn't being used anymore.
```

reviewboard/reviews/models/review_request.py (Diff revision 6)

The issue has been resolved. Show all issues

This should not return DiffSets associated with review request drafts, i.e. review_request_draft=None.

reviewboard/reviews/models/review_request.py (Diff revision 6)
The issue has been resolved. Show all issues
```
This should make sure that the reviews aren't drafts, i.e., public=True.
```

reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js (Diff revision 6)

The issue has been resolved. Show all issues

timestamp doesn't need to be in quotes. Also can you change this to:

data: {
    timestamp: this._lastUpdateTimestamp,
},

reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Make sure this doesn't stay :)
```
1. RS
  
  rswanson Nov. 24, 2016, 1:09 p.m.
  Haha! Of course.
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Instead of importing pytz, you should do from django.utils.timezone import utc.
```

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

The issue has been resolved. Show all issues

Single import statement, one name per line.

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6)
The issue has been resolved. Show all issues
```
This should be the WebAPI resource and not the model, i.e. [reviewboard.webapi.resources.diff.DiffResource]
```

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

The issue has been resolved. Show all issues

This should be the WebAPI resource and not the model, i.e. [reviewboard.webapi.resources.review.ReviewResource]

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

An issue was opened. Show all issues

This should be the WebAPI resource and not the model, i.e. [reviewboard.webapi.resources.base_comment.BaseCommentResource] (I think?) You will want to build the docs and see if this one works. If so, the BaseCommentResource will likely want to include links to all resource types in its class docstring.
The docs you want to check are the webapi/ docs, not the class documentation for the resource.

rswanson Nov. 24, 2016, 1:09 p.m.

What do you mean by 'build the docs' exactly? I've updated this field accordingly - but I think I'm missing part of the goal.

brennie Dec. 2, 2016, 12:10 p.m.

Sorry I missed this!

go into docs/manual and do make html. Then open _build/html/index.html and find the webapi documentation.

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Since you never use timestamp again, just reassign to timestamp.
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Just call this reviews.
```

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

The issue has been resolved. Show all issues

This isn't too gross :). However, instead of iterating over this twice, we can iterate once:


new_reviews = []
new_replies = []

for review in reviews:
    if review.base_reply_to:
        new_replies.append(review)
    else:
        new_reviews.append(review)

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Docstring etc.
I would rename new_reviews to just reviews.
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6)
```
review_ids or review_pks are fine here.
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Can you make this a constant on the class?
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6)
The issue has been resolved. Show all issues
```
This needs to be review_id__in or review__pk__in. Also, one per line.
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 6)
The issue has been resolved. Show all issues
```
from django.utils import six
```

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

The issue has been dropped. Show all issues

After you remove the pytz import you can convert to a  aware time with

(timezone.get_default_timezone()
    .localize(datetime
              .strptime(timestamp,
                        '%Y-%m-%dT%H:%M:%SZ')))

rswanson Nov. 24, 2016, 1:09 p.m.

In this case we actually want the timezone to stay as UTC.

The overall results of the API look good. There's some additions in this change that don't need to be there, and some consistency things to work on. Good work so far, though!

reviewboard/reviews/models/review.py (Diff revision 6)

The issue has been resolved. Show all issues

Timestamps on a review are all set to the same value when published. This whole function wouldn't be doing anything other than get_all_comments.

rswanson Nov. 24, 2016, 3:40 p.m.

brennie suggested removing this function all together, which is what I ended up doing.

reviewboard/reviews/models/review.py (Diff revision 6)

The issue has been resolved. Show all issues

Make sure to follow the conventions for docstrings, specifically "Args" and "Returns".

https://www.reviewboard.org/docs/codebase/dev/docs/writing-codebase-docs/

rswanson Nov. 24, 2016, 3:40 p.m.

Sounds good. This method actually ended up getting removed, however.

reviewboard/reviews/models/review_request.py (Diff revision 6)

The issue has been resolved. Show all issues

I don't think we should be adding new methods that just produce different variations of a query here. This query is simple enough that it can and should be used by the caller.
The reason is that we don't want to establish precedent. We probably have a hundred or more distinct queries involving attributes of a ReviewRequest throughout the codebase, and if we had a method for each one in this class, the class would massively balloon up.
So instead, let's get rid of this and just do this query directly where it needs to be called.

reviewboard/reviews/models/review_request.py (Diff revision 6)
The issue has been resolved. Show all issues
```
We should always say "review request" instead of "request" (which tends to be the term for HTTP Request).
```
1. RS
  
  rswanson Nov. 24, 2016, 3:40 p.m.
  Noted.
reviewboard/reviews/models/review_request.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Same here as above.
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6)
The issue has been resolved. Show all issues
```
This shouldn't be a string. It should be a reference to the resource model.
Same below.
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6)
The issue has been resolved. Show all issues
```
"out-of-date"
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6)
The issue has been resolved. Show all issues
```
No blank line here.
```

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

The issue has been resolved. Show all issues

This comment isn't really clear. I think this is meant to be an example of the format, but if so, the comment should state that. However, saying that it's a ISO8601 timestamp is sufficient.

rswanson Nov. 24, 2016, 3:40 p.m.

You're right, it's meant to be an example. I'll make sure to point that out.

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Comments should be proper sentences, periods and all.
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Blank line between code and new comments.
```

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

The issue has been resolved. Show all issues

This isn't too helpful to the caller. There should be suitable error information.

However, we're also not actually working with form data, so the error is incorrect.

There's really a lot of code in this try/except. What part is expected to raise the ValueError? We should limit the code around that, and perhaps not error out but instead fall back to old behavior (not showing the new fields).

rswanson Nov. 24, 2016, 3:40 p.m.

The datetime conversion of the timestamp is what is expected to raise the ValueError. Your suggestion makes a lot of sense, we can just give the user the same data they're used to if the timestamp isn't correct.

reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 6)

The issue has been resolved. Show all issues

This file is missing a:

from __future__ import unicode_literals


Which must be at the top.

reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Missing a docstring.
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Blank line between these.
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Blank line between these.
```

Commit:

c1fd6e792305a399623769e098705f917977de35

feee98a44aee95d04f4886eb9919a95a4496b766

Diff:

Revision 7 (+172 -7)

Show changes

	reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
	reviewboard/webapi/resources/review_request_last_update.py
	reviewboard/webapi/tests/test_review_request_last_update.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/review_request_last_update.py
    reviewboard/webapi/tests/test_review_request_last_update.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/review_request_last_update.py
    reviewboard/webapi/tests/test_review_request_last_update.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 7)
The issue has been resolved. Show all issues
```
 'INVALID_FORM_DATA' imported but unused
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 9
 E303 too many blank lines (2)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 7)
The issue has been dropped. Show all issues
```
Col: 13
 E116 unexpected indentation (comment)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 7)
The issue has been dropped. Show all issues
```
Col: 13
 E265 block comment should start with '# '
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 7)
The issue has been dropped. Show all issues
```
Col: 13
 E116 unexpected indentation (comment)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 7)
The issue has been dropped. Show all issues
```
Col: 13
 E116 unexpected indentation (comment)
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 63
 E127 continuation line over-indented for visual indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 7)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Commit:

feee98a44aee95d04f4886eb9919a95a4496b766

f90ffa0c16d35b0244b57de8330da9f999ef52ba

Diff:

Revision 8 (+171 -7)

Show changes

	reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
	reviewboard/webapi/resources/review_request_last_update.py
	reviewboard/webapi/tests/test_review_request_last_update.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/review_request_last_update.py
    reviewboard/webapi/tests/test_review_request_last_update.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/review_request_last_update.py
    reviewboard/webapi/tests/test_review_request_last_update.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 8)
The issue has been resolved. Show all issues
```
 undefined name 'DOES_NOT_EXIST'
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 8)

The issue has been dropped. Show all issues

These can probably all be condensed into one test.

Commit:

f90ffa0c16d35b0244b57de8330da9f999ef52ba

42dc1cc02999eb438d15adaa6ba42b7e836e2d3e

Diff:

Revision 9 (+190 -7)

Show changes

	reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
	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/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/resources/models/reviewRequestModel.es6.js



Tool: PEP8 Style Checker
Processed Files:
    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/resources/models/reviewRequestModel.es6.js

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 9)
The issue has been resolved. Show all issues
```
 undefined name 'DOES_NOT_EXIST'
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 17
 E126 continuation line over-indented for hanging indent
```
reviewboard/webapi/tests/urls.py (Diff revision 9)
The issue has been dropped. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/webapi/tests/urls.py (Diff revision 9)
The issue has been dropped. Show all issues
```
 undefined name 'review_request'
```

Change Summary:

Trying to get the default get tests running on test_review_request_last_update.py - running in to some issues.

Testing Done:

Added unit tests.

Commit:

42dc1cc02999eb438d15adaa6ba42b7e836e2d3e

3a4694b1a3edc24e476b0b168b952d806b8cb436

Diff:

Revision 10 (+196 -7)

Show changes

	reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
	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

Change Summary:

Built in get tests running.

Commit:

3a4694b1a3edc24e476b0b168b952d806b8cb436

22f974e40f4846c7f48bfaca6868146ee815ae81

Diff:

Revision 11 (+195 -7)

Show changes

	reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
	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

Commit:

22f974e40f4846c7f48bfaca6868146ee815ae81

1629719369c1aa1646325e73dc429a43e6f1560c

Diff:

Revision 12 (+206 -6)

Show changes

	reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
	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/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/resources/models/reviewRequestModel.es6.js



Tool: PEP8 Style Checker
Processed Files:
    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/resources/models/reviewRequestModel.es6.js

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 11)
The issue has been dropped. Show all issues
```
 undefined name 'DOES_NOT_EXIST'
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 11)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 11)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 11)
The issue has been dropped. Show all issues
```
Col: 47
 E128 continuation line under-indented for visual indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 11)
The issue has been dropped. Show all issues
```
Col: 17
 E126 continuation line over-indented for hanging indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 11)
The issue has been dropped. Show all issues
```
Col: 17
 E126 continuation line over-indented for hanging indent
```
reviewboard/webapi/tests/urls.py (Diff revision 11)
The issue has been dropped. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```

Tool: Pyflakes
Processed Files:
    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/resources/models/reviewRequestModel.es6.js



Tool: Pyflakes
Processed Files:
    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/resources/models/reviewRequestModel.es6.js



Tool: PEP8 Style Checker
Processed Files:
    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/resources/models/reviewRequestModel.es6.js



Tool: PEP8 Style Checker
Processed Files:
    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/resources/models/reviewRequestModel.es6.js

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 10)
The issue has been dropped. Show all issues
```
 undefined name 'DOES_NOT_EXIST'
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 10)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 10)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 10)
The issue has been dropped. Show all issues
```
 undefined name 'review'
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 10)
The issue has been dropped. Show all issues
```
 undefined name 'review'
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 10)
The issue has been dropped. Show all issues
```
Col: 47
 E128 continuation line under-indented for visual indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 10)
The issue has been dropped. Show all issues
```
Col: 17
 E126 continuation line over-indented for hanging indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 10)
The issue has been dropped. Show all issues
```
Col: 17
 E126 continuation line over-indented for hanging indent
```
reviewboard/webapi/tests/urls.py (Diff revision 10)
The issue has been dropped. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (88 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 12)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 12)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Col: 47
 E128 continuation line under-indented for visual indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Col: 17
 E126 continuation line over-indented for hanging indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Col: 17
 E126 continuation line over-indented for hanging indent
```
reviewboard/webapi/tests/urls.py (Diff revision 12)
The issue has been dropped. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```

Commit:

1629719369c1aa1646325e73dc429a43e6f1560c

2f92034fe3f0ce6b40d7b8c50a0b805cf693041a

Diff:

Revision 13 (+222 -6)

Show changes

	reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
	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/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/resources/models/reviewRequestModel.es6.js



Tool: PEP8 Style Checker
Processed Files:
    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/resources/models/reviewRequestModel.es6.js

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 13)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (88 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 13)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 13)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 13)
The issue has been resolved. Show all issues
```
Col: 47
 E128 continuation line under-indented for visual indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 13)
The issue has been dropped. Show all issues
```
Col: 17
 E126 continuation line over-indented for hanging indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 13)
The issue has been resolved. Show all issues
```
Col: 17
 E126 continuation line over-indented for hanging indent
```
reviewboard/webapi/tests/urls.py (Diff revision 13)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```

Commit:

2f92034fe3f0ce6b40d7b8c50a0b805cf693041a

c00252b533afab2dc267068d2e12a189e2b072fd

Diff:

Revision 14 (+343 -8)

Show changes

	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/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/resources/models/reviewRequestModel.es6.js

reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 14)
The issue has been resolved. Show all issues
```
 'datetime' imported but unused
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 14)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 14)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Col: 17
 E126 continuation line over-indented for hanging indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Col: 37
 E128 continuation line under-indented for visual indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Col: 42
 E128 continuation line under-indented for visual indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Col: 27
 E201 whitespace after '{'
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Col: 32
 E127 continuation line over-indented for visual indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Col: 33
 E201 whitespace after '{'
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Col: 27
 E201 whitespace after '{'
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Col: 32
 E127 continuation line over-indented for visual indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Col: 33
 E201 whitespace after '{'
```
reviewboard/webapi/tests/urls.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```

Summary:: [WIP] Adding smarter update notifications
Adding smarter update notifications
Testing Done:: ~
Added unit tests.
~
Added unit tests for backend code - all pass.
+ Manually verified API response locally.

Commit:

c00252b533afab2dc267068d2e12a189e2b072fd

efe772e8044a9ee596543afed738bbf42fbfd7a3

Diff:

Revision 15 (+353 -8)

Show changes

	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/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/resources/models/reviewRequestModel.es6.js

reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15)
The issue has been resolved. Show all issues
```
 'datetime' imported but unused
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Col: 17
 E126 continuation line over-indented for hanging indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15)
The issue has been dropped. Show all issues
```
Col: 26
 E126 continuation line over-indented for hanging indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Col: 17
 E126 continuation line over-indented for hanging indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Col: 37
 E128 continuation line under-indented for visual indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Col: 42
 E128 continuation line under-indented for visual indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Col: 27
 E201 whitespace after '{'
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Col: 32
 E127 continuation line over-indented for visual indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Col: 33
 E201 whitespace after '{'
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Col: 27
 E201 whitespace after '{'
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Col: 32
 E127 continuation line over-indented for visual indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Col: 33
 E201 whitespace after '{'
```
reviewboard/webapi/tests/urls.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```

Change Summary:

Resolving unit test comparator function.

Commit:

efe772e8044a9ee596543afed738bbf42fbfd7a3

9339cd39c020ad595911459603a96d2e90068b12

Diff:

Revision 16 (+359 -8)

Show changes

	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/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/resources/models/reviewRequestModel.es6.js

reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 16)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 16)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (83 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Col: 17
 E126 continuation line over-indented for hanging indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Col: 26
 E126 continuation line over-indented for hanging indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Col: 26
 E126 continuation line over-indented for hanging indent
```

reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js (Diff revision 16)
The issue has been resolved. Show all issues
```
Undo this.
```

reviewboard/testing/testcase.py (Diff revision 16)

The issue has been resolved. Show all issues

This won't (or shouldn't, anyway) result with the timestamp saved to the review request in the DB. Instead of setting review.timestamp, and calling save() (which is what publish()), you will have to do:

Review.objects.filter(pk=review.pk).update(timestamp=timestamp)

rswanson Dec. 2, 2016, 3:35 p.m.

I needed to add this in order to specify a non-current timestamp on a mock review for my unit tests. Would your suggestion still apply in that case?

brennie Dec. 2, 2016, 7:23 p.m.
```
Yes.
```

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

The issue has been resolved. Show all issues

This can eventually lead to fun to debug circular reference issues. Instead, when you want to refer to ReviewResource you can do: resources.review.

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

The issue has been resolved. Show all issues

This should actually be:

['reviewboard.webapi.resources.diff.DiffResource']

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Same goes for here with the appropriate resource
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Same goes for here with the appropriate resource
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Same goes for here with the appropriate resource
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Same goes for here with the appropriate resource
```

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

The issue has been resolved. Show all issues

Constant names should be ALL_CAPS.
Also, can you format this as:

COMMENT_TYPES = (
    Comment,
    # ...
)

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

The issue has been resolved. Show all issues

Don't use exceptions like this for flow control.
Instead:


if not timestamp:
    # old behaviour
    return ...

try:
    timestamp = datetime.strptime(...)
except ValueError:
    return INVALID_FORM_DATA, {
        fields: {
            'timestamp': [
                'error here',
            ],
        },
    }

# continue calculating advanced response.

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 16)
The issue has been resolved. Show all issues
```
This documentation needs to go into the docs for the timestamp field.
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 16)
The issue has been resolved. Show all issues
```
No blank line here.
```

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

The issue has been dropped. Show all issues

When the timestamp is provided and invalid, we want to give an INVALID_FORM_DATA response.

rswanson Dec. 2, 2016, 3:35 p.m.

Christian suggested a different alternative in his most recent review, which I chose to follow. His argument was that this isn't the appropraite case for INVALID_FORM_DATA.

brennie Dec. 2, 2016, 7:23 p.m.

Alright, I missed that. Disregard this, then.

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 16)
The issue has been resolved. Show all issues
```
missing docstring.
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Needs file docstring.
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Blank line between these.
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 16)
The issue has been dropped. Show all issues
```
Blank line between these.
```
reviewboard/webapi/tests/urls.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Docstring.
```
1. RS
  
  rswanson Dec. 2, 2016, 3:35 p.m.
  I'll add a docstring, but I noticed that every other method in here doesn't seem to have one.
2. brennie Dec. 2, 2016, 7:23 p.m.
  Still, doesn't hurt :)

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

is the timestamp param passed into this function the curent timestamp or is when the last update was made (which I assumed) and how is it differnt from the server_timestamp?

rswanson Dec. 4, 2016, 2:05 p.m.

The server keeps track of a review request / review / diffset, etc with a timestamp related to when it was created or last updated. The timestamp parameter being passed in here allows the user to see all updates that have occurred SINCE a specified timestamp. Effectively the timestamp could be anything!

Commit:

9339cd39c020ad595911459603a96d2e90068b12

30f51d395ec06d1bf00332c629dd44617a9637ff

Diff:

Revision 17 (+414 -9)

Show changes

	reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
	reviewboard/testing/testcase.py
	reviewboard/webapi/resources/review_request_last_update.py
	reviewboard/webapi/resources/root.py
	reviewboard/webapi/tests/mimetypes.py
	reviewboard/webapi/tests/test_review_request_last_update.py
	reviewboard/webapi/tests/urls.py

Fix it!

Tool: Pyflakes
Processed Files:
    reviewboard/testing/testcase.py
    reviewboard/webapi/tests/test_review_request_last_update.py
    reviewboard/webapi/resources/root.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/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/root.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/resources/models/reviewRequestModel.es6.js

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 17)
An issue was opened. Show all issues
```
Col: 21
 E128 continuation line under-indented for visual indent
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 17)
An issue was opened. Show all issues
```
Col: 5
 E124 closing bracket does not match visual indentation
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 17)
An issue was opened. Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 17)
An issue was opened. Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 17)
An issue was opened. Show all issues
```
Col: 80
 E501 line too long (83 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 17)
An issue was opened. Show all issues
```
Col: 26
 E126 continuation line over-indented for hanging indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 17)
An issue was opened. Show all issues
```
Col: 9
 E124 closing bracket does not match visual indentation
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 17)
An issue was opened. Show all issues
```
Col: 26
 E126 continuation line over-indented for hanging indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 17)
An issue was opened. Show all issues
```
Col: 9
 E124 closing bracket does not match visual indentation
```

reviewboard/webapi/tests/urls.py (Diff revision 17)
The issue has been dropped. Show all issues
```
This line should be empty
```

reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js (Diff revision 17)
The issue has been resolved. Show all issues
```
Let's leave the comment, please.
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 17)
The issue has been resolved. Show all issues
```
Even though the line would go over 80 columns, let's keep this in one piece.
```

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

The issue has been resolved. Show all issues

This is only used once in an internal method, so let's just have it inline there.

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

The issue has been resolved. Show all issues

We only use this in the one place, so let's avoid assigning the extra variable (or constructing the list):

for review in Review.objects.filter(
    review_request=review_request,
    timestamp__gt=timestamp,
    public=True):
    if review.base_reply_to:
        ...

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 17)
The issue has been resolved. Show all issues
```
We shouldn't need to convert to list here.
```

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

The issue has been resolved. Show all issues

Keep this on one line, even though it's long.

Should also be list of reviewboard.webapi.resource.review.ReviewResource

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 17)
The issue has been resolved. Show all issues
```
Same here re list of .... This also doesn't need the parens.
```

reviewboard/webapi/resources/root.py (Diff revision 17)

The issue has been resolved. Show all issues

It looks like some of your other change has snuck in here. Do you have one branch dependent on the other?

reviewboard/webapi/tests/urls.py (Diff revision 17)
The issue has been resolved. Show all issues
```
Blank line in between here.
```

reviewboard/webapi/tests/urls.py (Diff revision 17)

The issue has been resolved. Show all issues

Should be formatted as:

Returns:
    unicode:
    The resulting absolute URL to teh item resource.

Branch:

master

release-3.0.x

Commit:

30f51d395ec06d1bf00332c629dd44617a9637ff

8fe69efe5d1a461b374bee2f0cb4b61439896ac7

Diff:

Revision 18 (+414 -7)

Show changes

	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

Fix it!

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/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/resources/models/reviewRequestModel.es6.js

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 18)
An issue was opened. Show all issues
```
Col: 46
 E127 continuation line over-indented for visual indent
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 18)
An issue was opened. Show all issues
```
Col: 46
 E127 continuation line over-indented for visual indent
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 18)
An issue was opened. Show all issues
```
 local variable 'comment_types' is assigned to but never used
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 18)
An issue was opened. Show all issues
```
Col: 9
 E124 closing bracket does not match visual indentation
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 18)
An issue was opened. Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 18)
An issue was opened. Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 18)
An issue was opened. Show all issues
```
Col: 80
 E501 line too long (83 > 79 characters)
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 18)
An issue was opened. Show all issues
```
Col: 26
 E126 continuation line over-indented for hanging indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 18)
An issue was opened. Show all issues
```
Col: 9
 E124 closing bracket does not match visual indentation
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 18)
An issue was opened. Show all issues
```
Col: 26
 E126 continuation line over-indented for hanging indent
```
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 18)
An issue was opened. Show all issues
```
Col: 9
 E124 closing bracket does not match visual indentation
```

Fix it!

reviewboard/webapi/resources/review_request_last_update.py (Diff revision 18)
An issue was opened. Show all issues
```
Blank line between these.
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 18)
An issue was opened. Show all issues
```
This should be the resource, e.g. ['reviewboard.webapi.resources.diff.DiffResource'].
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 18)
An issue was opened. Show all issues
```
This should be the resource, e.g. ['reviewboard.webapi.resources.review.ReviewResource'].
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 18)
An issue was opened. Show all issues
```
This should be the resource, e.g. ['reviewboard.webapi.resources.review_reply.ReviewReplyResource'].
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 18)
An issue was opened. Show all issues
```
This should be the resource, e.g. ['reviewboard.webapi.resources.base_comment.BaseCommentResource'].
```
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 18)
An issue was opened. Show all issues
```
This should be the resource, e.g. ['reviewboard.webapi.resources.change.ChangeResource'].
```

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

An issue was opened. Show all issues

You can refactor this to remove code duplication.


try:
    timestamp = datetime.strptime(timestamp, ...).replace(tzinfo=utc)
except ValueError:
    timestamp = None

if timestamp is None:
    # Old behaviour.
    return 200, { ... }, { 'ETag': etag, }

# New behaviour ...

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

An issue was opened. Show all issues

These keys are common between all responses, so you can further refactor the code to do:


response_keys = {
    'timestamp': server_timestamp,
    'user': user,
    'summary': summary,
    'type': update_type,
}

# try/except to check for timestamp ...

if timestamp is None:
    # Calculate new_diffsets, etc.
    response_keys.update({
        'diffset_update': new_diffsets,
        'review_updates': new_reviews,
        'reply_updates': new_replies,
        'comment_updates': new_comments,
        'field_updates': field_updates,
    })

return 200, {
        self.item_result_key: response_keys,
    }, {
        'ETag': etag,
    }


This eliminates the multiple returns and makes it easier to follow.

Additionally, can you ensure the keys are listed in alphabetical order when being inserted?

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

An issue was opened. Show all issues

Make this a class level constant, e.g., self.COMMENT_TYPES.

Status:: Discarded

-		[WIP] Adding smarter update notifications
-
		When viewing a review request, ReviewBoard periodically checks
		to see if there have been any updates to the review request.
		Currently we only receive updates regarding the type of change
		and who it was made by. The goal of this project is to improve
		on the detail provided.

~		Added unit tests.
	~	Added unit tests for backend code - all pass.
	+	Manually verified API response locally.