Account Options

  1. Sign in
The old Google Groups will be going away soon, but your browser is incompatible with the new version.
Google Groups Home
« Groups Home
Review Board Reviews
Home
About this group
Join this group
Message from discussion Review Request 3401: URL to diff comments returns 404 if at least one of diff-id and file-id are invalid.
The group you are posting to is a Usenet group. Messages posted to this group will make your email address visible to anyone on the Internet.
Your reply message has not been sent.
Your post was successful
 
From:
To:
Cc:
Followup To:
Add Cc | Add Followup-to | Edit Subject
Subject:
Validation:
For verification purposes please type the characters you see in the picture below or the numbers you hear by clicking the accessibility icon. Listen and type the numbers you hear
 
Christian Hammond  
View profile  
 More options Oct 19 2012, 4:02 am
From: "Christian Hammond" <chip...@chipx86.com>
Date: Fri, 19 Oct 2012 08:02:00 -0000
Subject: Re: Review Request 3401: URL to diff comments returns 404 if at least one of diff-id and file-id are invalid.
Print | View thread | Show original | Report this message | Find messages by this author

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.reviewboard.org/r/3401/#review4398
-----------------------------------------------------------

So, I made some comments on style, since I think that'll be helpful, but I have a couple questions/comments about the change.

1) Does this do anything for accessing actual individual comments with fake diff revisions or review IDs? (Is that broken too?)

2) Can you go into more detail on the second part of the change involving the local IDs? This doesn't seem completely right to me...

3) There should be unit tests covering each failure case, and making sure the results are expected. These should fail before this change, and pass after.

reviewboard/webapi/resources.py
<http://reviews.reviewboard.org/r/3401/#comment7070>

    Needs to be < 80 columns.

reviewboard/webapi/resources.py
<http://reviews.reviewboard.org/r/3401/#comment7071>

    Put these in the parameter list to the function, defaulting to None.

reviewboard/webapi/resources.py
<http://reviews.reviewboard.org/r/3401/#comment7074>

    This is specific to FileDiffCommentResource, and so should live there.

reviewboard/webapi/resources.py
<http://reviews.reviewboard.org/r/3401/#comment7073>

    Is there a time when we would not have a review ID?

reviewboard/webapi/resources.py
<http://reviews.reviewboard.org/r/3401/#comment7069>

    Needs to be < 80 columns. Change *args, **kwargs to be on the next line.

reviewboard/webapi/resources.py
<http://reviews.reviewboard.org/r/3401/#comment7075>

    We should just reuse review_request_id.

reviewboard/webapi/resources.py
<http://reviews.reviewboard.org/r/3401/#comment7076>

    Blank line before this.

reviewboard/webapi/resources.py
<http://reviews.reviewboard.org/r/3401/#comment7077>

    Given the length of arguments and the wrapping, do one parameter per line, all aligned. That includes 'request' (on the next line).

- Christian Hammond

On Oct. 14, 2012, 7:19 p.m., Michelle Chuang wrote:

> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.reviewboard.org/r/3401/
> -----------------------------------------------------------

> (Updated Oct. 14, 2012, 7:19 p.m.)

> Review request for Review Board.

> Description
> -------

> - Fixed URL to return 404.
> - Changed to use pk when getting a FileDiffResource queryset instead of local id to pass unit test.

> This addresses bug 2552.
>     http://code.google.com/p/reviewboard/issues/detail?id=2552

> Diffs
> -----

>   reviewboard/webapi/resources.py 7b40f4ea1b7e23787922071271def03500f463cc

> Diff: http://reviews.reviewboard.org/r/3401/diff/

> Testing
> -------

> Tested on local development server against custom urls.

> Thanks,

> Michelle Chuang


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Create a group - Google Groups - Google Home - Terms of Service - Privacy Policy
©2013 Google