Skip review requests for inactive reviewer accounts

What does this MR do and why?

The platform suggested three reviewers whose GitLab accounts had been blocked after they left the company. It posted /request_review for all three on my behalf, so the request reached nobody, and the merge request sat idle for five weeks until a team member noticed and replaced them by hand.

Nothing checked that a requested account was still active at the moment the review was requested. This MR adds that check on the server, so no /request_review is posted for an account that cannot receive it.

Gitlab::Rest::Users gains a user_exists? predicate over the existing get_user, which already returns nil for an account that no longer resolves. Api::V1::ManageMergeRequestController#request_review resolves each requested reviewer through it before building the mention list and drops the ones that no longer resolve. If none of them do, it returns 422 with an error message rather than opening a thread that mentions nobody, so the request fails visibly instead of looking like success. A GitLab Duo request on its own still goes through, because Duo is mentioned independently of the reviewer list.

The username sanitiser that was inline in user_mentions is now a sanitize_username method in the same concern, so the existence check runs against the same sanitised name that ends up in the mention. One side effect of that ordering is that reviewers now de-duplicate after sanitising rather than before, so ['@coach', 'coach'] produces one mention instead of two.

Closes #834 (moved)

Design and production fit

The problem is that the suggestion pool is a cached mirror of a third-party feed. SuggestedReviewerService#execute picks from TeamMember.available, which Users::GrabTeamMembersService rebuilds from the roulette feed when the rake task in lib/tasks/data.rake runs. A departure only reaches the platform once the feed drops the person and that task runs again, and the feed has since corrected itself for these three. The gap is the window before it does.

The issue weighed two places for the fix. SuggestedReviewerService is narrower and keeps a stale name off the screen, but it only covers the suggested path. request_review covers whichever path produced the name, including a reviewer typed into the field, and lets the UI say why the request did not go through. I took the controller because failing closed at the point of posting is the guarantee that actually matters, but the location is open to a steer. The two are not exclusive, and filtering the suggestion as well would be a reasonable follow-up.

In production this adds one GET /users?username= per requested reviewer, which is one or two calls in practice. The check runs on an explicit user action, not a hot path. There are no new database queries.

The behaviour on an API failure is inherited from get_user, which is how Users::FindOrCreateService already consumes this endpoint, rather than being special-cased here.

Screenshots or screen recordings

Not applicable: this is a server-side change with no UI of its own. The observable difference is the content of the thread the platform posts, which the request specs assert directly.

While tracing the journey I noticed that RequestReviewAction.vue does not wrap its axios.post in a try/catch, so a non-2xx response rejects the promise and leaves loading stuck true. That affects the existing head :internal_server_error path identically and is not introduced here, so I have left it out of this MR rather than widening the scope. It is worth its own issue, and I am happy to raise one.

Validation steps

The pipeline is green. I could not run RSpec locally, so the first run caught two things I had got wrong and the branch now carries their fixes: the specs stubbed user_exists? in their own before block, which runs after the one that already posts the request, so every new example was asserting against a thread built with the default stub; and :unprocessable_entity is no longer a name Rack knows under this Rails version, so it is :unprocessable_content.

New coverage in spec/requests/api/v1/manage_merge_request_controller_spec.rb:

  • a reviewer who no longer resolves is dropped while an active co-reviewer is still mentioned and still gets /request_review
  • when no requested reviewer resolves, the endpoint returns 422 and no thread is started
  • when no requested reviewer resolves but GitLab Duo was also requested, the thread is still started for Duo

New coverage in spec/services/gitlab_rest_service_spec.rb for #user_exists?, both cases backed by the same recorded responses the existing #get_user examples use.

The GitlabRestService double now answers user_exists? from a blocked_usernames list that defaults to empty, so the existing examples are unaffected and a nested context can change the outcome of the request the shared before block makes.

The generated OpenAPI document and TypeScript response type gained the 422 case, taken verbatim from the patch the api-contracts job publishes.

To check by hand against a live instance: request a review naming a blocked or non-existent account and confirm no thread is posted and the response is 422; then name it alongside an active account and confirm only the active one is mentioned.

/cc @leetickett-gitlab

Edited by Jeston Singh

Merge request reports

Loading
Loading