Summary

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 May 23, 2014, 3:06 p.m.

Information

Owner

matthewcmaclean

Repository

Review Board

Branch

master

Bugs

3329

Depends On

Commit

f7060b2...

Reviewers

Groups

reviewboard, students

People

Description

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.

Testing Done

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.

Issues

Description	From	Last Updated
This line is too long now (we wrap to 80 columns). You can put parens around it and move the …	david	May 23, 2014, 2:03 p.m.
Can you keep these in alphabetical order?	david	May 23, 2014, 2:03 p.m.

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.

chipx86 May 13, 2014, 11:26 a.m.

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.

Change Summary:

Changed creation of ReviewRequest from create to model so that on failure, a partial ReviewRequest does not exist on the server.

Description:

		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 a
~		validation step was added prior to saving, a new webapi error was
~		added, and the error code was documented in the manual.
	~	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.

Testing Done:

		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 tested to ensure that posts could still be made
~		if they had a unique commit id.
	~	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.

Commit:

-	8b6dcfa681eb8793eed990295148685122229653
+	fdc6782392f9b4c5a06a14212228405767f5b51f

Diff:

Revision 2 (+19 -3)

Show changes

	docs/manual/webapi/2.0/errors/227-commit-id-already-exists.rst
	docs/manual/webapi/2.0/errors/index.rst
	reviewboard/reviews/managers.py
	reviewboard/webapi/errors.py
	reviewboard/webapi/resources/review_request.py

reviewboard/webapi/resources/review_request.py (Diff revision 2)

The issue has been resolved. 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.

reviewboard/webapi/resources/review_request.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can you keep these in alphabetical order?
```

Commit:

-	fdc6782392f9b4c5a06a14212228405767f5b51f
+	f7060b2ca0745ba371cc01cf9f78b27040a19169

Diff:

Revision 3 (+20 -2)

Show changes

	docs/manual/webapi/2.0/errors/227-commit-id-already-exists.rst
	docs/manual/webapi/2.0/errors/index.rst
	reviewboard/reviews/managers.py
	reviewboard/webapi/errors.py
	reviewboard/webapi/resources/review_request.py

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

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

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (eec377b)