Skip to content

THREESCALE-9973: Bullet n plus 1 offences#4305

Open
madnialihussain wants to merge 10 commits into
masterfrom
bullet-n-plus-1-offences
Open

THREESCALE-9973: Bullet n plus 1 offences#4305
madnialihussain wants to merge 10 commits into
masterfrom
bullet-n-plus-1-offences

Conversation

@madnialihussain
Copy link
Copy Markdown
Contributor

@madnialihussain madnialihussain commented May 21, 2026

What this PR does / why we need it

This PR removes 7 n_plus_one_query safelist entries from config/environments/test.rb by fixing the underlying N+1 queries with proper eager loading.

The changes include:

  • Adding missing .includes
  • Making eager loading conditional when associations are only accessed in specific contexts
  • Decoupling single-record finders from collection-level includes to avoid AVOID eager loading warnings
  • Using conditional preloading for associations only needed for certain record types

Related issue: THREESCALE-9973


Changes by commit

1cf51db5f

Add conditional :service include to ApplicationsIndexPresenter (only when no service filter and multiservice) and add :service + user_account: [:admin_user] includes to Buyers::Applications::Bulk::BaseController.

Removes:

  • Cinstance :service

26e446521

Add conditional :user_account include to ApplicationsIndexPresenter (only when no buyer context). Add .with_account to Service#latest_applications to fix N+1 on service show page.

Removes:

  • Cinstance :user_account

b8f4015c3

Extend user_account include to user_account: [:admin_user] in ApplicationsIndexPresenter.

The view accesses account.admin_user via link_to_buyer_or_deleted.

Removes:

  • Account :admin_user

e8cf2b66b

Add conditional bought_account_plan and bought_account_contract: [:plan] includes to Buyers::AccountsIndexPresenter (only when account_plans_size > 1).

Removes:

  • Account :bought_account_plan
  • Account :bought_account_contract

66f91e0d3

Add .includes(:user) to proxy configs collection query and decouple single-record finder to avoid AVOID eager loading warnings on show action.

Removes:

  • ProxyConfig :user

8a12d0f73

Add .includes(message: { sender: [:admin_user] }) to inbox controller and .includes(:sender) to trash controller.

  • Inbox uses MessageRecipient (sender accessed through Message)
  • Trash uses Message directly

Removes:

  • Message :sender

1c76daf1a

Add .includes({ buyer_account: [:admin_user] }, :provider_account) to Finance::Provider::InvoicesIndexPresenter, .includes(:buyer_account, :provider_account) to Provider::Admin::Account::InvoicesController, and .includes(:provider_account) to developer portal InvoicesController.

Removes:

  • Invoice :buyer_account
  • Invoice :provider_account

4042a5f20

Use ActiveRecord::Associations::Preloader to conditionally preload :member_permissions only for member users in UsersIndexPresenter, avoiding AVOID eager loading warnings when all users are admins.

Removes:

  • User :member_permissions

7b6ec834e

Revert Account :admin_user, :bought_account_contract, and :bought_account_plan fixes and restore their safelist entries.

Bulk controllers (Buyers::Accounts::Bulk) share a single collection method across new and create actions with different eager loading requirements. Adding includes to the shared collection causes unused_eager_loading warnings on the action that doesn't need them. Fixing this cleanly would require action specific preloading, which adds too much complexity for this PR so I reverted it to keep the PR clean.

Restores:

  • Account :admin_user
  • Account :bought_account_contract
  • Account :bought_account_plan


@qltysh
Copy link
Copy Markdown

qltysh Bot commented May 21, 2026

❌ 3 blocking issues (4 total)

Tool Category Rule Count
reek Lint ApplicationsIndexPresenter#applications performs a nil-check 2
rubocop Lint Assignment Branch Condition size for applications is too high. [<2, 21, 5> 21.68/20] 1
qlty Structure Function with high complexity (count = 5): applications 1

Copy link
Copy Markdown
Contributor

@jlledom jlledom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All changes look good to me so far, but there are a lot of cukes failing because of the removed exceptions. Do you plan to fix them?

@jlledom
Copy link
Copy Markdown
Contributor

jlledom commented May 22, 2026

This comment is not needed anymore:

# probably should preload :service and :user_account

@madnialihussain madnialihussain force-pushed the bullet-n-plus-1-offences branch from ba9d0b4 to 34085b1 Compare May 22, 2026 14:11
@madnialihussain
Copy link
Copy Markdown
Contributor Author

madnialihussain commented May 22, 2026

All changes look good to me so far, but there are a lot of cukes failing because of the removed exceptions. Do you plan to fix them?

Thanks for the review. Yes, they're fixed in the latest push

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants