Follow-up from "Resolve admin plan limits API plans through SystemDefined::Plan"

Everyone can contribute. Help move this issue forward while earning points, leveling up and collecting rewards.

The following discussions from !257670 (merged) should be addressed:

  • @marcogreg started a discussion:

    nitpick (non-blocking): After this change, Admin::PlansFinder has no callers. Please remove it in this MR, or in a follow-up MR. Which do you prefer? If you choose a follow-up MR, please note it on #596997 (closed).

    Why
    • The current_plan helper was the only caller of the finder.
    • A grep of the repo finds only the class, its spec, and allowlist and todo entries.
    • The finder was added together with this API endpoint.
    • Phase 2, item 4 of #596997 (closed) says that after this switch "the finder can go".
    • Files to remove:
      • app/finders/admin/plans_finder.rb
      • spec/finders/admin/plans_finder_spec.rb
      • The entries for the finder or its spec in spec/support/finder_collection_allowlist.yml, .rubocop_todo/gitlab/bounded_contexts.yml, .rubocop_todo/rspec/instance_variable.yml, and .rubocop_todo/style/inline_disable_annotation.yml

    🤖

  • @marcogreg started a discussion:

    nitpick (non-blocking): The new "no plans row" contexts use plan_name: 'premium', so they need if: Gitlab.ee?. Please delete the default plans row inside the example and request default instead. Then you can remove the guard, and both editions run the test. Here is the change for the GET context. Use the same idea for the PUT context at line 227, with the put call and the not_to change { PlanLimits.count } expectation.

    context 'when the plan has no plans row' do
  before do
    Plan.by_name(Plan::DEFAULT).delete_all
  end

  it 'returns not found', :aggregate_failures do
    get api(path, admin, admin_mode: true), params: { plan_name: 'default' }

    expect(response).to have_gitlab_http_status(:not_found)
    expect(json_response['message']).to eq('404 Plan Not Found')
  end
end
    Why
    • In CE, premium is not in all_plans. The request returns 400, so the example cannot pass there. This is why the guard is needed now.
    • The 404 branch is CE code: not_found!('Plan') unless plan && ::Plan.by_plan_name_uid(plan.id).exists?. With the guard, the as-if-foss run never tests this branch.
    • The backend EE guideline says: do not add EE-only examples to existing CE spec files.
    • Use delete_all on a relation. Do not use plan.delete. The plan record is a let_it_be record that all examples share. delete marks that shared Ruby object as destroyed and frozen.
    • The example transaction restores the row after each example. Both foreign keys to plans are ON DELETE CASCADE, so the delete cannot fail on a constraint.
    • Nothing in the plan limits request path calls Plan.default. That method would create the row again. So the request reaches the 404 branch.

    🤖

Edited by 🤖 GitLab Bot 🤖