From 555bedbb6c8152906132fbedee43d8c34593513d Mon Sep 17 00:00:00 2001 From: Dave Richer Date: Mon, 3 Mar 2025 23:11:03 -0500 Subject: [PATCH] feature/IO-3096-GlobalNotifications - Code Review Part 2 --- .../job-watcher-toggle.component.jsx | 22 +++--- server/notifications/eventParser.js | 24 ++---- server/notifications/scenarioParser.js | 79 +++++++++++++------ 3 files changed, 67 insertions(+), 58 deletions(-) diff --git a/client/src/pages/jobs-detail/job-watcher-toggle.component.jsx b/client/src/pages/jobs-detail/job-watcher-toggle.component.jsx index 1dd428091..8b6efbbf7 100644 --- a/client/src/pages/jobs-detail/job-watcher-toggle.component.jsx +++ b/client/src/pages/jobs-detail/job-watcher-toggle.component.jsx @@ -40,9 +40,9 @@ const JobWatcherToggle = ({ job, currentUser, bodyshop }) => { // Extract watchers list const jobWatchers = useMemo(() => watcherData?.job_watchers || [], [watcherData]); - const isWatching = useMemo(() => jobWatchers.some((w) => w.user_email === userEmail), [jobWatchers, userEmail]); - // Add watcher mutation + const isWatching = jobWatchers.some((w) => w.user_email === userEmail); + const [addWatcher, { loading: adding }] = useMutation(ADD_JOB_WATCHER, { refetchQueries: [{ query: GET_JOB_WATCHERS, variables: { jobid } }] }); @@ -81,12 +81,11 @@ const JobWatcherToggle = ({ job, currentUser, bodyshop }) => { ); } - // Clear selection setSelectedWatcher(null); }; const handleTeamSelect = (team) => { - const selectedTeamMembers = JSON.parse(team); // Parse the array of emails + const selectedTeamMembers = JSON.parse(team); const newWatchers = selectedTeamMembers.filter( (email) => !jobWatchers.some((watcher) => watcher.user_email === email) @@ -99,7 +98,6 @@ const JobWatcherToggle = ({ job, currentUser, bodyshop }) => { ); }); - // Clear selection setSelectedTeam(null); }; @@ -147,14 +145,12 @@ const JobWatcherToggle = ({ job, currentUser, bodyshop }) => { style={{ minWidth: "100%" }} options={bodyshop.employees.filter((e) => jobWatchers.every((w) => w.user_email !== e.user_email))} placeholder={t("notifications.labels.employee-search")} - value={selectedWatcher} // Controlled value + value={selectedWatcher} onChange={(value) => { - setSelectedWatcher(value); // Update selected state - handleWatcherSelect(value); // Add watcher logic + setSelectedWatcher(value); + handleWatcherSelect(value); }} /> - {/* Divider for UI separation */} - {/* Only show team selection if there are available teams */} {Enhanced_Payroll && bodyshop?.employee_teams?.length > 0 && ( <> @@ -172,11 +168,11 @@ const JobWatcherToggle = ({ job, currentUser, bodyshop }) => { const employee = bodyshop.employees.find((e) => e.id === member.employeeid); return employee ? employee.user_email : null; }) - .filter(Boolean); // Remove nulls + .filter(Boolean); return { - value: JSON.stringify(teamMembers), // Store array as string - label: team.name // Use team name as label + value: JSON.stringify(teamMembers), + label: team.name }; })} /> diff --git a/server/notifications/eventParser.js b/server/notifications/eventParser.js index 6d9985697..025cbd02d 100644 --- a/server/notifications/eventParser.js +++ b/server/notifications/eventParser.js @@ -3,15 +3,14 @@ * * This function analyzes the differences between previous (`oldData`) and current (`newData`) * data states to identify changed fields. It determines if the event is a new entry or an update - * and optionally extracts a `jobId` based on a specified field. The result includes details - * about changed fields, the event type, and associated metadata. + * and returns details about changed fields, the event type, and associated metadata. * * @param {Object} options - Configuration options for parsing the event. * @param {Object} [options.oldData] - The previous state of the data (undefined for new entries). * @param {Object} options.newData - The current state of the data. * @param {string} options.trigger - The type of event trigger (e.g., 'INSERT', 'UPDATE'). * @param {string} options.table - The name of the table associated with the event. - * @param {string} [options.jobIdField] - The field name used to extract the jobId (optional). + * @param {string} [options.jobId] - The job ID, if already extracted by the caller (optional). * @returns {Object} An object containing the parsed event details: * - {Array} changedFieldNames - List of field names that have changed. * - {Object} changedFields - Map of changed fields with their old and new values. @@ -19,9 +18,9 @@ * - {Object} data - The current data state (`newData`). * - {string} trigger - The event trigger type. * - {string} table - The table name. - * - {string|null} jobId - The extracted jobId or null if not applicable. + * - {string|null} jobId - The provided jobId or null if not provided. */ -const eventParser = async ({ oldData, newData, trigger, table, jobIdField }) => { +const eventParser = async ({ oldData, newData, trigger, table, jobId = null }) => { const isNew = !oldData; // True if no old data exists, indicating a new entry let changedFields = {}; let changedFieldNames = []; @@ -61,19 +60,6 @@ const eventParser = async ({ oldData, newData, trigger, table, jobIdField }) => } } - // Extract jobId if jobIdField is provided - let jobId = null; - if (jobIdField) { - let keyName = jobIdField; - const prefix = "req.body.event.new."; - // Strip prefix if present to isolate the actual field name - if (keyName.startsWith(prefix)) { - keyName = keyName.slice(prefix.length); - } - // Look for jobId in newData first, then fallback to oldData if necessary - jobId = newData[keyName] || (oldData && oldData[keyName]) || null; - } - return { changedFieldNames, // Array of fields that changed changedFields, // Object with old/new values for changed fields @@ -81,7 +67,7 @@ const eventParser = async ({ oldData, newData, trigger, table, jobIdField }) => data: newData, // Current data state trigger, // Event trigger (e.g., 'INSERT', 'UPDATE') table, // Associated table name - jobId // Extracted jobId or null + jobId // Provided jobId or null }; }; diff --git a/server/notifications/scenarioParser.js b/server/notifications/scenarioParser.js index 6ec50eeb4..1a5e2bf97 100644 --- a/server/notifications/scenarioParser.js +++ b/server/notifications/scenarioParser.js @@ -23,7 +23,7 @@ const FILTER_SELF_FROM_WATCHERS = process.env?.FILTER_SELF_FROM_WATCHERS === "tr * Queries job watchers and notification settings before triggering scenario builders. * * @param {Object} req - The request object containing event data, trigger, table, and logger. - * @param {string} jobIdField - The field name used to extract the job ID from the event data. + * @param {string} jobIdField - The field path (e.g., "req.body.event.new.id") to extract the job ID. * @returns {Promise} Resolves when the parsing and notification dispatching process is complete. * @throws {Error} If required request fields (event data, trigger, or table) or body shop data are missing. */ @@ -34,8 +34,9 @@ const scenarioParser = async (req, jobIdField) => { // Validate we know what user committed the action that fired the parser const hasuraUserId = event?.session_variables?.["x-hasura-user-id"]; - // Bail if we don't know + // Bail if we don't know who started the scenario if (!hasuraUserId) { + logger.log("No Hasura user ID found, skipping notification parsing", "info", "notifications"); return; } @@ -44,18 +45,25 @@ const scenarioParser = async (req, jobIdField) => { throw new Error("Missing required request fields: event data, trigger, or table."); } - // Step 1: Parse the event data to extract details like job ID and changed fields - const eventData = await eventParser({ - newData: event.data.new, - oldData: event.data.old, - trigger, - table, - jobIdField - }); + // Step 1a: Extract just the jobId using the provided jobIdField + let jobId = null; + if (jobIdField) { + let keyName = jobIdField; + const prefix = "req.body.event.new."; + if (keyName.startsWith(prefix)) { + keyName = keyName.slice(prefix.length); + } + jobId = event.data.new[keyName] || (event.data.old && event.data.old[keyName]) || null; + } + + if (!jobId) { + logger.log(`No jobId found using path "${jobIdField}", skipping notification parsing`, "info", "notifications"); + return; + } // Step 2: Query job watchers associated with the job ID using GraphQL const watcherData = await gqlClient.request(queries.GET_JOB_WATCHERS, { - jobid: eventData.jobId + jobid: jobId }); // Transform watcher data into a simplified format with email and employee details @@ -73,9 +81,19 @@ const scenarioParser = async (req, jobIdField) => { // Exit early if no job watchers are found for this job if (isEmpty(jobWatchers)) { + logger.log(`No watchers found for jobId "${jobId}", skipping notification parsing`, "info", "notifications"); return; } + // Step 1b: Perform the full event diff now that we know there are watchers + const eventData = await eventParser({ + newData: event.data.new, + oldData: event.data.old, + trigger, + table, + jobId + }); + // Step 3: Extract body shop information from the job data const bodyShopId = watcherData?.job?.bodyshop?.id; const bodyShopName = watcherData?.job?.bodyshop?.shopname; @@ -97,6 +115,11 @@ const scenarioParser = async (req, jobIdField) => { // Exit early if no matching scenarios are identified if (isEmpty(matchingScenarios)) { + logger.log( + `No matching scenarios found for jobId "${jobId}", skipping notification dispatch`, + "info", + "notifications" + ); return; } @@ -117,6 +140,11 @@ const scenarioParser = async (req, jobIdField) => { // Exit early if no notification associations are found if (isEmpty(associationsData?.associations)) { + logger.log( + `No notification associations found for jobId "${jobId}", skipping notification dispatch`, + "info", + "notifications" + ); return; } @@ -150,6 +178,11 @@ const scenarioParser = async (req, jobIdField) => { // Exit early if no scenarios have eligible watchers after filtering if (isEmpty(finalScenarioData?.matchingScenarios)) { + logger.log( + `No eligible watchers after filtering for jobId "${jobId}", skipping notification dispatch`, + "info", + "notifications" + ); return; } @@ -204,35 +237,29 @@ const scenarioParser = async (req, jobIdField) => { ); } - // Exit early if no scenarios are ready to dispatch if (isEmpty(scenariosToDispatch)) { + logger.log(`No scenarios to dispatch for jobId "${jobId}" after building`, "info", "notifications"); return; } - // Step 9: Dispatch email notifications to the email queue + // Step 8: Dispatch email notifications to the email queue const emailsToDispatch = scenariosToDispatch.map((scenario) => scenario?.email); if (!isEmpty(emailsToDispatch)) { - dispatchEmailsToQueue({ - emailsToDispatch, - logger - }).catch((e) => - // Log any errors encountered during email dispatching + dispatchEmailsToQueue({ emailsToDispatch, logger }).catch((e) => logger.log("Something went wrong dispatching emails to the Email Notification Queue", "error", "queue", null, { - message: e?.message + message: e?.message, + stack: e?.stack }) ); } - // Step 10: Dispatch app notifications to the app queue + // Step 9: Dispatch app notifications to the app queue const appsToDispatch = scenariosToDispatch.map((scenario) => scenario?.app); if (!isEmpty(appsToDispatch)) { - dispatchAppsToQueue({ - appsToDispatch, - logger - }).catch((e) => - // Log any errors encountered during app notification dispatching + dispatchAppsToQueue({ appsToDispatch, logger }).catch((e) => logger.log("Something went wrong dispatching apps to the App Notification Queue", "error", "queue", null, { - message: e?.message + message: e?.message, + stack: e?.stack }) ); }