Merge pull request #1716 from Mephistic/email-update

First Notifications Cleanup Pass

Author: mertbagt

.github/workflows/repo-checks.yml

@@ -49,7 +49,7 @@ jobs:
       - uses: actions/checkout@v2
       - uses: ./.github/actions/setup-repo
       - name: Cache Emulators
-        uses: actions/cache@v2
+        uses: actions/cache@v4
         with:
           path: /home/runner/.cache/firebase/emulators
           key: ${{ runner.os }}-firebase-emulators-${{ hashFiles('~/.cache/firebase/emulators/**') }}

extensions/firestore-send-email.env

@@ -1,6 +1,6 @@
 DEFAULT_FROM=[email protected]
-LOCATION=us-east4
-MAIL_COLLECTION=notification_mails
+LOCATION=us-central1
+MAIL_COLLECTION=emails
 SMTP_CONNECTION_URI=smtps://[email protected]:465
 TTL_EXPIRE_TYPE=never
 TTL_EXPIRE_VALUE=1

firebase.json

@@ -41,6 +41,6 @@
     "rules": "storage.rules"
   },
   "extensions": {
-    "firestore-send-email": "firebase/[email protected].34"
+    "firestore-send-email": "firebase/[email protected].36"
   }
 }

firestore.indexes.json

@@ -786,6 +786,20 @@
         { "queryScope": "COLLECTION", "order": "DESCENDING" },
         { "queryScope": "COLLECTION", "arrayConfig": "CONTAINS" }
       ]
+    },
+    {
+      "collectionGroup": "userNotificationFeed",
+      "queryScope": "COLLECTION",
+      "fields": [
+        {
+          "fieldPath": "notification.type",
+          "order": "ASCENDING"
+        },
+        {
+          "fieldPath": "notification.timestamp",
+          "order": "ASCENDING"
+        }
+      ]
     }
   ]
 }

functions/src/email/handlebarsHelpers.ts

@@ -0,0 +1,34 @@
+import * as handlebars from "handlebars"
+import * as fs from "fs"
+import * as path from "path"
+import * as helpers from "./helpers"
+
+const PARTIALS_DIR = "../../lib/email/partials"
+
+// Register Handlebars helper functions
+export const registerHelpers = () => {
+  handlebars.registerHelper("toLowerCase", helpers.toLowerCase)
+  handlebars.registerHelper("noUpdatesFormat", helpers.noUpdatesFormat)
+  handlebars.registerHelper("isDefined", helpers.isDefined)
+}
+
+// Register all Handlebars partials
+export const registerPartials = (directoryPath: string) => {
+  const filenames = fs.readdirSync(directoryPath)
+  filenames.forEach(filename => {
+    const partialPath = path.join(directoryPath, filename)
+    const stats = fs.statSync(partialPath)
+    if (stats.isDirectory()) {
+      registerPartials(partialPath)
+    } else if (stats.isFile() && path.extname(filename) === ".handlebars") {
+      const partialName = path.basename(filename, ".handlebars")
+      const partialContent = fs.readFileSync(partialPath, "utf8")
+      handlebars.registerPartial(partialName, partialContent)
+    }
+  })
+}
+
+export const prepareHandlebars = () => {
+  registerHelpers()
+  registerPartials(path.join(__dirname, PARTIALS_DIR))
+}

functions/src/index.ts

@@ -36,7 +36,6 @@ export {
   populateTestimonySubmissionNotificationEvents,
   cleanupNotifications,
   deliverNotifications,
-  httpsPublishNotifications,
   httpsDeliverNotifications,
   httpsCleanupNotifications,
   updateUserNotificationFrequency

functions/src/notifications/README.md

@@ -42,18 +42,18 @@ The following cloud functions are involved in the notification process:
 1. **deliverNotifications**:
 
    - Sends notifications to users who have a `notificationFrequency` of 'daily' and whose `nextDigestAt` is less than or equal to the current time.
-   - Populates the `notification_mails` collection with a notification document.
+   - Populates the `emails` collection with a notification document.
 
 2. **cleanUpNotifications**:
    - Removes notifications from the users' userNotificationFeed collection that are older than 60 days.
-   - Removes notifications from the events collection that are older than 60 days.
-   - Removes notifications from the notifications_mails collection that are older than 60 days.
+   - Removes notifications from the notificationEvents collection that are older than 60 days.
+   - Removes notifications from the emails collection that are older than 60 days.
 
 ### Database Collections
 
 - `activeTopicSubscriptions`: Stores the active topic subscriptions for users.
-- `notification_mails`: Stores the notification mails sent to users.
-- `events`: Stores the events that trigger notifications.
+- `emails`: Stores the notification mails sent to users.
+- `notificationEvents`: Stores the events that trigger notifications.
 
 ### Query Logic
 
@@ -69,7 +69,7 @@ yarn firebase-admin -e local run-script <name-of-script>
 
 ## Future Considerations
 
-- `publishNotifications` currently listens to the `events` collection but could be extended to include other collections.
+- `publishNotifications` currently listens to the `notificationEvents` collection but could be extended to include other collections.
 - Formatting of fields may need to be changed if scraping of hearings is included.
 
 ## References

functions/src/notifications/cleanupNotifications.ts

@@ -2,48 +2,87 @@ import * as functions from "firebase-functions"
 import * as admin from "firebase-admin"
 import { Timestamp } from "../firebase"
 
+const RETENTION_PERIOD_DAYS = 60
+
 // Get a reference to the Firestore database
 const db = admin.firestore()
 
+// TODO
+// 1.) Do we actually want to delete old notifications? (check with Matt V)
+// 2.) Do we actually want to delete old notificaiton events? Why not keep the history?
+// 3.) Should we just use the builtin TTL feature for all of these?
+// 4.) Should we use a bulk delete operation for firestore for all of these?
+
+// Delete old notifications from userNotificationFeed collections
+const cleanupUserNotificationFeed = async (threshold: Timestamp) => {
+  const notificationsSnapshot = await db
+    .collectionGroup("userNotificationFeed")
+    .where("createdAt", "<", threshold)
+    .get()
+
+  const deleteNotificationPromises = notificationsSnapshot.docs.map(doc =>
+    doc.ref.delete()
+  )
+  await Promise.all(deleteNotificationPromises)
+}
+
+const cleanupNotificationEvents = async (threshold: Timestamp) => {
+  // Delete old topic events from the events collection
+  const notificationEventsSnapshot = await db
+    .collection("notificationEvents")
+    .where("createdAt", "<", threshold)
+    .get()
+
+  const deleteNotificationEventPromises = notificationEventsSnapshot.docs.map(
+    doc => doc.ref.delete()
+  )
+  await Promise.all(deleteNotificationEventPromises)
+}
+
+const cleanupNotificationEmails = async (threshold: Timestamp) => {
+  // Delete old email documents from the emails collection
+  const emailsSnapshot = await db
+    .collection("emails")
+    .where("createdAt", "<", threshold)
+    .get()
+
+  const deleteEmailPromises = emailsSnapshot.docs.map(doc => doc.ref.delete())
+  await Promise.all(deleteEmailPromises)
+}
+
+const runCleanupNotifications = async () => {
+  // todo better date math
+  // Define the time threshold for old notifications, topic events, and email documents
+  const threshold = new Timestamp(
+    Date.now() - RETENTION_PERIOD_DAYS * 24 * 60 * 60 * 1000,
+    0
+  )
+
+  await cleanupUserNotificationFeed(threshold)
+  await cleanupNotificationEvents(threshold)
+  await cleanupNotificationEmails(threshold)
+}
+
 // Define the cleanupNotifications function
 export const cleanupNotifications = functions.pubsub
   .schedule("every 24 hours")
-  .onRun(async context => {
-    // Define the time threshold for old notifications, topic events, and email documents
-    const retentionPeriodDays = 60 // Adjust this value as needed
-    const threshold = new Timestamp(
-      Date.now() - retentionPeriodDays * 24 * 60 * 60 * 1000,
-      0
-    )
-
-    // Delete old notifications from userNotificationFeed collections
-    const notificationsSnapshot = await db
-      .collectionGroup("userNotificationFeed")
-      .where("createdAt", "<", threshold)
-      .get()
-
-    const deleteNotificationPromises = notificationsSnapshot.docs.map(doc =>
-      doc.ref.delete()
-    )
-    await Promise.all(deleteNotificationPromises)
-
-    // Delete old topic events from the events collection
-    const eventsSnapshot = await db
-      .collection("events")
-      .where("createdAt", "<", threshold)
-      .get()
-
-    const deleteTopicEventPromises = eventsSnapshot.docs.map(doc =>
-      doc.ref.delete()
-    )
-    await Promise.all(deleteTopicEventPromises)
-
-    // Delete old email documents from the notifications_mails collection
-    const emailsSnapshot = await db
-      .collection("notifications_mails")
-      .where("createdAt", "<", threshold)
-      .get()
-
-    const deleteEmailPromises = emailsSnapshot.docs.map(doc => doc.ref.delete())
-    await Promise.all(deleteEmailPromises)
-  })
+  .onRun(runCleanupNotifications)
+
+export const httpsCleanupNotifications = functions.https.onRequest(
+  async (request, response) => {
+    try {
+      console.log("httpsCleanupNotifications triggered")
+      await runCleanupNotifications()
+      console.log("DEBUG: cleanupNotifications completed")
+
+      response
+        .status(200)
+        .send(
+          "Successfully cleaned up old notifications, topic events, and email documents"
+        )
+    } catch (error) {
+      console.error("Error in httpsCleanupNotifications:", error)
+      response.status(500).send("Internal server error")
+    }
+  }
+)