fix: changing nextDigestAt logic to lock to the specific sending days - not just to key off the current time plus one week/month. This should make this more resilient to bugs caused by inconsistent data. Also adding unit tests to cover this function now that it is getting more complicated

Author: Mephistic

functions/src/notifications/helpers.test.ts

@@ -0,0 +1,82 @@
+import { getNextDigestAt } from "./helpers"
+import { Timestamp } from "../firebase"
+import { Frequency } from "../auth/types"
+
+describe("getNextDigestAt", () => {
+  beforeEach(() => {
+    jest.useFakeTimers()
+  })
+
+  afterEach(() => {
+    jest.useRealTimers()
+  })
+
+  it("should return the next Tuesday for weekly frequency", () => {
+    // Set a fixed date: Monday, March 3, 2025
+    const fixedDate = new Date(2025, 2, 3)
+
+    // Next Tuesday, March 4, 2025
+    const expectedDate = new Date(2025, 2, 4)
+
+    jest.setSystemTime(fixedDate)
+
+    const result = getNextDigestAt("Weekly")
+    expect(result).toEqual(Timestamp.fromDate(expectedDate))
+  })
+
+  // Extra test covering this case because getting this
+  // wrong could cause an expensive infinite function loop
+  it("should return the next Tuesday for weekly frequency when the current day is Tuesday", () => {
+    // Set a fixed date: Tuesday, March 4, 2025
+    const fixedDate = new Date(2025, 2, 4)
+
+    // Next Tuesday, March 11, 2025
+    const expectedDate = new Date(2025, 2, 11)
+
+    jest.setSystemTime(fixedDate)
+
+    const result = getNextDigestAt("Weekly")
+    expect(result).toEqual(Timestamp.fromDate(expectedDate))
+  })
+
+  it("should return the next 1st of the month for monthly frequency", () => {
+    // Set a fixed date: March 15, 2025
+    const fixedDate = new Date(2025, 2, 15)
+    // Next 1st of the month, April 1, 2025
+    const expectedDate = new Date(2025, 3, 1)
+
+    jest.setSystemTime(fixedDate)
+
+    const result = getNextDigestAt("Monthly")
+    expect(result).toEqual(Timestamp.fromDate(expectedDate))
+  })
+
+  // Extra test covering this case because getting this
+  // wrong could cause an expensive infinite function loop
+  it("should return the next 1st of the month for monthly frequency when it is the 1st of the month", () => {
+    // Set a fixed date: March 1, 2025
+    const fixedDate = new Date(2025, 2, 1)
+    // Next 1st of the month, April 1, 2025
+    const expectedDate = new Date(2025, 3, 1)
+
+    jest.setSystemTime(fixedDate)
+
+    const result = getNextDigestAt("Monthly")
+    expect(result).toEqual(Timestamp.fromDate(expectedDate))
+  })
+
+  it("should return null for none frequency", () => {
+    const result = getNextDigestAt("None")
+    expect(result).toBeNull()
+  })
+
+  it("should log an error and return null for unknown frequency", () => {
+    const consoleSpy = jest.spyOn(console, "error").mockImplementation()
+    const result = getNextDigestAt("SomeNewFrequency" as Frequency)
+    expect(result).toBeNull()
+    expect(consoleSpy).toHaveBeenCalledWith(
+      "Unknown notification frequency: SomeNewFrequency"
+    )
+    consoleSpy.mockRestore()
+  })
+})

functions/src/notifications/helpers.ts

@@ -1,21 +1,23 @@
-import { Timestamp } from "firebase-admin/firestore"
+import { Timestamp } from "../firebase"
 import { Frequency } from "../auth/types"
-import { startOfDay } from "date-fns"
+import { startOfDay, addDays, addMonths, setDay, setDate } from "date-fns"
 
-// TODO - Unit tests
 export const getNextDigestAt = (notificationFrequency: Frequency) => {
   const now = startOfDay(new Date())
   let nextDigestAt = null
   switch (notificationFrequency) {
     case "Weekly":
-      const weekAhead = new Date(now)
-      weekAhead.setDate(weekAhead.getDate() + 7)
-      nextDigestAt = Timestamp.fromDate(weekAhead)
+      // Weekly notifications are sent on Tuesdays
+      // - this should be the next available Tuesday
+      const nextTuesday = setDay(now, 2, { weekStartsOn: 1 })
+      nextDigestAt = Timestamp.fromDate(
+        nextTuesday <= now ? addDays(nextTuesday, 7) : nextTuesday
+      )
       break
     case "Monthly":
-      const monthAhead = new Date(now)
-      monthAhead.setMonth(monthAhead.getMonth() + 1)
-      nextDigestAt = Timestamp.fromDate(monthAhead)
+      // Monthly notifications are sent on the 1st of the month
+      const nextMonthFirst = setDate(addMonths(now, 1), 1)
+      nextDigestAt = Timestamp.fromDate(nextMonthFirst)
       break
     case "None":
       nextDigestAt = null
@@ -28,6 +30,8 @@ export const getNextDigestAt = (notificationFrequency: Frequency) => {
   return nextDigestAt
 }
 
+// TODO: Rewrite with date-fns for consistency
+// TODO: Unit tests
 export const getNotificationStartDate = (
   notificationFrequency: Frequency,
   now: Timestamp

scripts/firebase-admin/backfillNextDigestAt.ts

@@ -17,9 +17,11 @@ const Args = Record({
   dryRun: Boolean
 })
 
-export const script: Script = async ({ db, auth, args }) => {
+export const script: Script = async ({ db, args }) => {
   const profilesSnapshot = await db.collection("profiles").get()
 
+  console.log(profilesSnapshot.docs.length, "profiles found")
+
   const updatePromises = profilesSnapshot.docs.map(async profileDoc => {
     const profile = profileDoc.data() as Profile
 
@@ -30,7 +32,7 @@ export const script: Script = async ({ db, auth, args }) => {
         await profileDoc.ref.update({ nextDigestAt })
       }
       console.log(
-        `Updated nextDigestAt for ${profileDoc.id} to ${nextDigestAt}`
+        `Updated nextDigestAt for ${profileDoc.id} to ${nextDigestAt?.toDate()}`
       )
     } else {
       console.log(