Merge pull request #1737 from Mephistic/fix-next-digest-at

Tweak nextDigestAt Logic

Author: Mephistic

.github/workflows/repo-checks.yml

@@ -19,10 +19,10 @@ jobs:
         if: always()
         run: yarn check-formatting
 
-      # TODO: Add unit tests
-      # - name: Unit Tests
-      #   if: always()
-      #   run: yarn test
+      # TODO: Fix non-functions unit tests
+      - name: Unit Tests
+        if: always()
+        run: yarn test
 
       - name: Types
         if: always()

functions/src/notifications/helpers.test.ts

@@ -0,0 +1,100 @@
+import { getNextDigestAt, getNotificationStartDate } 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()
+  })
+})
+
+describe("getNotificationStartDate", () => {
+  it("should return the previous Tuesday for weekly frequency", () => {
+    const fixedDate = Timestamp.fromDate(new Date(2025, 2, 11))
+    const expectedDate = Timestamp.fromDate(new Date(2025, 2, 4))
+
+    const result = getNotificationStartDate("Weekly", fixedDate)
+    expect(result).toEqual(expectedDate)
+  })
+
+  it("should return the previous 1st of the month for monthly frequency", () => {
+    const fixedDate = Timestamp.fromDate(new Date(2025, 2, 1))
+    const expectedDate = Timestamp.fromDate(new Date(2025, 1, 1))
+
+    const result = getNotificationStartDate("Monthly", fixedDate)
+    expect(result).toEqual(expectedDate)
+  })
+})

functions/src/notifications/helpers.ts

@@ -1,21 +1,25 @@
-import { Timestamp } from "firebase-admin/firestore"
+import { Timestamp } from "../firebase"
 import { Frequency } from "../auth/types"
-import { startOfDay } from "date-fns"
+import {
+  startOfDay,
+  addMonths,
+  setDate,
+  previousTuesday,
+  nextTuesday,
+  subMonths
+} 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)
+      nextDigestAt = Timestamp.fromDate(nextTuesday(now))
       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
@@ -34,13 +38,11 @@ export const getNotificationStartDate = (
 ) => {
   switch (notificationFrequency) {
     case "Weekly":
-      const weekAgo = new Date(now.toDate())
-      weekAgo.setDate(weekAgo.getDate() - 7)
-      return Timestamp.fromDate(weekAgo)
+      return Timestamp.fromDate(previousTuesday(now.toDate()))
     case "Monthly":
-      const monthAgo = new Date(now.toDate())
-      monthAgo.setMonth(monthAgo.getMonth() - 1)
-      return Timestamp.fromDate(monthAgo)
+      const firstOfMonth = setDate(now.toDate(), 1)
+      const previousFirstOfMonth = subMonths(firstOfMonth, 1)
+      return Timestamp.fromDate(previousFirstOfMonth)
     // We can safely fallthrough here because if the user has no notification frequency set,
     // we won't even send a notification
     default:

package.json

@@ -34,6 +34,7 @@
     "lint:fix": "next lint --fix",
     "serve": "firebase emulators:start --only hosting",
     "start": "next start",
+    "test": "yarn --cwd functions test",
     "test:e2e": "playwright test --ui",
     "test:e2e:headless": "playwright test",
     "test:integration": "jest -c tests/jest.integration.config.ts -w 1",

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(