Adding smarter update notifications

Review Request #8519 — Created Nov. 3, 2016 and discarded

rswanson
Review Board
release-3.0.x
reviewboard, students

When viewing a review request, Review Board periodically checks
to see if there have been any updates to the review request.
Currently we only receive 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 for backend code - all pass.
Manually verified API response locally.

Description From Last Updated

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

reviewbotreviewbot

undefined name 'DiffSet'

reviewbotreviewbot

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

reviewbotreviewbot

The way you are currently doing this is going to build 7 lists. Each call to list() will create one ...

brenniebrennie

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

reviewbotreviewbot

This can throw if someone does ?timestamp=0 or some other invalid format.

brenniebrennie

utc_timestamp will always be truthy. If we fail to parse the timestamp, we will get an exception.

brenniebrennie

So there are 4 comment types (comments, general, file attachment, and screenshot). That means for every review we will be ...

brenniebrennie

We will probably want to get reviews and replies in two separate lists (may want to confer with christian on ...

brenniebrennie

This isn't being used anymore.

brenniebrennie

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

chipx86chipx86

Make sure to follow the conventions for docstrings, specifically "Args" and "Returns". https://www.reviewboard.org/docs/codebase/dev/docs/writing-codebase-docs/

chipx86chipx86

I don't think we should be adding new methods that just produce different variations of a query here. This query ...

chipx86chipx86

We should always say "review request" instead of "request" (which tends to be the term for HTTP Request).

chipx86chipx86

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

brenniebrennie

Same here as above.

chipx86chipx86

This should make sure that the reviews aren't drafts, i.e., public=True.

brenniebrennie

timestamp doesn't need to be in quotes. Also can you change this to: data: { timestamp: this._lastUpdateTimestamp, },

brenniebrennie

Make sure this doesn't stay :)

brenniebrennie

Instead of importing pytz, you should do from django.utils.timezone import utc.

brenniebrennie

Single import statement, one name per line.

brenniebrennie

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

brenniebrennie

This shouldn't be a string. It should be a reference to the resource model. Same below.

chipx86chipx86

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

brenniebrennie

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

brenniebrennie

"out-of-date"

chipx86chipx86

No blank line here.

chipx86chipx86

This comment isn't really clear. I think this is meant to be an example of the format, but if so, ...

chipx86chipx86

Since you never use timestamp again, just reassign to timestamp.

brenniebrennie

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

Comments should be proper sentences, periods and all.

chipx86chipx86

Just call this reviews.

brenniebrennie

Blank line between code and new comments.

chipx86chipx86

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

brenniebrennie

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

This isn't too helpful to the caller. There should be suitable error information. However, we're also not actually working with ...

chipx86chipx86

Docstring etc. I would rename new_reviews to just reviews.

brenniebrennie

Can you make this a constant on the class?

brenniebrennie

This needs to be review_id__in or review__pk__in. Also, one per line.

brenniebrennie

from django.utils import six

brenniebrennie

This file is missing a: from __future__ import unicode_literals Which must be at the top.

chipx86chipx86

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

reviewbotreviewbot

Missing a docstring.

chipx86chipx86

Blank line between these.

chipx86chipx86

Blank line between these.

chipx86chipx86

'INVALID_FORM_DATA' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

Col: 9 E303 too many blank lines (2)

reviewbotreviewbot

Col: 13 E116 unexpected indentation (comment)

reviewbotreviewbot

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

reviewbotreviewbot

Col: 13 E116 unexpected indentation (comment)

reviewbotreviewbot

Col: 13 E116 unexpected indentation (comment)

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

undefined name 'DOES_NOT_EXIST'

reviewbotreviewbot

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

reviewbotreviewbot

These can probably all be condensed into one test.

brenniebrennie

undefined name 'DOES_NOT_EXIST'

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

undefined name 'review_request'

reviewbotreviewbot

undefined name 'DOES_NOT_EXIST'

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

undefined name 'review'

reviewbotreviewbot

undefined name 'review'

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

undefined name 'DOES_NOT_EXIST'

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

'datetime' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 27 E201 whitespace after '{'

reviewbotreviewbot

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

reviewbotreviewbot

Col: 33 E201 whitespace after '{'

reviewbotreviewbot

Col: 27 E201 whitespace after '{'

reviewbotreviewbot

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

reviewbotreviewbot

Col: 33 E201 whitespace after '{'

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

'datetime' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 27 E201 whitespace after '{'

reviewbotreviewbot

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

reviewbotreviewbot

Col: 33 E201 whitespace after '{'

reviewbotreviewbot

Col: 27 E201 whitespace after '{'

reviewbotreviewbot

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

reviewbotreviewbot

Col: 33 E201 whitespace after '{'

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Undo this.

brenniebrennie

This won't (or shouldn't, anyway) result with the timestamp saved to the review request in the DB. Instead of setting ...

brenniebrennie

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

brenniebrennie

This should actually be: ['reviewboard.webapi.resources.diff.DiffResource']

brenniebrennie

Same goes for here with the appropriate resource

brenniebrennie

Same goes for here with the appropriate resource

brenniebrennie

Same goes for here with the appropriate resource

brenniebrennie

Same goes for here with the appropriate resource

brenniebrennie

Constant names should be ALL_CAPS. Also, can you format this as: COMMENT_TYPES = ( Comment, # ... )

brenniebrennie

Don't use exceptions like this for flow control. Instead: if not timestamp: # old behaviour return ... try: timestamp = ...

brenniebrennie

This documentation needs to go into the docs for the timestamp field.

brenniebrennie

No blank line here.

brenniebrennie

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

brenniebrennie

missing docstring.

brenniebrennie

Needs file docstring.

brenniebrennie

Blank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Docstring.

brenniebrennie

Let's leave the comment, please.

daviddavid

Even though the line would go over 80 columns, let's keep this in one piece.

daviddavid

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

reviewbotreviewbot

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

daviddavid

Col: 5 E124 closing bracket does not match visual indentation

reviewbotreviewbot

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

daviddavid

We shouldn't need to convert to list here.

daviddavid

Keep this on one line, even though it's long. Should also be list of reviewboard.webapi.resource.review.ReviewResource

daviddavid

Same here re list of .... This also doesn't need the parens.

daviddavid

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

daviddavid

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 9 E124 closing bracket does not match visual indentation

reviewbotreviewbot

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

reviewbotreviewbot

Col: 9 E124 closing bracket does not match visual indentation

reviewbotreviewbot

This line should be empty

FI finaiized

Blank line in between here.

daviddavid

Should be formatted as: Returns: unicode: The resulting absolute URL to teh item resource.

daviddavid

Blank line between these.

brenniebrennie

This should be the resource, e.g. ['reviewboard.webapi.resources.diff.DiffResource'].

brenniebrennie

This should be the resource, e.g. ['reviewboard.webapi.resources.review.ReviewResource'].

brenniebrennie

This should be the resource, e.g. ['reviewboard.webapi.resources.review_reply.ReviewReplyResource'].

brenniebrennie

This should be the resource, e.g. ['reviewboard.webapi.resources.base_comment.BaseCommentResource'].

brenniebrennie

This should be the resource, e.g. ['reviewboard.webapi.resources.change.ChangeResource'].

brenniebrennie

You can refactor this to remove code duplication. try: timestamp = datetime.strptime(timestamp, ...).replace(tzinfo=utc) except ValueError: timestamp = None if timestamp ...

brenniebrennie

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

reviewbotreviewbot

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

reviewbotreviewbot

These keys are common between all responses, so you can further refactor the code to do: response_keys = { 'timestamp': ...

brenniebrennie

local variable 'comment_types' is assigned to but never used

reviewbotreviewbot

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

brenniebrennie

Col: 9 E124 closing bracket does not match visual indentation

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 9 E124 closing bracket does not match visual indentation

reviewbotreviewbot

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

reviewbotreviewbot

Col: 9 E124 closing bracket does not match visual indentation

reviewbotreviewbot
RS
RS
reviewbot
  1. 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
    
    
  2. 
      
RS
reviewbot
  1. 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
    
    
  2. Col: 80
     E501 line too long (84 > 79 characters)
    
  3. 
      
RS
RS
reviewbot
  1. 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
    
    
  2.  undefined name 'DiffSet'
    
  3. 
      
RS
reviewbot
  1. 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
    
    
  2. Col: 80
     E501 line too long (97 > 79 characters)
    
  3. 
      
RS
reviewbot
  1. 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
    
    
  2. Col: 80
     E501 line too long (97 > 79 characters)
    
  3. 
      
brennie
  1. 
      
  2. reviewboard/reviews/models/review.py (Diff revision 5)
     
     
     
     
     
     
     

    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))
    
    1. Thanks for the suggestion!

  3. This can throw if someone does ?timestamp=0 or some other invalid format.

    1. I'll handle this with a try/except.

  4. utc_timestamp will always be truthy. If we fail to parse the timestamp, we will get an exception.

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

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

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

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

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

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

  8. 
      
RS
reviewbot
  1. 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
    
    
  2. Col: 80
     E501 line too long (100 > 79 characters)
    
  3. Col: 80
     E501 line too long (101 > 79 characters)
    
  4. Col: 80
     E501 line too long (80 > 79 characters)
    
  5. Col: 80
     E501 line too long (80 > 79 characters)
    
  6. 
      
brennie
  1. 
      
  2. reviewboard/reviews/models/review.py (Diff revision 6)
     
     

    This isn't being used anymore.

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

  4. This should make sure that the reviews aren't drafts, i.e., public=True.

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

    data: {
        timestamp: this._lastUpdateTimestamp,
    },
    
  6. Make sure this doesn't stay :)

    1. Haha! Of course.

  7. Instead of importing pytz, you should do from django.utils.timezone import utc.

  8. Single import statement, one name per line.

  9. This should be the WebAPI resource and not the model, i.e. [reviewboard.webapi.resources.diff.DiffResource]

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

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

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

    2. Sorry I missed this!

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

  12. Since you never use timestamp again, just reassign to timestamp.

  13. Just call this reviews.

  14. 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)
    
  15. Docstring etc.

    I would rename new_reviews to just reviews.

  16. review_ids or review_pks are fine here.

  17. Can you make this a constant on the class?

  18. This needs to be review_id__in or review__pk__in. Also, one per line.

  19. from django.utils import six
    
  20. 
      
CO
  1. 
      
  2. 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')))
    
    1. In this case we actually want the timezone to stay as UTC.

  3. 
      
chipx86
  1. 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!

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

    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.

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

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

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

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

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

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

  5. We should always say "review request" instead of "request" (which tends to be the term for HTTP Request).

  6. Same here as above.

  7. This shouldn't be a string. It should be a reference to the resource model.

    Same below.

  8. No blank line here.

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

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

  10. Comments should be proper sentences, periods and all.

  11. Blank line between code and new comments.

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

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

  13. This file is missing a:

    from __future__ import unicode_literals
    

    Which must be at the top.

  14. Blank line between these.

  15. Blank line between these.

  16. 
      
RS
reviewbot
  1. 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
    
    
  2.  'INVALID_FORM_DATA' imported but unused
    
  3. Col: 80
     E501 line too long (80 > 79 characters)
    
  4. Col: 9
     E303 too many blank lines (2)
    
  5. Col: 13
     E116 unexpected indentation (comment)
    
  6. Col: 13
     E265 block comment should start with '# '
    
  7. Col: 13
     E116 unexpected indentation (comment)
    
  8. Col: 13
     E116 unexpected indentation (comment)
    
  9. Col: 63
     E127 continuation line over-indented for visual indent
    
  10. Col: 80
     E501 line too long (80 > 79 characters)
    
  11. 
      
RS
reviewbot
  1. 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
    
    
  2.  undefined name 'DOES_NOT_EXIST'
    
  3. Col: 80
     E501 line too long (80 > 79 characters)
    
  4. 
      
brennie
  1. 
      
  2. reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 8)
     
     
     
     
     
     
     
     
     
     
     
     

    These can probably all be condensed into one test.

  3. 
      
RS
reviewbot
  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: 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
    
    
  2.  undefined name 'DOES_NOT_EXIST'
    
  3. Col: 80
     E501 line too long (82 > 79 characters)
    
  4. Col: 80
     E501 line too long (85 > 79 characters)
    
  5. Col: 17
     E126 continuation line over-indented for hanging indent
    
  6. reviewboard/webapi/tests/urls.py (Diff revision 9)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  7. reviewboard/webapi/tests/urls.py (Diff revision 9)
     
     
     undefined name 'review_request'
    
  8. 
      
RS
RS
RS
reviewbot
  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: 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
    
    
  2.  undefined name 'DOES_NOT_EXIST'
    
  3. Col: 80
     E501 line too long (82 > 79 characters)
    
  4. Col: 80
     E501 line too long (85 > 79 characters)
    
  5. Col: 47
     E128 continuation line under-indented for visual indent
    
  6. Col: 17
     E126 continuation line over-indented for hanging indent
    
  7. Col: 17
     E126 continuation line over-indented for hanging indent
    
  8. reviewboard/webapi/tests/urls.py (Diff revision 11)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  9. 
      
reviewbot
  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
    
    
  2.  undefined name 'DOES_NOT_EXIST'
    
  3. Col: 80
     E501 line too long (82 > 79 characters)
    
  4. Col: 80
     E501 line too long (85 > 79 characters)
    
  5.  undefined name 'review'
    
  6.  undefined name 'review'
    
  7. Col: 47
     E128 continuation line under-indented for visual indent
    
  8. Col: 17
     E126 continuation line over-indented for hanging indent
    
  9. Col: 17
     E126 continuation line over-indented for hanging indent
    
  10. reviewboard/webapi/tests/urls.py (Diff revision 10)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  11. Col: 80
     E501 line too long (88 > 79 characters)
    
  12. Col: 80
     E501 line too long (82 > 79 characters)
    
  13. Col: 80
     E501 line too long (85 > 79 characters)
    
  14. Col: 47
     E128 continuation line under-indented for visual indent
    
  15. Col: 17
     E126 continuation line over-indented for hanging indent
    
  16. Col: 17
     E126 continuation line over-indented for hanging indent
    
  17. reviewboard/webapi/tests/urls.py (Diff revision 12)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  18. 
      
RS
reviewbot
  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: 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
    
    
  2. Col: 80
     E501 line too long (88 > 79 characters)
    
  3. Col: 80
     E501 line too long (82 > 79 characters)
    
  4. Col: 80
     E501 line too long (85 > 79 characters)
    
  5. Col: 47
     E128 continuation line under-indented for visual indent
    
  6. Col: 17
     E126 continuation line over-indented for hanging indent
    
  7. Col: 17
     E126 continuation line over-indented for hanging indent
    
  8. reviewboard/webapi/tests/urls.py (Diff revision 13)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  9. 
      
RS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/tests/test_review_request_last_update.py
        reviewboard/webapi/resources/review_request_last_update.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
    
    Ignored Files:
        reviewboard/static/rb/js/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
    
    
  2.  'datetime' imported but unused
    
  3. Col: 80
     E501 line too long (82 > 79 characters)
    
  4. Col: 80
     E501 line too long (85 > 79 characters)
    
  5. Col: 17
     E126 continuation line over-indented for hanging indent
    
  6. Col: 37
     E128 continuation line under-indented for visual indent
    
  7. Col: 42
     E128 continuation line under-indented for visual indent
    
  8. Col: 27
     E201 whitespace after '{'
    
  9. Col: 32
     E127 continuation line over-indented for visual indent
    
  10. Col: 33
     E201 whitespace after '{'
    
  11. Col: 27
     E201 whitespace after '{'
    
  12. Col: 32
     E127 continuation line over-indented for visual indent
    
  13. Col: 33
     E201 whitespace after '{'
    
  14. reviewboard/webapi/tests/urls.py (Diff revision 14)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  15. 
      
RS
RS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/tests/test_review_request_last_update.py
        reviewboard/webapi/resources/review_request_last_update.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
    
    Ignored Files:
        reviewboard/static/rb/js/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
    
    
  2.  'datetime' imported but unused
    
  3. Col: 80
     E501 line too long (82 > 79 characters)
    
  4. Col: 80
     E501 line too long (85 > 79 characters)
    
  5. Col: 80
     E501 line too long (81 > 79 characters)
    
  6. Col: 17
     E126 continuation line over-indented for hanging indent
    
  7. Col: 26
     E126 continuation line over-indented for hanging indent
    
  8. Col: 17
     E126 continuation line over-indented for hanging indent
    
  9. Col: 37
     E128 continuation line under-indented for visual indent
    
  10. Col: 42
     E128 continuation line under-indented for visual indent
    
  11. Col: 27
     E201 whitespace after '{'
    
  12. Col: 32
     E127 continuation line over-indented for visual indent
    
  13. Col: 33
     E201 whitespace after '{'
    
  14. Col: 27
     E201 whitespace after '{'
    
  15. Col: 32
     E127 continuation line over-indented for visual indent
    
  16. Col: 33
     E201 whitespace after '{'
    
  17. reviewboard/webapi/tests/urls.py (Diff revision 15)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  18. 
      
RS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/tests/test_review_request_last_update.py
        reviewboard/webapi/resources/review_request_last_update.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
    
    Ignored Files:
        reviewboard/static/rb/js/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
    
    
  2. Col: 80
     E501 line too long (82 > 79 characters)
    
  3. Col: 80
     E501 line too long (85 > 79 characters)
    
  4. Col: 80
     E501 line too long (83 > 79 characters)
    
  5. Col: 17
     E126 continuation line over-indented for hanging indent
    
  6. Col: 26
     E126 continuation line over-indented for hanging indent
    
  7. Col: 26
     E126 continuation line over-indented for hanging indent
    
  8. 
      
brennie
  1. 
      
  2. reviewboard/testing/testcase.py (Diff revision 16)
     
     
     
     
     
     

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

    2. Yes.

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

  4. This should actually be:

    ['reviewboard.webapi.resources.diff.DiffResource']
    
  5. Same goes for here with the appropriate resource

  6. Same goes for here with the appropriate resource

  7. Same goes for here with the appropriate resource

  8. Same goes for here with the appropriate resource

  9. Constant names should be ALL_CAPS.

    Also, can you format this as:

    COMMENT_TYPES = (
        Comment,
        # ...
    )
    
  10. 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.
    
  11. This documentation needs to go into the docs for the timestamp field.

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

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

    2. Alright, I missed that. Disregard this, then.

  13. Needs file docstring.

  14. Blank line between these.

  15. Blank line between these.

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

    Docstring.

    1. I'll add a docstring, but I noticed that every other method in here doesn't seem to have one.

    2. Still, doesn't hurt :)

  17. 
      
LA
  1. 
      
  2. 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?

    1. 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!

  3. 
      
RS
reviewbot
  1. 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
    
    
  2. Col: 21
     E128 continuation line under-indented for visual indent
    
  3. Col: 5
     E124 closing bracket does not match visual indentation
    
  4. Col: 80
     E501 line too long (82 > 79 characters)
    
  5. Col: 80
     E501 line too long (85 > 79 characters)
    
  6. Col: 80
     E501 line too long (83 > 79 characters)
    
  7. Col: 26
     E126 continuation line over-indented for hanging indent
    
  8. Col: 9
     E124 closing bracket does not match visual indentation
    
  9. Col: 26
     E126 continuation line over-indented for hanging indent
    
  10. Col: 9
     E124 closing bracket does not match visual indentation
    
  11. 
      
FI
  1. 
      
  2. reviewboard/webapi/tests/urls.py (Diff revision 17)
     
     

    This line should be empty

  3. 
      
david
  1. 
      
  2. Let's leave the comment, please.

  3. Even though the line would go over 80 columns, let's keep this in one piece.

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

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

  5. 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:
            ...
    
  6. We shouldn't need to convert to list here.

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

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

  8. Same here re list of .... This also doesn't need the parens.

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

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

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

    Blank line in between here.

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

    Should be formatted as:

    Returns:
        unicode:
        The resulting absolute URL to teh item resource.
    
  12. 
      
RS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/webapi/tests/test_review_request_last_update.py
        reviewboard/webapi/resources/review_request_last_update.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/mimetypes.py
    
    Ignored Files:
        reviewboard/static/rb/js/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
    
    
  2. Col: 46
     E127 continuation line over-indented for visual indent
    
  3. Col: 46
     E127 continuation line over-indented for visual indent
    
  4.  local variable 'comment_types' is assigned to but never used
    
  5. Col: 9
     E124 closing bracket does not match visual indentation
    
  6. Col: 80
     E501 line too long (82 > 79 characters)
    
  7. Col: 80
     E501 line too long (85 > 79 characters)
    
  8. Col: 80
     E501 line too long (83 > 79 characters)
    
  9. Col: 26
     E126 continuation line over-indented for hanging indent
    
  10. Col: 9
     E124 closing bracket does not match visual indentation
    
  11. Col: 26
     E126 continuation line over-indented for hanging indent
    
  12. Col: 9
     E124 closing bracket does not match visual indentation
    
  13. 
      
brennie
  1. 
      
  2. Blank line between these.

  3. This should be the resource, e.g. ['reviewboard.webapi.resources.diff.DiffResource'].

  4. This should be the resource, e.g. ['reviewboard.webapi.resources.review.ReviewResource'].

  5. This should be the resource, e.g. ['reviewboard.webapi.resources.review_reply.ReviewReplyResource'].

  6. This should be the resource, e.g. ['reviewboard.webapi.resources.base_comment.BaseCommentResource'].

  7. This should be the resource, e.g. ['reviewboard.webapi.resources.change.ChangeResource'].

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

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

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

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

  11. 
      
david
Review request changed

Status: Discarded

Loading...