• 
      

    Review board returns informative error message when a review is posted with a duplicate commit id.

    Review Request #5815 — Created May 13, 2014 and submitted

    Information

    Review Board
    master
    f7060b2...

    Reviewers

    The reproduction steps described in bug 3328 could not be reproduced.
    When following the steps, I would receive a HTTP 500 error when
    making the second post. This is a result of Django not being used
    previously to validate the data. Instead it was being left to the
    database, which is checked and handled differently depending on
    which database is used and it's configuration. To fix this creation
    was changed to instantiation to prevent partial saving, a validation
    step was added prior to saving, a new webapi error was added, and
    the error code was documented in the manual.

    I tested to ensure when making the second rbt post that the error was
    caught and an informative error message was displayed, rather than an
    HTTP 500 error. I also checked after the failure to ensure that a
    partial review object was not created. I also tested to ensure that
    posts could still be created if they had a unique commit id.

    Description From Last Updated

    This line is too long now (we wrap to 80 columns). You can put parens around it and move the …

    daviddavid

    Can you keep these in alphabetical order?

    daviddavid
    MA
    1. 
        
    2. reviewboard/reviews/managers.py (Diff revision 1)
       
       

      Why do we create the object here, add a commit_id, and then update it with the id? This results in a review being created and saved before we validate the commit id. As such, previously and in this version, a review post with only basic information persists after the error.

      1. We need an object to work with, and so we create it first. Otherwise, there's nothing to call update_from_commit_id on.

        We can probably just change this to construct a new ReviewRequest, populate everything, and then save.

    3. 
        
    MA
    david
    1. 
        
    2. Show all issues

      This line is too long now (we wrap to 80 columns). You can put parens around it and move the ValidationError to the next line.

    3. Show all issues

      Can you keep these in alphabetical order?

    4. 
        
    MA
    reviewbot
    1. This is a review from Review Bot.
        Tool: PEP8 Style Checker
        Processed Files:
          reviewboard/webapi/errors.py
          reviewboard/webapi/resources/review_request.py
          reviewboard/reviews/managers.py
        Ignored Files:
          docs/manual/webapi/2.0/errors/227-commit-id-already-exists.rst
          docs/manual/webapi/2.0/errors/index.rst
      
      
    2. 
        
    reviewbot
    1. This is a review from Review Bot.
        Tool: Pyflakes
        Processed Files:
          reviewboard/webapi/errors.py
          reviewboard/webapi/resources/review_request.py
          reviewboard/reviews/managers.py
        Ignored Files:
          docs/manual/webapi/2.0/errors/227-commit-id-already-exists.rst
          docs/manual/webapi/2.0/errors/index.rst
      
      
    2. 
        
    david
    1. Ship It!

    2. 
        
    MA
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-2.0.x (eec377b)