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.

  • 30
  • 0
  • 110
  • 48
  • 188
Description From Last Updated
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 brennie
Col: 21 E128 continuation line under-indented for visual indent reviewbot
Col: 5 E124 closing bracket does not match visual indentation reviewbot
Col: 80 E501 line too long (82 > 79 characters) reviewbot
Col: 80 E501 line too long (85 > 79 characters) reviewbot
Col: 80 E501 line too long (83 > 79 characters) reviewbot
Col: 26 E126 continuation line over-indented for hanging indent reviewbot
Col: 9 E124 closing bracket does not match visual indentation reviewbot
Col: 26 E126 continuation line over-indented for hanging indent reviewbot
Col: 9 E124 closing bracket does not match visual indentation reviewbot
Blank line between these. brennie brennie
This should be the resource, e.g. ['reviewboard.webapi.resources.diff.DiffResource']. brennie brennie
This should be the resource, e.g. ['reviewboard.webapi.resources.review.ReviewResource']. brennie brennie
This should be the resource, e.g. ['reviewboard.webapi.resources.review_reply.ReviewReplyResource']. brennie brennie
This should be the resource, e.g. ['reviewboard.webapi.resources.base_comment.BaseCommentResource']. brennie brennie
This should be the resource, e.g. ['reviewboard.webapi.resources.change.ChangeResource']. brennie brennie
You can refactor this to remove code duplication. try: timestamp = datetime.strptime(timestamp, ...).replace(tzinfo=utc) except ValueError: timestamp = None if timestamp ... brennie brennie
Col: 46 E127 continuation line over-indented for visual indent reviewbot
Col: 46 E127 continuation line over-indented for visual indent reviewbot
These keys are common between all responses, so you can further refactor the code to do: response_keys = { 'timestamp': ... brennie brennie
local variable 'comment_types' is assigned to but never used reviewbot
Make this a class level constant, e.g., self.COMMENT_TYPES. brennie brennie
Col: 9 E124 closing bracket does not match visual indentation reviewbot
Col: 80 E501 line too long (82 > 79 characters) reviewbot
Col: 80 E501 line too long (85 > 79 characters) reviewbot
Col: 80 E501 line too long (83 > 79 characters) reviewbot
Col: 26 E126 continuation line over-indented for hanging indent reviewbot
Col: 9 E124 closing bracket does not match visual indentation reviewbot
Col: 26 E126 continuation line over-indented for hanging indent reviewbot
Col: 9 E124 closing bracket does not match visual indentation 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. 
      
  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. 
      
  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. 
      
  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. 
      
  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. 
      
  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 :)

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