From 0c60ad1c82c1e869fc0317faa8ec1ca2b19465b1 Mon Sep 17 00:00:00 2001 From: sean-brydon <55134778+sean-brydon@users.noreply.github.com> Date: Fri, 1 Nov 2024 18:54:34 +0000 Subject: [PATCH] fix: no availability email frequency fix (#17430) * pass teamID to only send to relevant team admins not org admins * a lot more tests aroundthis * improve test cases * change reddis cache key to take into account teamID * add start/endtime to template * fix text in email * add endTime to tests --- .../templates/OrganizationAdminNoSlots.tsx | 17 +- .../organization-admin-no-slots-email.ts | 6 +- .../handleNotificationWhenNoSlots.test.ts | 367 +++++++++++++++++- .../slots/handleNotificationWhenNoSlots.ts | 32 +- .../trpc/server/routers/viewer/slots/util.ts | 2 + 5 files changed, 404 insertions(+), 20 deletions(-) diff --git a/packages/emails/src/templates/OrganizationAdminNoSlots.tsx b/packages/emails/src/templates/OrganizationAdminNoSlots.tsx index 26000982c8..d0da37e2aa 100644 --- a/packages/emails/src/templates/OrganizationAdminNoSlots.tsx +++ b/packages/emails/src/templates/OrganizationAdminNoSlots.tsx @@ -11,7 +11,9 @@ export type OrganizationAdminNoSlotsEmailInput = { user: string; slug: string; startTime: string; + endTime: string; editLink: string; + teamSlug: string; }; export const OrganizationAdminNoSlotsEmail = ( @@ -33,14 +35,19 @@ export const OrganizationAdminNoSlotsEmail = (

Please note: It has been brought to our attention that {props.user} has not had any availability - when a user has visited {props.user}/{props.slug} + when a user has visited {props.teamSlug}/{props.slug}. +
+ Start time: {props.startTime} +
+ End time: {props.endTime}

- There’s a few reasons why this could be happening + There’s a few reasons why this could be happening:
- The user does not have any calendars connected -
- Their schedules attached to this event are not enabled +

diff --git a/packages/emails/templates/organization-admin-no-slots-email.ts b/packages/emails/templates/organization-admin-no-slots-email.ts index e929b2fc50..3c8e16fc31 100644 --- a/packages/emails/templates/organization-admin-no-slots-email.ts +++ b/packages/emails/templates/organization-admin-no-slots-email.ts @@ -13,6 +13,8 @@ export type OrganizationAdminNoSlotsEmailInput = { user: string; slug: string; startTime: string; + endTime: string; + teamSlug: string; editLink: string; }; @@ -42,8 +44,8 @@ Hi Admins, It has been brought to our attention that ${this.adminNoSlots.user} has not had availability users have visited ${this.adminNoSlots.user}/${this.adminNoSlots.slug}. There’s a few reasons why this could be happening -The user does not have any calendars connected -Their schedules attached to this event are not enabled +* The user does not have any calendars connected +* Their schedules attached to this event are not enabled We recommend checking their availability to resolve this `; diff --git a/packages/trpc/server/routers/viewer/slots/handleNotificationWhenNoSlots.test.ts b/packages/trpc/server/routers/viewer/slots/handleNotificationWhenNoSlots.test.ts index 66748345a3..202e3de41c 100644 --- a/packages/trpc/server/routers/viewer/slots/handleNotificationWhenNoSlots.test.ts +++ b/packages/trpc/server/routers/viewer/slots/handleNotificationWhenNoSlots.test.ts @@ -77,21 +77,22 @@ describe("(Orgs) Send admin notifications when a user has no availability", () = username: "user1", eventSlug: "event1", startTime: dayjs(), // Mocking Dayjs format function + endTime: dayjs().add(1, "hour"), }; const orgDetails = { currentOrgDomain: "org1", isValidOrgDomain: true, }; - // Call the function - await handleNotificationWhenNoSlots({ eventDetails, orgDetails }); + // Call the function with teamId + await handleNotificationWhenNoSlots({ eventDetails, orgDetails, teamId: 123 }); expect(CalcomEmails.sendOrganizationAdminNoSlotsNotification).not.toHaveBeenCalled(); // Mock length to be one then recall to trigger email mocked.lrange.mockResolvedValueOnce([""]); - await handleNotificationWhenNoSlots({ eventDetails, orgDetails }); + await handleNotificationWhenNoSlots({ eventDetails, orgDetails, teamId: 123 }); expect(CalcomEmails.sendOrganizationAdminNoSlotsNotification).toHaveBeenCalled(); }); it("Should not send a notification if the org has them disabled", async () => { @@ -106,15 +107,371 @@ describe("(Orgs) Send admin notifications when a user has no availability", () = username: "user1", eventSlug: "event1", startTime: dayjs(), // Mocking Dayjs format function + endTime: dayjs().add(1, "hour"), }; const orgDetails = { currentOrgDomain: "org1", isValidOrgDomain: true, }; - // Call the function - await handleNotificationWhenNoSlots({ eventDetails, orgDetails }); + await handleNotificationWhenNoSlots({ eventDetails, orgDetails, teamId: 123 }); expect(CalcomEmails.sendOrganizationAdminNoSlotsNotification).not.toHaveBeenCalled(); }); + it("Should only send notifications to admins of the specified teamId", async () => { + const redisService = new RedisService(); + const mocked = vi.mocked(redisService); + + prismaMock.team.findFirst.mockResolvedValue({ + organizationSettings: { + adminGetsNoSlotsNotification: true, + }, + }); + + // Mock finding team members + prismaMock.membership.findMany.mockResolvedValue([ + { + user: { + email: "correctteam@test.com", + locale: "en", + }, + }, + ]); + + const eventDetails = { + username: "user1", + eventSlug: "event1", + startTime: dayjs(), + endTime: dayjs().add(1, "hour"), + }; + + const orgDetails = { + currentOrgDomain: "org1", + isValidOrgDomain: true, + }; + + // Mock Redis to simulate second no-slots occurrence + mocked.lrange.mockResolvedValueOnce([""]); // This will trigger email sending + + // Call with specific teamId + await handleNotificationWhenNoSlots({ + eventDetails, + orgDetails, + teamId: 123, // specific teamId + }); + + // Verify that membership query included correct teamId + expect(prismaMock.membership.findMany).toHaveBeenCalledWith( + expect.objectContaining({ + where: expect.objectContaining({ + team: expect.objectContaining({ + id: 123, + }), + }), + }) + ); + + // Verify email was sent only once (to the one correct team member) + expect(CalcomEmails.sendOrganizationAdminNoSlotsNotification).toHaveBeenCalledTimes(1); + expect(CalcomEmails.sendOrganizationAdminNoSlotsNotification).toHaveBeenCalledWith( + expect.objectContaining({ + to: { + email: "correctteam@test.com", + }, + }) + ); + }); + + it("Should not send notifications when no teamId is provided", async () => { + const redisService = new RedisService(); + const mocked = vi.mocked(redisService); + + prismaMock.team.findFirst.mockResolvedValue({ + organizationSettings: { + adminGetsNoSlotsNotification: true, + }, + }); + + const eventDetails = { + username: "user1", + eventSlug: "event1", + startTime: dayjs(), + endTime: dayjs().add(1, "hour"), + }; + + const orgDetails = { + currentOrgDomain: "org1", + isValidOrgDomain: true, + }; + + mocked.lrange.mockResolvedValueOnce([""]); + + await handleNotificationWhenNoSlots({ + eventDetails, + orgDetails, + // teamId intentionally omitted + }); + + expect(prismaMock.membership.findMany).not.toHaveBeenCalled(); + expect(CalcomEmails.sendOrganizationAdminNoSlotsNotification).not.toHaveBeenCalled(); + }); + + it("Should not send notifications when no orgDomain is provided", async () => { + const redisService = new RedisService(); + const mocked = vi.mocked(redisService); + + const eventDetails = { + username: "user1", + eventSlug: "event1", + startTime: dayjs(), + endTime: dayjs().add(1, "hour"), + }; + + const orgDetails = { + currentOrgDomain: null, // No org domain + isValidOrgDomain: true, + }; + + mocked.lrange.mockResolvedValueOnce([""]); + + await handleNotificationWhenNoSlots({ + eventDetails, + orgDetails, + teamId: 123, + }); + + expect(prismaMock.team.findFirst).not.toHaveBeenCalled(); + expect(prismaMock.membership.findMany).not.toHaveBeenCalled(); + expect(CalcomEmails.sendOrganizationAdminNoSlotsNotification).not.toHaveBeenCalled(); + }); + + it("Should not send notifications when Redis environment variables are not set", async () => { + // Temporarily unset Redis env vars + const originalToken = process.env.UPSTASH_REDIS_REST_TOKEN; + const originalUrl = process.env.UPSTASH_REDIS_REST_URL; + delete process.env.UPSTASH_REDIS_REST_TOKEN; + delete process.env.UPSTASH_REDIS_REST_URL; + + const eventDetails = { + username: "user1", + eventSlug: "event1", + startTime: dayjs(), + endTime: dayjs().add(1, "hour"), + }; + + const orgDetails = { + currentOrgDomain: "org1", + isValidOrgDomain: true, + }; + + await handleNotificationWhenNoSlots({ + eventDetails, + orgDetails, + teamId: 123, + }); + + expect(prismaMock.team.findFirst).not.toHaveBeenCalled(); + expect(prismaMock.membership.findMany).not.toHaveBeenCalled(); + expect(CalcomEmails.sendOrganizationAdminNoSlotsNotification).not.toHaveBeenCalled(); + + // Restore env vars + process.env.UPSTASH_REDIS_REST_TOKEN = originalToken; + process.env.UPSTASH_REDIS_REST_URL = originalUrl; + }); + + it("Should handle multiple admins correctly", async () => { + const redisService = new RedisService(); + const mocked = vi.mocked(redisService); + + prismaMock.team.findFirst.mockResolvedValue({ + organizationSettings: { + adminGetsNoSlotsNotification: true, + }, + }); + + // Mock multiple team admins + prismaMock.membership.findMany.mockResolvedValue([ + { + user: { + email: "admin1@test.com", + locale: "en", + }, + }, + { + user: { + email: "admin2@test.com", + locale: "fr", + }, + }, + ]); + + const eventDetails = { + username: "user1", + eventSlug: "event1", + startTime: dayjs(), + endTime: dayjs().add(1, "hour"), + }; + + const orgDetails = { + currentOrgDomain: "org1", + isValidOrgDomain: true, + }; + + mocked.lrange.mockResolvedValueOnce([""]); + + await handleNotificationWhenNoSlots({ + eventDetails, + orgDetails, + teamId: 123, + }); + + // Verify emails were sent to both admins + expect(CalcomEmails.sendOrganizationAdminNoSlotsNotification).toHaveBeenCalledTimes(2); + expect(CalcomEmails.sendOrganizationAdminNoSlotsNotification).toHaveBeenCalledWith( + expect.objectContaining({ + to: { + email: "admin1@test.com", + }, + }) + ); + expect(CalcomEmails.sendOrganizationAdminNoSlotsNotification).toHaveBeenCalledWith( + expect.objectContaining({ + to: { + email: "admin2@test.com", + }, + }) + ); + }); + + it("Should not send duplicate notifications within NO_SLOTS_NOTIFICATION_FREQUENCY", async () => { + const redisService = new RedisService(); + const mocked = vi.mocked(redisService); + + prismaMock.team.findFirst.mockResolvedValue({ + organizationSettings: { + adminGetsNoSlotsNotification: true, + }, + }); + + prismaMock.membership.findMany.mockResolvedValue([ + { + user: { + email: "admin@test.com", + locale: "en", + }, + }, + ]); + + const eventDetails = { + username: "user1", + eventSlug: "event1", + startTime: dayjs(), + endTime: dayjs().add(1, "hour"), + }; + + const orgDetails = { + currentOrgDomain: "org1", + isValidOrgDomain: true, + }; + + // First notification cycle - simulate having one previous occurrence + mocked.lrange.mockResolvedValueOnce([""]); // One previous occurrence + await handleNotificationWhenNoSlots({ + eventDetails, + orgDetails, + teamId: 123, + }); + + // Verify first notification was sent + expect(CalcomEmails.sendOrganizationAdminNoSlotsNotification).toHaveBeenCalledTimes(1); + + // Reset call counts + vi.clearAllMocks(); + + // For the second attempt, simulate having TWO occurrences already in Redis + // This better simulates the real Redis state after the first notification + mocked.lrange.mockResolvedValueOnce(["", ""]); // Two occurrences now + + await handleNotificationWhenNoSlots({ + eventDetails, + orgDetails, + teamId: 123, + }); + + // Verify no additional notifications were sent + expect(CalcomEmails.sendOrganizationAdminNoSlotsNotification).not.toHaveBeenCalled(); + + // Verify Redis operations + expect(mocked.lpush).toHaveBeenCalledTimes(1); // Still records the occurrence + }); + + it("Should maintain separate notification frequencies for different event types", async () => { + const redisService = new RedisService(); + const mocked = vi.mocked(redisService); + + prismaMock.team.findFirst.mockResolvedValue({ + organizationSettings: { + adminGetsNoSlotsNotification: true, + }, + }); + + prismaMock.membership.findMany.mockResolvedValue([ + { + user: { + email: "admin@test.com", + locale: "en", + }, + }, + ]); + + const baseEventDetails = { + username: "user1", + startTime: dayjs(), + endTime: dayjs().add(1, "hour"), + }; + + const orgDetails = { + currentOrgDomain: "org1", + isValidOrgDomain: true, + }; + + // First event type notification + mocked.lrange.mockResolvedValueOnce([""]); // Simulate one previous occurrence for first event + await handleNotificationWhenNoSlots({ + eventDetails: { ...baseEventDetails, eventSlug: "event1" }, + orgDetails, + teamId: 123, + }); + + // Verify first notification was sent + expect(CalcomEmails.sendOrganizationAdminNoSlotsNotification).toHaveBeenCalledTimes(1); + + // Reset only the email mock, keep Redis mocks + vi.mocked(CalcomEmails.sendOrganizationAdminNoSlotsNotification).mockClear(); + + // For the second event type, also simulate one previous occurrence + // This needs to be a separate mock since it's a different key in Redis + mocked.lrange.mockResolvedValueOnce([""]); // Simulate one previous occurrence for second event + await handleNotificationWhenNoSlots({ + eventDetails: { ...baseEventDetails, eventSlug: "event2" }, + orgDetails, + teamId: 123, + }); + + // Verify second notification was sent (different event type) + expect(CalcomEmails.sendOrganizationAdminNoSlotsNotification).toHaveBeenCalledTimes(1); + + // Get all lpush calls + const lpushCalls = mocked.lpush.mock.calls; + expect(lpushCalls.length).toBe(2); + + // Extract the Redis keys used for each event + const firstEventKey = lpushCalls[0][0]; + const secondEventKey = lpushCalls[1][0]; + + // Verify different keys were used + expect(firstEventKey).not.toBe(secondEventKey); + expect(firstEventKey).toContain("event1"); + expect(secondEventKey).toContain("event2"); + }); }); diff --git a/packages/trpc/server/routers/viewer/slots/handleNotificationWhenNoSlots.ts b/packages/trpc/server/routers/viewer/slots/handleNotificationWhenNoSlots.ts index 6434af59ae..0318cbff37 100644 --- a/packages/trpc/server/routers/viewer/slots/handleNotificationWhenNoSlots.ts +++ b/packages/trpc/server/routers/viewer/slots/handleNotificationWhenNoSlots.ts @@ -9,6 +9,7 @@ type EventDetails = { username: string; eventSlug: string; startTime: Dayjs; + endTime: Dayjs; visitorTimezone?: string; visitorUid?: string; }; @@ -21,9 +22,9 @@ const NO_SLOTS_NOTIFICATION_FREQUENCY = IS_PRODUCTION ? 7 * 24 * 3600 : 60; const NO_SLOTS_COUNT_FOR_NOTIFICATION = 2; -const constructRedisKey = (eventDetails: EventDetails, orgSlug?: string) => { +const constructRedisKey = (eventDetails: EventDetails, orgSlug?: string, teamId?: number) => { return `${REDIS_KEY_VERSION}.${eventDetails.username}:${eventDetails.eventSlug}${ - orgSlug ? `@${orgSlug}` : "" + orgSlug ? `@${orgSlug}:team_${teamId}` : "" }`; }; @@ -40,12 +41,14 @@ const constructDataHash = (eventDetails: EventDetails) => { export const handleNotificationWhenNoSlots = async ({ eventDetails, orgDetails, + teamId, }: { eventDetails: EventDetails; orgDetails: { currentOrgDomain: string | null }; + teamId?: number; }) => { // Check for org - if (!orgDetails.currentOrgDomain) return; + if (!orgDetails.currentOrgDomain || !teamId) return; const UPSTASH_ENV_FOUND = process.env.UPSTASH_REDIS_REST_TOKEN && process.env.UPSTASH_REDIS_REST_URL; if (!UPSTASH_ENV_FOUND) return; @@ -68,7 +71,7 @@ export const handleNotificationWhenNoSlots = async ({ const redis = new RedisService(); - const usersUniqueKey = constructRedisKey(eventDetails, orgDetails.currentOrgDomain); + const usersUniqueKey = constructRedisKey(eventDetails, orgDetails.currentOrgDomain, teamId); // Get only the required amount of data so the request is as small as possible // We may need to get more data and check the startDate occurrence of this // Not trigger email if the start months are the same @@ -82,12 +85,14 @@ export const handleNotificationWhenNoSlots = async ({ // We add one as we know we just added one to the list - saves us re-fetching the data if (usersExistingNoSlots.length + 1 === NO_SLOTS_COUNT_FOR_NOTIFICATION) { - // Get all org admins to send the email too + // Get all team admins to send the email too const foundAdmins = await prisma.membership.findMany({ where: { team: { - slug: orgDetails.currentOrgDomain, - isOrganization: true, + id: teamId, + parentId: { + not: null, + }, }, role: { in: ["ADMIN", "OWNER"], @@ -102,6 +107,15 @@ export const handleNotificationWhenNoSlots = async ({ }, }, }); + + const teamSlug = await prisma.team.findUnique({ + where: { + id: teamId, + }, + select: { + slug: true, + }, + }); // TODO: use new tasker as we dont want this blocking loading slots (Just out of scope for this PR) // Tasker isn't 100% working with emails - will refactor after i have made changes to Tasker in another PR. const emailsToSend: Array> = []; @@ -116,9 +130,11 @@ export const handleNotificationWhenNoSlots = async ({ }, user: eventDetails.username, slug: eventDetails.eventSlug, - startTime: eventDetails.startTime.format("YYYY-MM"), + startTime: eventDetails.startTime.format("YYYY-MM-DD"), + endTime: eventDetails.endTime.format("YYYY-MM-DD"), // For now navigate here - when impersonation via parameter has been pushed we will impersonate and then navigate to availability editLink: `${WEBAPP_URL}/availability?type=team`, + teamSlug: teamSlug?.slug ?? "", }; emailsToSend.push(sendOrganizationAdminNoSlotsNotification(payload)); diff --git a/packages/trpc/server/routers/viewer/slots/util.ts b/packages/trpc/server/routers/viewer/slots/util.ts index 4a6304ca50..cf49e4812f 100644 --- a/packages/trpc/server/routers/viewer/slots/util.ts +++ b/packages/trpc/server/routers/viewer/slots/util.ts @@ -846,9 +846,11 @@ async function _getAvailableSlots({ input, ctx }: GetScheduleOptions): Promise