Skip to content

fix: opportunity district resolution used the wrong source for both types #926

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nadavosa wants to merge 1 commit into
base: develop
Choose a base branch
from
+118 −47
Open

fix: opportunity district resolution used the wrong source for both types #926

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 22 additions & 13 deletions src/server/utils/data/add-district-to-opp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { OpportunityType } from "need4deed-sdk";
import Postcode from "../../../data/entity/location/postcode.entity";
import Opportunity from "../../../data/entity/opportunity/opportunity.entity";
import { getDistrictFromPostcode } from "../../../data/utils/get-district";

Expand All @@ -12,29 +13,37 @@ export function getDistrictToOpportunityHandler() {
if (opportunity.districtId) {
return opportunity;
}
if (opportunity.type !== OpportunityType.ACCOMPANYING) {
const district = opportunity.deal?.dealDistrict?.[0]?.district;

// ACCOMPANYING: derive from the appointment's own postcode (be#895).
// `deal.postcode` mirrors the agent's own postcode — an unrelated
// concept — so it isn't used here.
if (opportunity.type === OpportunityType.ACCOMPANYING) {
const postcode =
opportunity.accompanying?.postcode ??
(opportunity.accompanying?.postcodeId
? new Postcode({ id: opportunity.accompanying.postcodeId })
: undefined);
const district = postcode
? await getDistrictFromPostcode(postcode)
: null;
if (district) {
opportunity.district = district;
updates.push(opportunity);
return opportunity;
}
return opportunity;
}
// accompanying: use appointment postcode from the form
const district = await getDistrictFromPostcode(
opportunity.deal?.postcode,
);
if (district) {
opportunity.district = district;
updates.push(opportunity);
return opportunity;
}
// fallback to agent

// REGULAR/EVENTS use the opportunity's own agent's district (be#895).
// Also the fallback for an ACCOMPANYING opportunity whose appointment
// postcode didn't resolve to a district. `agent.districtId` is an
// already-loaded FK column wherever `agent` is loaded at all, no extra
// relation needed.
if (opportunity.agent?.districtId) {
opportunity.districtId = opportunity.agent.districtId;
updates.push(opportunity);
return opportunity;
}

const districtFromAgent = await getDistrictFromPostcode(
opportunity.agent?.address?.postcode,
);
Expand Down
130 changes: 96 additions & 34 deletions src/test/server/utils/data/add-district-to-opp.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ function makeOpportunity(
district: undefined,
agent: undefined,
deal: undefined,
accompanying: undefined,
...overrides,
} as Opportunity;
}
Expand All @@ -43,69 +44,131 @@ describe("getDistrictToOpportunityHandler", () => {
});

describe("addDistrictToOpportunity", () => {
describe("Non-ACCOMPANYING Type", () => {
it("assigns district from deal location and returns early", async () => {
it("does nothing if opportunity.districtId is already set", async () => {
const handler = getDistrictToOpportunityHandler();
const opportunity = makeOpportunity({
type: OpportunityType.REGULAR,
districtId: 3,
agent: { districtId: 99 } as any,
});

const result = await handler.addDistrictToOpportunity(opportunity);

expect(result.districtId).toBe(3);
expect(handler.updates).toHaveLength(0);
expect(mockedGetDistrictFromPostcode).not.toHaveBeenCalled();
});

describe("REGULAR / EVENTS", () => {
it("uses the opportunity's own agent's districtId", async () => {
const handler = getDistrictToOpportunityHandler();
const district = makeDistrict({ id: 5 });
const opportunity = makeOpportunity({
type: OpportunityType.REGULAR,
deal: {
dealDistrict: [{ district }],
} as any,
agent: { districtId: 5 } as any,
});

const result = await handler.addDistrictToOpportunity(opportunity);

expect(result.district).toBe(district);
expect(result.districtId).toBe(5);
expect(handler.updates).toContain(opportunity);
expect(mockedGetDistrictFromPostcode).not.toHaveBeenCalled();
});

it("falls back to the agent's own address postcode when agent.districtId is missing", async () => {
const handler = getDistrictToOpportunityHandler();
const district = makeDistrict({ id: 202 });
const opportunity = makeOpportunity({
type: OpportunityType.EVENTS,
agent: { address: { postcode: "54321" } } as any,
});
mockedGetDistrictFromPostcode.mockResolvedValue(district as any);

const result = await handler.addDistrictToOpportunity(opportunity);

expect(mockedGetDistrictFromPostcode).toHaveBeenCalledWith("54321");
expect(result.district).toBe(district);
expect(handler.updates).toContain(opportunity);
});

it("does not add to updates if no agent info is available", async () => {
const handler = getDistrictToOpportunityHandler();
const opportunity = makeOpportunity({
type: OpportunityType.REGULAR,
agent: undefined,
});
mockedGetDistrictFromPostcode.mockResolvedValue(undefined!);

const result = await handler.addDistrictToOpportunity(opportunity);

expect(result.district).toBeUndefined();
expect(result.districtId).toBeUndefined();
expect(handler.updates).toHaveLength(0);
});
});

describe("ACCOMPANYING Type Resolution Priority", () => {
it("Priority 1: assigns district from deal postcode", async () => {
describe("ACCOMPANYING resolution priority", () => {
it("Priority 1: resolves from the accompanying's own postcode relation", async () => {
const handler = getDistrictToOpportunityHandler();
const district = makeDistrict({ id: 101 });
const postcode = { id: 7, value: "12345" };
const opportunity = makeOpportunity({
type: OpportunityType.ACCOMPANYING,
deal: { postcode: "12345" } as any,
accompanying: { postcode } as any,
});
mockedGetDistrictFromPostcode.mockResolvedValue(district as any);

const result = await handler.addDistrictToOpportunity(opportunity);

expect(mockedGetDistrictFromPostcode).toHaveBeenCalledWith("12345");
expect(mockedGetDistrictFromPostcode).toHaveBeenCalledWith(postcode);
expect(result.district).toBe(district);
expect(handler.updates).toContain(opportunity);
});

it("Priority 2: falls back to agent.districtId if deal postcode lookup fails", async () => {
it("resolves from accompanying.postcodeId when the postcode relation isn't loaded", async () => {
const handler = getDistrictToOpportunityHandler();
const district = makeDistrict({ id: 303 });
const opportunity = makeOpportunity({
type: OpportunityType.ACCOMPANYING,
deal: { postcode: "INVALID" } as any,
accompanying: { postcodeId: 42 } as any,
});
mockedGetDistrictFromPostcode.mockResolvedValue(district as any);

const result = await handler.addDistrictToOpportunity(opportunity);

expect(mockedGetDistrictFromPostcode).toHaveBeenCalledWith(
expect.objectContaining({ id: 42 }),
);
expect(result.district).toBe(district);
expect(handler.updates).toContain(opportunity);
});

it("Priority 2: falls back to agent.districtId if the appointment postcode lookup fails", async () => {
const handler = getDistrictToOpportunityHandler();
const opportunity = makeOpportunity({
type: OpportunityType.ACCOMPANYING,
accompanying: { postcode: { id: 7 } } as any,
agent: { districtId: 99 } as any,
});
mockedGetDistrictFromPostcode.mockResolvedValue(undefined!);

const result = await handler.addDistrictToOpportunity(opportunity);

expect(mockedGetDistrictFromPostcode).toHaveBeenCalledTimes(1);
expect(result.districtId).toBe(99);
expect(handler.updates).toContain(opportunity);
});

it("Priority 3: falls back to agent postcode if agent.districtId is missing", async () => {
it("Priority 3: falls back to the agent's address postcode if agent.districtId is also missing", async () => {
const handler = getDistrictToOpportunityHandler();
const district = makeDistrict({ id: 202 });
const opportunity = makeOpportunity({
type: OpportunityType.ACCOMPANYING,
deal: { postcode: "INVALID" } as any,
agent: {
address: { postcode: "54321" },
} as any,
accompanying: { postcode: { id: 7 } } as any,
agent: { address: { postcode: "54321" } } as any,
});

// First call (deal postcode) returns null, second call (agent postcode) returns district
// First call (appointment postcode) returns nothing, second call
// (agent's own postcode) resolves.
mockedGetDistrictFromPostcode
.mockResolvedValueOnce(undefined!)
.mockResolvedValueOnce(district as any);
Expand All @@ -116,36 +179,35 @@ describe("getDistrictToOpportunityHandler", () => {
expect(result.district).toBe(district);
expect(handler.updates).toContain(opportunity);
});
});

describe("Edge Cases", () => {
it("does not add to updates if no district info is found anywhere", async () => {
it("handles a missing accompanying object gracefully", async () => {
const handler = getDistrictToOpportunityHandler();
const opportunity = makeOpportunity({
type: OpportunityType.ACCOMPANYING,
deal: undefined,
agent: undefined,
accompanying: undefined,
agent: { districtId: 77 } as any,
});
mockedGetDistrictFromPostcode.mockResolvedValue(undefined!);

const result = await handler.addDistrictToOpportunity(opportunity);

expect(result.district).toBeUndefined();
expect(handler.updates).toHaveLength(0);
expect(result.districtId).toBe(77);
expect(handler.updates).toHaveLength(1);
expect(mockedGetDistrictFromPostcode).not.toHaveBeenCalled();
});

it("handles missing deal object gracefully for ACCOMPANYING type", async () => {
it("does not add to updates if no district info is found anywhere", async () => {
const handler = getDistrictToOpportunityHandler();
const opportunity = makeOpportunity({
type: OpportunityType.ACCOMPANYING,
agent: { districtId: 77 } as any,
accompanying: undefined,
agent: undefined,
});
mockedGetDistrictFromPostcode.mockResolvedValue(undefined!);

await handler.addDistrictToOpportunity(opportunity);
const result = await handler.addDistrictToOpportunity(opportunity);

expect(opportunity.districtId).toBe(77);
expect(handler.updates).toHaveLength(1);
expect(result.district).toBeUndefined();
expect(handler.updates).toHaveLength(0);
});
});
});
Expand All @@ -157,11 +219,11 @@ describe("getDistrictToOpportunityHandler", () => {

const o1 = makeOpportunity({
type: OpportunityType.ACCOMPANYING,
deal: { postcode: "1" } as any,
accompanying: { postcode: { id: 1 } } as any,
});
const o2 = makeOpportunity({
type: OpportunityType.ACCOMPANYING,
deal: { postcode: "2" } as any,
accompanying: { postcode: { id: 2 } } as any,
});

await handler.addDistrictToOpportunity(o1);
Expand Down