fix(fixes): replace N+1 getSuggestions loop with batch query

[mimchome]

SITES-45307

Summary

• Replace per-fix getSuggestions() calls with single getAllFixesWithSuggestionsByOpportunityId batch query (2-3 DB calls instead of N+1)
• Fixes without mapping table entries now return with empty suggestions instead of being silently dropped (58% of fixes on dev were affected)

Test plan

• Unit tests pass (new empty-array test + updated stubs)
• Integration tests pass (added FIX_2 junction seed record)
• codecov/patch green
• After deploy: sunstar.com opp 78c9837b returns 3 fixes (was 2)

[github-actions]

This PR will trigger a patch release when merged.

[MysticatBot-Dev]

Hey @mimchome,

Strengths

• N+1 elimination follows existing pattern (src/controllers/fixes.js:150-151): Aligns with getAllFixesWithSuggestionByCreatedAt already used on the fixCreatedDate path, reducing the number of distinct query shapes the team needs to reason about.
• Corrects silent data loss: Fixes without junction table entries (58% on dev per PR description) now return with empty suggestions instead of being silently dropped. The right contract for a list endpoint.
• Integration tests assert on actual IDs (test/it/shared/tests/fixes.js:64-71): Verifying specific suggestion IDs per fix, not just array lengths. This is the kind of test that would have caught the original silent-drop bug.
• Tightly scoped change: One controller method, its tests, and seed data. No unrelated cleanup mixed in.
• New unit test for early-return path (test/controllers/fixes.test.js:194-203): Clean AAA structure, asserts on both status and body.
• Conventions compliance: PR title follows conventional commit format, unit and integration tests are present, DTOs are used correctly.

Issues

Important (Should Fix)

1. src/controllers/fixes.js:153-155 - Early return on empty results bypasses ownership check

   if (fixEntitiesWithSuggestions.length === 0) {
     return ok([]);
   }

   The old code called checkOwnership(fixEntities[0], opportunityId, siteId, this.#Opportunity) regardless of whether fixes existed. With the new early return, if an opportunity has zero fixes but belongs to a different site, the caller gets 200 [] instead of the 404 that checkOwnership would have produced.

   Why it matters: A caller with a valid siteId they own and a guessed opportunityId from a different site now gets 200 instead of 404 when that opportunity has no fixes. Returning 200 for a resource the caller does not own is incorrect regardless of what data it carries. The unit test at test/controllers/fixes.test.js:176 previously tested this scenario (empty result + wrong site = 404) but was changed to return a non-empty array, masking the behavioral change.

   How to fix: Validate the opportunity-to-site relationship before the early return. One approach - move the ownership check above the empty guard:

   const fixEntitiesWithSuggestions = await this.#FixEntity
     .getAllFixesWithSuggestionsByOpportunityId(opportunityId);
   
   res = await checkOwnership(
     fixEntitiesWithSuggestions[0]?.fixEntity,
     opportunityId, siteId, this.#Opportunity
   );
   if (res) {
     return res;
   }
   
   if (fixEntitiesWithSuggestions.length === 0) {
     return ok([]);
   }

   If checkOwnership requires a non-null first argument to trace the relationship, validate the opportunity's siteId directly before the early return instead.

Recommendations

• The integration test verifies the happy path (2 fixes, each with 1 suggestion). Consider adding a seed fixture where a fix has no junction entry to validate the stated behavior change at the integration level ("fixes without mapping table entries now return with empty suggestions").

Assessment

Ready to merge? With fixes
Reasoning: The N+1 elimination and the silent-drop bug fix are both correct and well-tested. The ownership bypass on the empty-results path is a behavioral regression in access control that should be addressed before merge - it is a reordering fix, not a redesign.

Next Steps

1. Address the ownership check ordering so the empty-results path still validates opportunity-site ownership.

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 42s | Cost: $5.81 | Commit: 3efd6010aa2796dce436c3e614475380e70be5ca
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[mimchome]

@MysticatBot-Dev issues are addressed