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
422and 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.