fix: opportunity district resolution used the wrong source for both types - #926
Open
nadavosa wants to merge 1 commit into
Open
fix: opportunity district resolution used the wrong source for both types#926nadavosa wants to merge 1 commit into
nadavosa wants to merge 1 commit into
Conversation
…ypes addDistrictToOpportunity (used by the opportunity list route, the legacy creation route, and the single-opportunity route) resolved the district from two concepts that don't represent the opportunity's own location: - REGULAR/EVENTS used deal.dealDistrict[0], the deal's volunteer-search target district(s) — unrelated to where the opportunity actually is. - ACCOMPANYING used deal.postcode, which mirrors the agent's own postcode (the same value the fallback branch right below it already derives), never the appointment's own postcode. Per product: REGULAR/EVENTS should use the opportunity's own agent's district (agent.districtId, an already-loaded FK column wherever agent is loaded — no new relation needed); ACCOMPANYING should use the appointment's own postcode (accompanying.postcode, falling back to accompanying.postcodeId when the relation isn't loaded), with the same agent-based fallback chain as before if that doesn't resolve.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
getDistrictToOpportunityHandler'saddDistrictToOpportunity(used by the opportunity list route, the legacy creation route, and the single-opportunity route) resolved an opportunity's district from two concepts that don't represent its own location:deal.dealDistrict[0], the deal's volunteer-search target district(s), an unrelated concept.deal.postcode, which mirrors the agent's own postcode (the exact same value the fallback branch right below it already derives independently), never the appointment's own postcode.The fix
Per the correct rule in the issue:
agent.districtId. That's an already-loaded FK column whereveragentis loaded at all (no new relation needed, confirmed against the list route's existing relations).accompanying.postcode(falling back toaccompanying.postcodeIdwhen only the FK is loaded, not the relation).Related Issues
Closes #895
Changes
src/server/utils/data/add-district-to-opp.ts: corrected district source for both opportunity types.src/test/server/utils/data/add-district-to-opp.test.ts: rewrote the unit tests, the old ones asserted the buggy behavior directly (e.g. "assigns district from deal location", "Priority 1: assigns district from deal postcode").Testing
No Postgres available in my sandbox by default, spun up a throwaway instance (
embedded-postgres, self-contained, no root/Docker needed) to run migrations, seed data, and the full suite for real rather than relying on unit tests alone.tsc --noEmit,eslint(changed files): clean.add-district-to-opp.test.ts: 11/11 passing (fully rewritten for the new behavior).swagger.test.ts, a 5s timeout onbuildServer()startup) is a pre-existing, unrelated flake, confirmed passing on its own with a longer timeout, and already independently reported by another contributor on a different PR.Checklist