Prevent publishing of drafts with same commits
Review Request #7011 — Created March 5, 2015 and submitted
Before support for commit histories was added, it was sufficient to
check if a change description had any modified fields to determine if
an actual change was present. However, theDiffFieldis marked as
changed if there is a new diff available. In the case of a squashed
review request, this is sufficient because the API will not allow you
to upload an identical diff. However, with multiple commit review
requests, where each commit is uploaded individually, it becomes more
difficult because we do not know the number of commits before
publishing is attempted.To address this issue, we now determine if only the
DiffFieldhas
been marked as changed and currentDiffSethas diff commits. In this
case, we know that theDiffCommits haven't changed and therefore the
publish should not be allowed.Added a unit test to capture this behaviour.
We now also use fewer database queries in the case where the history
was previously squashed and no longer is.
Ran units tests.
Created a review request with commit history. Upadated the review
request with the same set of commits and tried to publish the review
request draft, but was not able to.The provided unit test fails without the patch and passes with it.
-
Tool: Pyflakes Processed Files: reviewboard/reviews/builtin_fields.py reviewboard/reviews/models/review_request_draft.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/builtin_fields.py reviewboard/reviews/models/review_request_draft.py
- Change Summary:
-
Only save diffset history if we are actually publishing the draft.
- Commit:
-
fd3793ba26367a528077bf938512e80710e03a81
-
Tool: Pyflakes Processed Files: reviewboard/reviews/builtin_fields.py reviewboard/reviews/models/review_request_draft.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/builtin_fields.py reviewboard/reviews/models/review_request_draft.py
- Change Summary:
-
Add unit test.
- Description:
-
Before support for commit histories was added, it was sufficient to
check if a change description had any modified fields to determine if an actual change was present. However, the DiffFieldis marked aschanged if there is a new diff available. In the case of a squashed review request, this is sufficient because the API will not allow you to upload an identical diff. However, with multiple commit review requests, where each commit is uploaded individually, it becomes more difficult because we do not know the number of commits before publishing is attempted. To address this issue, we now determine if only the
DiffFieldhasbeen marked as changed and current DiffSethas diff commits. In thiscase, we know that the DiffCommits haven't changed and therefore thepublish should not be allowed. + Add a unit test to capture this behaviour.
+ We now also use less database queries in the case where the history
was previously squashed and no longer is. - Testing Done:
-
Ran units tests.
Created a review request with commit history. Upadated the review
request with the same set of commits and tried to publish the review request draft, but was not able to. + + The provided unit test fails without the patch and passes with it.
- Commit:
-
fd3793ba26367a528077bf938512e80710e03a81b6891f5ceac0503a24422e7208238eb660512061
-
Tool: Pyflakes Processed Files: reviewboard/reviews/builtin_fields.py reviewboard/reviews/tests.py reviewboard/reviews/models/review_request_draft.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/builtin_fields.py reviewboard/reviews/tests.py reviewboard/reviews/models/review_request_draft.py -
- Change Summary:
-
PEP8
- Commit:
-
b6891f5ceac0503a24422e7208238eb6605120613d5b604e79a3ed523fedca0d9f09a432c21a8eb9
-
Tool: Pyflakes Processed Files: reviewboard/reviews/builtin_fields.py reviewboard/reviews/tests.py reviewboard/reviews/models/review_request_draft.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/builtin_fields.py reviewboard/reviews/tests.py reviewboard/reviews/models/review_request_draft.py -
- Change Summary:
-
Also PEP8
- Commit:
-
3d5b604e79a3ed523fedca0d9f09a432c21a8eb95b8c8634928751ffae32a00d40cb17a7b67c76b1
-
Tool: Pyflakes Processed Files: reviewboard/reviews/builtin_fields.py reviewboard/reviews/tests.py reviewboard/reviews/models/review_request_draft.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/builtin_fields.py reviewboard/reviews/tests.py reviewboard/reviews/models/review_request_draft.py
- Description:
-
Before support for commit histories was added, it was sufficient to
check if a change description had any modified fields to determine if an actual change was present. However, the DiffFieldis marked aschanged if there is a new diff available. In the case of a squashed review request, this is sufficient because the API will not allow you to upload an identical diff. However, with multiple commit review requests, where each commit is uploaded individually, it becomes more difficult because we do not know the number of commits before publishing is attempted. To address this issue, we now determine if only the
DiffFieldhasbeen marked as changed and current DiffSethas diff commits. In thiscase, we know that the DiffCommits haven't changed and therefore thepublish should not be allowed. ~ Add a unit test to capture this behaviour.
~ Added a unit test to capture this behaviour.
We now also use less database queries in the case where the history
was previously squashed and no longer is.
-
Oh, one little nit in your description:
"We now also use less database queries" -> should be "fewer database queries"
- Change Summary:
-
Grammar.
- Description:
-
Before support for commit histories was added, it was sufficient to
check if a change description had any modified fields to determine if an actual change was present. However, the DiffFieldis marked aschanged if there is a new diff available. In the case of a squashed review request, this is sufficient because the API will not allow you to upload an identical diff. However, with multiple commit review requests, where each commit is uploaded individually, it becomes more difficult because we do not know the number of commits before publishing is attempted. To address this issue, we now determine if only the
DiffFieldhasbeen marked as changed and current DiffSethas diff commits. In thiscase, we know that the DiffCommits haven't changed and therefore thepublish should not be allowed. Added a unit test to capture this behaviour.
~ We now also use less database queries in the case where the history
~ We now also use fewer database queries in the case where the history
was previously squashed and no longer is.
Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/builtin_fields.py reviewboard/reviews/models/review_request_draft.py Tool: Pyflakes Processed Files: reviewboard/reviews/builtin_fields.py reviewboard/reviews/models/review_request_draft.py