From 318931be94d033383b0184b6e963d954f081144d Mon Sep 17 00:00:00 2001 From: Eduardo Peredo Date: Thu, 30 Jul 2026 14:08:18 -0500 Subject: [PATCH 1/2] get user details before data review notification --- src/models/ProjectNotification.ts | 129 +++++--- .../__tests__/ProjectNotification.spec.ts | 298 ++++++++++++++++++ 2 files changed, 385 insertions(+), 42 deletions(-) create mode 100644 src/models/__tests__/ProjectNotification.spec.ts diff --git a/src/models/ProjectNotification.ts b/src/models/ProjectNotification.ts index b3916b9a..a9d61cc7 100644 --- a/src/models/ProjectNotification.ts +++ b/src/models/ProjectNotification.ts @@ -4,15 +4,29 @@ import Project, { DataSetType, ProjectAction } from "./Project"; import i18n from "../locales"; import User from "./user"; import { generateUrl } from "../router"; -import { D2Api } from "../types/d2-api"; +import { D2Api, Id } from "../types/d2-api"; import ProjectDb, { ExistingData, getStringDataValue } from "./ProjectDb"; import { baseConfig } from "./Config"; import moment from "moment"; import { appConfig } from "../app-config"; +import { promiseMap } from "../migrations/utils"; +import { Maybe } from "../types/utils"; type Email = string; type Action = ProjectAction; +// Keeps the id filter of the users requests within URL length limits. +const usersPerRequest = 100; + +const countryAdminGroupName = "Country Admin"; + +type DataReviewer = Readonly<{ + id: Id; + email: Maybe; + isDisabled: boolean; + userGroupIds: ReadonlyArray; +}>; + export class ProjectNotification { constructor( private api: D2Api, @@ -32,9 +46,7 @@ export class ProjectNotification { }) .getData(); - const users = _(usersInGroup) - .reject(user => user.userCredentials.disabled) - .value(); + const users = _(usersInGroup).reject(isUserDisabled).value(); return _(appConfig.app.notifyEmailOnProjectSave) .concat(users.map(user => user.email)) @@ -58,18 +70,8 @@ export class ProjectNotification { const res = await this.api.metadata .get({ userRoles: { - fields: { - id: true, - users: { - email: true, - id: true, - userCredentials: { disabled: true }, - userGroups: { - id: true, - name: true, - }, - }, - }, + // Only ids are requested: any other field would be silently dropped by the API. + fields: { id: true, users: { id: true } }, filter: { name: { in: baseConfig.userRoles.dataReviewer } }, }, dataSets: { @@ -86,6 +88,9 @@ export class ProjectNotification { }) .getData(); + const dataSet = res.dataSets[0]; + if (!dataSet) return false; + const { displayName: user, username } = this.currentUser.data; const subject = i18n.t("[SP Platform] Request for Data Review: {{-name}} ({{code}})", { @@ -100,34 +105,36 @@ export class ProjectNotification { const projectId = this.project.id; const path = generateUrl("dataApproval", { id: projectId, dataSetType, period }); const dataApprovalLink = getFullUrl(path); - const dataSet = res.dataSets[0]; - const users = _(res.userRoles) + const reviewerIds = _(res.userRoles) .flatMap(userRole => userRole.users) - .reject(user => user.userCredentials.disabled) + .map(reviewer => reviewer.id) + .uniq() .value(); + const reviewers = await this.getDataReviewers(reviewerIds); + + const sharedUserIds = new Set(dataSet.userAccesses.map(userAccess => userAccess.id)); + const sharedCountryAdminGroupIds = new Set( + dataSet.userGroupAccesses + .filter(userGroupAccess => + userGroupAccess.displayName.includes(countryAdminGroupName) + ) + .map(userGroupAccess => userGroupAccess.id) + ); - const userAccessEmails = users - .filter(user => { - return dataSet.userAccesses.some(userAccess => { - return userAccess.id === user.id; - }); - }) - .map(user => user.email); - - const userGroupEmails = users - .filter(user => { - return dataSet.userGroupAccesses - .filter(ug => ug.displayName.includes("Country Admin")) - .some(userGroupAccess => { - return user.userGroups.some(userGroup => { - return userGroupAccess.id === userGroup.id; - }); - }); - }) - .map(user => user.email); - - const recipients = _.union(userAccessEmails, userGroupEmails); + const recipients = _(reviewers) + .reject(reviewer => reviewer.isDisabled) + .filter( + reviewer => + sharedUserIds.has(reviewer.id) || + reviewer.userGroupIds.some(userGroupId => + sharedCountryAdminGroupIds.has(userGroupId) + ) + ) + .map(reviewer => reviewer.email) + .compact() + .uniq() + .value(); const text = i18n.t( ` @@ -154,6 +161,40 @@ Go to approval screen: {{- projectUrl}}`, return this.sendMessage({ recipients, subject, text: text.trim() }); } + /* DHIS2 serializes users nested in other metadata objects (userRoles.users) with basic fields + only (id, code, name, displayName, username), whichever fields are requested, so the details + needed to notify them must be requested on the top-level users collection. */ + private async getDataReviewers(userIds: Id[]): Promise { + const usersByChunk = await promiseMap(_.chunk(userIds, usersPerRequest), ids => + this.getUsersDetails(ids) + ); + + return _.flatten(usersByChunk); + } + + private async getUsersDetails(userIds: Id[]): Promise { + const { users } = await this.api.metadata + .get({ + users: { + fields: { + id: true, + email: true, + userCredentials: { disabled: true }, + userGroups: { id: true }, + }, + filter: { id: { in: userIds } }, + }, + }) + .getData(); + + return users.map(user => ({ + id: user.id, + email: user.email, + isDisabled: isUserDisabled(user), + userGroupIds: user.userGroups.map(userGroup => userGroup.id), + })); + } + async sendMessageForIndicatorsRemoval(options: { currentUser: User; message: string; @@ -274,13 +315,17 @@ The reason provided by the user was: await api.email.sendMessage({ ...options, recipients }).getData(); return true; } catch (err) { - // If the message could not be sent, just log to the console and continue the process. + // If the message could not be sent, log to the console and let the caller report it. console.error(err); - return true; + return false; } } } +function isUserDisabled(user: { userCredentials?: { disabled?: boolean } }): boolean { + return user.userCredentials?.disabled ?? true; +} + function getProjectUrl(project: Project) { const path = generateUrl("projects", undefined, { search: project.code }); return getFullUrl(path); diff --git a/src/models/__tests__/ProjectNotification.spec.ts b/src/models/__tests__/ProjectNotification.spec.ts new file mode 100644 index 00000000..3a95a623 --- /dev/null +++ b/src/models/__tests__/ProjectNotification.spec.ts @@ -0,0 +1,298 @@ +import _ from "lodash"; + +import { appConfig } from "../../app-config"; +import { getMockApi, Id } from "../../types/d2-api"; +import { Config } from "../Config"; +import Project from "../Project"; +import { ProjectNotification } from "../ProjectNotification"; +import User from "../user"; +import config from "./config"; + +const { api, mock } = getMockApi(); + +const period = "202401"; +const dataSetId = "SdOUI2yT46H"; +const dataSetType = "actual" as const; +const reviewerRoleId = "E5ZNGn2ive3"; +const countryAdminGroupId = "sF8fYSlGLPO"; +const otherGroupId = "mGK0RBQPqAr"; +const emailPath = "/email/notification"; +const metadataPath = "/metadata"; + +/* Users nested in userRoles are serialized by DHIS2 with basic fields only, no matter which fields + are requested (see user_roles.json). */ +type NestedUser = { + id: Id; + code: string | null; + name: string; + displayName: string; + username: string; +}; + +type UserDetails = { + id: Id; + email?: string; + userCredentials?: { disabled: boolean }; + userGroups: Array<{ id: Id }>; +}; + +type DataSetSharing = { + id: Id; + userAccesses: Array<{ id: Id }>; + userGroupAccesses: Array<{ id: Id; displayName: string }>; +}; + +type ApiOptions = { + reviewers: UserDetails[]; + dataSets?: DataSetSharing[]; + emailStatus?: number; +}; + +function getNestedUser(id: Id): NestedUser { + return { id, code: null, name: id, displayName: id, username: id }; +} + +function getReviewer(id: Id, attributes: Partial = {}): UserDetails { + return { + id, + email: `${id}@example.com`, + userCredentials: { disabled: false }, + userGroups: [], + ...attributes, + }; +} + +function getDataSetSharing(attributes: Partial = {}): DataSetSharing { + return { id: dataSetId, userAccesses: [], userGroupAccesses: [], ...attributes }; +} + +function getIdsFromFilter(filters: string[]): Id[] { + const idFilter = filters.find(filter => filter.startsWith("id:in:")) || ""; + const ids = idFilter.replace("id:in:[", "").replace("]", ""); + return _.compact(ids.split(",")); +} + +/* Returns the id filters used on each users request, so tests can assert that details are only + requested for the reviewers, in chunks. */ +function setupApi(options: ApiOptions): { userRequests: Id[][] } { + const { reviewers, dataSets = [getDataSetSharing()], emailStatus = 200 } = options; + const userRequests: Id[][] = []; + mock.reset(); + + mock.onGet(metadataPath).reply(requestConfig => { + const params = requestConfig.params; + + if (params["users:filter"]) { + const ids = getIdsFromFilter(params["users:filter"]); + userRequests.push(ids); + return [200, { users: reviewers.filter(reviewer => ids.includes(reviewer.id)) }]; + } + + return [ + 200, + { + userRoles: [ + { id: reviewerRoleId, users: reviewers.map(({ id }) => getNestedUser(id)) }, + ], + dataSets, + }, + ]; + }); + + mock.onPost(emailPath).reply(emailStatus, {}); + + return { userRequests }; +} + +function getNotificator(): ProjectNotification { + const project = Project.create(api, config) + .set("id", "BvNo8zQaol8") + .set("name", "Project name"); + const currentUser = new User({ + ...config, + currentUser: { ...config.currentUser, userRoles: [{ name: "DM Data Entry" }] }, + } as Config); + + return new ProjectNotification(api, project, currentUser, true); +} + +function notifyForDataReview(): Promise { + return getNotificator().notifyForDataReview(period, dataSetId, dataSetType); +} + +function getSentRecipients(): string[] | undefined { + const request = _.last(mock.history.post); + return request?.params?.recipients; +} + +describe("ProjectNotification.notifyForDataReview", () => { + it("sends the email to reviewers shared with the data set, even though the API returns no details for nested users", async () => { + const reviewer = getReviewer("reviewer1"); + setupApi({ + reviewers: [reviewer, getReviewer("notShared")], + dataSets: [getDataSetSharing({ userAccesses: [{ id: reviewer.id }] })], + }); + + const emailSent = await notifyForDataReview(); + + expect(emailSent).toBe(true); + expect(getSentRecipients()).toEqual(["reviewer1@example.com"]); + }); + + it("requests the details only for the users of the data reviewer role", async () => { + const { userRequests } = setupApi({ + reviewers: [getReviewer("reviewer1"), getReviewer("reviewer2")], + }); + + await notifyForDataReview(); + + expect(userRequests).toEqual([["reviewer1", "reviewer2"]]); + }); + + it("requests the details in chunks of 100 users", async () => { + const reviewers = _.range(250).map(index => getReviewer(`reviewer${index}`)); + const { userRequests } = setupApi({ + reviewers, + dataSets: [getDataSetSharing({ userAccesses: [{ id: "reviewer249" }] })], + }); + + const emailSent = await notifyForDataReview(); + + expect(userRequests.map(ids => ids.length)).toEqual([100, 100, 50]); + expect(emailSent).toBe(true); + expect(getSentRecipients()).toEqual(["reviewer249@example.com"]); + }); + + it("sends the email to reviewers in a country admin group shared with the data set", async () => { + setupApi({ + reviewers: [ + getReviewer("countryAdmin", { userGroups: [{ id: countryAdminGroupId }] }), + getReviewer("otherGroup", { userGroups: [{ id: otherGroupId }] }), + ], + dataSets: [ + getDataSetSharing({ + userGroupAccesses: [ + { id: countryAdminGroupId, displayName: "Country Admin Armenia" }, + { id: otherGroupId, displayName: "Data Viewers" }, + ], + }), + ], + }); + + await notifyForDataReview(); + + expect(getSentRecipients()).toEqual(["countryAdmin@example.com"]); + }); + + it("excludes disabled reviewers", async () => { + setupApi({ + reviewers: [ + getReviewer("enabled"), + getReviewer("disabled", { userCredentials: { disabled: true } }), + ], + dataSets: [ + getDataSetSharing({ userAccesses: [{ id: "enabled" }, { id: "disabled" }] }), + ], + }); + + await notifyForDataReview(); + + expect(getSentRecipients()).toEqual(["enabled@example.com"]); + }); + + it("excludes reviewers whose disabled flag is not returned by the API", async () => { + setupApi({ + reviewers: [ + getReviewer("known"), + getReviewer("unknownStatus", { userCredentials: undefined }), + ], + dataSets: [ + getDataSetSharing({ + userAccesses: [{ id: "known" }, { id: "unknownStatus" }], + }), + ], + }); + + await notifyForDataReview(); + + expect(getSentRecipients()).toEqual(["known@example.com"]); + }); + + it("skips reviewers without email", async () => { + setupApi({ + reviewers: [getReviewer("withEmail"), getReviewer("noEmail", { email: undefined })], + dataSets: [ + getDataSetSharing({ userAccesses: [{ id: "withEmail" }, { id: "noEmail" }] }), + ], + }); + + await notifyForDataReview(); + + expect(getSentRecipients()).toEqual(["withEmail@example.com"]); + }); + + it("sends no email when the data set is not found", async () => { + const { userRequests } = setupApi({ reviewers: [getReviewer("reviewer1")], dataSets: [] }); + + const emailSent = await notifyForDataReview(); + + expect(emailSent).toBe(false); + expect(userRequests).toEqual([]); + expect(mock.history.post).toEqual([]); + }); + + it("sends no email when there are no reviewers", async () => { + const { userRequests } = setupApi({ reviewers: [] }); + + const emailSent = await notifyForDataReview(); + + expect(emailSent).toBe(false); + expect(userRequests).toEqual([]); + expect(mock.history.post).toEqual([]); + }); + + it("returns false when the email request fails", async () => { + setupApi({ + reviewers: [getReviewer("reviewer1")], + dataSets: [getDataSetSharing({ userAccesses: [{ id: "reviewer1" }] })], + emailStatus: 409, + }); + + const emailSent = await notifyForDataReview(); + + expect(emailSent).toBe(false); + }); +}); + +describe("ProjectNotification.getRecipients", () => { + const configuredEmails = appConfig.app.notifyEmailOnProjectSave; + + function setupUsersApi( + users: Array<{ email: string; userCredentials?: { disabled: boolean } }> + ) { + mock.reset(); + mock.onGet(metadataPath).reply(200, { users }); + } + + it("returns the configured emails and the emails of the enabled users in the notification group", async () => { + setupUsersApi([ + { email: "enabled@example.com", userCredentials: { disabled: false } }, + { email: "disabled@example.com", userCredentials: { disabled: true } }, + ]); + + const recipients = await ProjectNotification.getRecipients(api); + + expect(recipients).toEqual([...configuredEmails, "enabled@example.com"]); + }); + + it("excludes users whose disabled flag is not returned by the API", async () => { + setupUsersApi([ + { email: "enabled@example.com", userCredentials: { disabled: false } }, + { email: "unknownStatus@example.com" }, + ]); + + const recipients = await ProjectNotification.getRecipients(api); + + expect(recipients).toEqual([...configuredEmails, "enabled@example.com"]); + }); +}); From 2c2e7a07c26848074e8faac5b0901d803275a8e5 Mon Sep 17 00:00:00 2001 From: Eduardo Peredo Date: Thu, 30 Jul 2026 22:12:40 -0500 Subject: [PATCH 2/2] fix codeql observation --- src/models/__tests__/ProjectNotification.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/models/__tests__/ProjectNotification.spec.ts b/src/models/__tests__/ProjectNotification.spec.ts index 3a95a623..d40dc75b 100644 --- a/src/models/__tests__/ProjectNotification.spec.ts +++ b/src/models/__tests__/ProjectNotification.spec.ts @@ -68,7 +68,7 @@ function getDataSetSharing(attributes: Partial = {}): DataSetSha function getIdsFromFilter(filters: string[]): Id[] { const idFilter = filters.find(filter => filter.startsWith("id:in:")) || ""; - const ids = idFilter.replace("id:in:[", "").replace("]", ""); + const ids = idFilter.match(/^id:in:\[(.*)\]$/)?.[1] || ""; return _.compact(ids.split(",")); }