Move admin impersonation to feature_category :system_access

Eduardo Sanz Garcíarequested to merge
eduardosanz/impersonation-feature-category into master

What does this MR do and why?

Moves the admin impersonation flow to feature_category :system_access, so the controller and all of its specs agree on a single owner.

File Before After
app/controllers/admin/impersonations_controller.rb :user_management :system_access
spec/controllers/admin/impersonations_controller_spec.rb (none) :system_access
spec/features/admin/users/admin_impersonates_user_spec.rb :user_management :system_access
ee/spec/controllers/admin/impersonations_controller_spec.rb :system_access unchanged

It also removes the now-stale spec/controllers/admin/impersonations_controller_spec.rb entry from .rubocop_todo/rspec/feature_category.yml, since that spec no longer needs the exclusion.

Where the inconsistency came from

The split dates back to the 2023 breakup of the authentication_and_authorization category, where the two halves of the same controller landed on opposite sides:

  • the EE spec moved to system_access in 0f23fac1, an Auth-team pass over auth specs
  • the controller moved to user_management in e0836f2d, a bulk automated remap

Neither looks like a deliberate decision about impersonation specifically.

Why :system_access

Per the feature categorization docs, a spec's feature_category is what routes flaky-test issues to the owning group. The flakiness in these specs comes from a session rotation clobbering the session cookie (#607459), which is groupauthentication work, so routing them there puts the failures in front of the group that can fix them. Aligning the controller keeps request, metrics and error attribution consistent with the specs rather than having code and tests disagree.

Note for groupseat management

:user_management is a Fulfillment category, so this MR moves the Admin::ImpersonationsController endpoint's feature-category attribution (request logs, metrics, error tracking) from groupseat management to groupauthentication. Flagging that explicitly rather than letting it change quietly. Happy to drop the controller hunk and keep this test-only if you would rather discuss the endpoint's ownership separately.

References

How to set up and validate locally

There is no user-visible change: this MR only moves feature category metadata. Validate by confirming the categories are consistent and the specs still pass.

  1. Confirm every impersonation spec and the controller now report :system_access:

    grep -rn "feature_category" \
  app/controllers/admin/impersonations_controller.rb \
  spec/controllers/admin/impersonations_controller_spec.rb \
  spec/features/admin/users/admin_impersonates_user_spec.rb \
  ee/spec/controllers/admin/impersonations_controller_spec.rb

  2. Run the affected specs, plus the suite-wide guard that every controller action resolves to a valid feature category:

    bundle exec rspec \
  spec/controllers/admin/impersonations_controller_spec.rb \
  ee/spec/controllers/admin/impersonations_controller_spec.rb \
  spec/controllers/every_controller_spec.rb
bundle exec rspec spec/features/admin/users/admin_impersonates_user_spec.rb

Local results on this branch, run sequentially:

Check Result
rubocop on the 3 changed Ruby files 3 files inspected, no offenses detected
spec/controllers/admin/impersonations_controller_spec.rb 16 examples, 0 failures
ee/spec/controllers/admin/impersonations_controller_spec.rb 1 example, 0 failures
spec/controllers/every_controller_spec.rb 4 examples, 0 failures
spec/features/admin/users/admin_impersonates_user_spec.rb (:js) 18 examples, 0 failures

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Merge request reports