From 96fbf2d4987673d796f4cb361991ffead0f20d8a Mon Sep 17 00:00:00 2001 From: Matt Bevilacqua Date: Mon, 8 Apr 2024 21:32:54 -0400 Subject: [PATCH 1/6] Fix bug --- src/constants.js | 2 +- .../getGoalIdsBySimiliarity.test.js | 248 +++++++++++++++++- src/goalServices/goals.js | 2 +- src/services/goalSimilarityGroup.test.js | 2 +- src/services/goalSimilarityGroup.ts | 7 +- 5 files changed, 250 insertions(+), 11 deletions(-) diff --git a/src/constants.js b/src/constants.js index 68c8e459aa..d48517af7c 100644 --- a/src/constants.js +++ b/src/constants.js @@ -261,7 +261,7 @@ const MAINTENANCE_CATEGORY = { const GOAL_CREATED_VIA = ['imported', 'activityReport', 'rtr', 'merge', 'admin', 'tr']; -const CURRENT_GOAL_SIMILARITY_VERSION = 2; +const CURRENT_GOAL_SIMILARITY_VERSION = 3; module.exports = { CURRENT_GOAL_SIMILARITY_VERSION, diff --git a/src/goalServices/getGoalIdsBySimiliarity.test.js b/src/goalServices/getGoalIdsBySimiliarity.test.js index ce84203a21..1b19104a61 100644 --- a/src/goalServices/getGoalIdsBySimiliarity.test.js +++ b/src/goalServices/getGoalIdsBySimiliarity.test.js @@ -1,8 +1,10 @@ +/* eslint-disable jest/no-conditional-expect */ import faker from '@faker-js/faker'; import { REPORT_STATUSES } from '@ttahub/common'; import { ActivityReportGoal, Grant, + GrantNumberLink, Recipient, Goal, GoalTemplate, @@ -350,6 +352,13 @@ describe('getGoalIdsBySimilarity', () => { await destroyReport(needsActionReport); await destroyReport(report); + await GrantNumberLink.destroy({ + where: { + grantId: activeGrant.id, + }, + force: true, + }); + await Grant.destroy({ where: { id: grants.map((g) => g.id), @@ -509,7 +518,7 @@ describe('getGoalIdsBySimilarity', () => { const [setOne, setTwo] = filteredSet; expect(setOne.goals.length).toBe(4); - expect(setTwo.goals.length).toBe(3); + expect(setTwo.goals.length).toBe(4); const goalIds = [...setOne.goals, ...setTwo.goals]; @@ -577,7 +586,7 @@ describe('getGoalIdsBySimilarity', () => { const [setOne, setTwo] = filteredSet; expect(setOne.goals.length).toBe(7); - expect(setTwo.goals.length).toBe(4); + expect(setTwo.goals.length).toBe(5); }); describe('getReportCountForGoals', () => { @@ -655,4 +664,239 @@ describe('getGoalIdsBySimilarity', () => { }]))).toBe(false); }); }); + + describe('getGoalIdsBySimilarity (closed, curated)', () => { + let goalGroup = []; + const goalTitle = faker.lorem.sentence(); + let recipientForClosedCurated; + let activeGrantForClosedCurated; + let templateForClosedCurated; + + beforeAll(async () => { + recipientForClosedCurated = await createRecipient(); + + activeGrantForClosedCurated = await createGrant({ + recipientId: recipient.id, + status: 'Active', + }); + + templateForClosedCurated = await createGoalTemplate({ + name: goalTitle, + creationMethod: CREATION_METHOD.CURATED, + }); + + goalGroup = await Promise.all([ + createGoal({ + status: GOAL_STATUS.NOT_STARTED, + name: goalTitle, + goalTemplateId: templateForClosedCurated.id, + grantId: activeGrantForClosedCurated.id, + }), + createGoal({ + status: GOAL_STATUS.NOT_STARTED, + name: goalTitle, + goalTemplateId: templateForClosedCurated.id, + grantId: activeGrantForClosedCurated.id, + }), + createGoal({ + status: GOAL_STATUS.CLOSED, + name: goalTitle, + goalTemplateId: templateForClosedCurated.id, + grantId: activeGrantForClosedCurated.id, + }), + ]); + }); + + afterAll(async () => { + const goals = [ + ...goalGroup, + ]; + const grants = await Grant.findAll({ + attributes: ['id', 'recipientId'], + where: { + id: goals.map((g) => g.grantId), + }, + }); + + const recipients = await Recipient.findAll({ + attributes: ['id'], + where: { + id: grants.map((g) => g.recipientId), + }, + }); + + await GoalSimilarityGroupGoal.destroy({ + where: { + goalId: goals.map((g) => g.id), + }, + }); + + await Goal.destroy({ + where: { + id: goals.map((g) => g.id), + }, + force: true, + }); + + await GoalTemplate.destroy({ + where: { + id: templateForClosedCurated.id, + }, + force: true, + }); + + await GrantNumberLink.destroy({ + where: { + grantId: activeGrantForClosedCurated.id, + }, + force: true, + }); + + await Grant.destroy({ + where: { + id: grants.map((g) => g.id), + }, + force: true, + individualHooks: true, + }); + + await GoalSimilarityGroup.destroy({ + where: { + recipientId: [recipientForClosedCurated.id], + }, + }); + + await Recipient.destroy({ + where: { + id: [...recipients.map((r) => r.id)], + }, + force: true, + }); + }); + + it('goal similiarity group, user has permission', async () => { + const similarityResponse = [ + goalGroup, + ].map((group) => ({ + id: group[0].id, + name: group[0].name, + matches: group.map((g) => ({ + id: g.id, + name: g.name, + })), + })); + + similarGoalsForRecipient.mockResolvedValue({ result: similarityResponse }); + + await getGoalIdsBySimilarity(recipientForClosedCurated.id); + + const goalGroupIds = goalGroup.map((g) => g.id); + const goalGroupSimilarityGroupGoals = await GoalSimilarityGroupGoal.findAll({ + where: { + goalId: goalGroupIds, + }, + include: [{ + model: Goal, + as: 'goal', + include: [{ + model: GoalTemplate, + as: 'goalTemplate', + }], + }], + }); + + expect(goalGroupSimilarityGroupGoals).toHaveLength(goalGroup.length); + + goalGroupSimilarityGroupGoals.forEach((g) => { + if (g.excludedIfNotAdmin) { + expect(g.goal.goalTemplate.creationMethod).toBe(CREATION_METHOD.CURATED); + expect(g.goal.status).toBe(GOAL_STATUS.CLOSED); + } + }); + + const excludedIfNotAdminGoalGroup = goalGroupSimilarityGroupGoals + .filter((g) => g.excludedIfNotAdmin); + + expect(excludedIfNotAdminGoalGroup).toHaveLength(1); + + const user = { + permissions: [], + flags: ['closed_goal_merge_override'], + }; + + const sets = await getGoalIdsBySimilarity( + recipientForClosedCurated.id, + activeGrantForClosedCurated.regionId, + user, + ); + + expect(sets).toHaveLength(2); + + const setsWithGoals = sets.filter((set) => set.goals.length); + expect(setsWithGoals).toHaveLength(1); + + const [set] = setsWithGoals; + expect(set.goals.length).toBe(3); + }); + + it('goal similiarity group, user does not have permission', async () => { + const similarityResponse = [ + goalGroup, + ].map((group) => ({ + id: group[0].id, + name: group[0].name, + matches: group.map((g) => ({ + id: g.id, + name: g.name, + })), + })); + + similarGoalsForRecipient.mockResolvedValue({ result: similarityResponse }); + + await getGoalIdsBySimilarity(recipientForClosedCurated.id); + + const goalGroupIds = goalGroup.map((g) => g.id); + const goalGroupSimilarityGroupGoals = await GoalSimilarityGroupGoal.findAll({ + where: { + goalId: goalGroupIds, + }, + include: [{ + model: Goal, + as: 'goal', + include: [{ + model: GoalTemplate, + as: 'goalTemplate', + }], + }], + }); + + expect(goalGroupSimilarityGroupGoals).toHaveLength(goalGroup.length); + + goalGroupSimilarityGroupGoals.forEach((g) => { + if (g.excludedIfNotAdmin) { + expect(g.goal.goalTemplate.creationMethod).toBe(CREATION_METHOD.CURATED); + expect(g.goal.status).toBe(GOAL_STATUS.CLOSED); + } + }); + + const allowedIfNotAdmin = goalGroupSimilarityGroupGoals + .filter((g) => !g.excludedIfNotAdmin).map((g) => g.goal.id); + + expect(allowedIfNotAdmin).toHaveLength(2); + + const sets = await getGoalIdsBySimilarity( + recipientForClosedCurated.id, + activeGrantForClosedCurated.regionId, + ); + + expect(sets).toHaveLength(2); + + const setsWithGoals = sets.filter((set) => set.goals.length); + expect(setsWithGoals).toHaveLength(1); + + const [set] = setsWithGoals; + expect(set.goals.length).toBe(2); + expect(set.goals).toEqual(expect.arrayContaining(allowedIfNotAdmin)); + }); + }); }); diff --git a/src/goalServices/goals.js b/src/goalServices/goals.js index a398ba508b..f8b9baf725 100644 --- a/src/goalServices/goals.js +++ b/src/goalServices/goals.js @@ -2812,7 +2812,7 @@ export async function getGoalIdsBySimilarity(recipientId, regionId, user = null) let closedCurated = false; if (current.goalTemplate && current.goalTemplate.creationMethod === CREATION_METHOD.CURATED) { - closedCurated = current.status !== GOAL_STATUS.CLOSED; + closedCurated = current.status === GOAL_STATUS.CLOSED; } // goal on an active report diff --git a/src/services/goalSimilarityGroup.test.js b/src/services/goalSimilarityGroup.test.js index 86d8bc29a7..47bbfac96f 100644 --- a/src/services/goalSimilarityGroup.test.js +++ b/src/services/goalSimilarityGroup.test.js @@ -168,7 +168,7 @@ describe('goalSimilarityGroup services', () => { expect(result).toEqual({ id: 'group-id', - goals: ['goal-1', 'goal-3', 'goal-4'], + goals: ['goal-1', 'goal-2', 'goal-3', 'goal-4'], }); }); }); diff --git a/src/services/goalSimilarityGroup.ts b/src/services/goalSimilarityGroup.ts index 2f4576f9e6..2b0f24d106 100644 --- a/src/services/goalSimilarityGroup.ts +++ b/src/services/goalSimilarityGroup.ts @@ -48,12 +48,7 @@ interface SimilarityGroup { export const flattenSimilarityGroupGoals = (group: SimilarityGroup) => ({ ...group.toJSON(), - goals: group.goals.filter((goal) => { - if (goal.goalTemplate && goal.goalTemplate.creationMethod === CREATION_METHOD.CURATED) { - return goal.status !== GOAL_STATUS.CLOSED; - } - return true; - }).map((goal) => goal.id), + goals: group.goals.map((goal) => goal.id), }); export async function getSimilarityGroupById( From de40307a36a528dcb520bc3dd817b82997ab7be8 Mon Sep 17 00:00:00 2001 From: Matt Bevilacqua Date: Thu, 11 Apr 2024 10:12:12 -0400 Subject: [PATCH 2/6] Remove unused variables --- src/services/goalSimilarityGroup.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/services/goalSimilarityGroup.ts b/src/services/goalSimilarityGroup.ts index 2b0f24d106..fa8e3856a5 100644 --- a/src/services/goalSimilarityGroup.ts +++ b/src/services/goalSimilarityGroup.ts @@ -2,8 +2,6 @@ import { Op, WhereOptions, Model } from 'sequelize'; import { uniq } from 'lodash'; import db from '../models'; import { - GOAL_STATUS, - CREATION_METHOD, CURRENT_GOAL_SIMILARITY_VERSION, } from '../constants'; From 22fcc15b2a64e5d442b6968ba64462618a3ce770 Mon Sep 17 00:00:00 2001 From: Matt Bevilacqua Date: Thu, 11 Apr 2024 11:35:14 -0400 Subject: [PATCH 3/6] Add test and additional func. --- .../getGoalIdsBySimiliarity.test.js | 129 +++++++++++++++++- src/goalServices/goals.js | 9 +- 2 files changed, 135 insertions(+), 3 deletions(-) diff --git a/src/goalServices/getGoalIdsBySimiliarity.test.js b/src/goalServices/getGoalIdsBySimiliarity.test.js index 1b19104a61..4639785b80 100644 --- a/src/goalServices/getGoalIdsBySimiliarity.test.js +++ b/src/goalServices/getGoalIdsBySimiliarity.test.js @@ -676,7 +676,7 @@ describe('getGoalIdsBySimilarity', () => { recipientForClosedCurated = await createRecipient(); activeGrantForClosedCurated = await createGrant({ - recipientId: recipient.id, + recipientId: recipientForClosedCurated.id, status: 'Active', }); @@ -899,4 +899,131 @@ describe('getGoalIdsBySimilarity', () => { expect(set.goals).toEqual(expect.arrayContaining(allowedIfNotAdmin)); }); }); + + describe('getGoalIdsBySimilarity (two grants, 2 goals)', () => { + let goalGroup = []; + const goalTitle = faker.lorem.sentence(); + let recipientFor2Grants2Goals; + let activeGrantFor2Grants2Goals; + let activeGrantFor2Grants2GoalsTwo; + + const region = 4; + + beforeAll(async () => { + recipientFor2Grants2Goals = await createRecipient(); + + activeGrantFor2Grants2Goals = await createGrant({ + recipientId: recipientFor2Grants2Goals.id, + status: 'Active', + regionId: region, + }); + + activeGrantFor2Grants2GoalsTwo = await createGrant({ + recipientId: recipientFor2Grants2Goals.id, + status: 'Active', + regionId: region, + }); + + goalGroup = await Promise.all([ + createGoal({ + status: GOAL_STATUS.NOT_STARTED, + name: goalTitle, + grantId: activeGrantFor2Grants2Goals.id, + }), + createGoal({ + status: GOAL_STATUS.NOT_STARTED, + name: goalTitle, + grantId: activeGrantFor2Grants2GoalsTwo.id, + }), + ]); + }); + + afterAll(async () => { + const goals = [ + ...goalGroup, + ]; + const grants = await Grant.findAll({ + attributes: ['id', 'recipientId'], + where: { + id: goals.map((g) => g.grantId), + }, + }); + + const recipients = await Recipient.findAll({ + attributes: ['id'], + where: { + id: grants.map((g) => g.recipientId), + }, + }); + + await GoalSimilarityGroupGoal.destroy({ + where: { + goalId: goals.map((g) => g.id), + }, + }); + + await Goal.destroy({ + where: { + id: goals.map((g) => g.id), + }, + force: true, + }); + + await GrantNumberLink.destroy({ + where: { + grantId: activeGrantFor2Grants2Goals.id, + }, + force: true, + }); + + await Grant.destroy({ + where: { + id: grants.map((g) => g.id), + }, + force: true, + individualHooks: true, + }); + + await GoalSimilarityGroup.destroy({ + where: { + recipientId: [recipientFor2Grants2Goals.id], + }, + }); + + await Recipient.destroy({ + where: { + id: [...recipients.map((r) => r.id)], + }, + force: true, + }); + }); + + it('goal similiarity group w/ 2 grants and 2 goals', async () => { + const similarityResponse = [ + goalGroup, + ].map((group) => ({ + id: group[0].id, + name: group[0].name, + matches: group.map((g) => ({ + id: g.id, + name: g.name, + })), + })); + + similarGoalsForRecipient.mockResolvedValue({ result: similarityResponse }); + + await getGoalIdsBySimilarity(recipientFor2Grants2Goals.id); + + const sets = await getGoalIdsBySimilarity( + recipientFor2Grants2Goals.id, + activeGrantFor2Grants2Goals.regionId, + ); + + expect(sets).toHaveLength(1); + const [set] = sets; + + // we expect no goals + expect(set.goals.length).toBe(0); + }); + }); }); diff --git a/src/goalServices/goals.js b/src/goalServices/goals.js index f8b9baf725..4c91ec0bd7 100644 --- a/src/goalServices/goals.js +++ b/src/goalServices/goals.js @@ -2843,11 +2843,16 @@ export async function getGoalIdsBySimilarity(recipientId, regionId, user = null) responsesForComparison: responsesForComparison(current), ids: [current.id], excludedIfNotAdmin, + grantId: grantLookup[current.grantId], }, ]; }, [])); - const groupsWithMoreThanOneGoal = goalGroupsDeduplicated.filter((group) => group.length > 1); + const groupsWithMoreThanOneGoalAndMoreGoalsThanGrants = goalGroupsDeduplicated + .filter((group) => { + const grantIds = uniq(group.map((goal) => goal.grantId)); + return group.length > 1 && group.length > grantIds.length; + }); // save the groups to the database // there should also always be an empty group @@ -2855,7 +2860,7 @@ export async function getGoalIdsBySimilarity(recipientId, regionId, user = null) // and that we've run these computations await Promise.all( - [...groupsWithMoreThanOneGoal, []] + [...groupsWithMoreThanOneGoalAndMoreGoalsThanGrants, []] .map((gg) => ( createSimilarityGroup( recipientId, From 6f897151c0d62783e35aa1c87443ba9d4da9c906 Mon Sep 17 00:00:00 2001 From: Matt Bevilacqua Date: Thu, 11 Apr 2024 11:41:19 -0400 Subject: [PATCH 4/6] Fix logic again --- src/goalServices/goals.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/goalServices/goals.js b/src/goalServices/goals.js index 4c91ec0bd7..327146e834 100644 --- a/src/goalServices/goals.js +++ b/src/goalServices/goals.js @@ -2851,7 +2851,7 @@ export async function getGoalIdsBySimilarity(recipientId, regionId, user = null) const groupsWithMoreThanOneGoalAndMoreGoalsThanGrants = goalGroupsDeduplicated .filter((group) => { const grantIds = uniq(group.map((goal) => goal.grantId)); - return group.length > 1 && group.length > grantIds.length; + return group.length > 1 && group.length !== grantIds.length; }); // save the groups to the database From f7be8c04bff6434dcef6ec085acc07c6f9c6df9d Mon Sep 17 00:00:00 2001 From: Matt Bevilacqua Date: Fri, 19 Apr 2024 14:19:54 -0400 Subject: [PATCH 5/6] Update test --- .../getGoalIdsBySimiliarity.test.js | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/goalServices/getGoalIdsBySimiliarity.test.js b/src/goalServices/getGoalIdsBySimiliarity.test.js index 4639785b80..d44aa2de9b 100644 --- a/src/goalServices/getGoalIdsBySimiliarity.test.js +++ b/src/goalServices/getGoalIdsBySimiliarity.test.js @@ -1,4 +1,3 @@ -/* eslint-disable jest/no-conditional-expect */ import faker from '@faker-js/faker'; import { REPORT_STATUSES } from '@ttahub/common'; import { @@ -808,10 +807,15 @@ describe('getGoalIdsBySimilarity', () => { expect(goalGroupSimilarityGroupGoals).toHaveLength(goalGroup.length); goalGroupSimilarityGroupGoals.forEach((g) => { + let creationMethod = expect.any(String); + let status = expect.any(String); if (g.excludedIfNotAdmin) { - expect(g.goal.goalTemplate.creationMethod).toBe(CREATION_METHOD.CURATED); - expect(g.goal.status).toBe(GOAL_STATUS.CLOSED); + creationMethod = CREATION_METHOD.CURATED; + status = GOAL_STATUS.CLOSED; } + + expect(g.goal.goalTemplate.creationMethod).toBe(creationMethod); + expect(g.goal.status).toBe(status); }); const excludedIfNotAdminGoalGroup = goalGroupSimilarityGroupGoals @@ -873,12 +877,16 @@ describe('getGoalIdsBySimilarity', () => { expect(goalGroupSimilarityGroupGoals).toHaveLength(goalGroup.length); goalGroupSimilarityGroupGoals.forEach((g) => { + let creationMethod = expect.any(String); + let status = expect.any(String); if (g.excludedIfNotAdmin) { - expect(g.goal.goalTemplate.creationMethod).toBe(CREATION_METHOD.CURATED); - expect(g.goal.status).toBe(GOAL_STATUS.CLOSED); + creationMethod = CREATION_METHOD.CURATED; + status = GOAL_STATUS.CLOSED; } - }); + expect(g.goal.goalTemplate.creationMethod).toBe(creationMethod); + expect(g.goal.status).toBe(status); + }); const allowedIfNotAdmin = goalGroupSimilarityGroupGoals .filter((g) => !g.excludedIfNotAdmin).map((g) => g.goal.id); From ea31a7eb72d62be98dbfd6b000df36bf8e730879 Mon Sep 17 00:00:00 2001 From: Matt Bevilacqua Date: Fri, 19 Apr 2024 14:52:09 -0400 Subject: [PATCH 6/6] Revert "Update test" This reverts commit f7be8c04bff6434dcef6ec085acc07c6f9c6df9d. --- .../getGoalIdsBySimiliarity.test.js | 20 ++++++------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/goalServices/getGoalIdsBySimiliarity.test.js b/src/goalServices/getGoalIdsBySimiliarity.test.js index d44aa2de9b..4639785b80 100644 --- a/src/goalServices/getGoalIdsBySimiliarity.test.js +++ b/src/goalServices/getGoalIdsBySimiliarity.test.js @@ -1,3 +1,4 @@ +/* eslint-disable jest/no-conditional-expect */ import faker from '@faker-js/faker'; import { REPORT_STATUSES } from '@ttahub/common'; import { @@ -807,15 +808,10 @@ describe('getGoalIdsBySimilarity', () => { expect(goalGroupSimilarityGroupGoals).toHaveLength(goalGroup.length); goalGroupSimilarityGroupGoals.forEach((g) => { - let creationMethod = expect.any(String); - let status = expect.any(String); if (g.excludedIfNotAdmin) { - creationMethod = CREATION_METHOD.CURATED; - status = GOAL_STATUS.CLOSED; + expect(g.goal.goalTemplate.creationMethod).toBe(CREATION_METHOD.CURATED); + expect(g.goal.status).toBe(GOAL_STATUS.CLOSED); } - - expect(g.goal.goalTemplate.creationMethod).toBe(creationMethod); - expect(g.goal.status).toBe(status); }); const excludedIfNotAdminGoalGroup = goalGroupSimilarityGroupGoals @@ -877,16 +873,12 @@ describe('getGoalIdsBySimilarity', () => { expect(goalGroupSimilarityGroupGoals).toHaveLength(goalGroup.length); goalGroupSimilarityGroupGoals.forEach((g) => { - let creationMethod = expect.any(String); - let status = expect.any(String); if (g.excludedIfNotAdmin) { - creationMethod = CREATION_METHOD.CURATED; - status = GOAL_STATUS.CLOSED; + expect(g.goal.goalTemplate.creationMethod).toBe(CREATION_METHOD.CURATED); + expect(g.goal.status).toBe(GOAL_STATUS.CLOSED); } - - expect(g.goal.goalTemplate.creationMethod).toBe(creationMethod); - expect(g.goal.status).toBe(status); }); + const allowedIfNotAdmin = goalGroupSimilarityGroupGoals .filter((g) => !g.excludedIfNotAdmin).map((g) => g.goal.id);