Submitting a review that assigns an already assigned user fails part way through
Summary
If you submit a merge request review and one of your comments assigns the request to an already assigned user, it will submit the previous comments in order before failing. When you submit again, comments are duplicated.
Steps to reproduce
- Open a merge request in any project on gitlab.com
- Assign the merge request to someone (probably yourself so you don't spam someone else)
- Make a comment anywhere in the code and add it to your review
- Make a second comment that also assigns the merge to the same person who is already assigned
- Make a third comment somewhere else in the code
- Submit the review
Example Project
alexives/serverless_with_cloudwatch!1 (merged) is currently in this state.
What is the current bug behavior?
When the review is submitted, the first two comments are added, but because the second comment fails to assign it aborts before adding the third comment and does not discard the draft. You can keep resubmitting without knowing whats going on and it will only apply the first two comments. It might not otherwise be clear that those comments were already submitted and a user might just keep hitting submit spamming all the watchers.
What is the expected correct behavior?
I would have expected it to just skip the assignment if it was already assigned to that person. But at the very least it either should add all of the comments and get rid of the draft, or none of the comments and save it.
Relevant logs and/or screenshots
https://sentry.gitlab.net/gitlab/gitlabcom/issues/1206615/
PG::UniqueViolation: ERROR: duplicate key value violates unique constraint "index_merge_request_assignees_on_merge_request_id_and_user_id"
DETAIL: Key (merge_request_id, user_id)=(48294507, 2709171) already exists.
active_record/connection_adapters/postgresql_adapter.rb:611:in `exec_params'
@connection.exec_params(sql, type_casted_binds)
active_record/connection_adapters/postgresql_adapter.rb:611:in `block (2 levels) in exec_no_cache'
@connection.exec_params(sql, type_casted_binds)
active_support/dependencies/interlock.rb:48:in `block in permit_concurrent_loads'
yield
active_support/concurrency/share_lock.rb:187:in `yield_shares'
yield
active_support/dependencies/interlock.rb:47:in `permit_concurrent_loads'
@lock.yield_shares(compatible: [:load]) do
...
(253 additional frame(s) were not displayed)
ActiveRecord::RecordNotUnique: PG::UniqueViolation: ERROR: duplicate key value violates unique constraint "index_merge_request_assignees_on_merge_request_id_and_user_id"
DETAIL: Key (merge_request_id, user_id)=(48294507, 2709171) already exists.
: INSERT INTO "merge_request_assignees" ("user_id", "merge_request_id") VALUES (2709171, 48294507) RETURNING "id"
ActiveRecord::RecordNotUnique: PG::UniqueViolation: ERROR: duplicate key value violates unique constraint "index_merge_request_assignees_on_merge_request_id_and_user_id"
DETAIL: Key (merge_request_id, user_id)=(48294507, 2709171) already exists.
: INSERT INTO "merge_request_assignees" ("user_id", "merge_request_id") VALUES (2709171, 48294507) RETURNING "id"
Output of checks
Happens on gitlab.com
Possible fixes
I'd say checking to see if the issue is already assigned to that person and skipping the command is probably the best bet.