Add a link to the latest diff revision in the review request resource

Review Request #8270 — Created July 6, 2016 and submitted

brennie
Review Board
release-2.5.x
9c5f21c...
reviewboard

The review request resource now links to the latest diff (as
latest_diff in the links section of the response) when the
associated review request has diffs.

Ran unit tests.

Description From Last Updated

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

reviewbotreviewbot

A useful method, but this is going to hit the database per item. Instead, I'd suggest updating get_queryset to do ...

chipx86chipx86

We should also do a self.assertNumQueries with this. I'd expect the value to be (old value) + 1, with the ...

chipx86chipx86

Too many blank lines.

chipx86chipx86

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

reviewbotreviewbot

Just to verify, this isn't doing any extra queryies, because of the prefetch_related, right? Can we add a comment here ...

chipx86chipx86

Can we put request on its own line? In this particular case, it helps the argument from getting a bit ...

chipx86chipx86

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

reviewbotreviewbot
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/tests/test_review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/tests/test_review_request.py
    
    
  2. Col: 80
     E501 line too long (95 > 79 characters)
    
  3. 
      
chipx86
  1. 
      
  2. A useful method, but this is going to hit the database per item. Instead, I'd suggest updating get_queryset to do a prefetch_related for DiffSet. (I wish we had the newer Prefetch stuff to limit fields and be smarter about queries, but alas). I think this would have to look something like:

    queryset = queryset.select_related('diffset_history').prefetch_related('diffsets')
    

    Then instead of get_latest_diffset, we'd need to grab the last diff in review_request.diffset_history.diffsets.all().

    Something like that.

  3. We should also do a self.assertNumQueries with this. I'd expect the value to be (old value) + 1, with the above query.

  4. reviewboard/webapi/tests/test_review_request.py (Diff revision 1)
     
     
     
     
     
     

    Too many blank lines.

  5. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/tests/test_review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/tests/test_review_request.py
    
    
  2. Col: 80
     E501 line too long (95 > 79 characters)
    
  3. 
      
chipx86
  1. 
      
  2. reviewboard/webapi/resources/review_request.py (Diff revision 2)
     
     
     
     
     

    Just to verify, this isn't doing any extra queryies, because of the prefetch_related, right? Can we add a comment here just mentioning that we already have this data and it's not a new query?

    Reason being that otherwise, diffsets.latest().only('revision') would be a better query, if we didn't already have the data.

  3. Can we put request on its own line? In this particular case, it helps the argument from getting a bit lost in the other noise.

  4. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/tests/test_review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/tests/test_review_request.py
    
    
  2. Col: 80
     E501 line too long (95 > 79 characters)
    
  3. 
      
david
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (6e19d96)
Loading...