Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds email notifications for data source tagging jobs and fixes import path case sensitivity issues. The changes enable users to receive email notifications when tagging operations complete or fail, and ensure consistent PascalCase naming for email component files.
Changes:
- Added email notifications (success and failure) for tagDataSource job with userEmail parameter
- Fixed import paths from kebab-case/lowercase to PascalCase for email components (ForgotPassword, Invite, Layout)
- Added syncToCrm feature flag to DataSourceFeatures configuration for ActionNetwork, Airtable, GoogleSheets, and Mailchimp
- Integrated DataSourceFeatures check into MapTable sync-to-crm button visibility logic
- Fixed logger.warning to logger.warn method call
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/server/jobs/tagDataSource.ts | Added email notification support with userEmail parameter, updated error messages and logging |
| src/server/trpc/routers/mapView.ts | Added userEmail to tagDataSource job queue arguments |
| src/server/emails/TaggingComplete.tsx | New email template for successful tagging completion |
| src/server/emails/TaggingFailed.tsx | New email template for tagging failures |
| src/server/emails/invite.tsx | Updated import path from ./layout to ./Layout |
| src/server/emails/ForgotPassword.tsx | Updated import path from ./layout to ./Layout |
| src/server/trpc/routers/invitation.ts | Updated import from @/server/emails/invite to @/server/emails/Invite |
| src/server/trpc/routers/auth.ts | Updated import from @/server/emails/forgot-password to @/server/emails/ForgotPassword |
| bin/cmd.ts | Updated import to @/server/emails/Invite and fixed logger.warning to logger.warn |
| src/features.ts | Added syncToCrm boolean flag to DataSourceFeatures type and configuration |
| src/app/map/[id]/components/table/MapTable.tsx | Moved feature flag check logic to combine with DataSourceFeatures.syncToCrm check, updated toast message |
Comments suppressed due to low confidence (3)
src/server/emails/ForgotPassword.tsx:12
- The import path is being changed from './layout' to './Layout' (PascalCase), but based on the PR diffs, the file 'layout.tsx' does not appear to be renamed to 'Layout.tsx'. This will cause import failures on case-sensitive file systems. The file should be renamed from 'layout.tsx' to 'Layout.tsx' to match this import and align with the naming convention used for other email component files.
src/server/emails/TaggingFailed.tsx:31 - The alt text "Mapped logo" is generic. Consider using more descriptive alt text like "Mapped" since this is a functional logo image that helps identify the sender of the email.
<Img
alt="Mapped logo"
src/server/emails/TaggingComplete.tsx:27
- The alt text "Mapped logo" is generic. Consider using more descriptive alt text like "Mapped" since this is a functional logo image that helps identify the sender of the email.
<Img
alt="Mapped logo"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import z from "zod"; | ||
| import ensureOrganisationMap from "@/server/commands/ensureOrganisationMap"; | ||
| import Invite from "@/server/emails/invite"; | ||
| import Invite from "@/server/emails/Invite"; |
There was a problem hiding this comment.
The import path is being changed to '@/server/emails/Invite', but the file appears to still be named 'invite.tsx' (lowercase) based on the file path in the diff. This import will fail on case-sensitive file systems. The file should be renamed from 'invite.tsx' to 'Invite.tsx' to match this import and align with the naming convention.
| import Invite from "@/server/emails/Invite"; | |
| import Invite from "@/server/emails/invite"; |
| } | ||
| const dataSourceId = String(args.dataSourceId); | ||
| const viewId = String(args.viewId); | ||
| const userEmail = String(args.userEmail); |
There was a problem hiding this comment.
The userEmail parameter is converted to a string without validating that it's a properly formatted email address. If an invalid email is passed, it could result in email sending failures. Consider adding email format validation similar to other email-handling code in the codebase (e.g., using z.string().email() validation).
| @@ -9,7 +9,7 @@ import { | |||
| } from "@react-email/components"; | |||
| import * as React from "react"; | |||
| import { getAbsoluteUrl } from "@/utils/appUrl"; | |||
| import { EmailLayout } from "./layout"; | |||
| import { EmailLayout } from "./Layout"; | |||
There was a problem hiding this comment.
The import path is being changed to use PascalCase 'Invite', but the actual file is still named 'invite.tsx' (lowercase). This import will fail on case-sensitive file systems. Either the file needs to be renamed from 'invite.tsx' to 'Invite.tsx', or the import should remain as '@/server/emails/invite'. Based on the codebase convention where other email component files use PascalCase names (ForgotPassword.tsx, TaggingComplete.tsx), the file should be renamed.
| logger.info(`Data source ${dataSourceId} not found.`); | ||
| const reason = `Failed to tag data source: ${dataSourceId} not found.`; | ||
| logger.warn(reason); | ||
| await sendFailureEmail(userEmail, dataSourceId, viewId, reason); |
There was a problem hiding this comment.
When the data source is not found, the function passes dataSourceId (a UUID) as the dataSourceName parameter and viewId (a UUID) as the viewName parameter. This will result in the user receiving an email with technical IDs instead of human-readable names. Consider passing more user-friendly values or having a fallback message in the email template for unknown entities.
| await sendFailureEmail(userEmail, dataSourceId, viewId, reason); | |
| await sendFailureEmail( | |
| userEmail, | |
| "Unknown data source", | |
| "Unknown view", | |
| reason, | |
| ); |
| logger.info(`View ${viewId} not found.`); | ||
| const reason = `View ${viewId} not found.`; | ||
| logger.warn(`Failed to tag data source ${dataSourceId}: ${reason}`); | ||
| await sendFailureEmail(userEmail, dataSource.name, viewId, reason); |
There was a problem hiding this comment.
When the view is not found, the function passes viewId (a UUID) as the viewName parameter. This will result in the user receiving an email with a technical ID instead of a human-readable name. Consider passing a more user-friendly value or having a fallback message in the email template for unknown views.
| await sendFailureEmail(userEmail, dataSource.name, viewId, reason); | |
| await sendFailureEmail(userEmail, dataSource.name, "Unknown view", reason); |
| const tagDataSource = async (args: object | null): Promise<boolean> => { | ||
| if (!args || !("dataSourceId" in args) || !("viewId" in args)) { | ||
| if ( | ||
| !args || | ||
| !("dataSourceId" in args) || | ||
| !("viewId" in args) || | ||
| !("userEmail" in args) | ||
| ) { | ||
| return false; | ||
| } | ||
| const dataSourceId = String(args.dataSourceId); | ||
| const viewId = String(args.viewId); | ||
| const userEmail = String(args.userEmail); | ||
|
|
||
| const dataSource = await findDataSourceById(dataSourceId); | ||
| if (!dataSource) { | ||
| logger.info(`Data source ${dataSourceId} not found.`); | ||
| const reason = `Failed to tag data source: ${dataSourceId} not found.`; | ||
| logger.warn(reason); | ||
| await sendFailureEmail(userEmail, dataSourceId, viewId, reason); | ||
| return false; | ||
| } | ||
|
|
||
| const view = await findMapViewById(viewId); | ||
| if (!view) { | ||
| logger.info(`View ${viewId} not found.`); | ||
| const reason = `View ${viewId} not found.`; | ||
| logger.warn(`Failed to tag data source ${dataSourceId}: ${reason}`); | ||
| await sendFailureEmail(userEmail, dataSource.name, viewId, reason); | ||
| return false; | ||
| } | ||
|
|
||
| const map = await findMapById(view.mapId); | ||
| if (!map) { | ||
| logger.info(`Map ${view.mapId} not found.`); | ||
| const reason = `Map ${view.mapId} not found.`; | ||
| logger.warn(`Failed to tag data source ${dataSourceId}: ${reason}`); | ||
| await sendFailureEmail(userEmail, dataSource.name, view.name, reason); | ||
| return false; | ||
| } | ||
|
|
||
| const dataSourceView = view.dataSourceViews.find( | ||
| (dsv) => dsv.dataSourceId === dataSourceId, | ||
| ); | ||
|
|
||
| logger.info( | ||
| `Tagging data source ${dataSourceId} with view ${view.name} (${view.id})`, | ||
| ); | ||
|
|
||
| const adaptor = getDataSourceAdaptor(dataSource); | ||
| if (!adaptor) { | ||
| logger.error( | ||
| `Could not get data source adaptor for source ${dataSourceId}, type ${dataSource.config.type}`, | ||
| ); | ||
| const reason = `Could not get data source adaptor for source ${dataSourceId}, type ${dataSource.config.type}`; | ||
| logger.error(reason); | ||
| await sendFailureEmail(userEmail, dataSource.name, view.name, reason); | ||
| return false; | ||
| } | ||
|
|
||
| try { | ||
| let count = 0; | ||
| const total = ( | ||
| await countDataRecordsForDataSource(dataSourceId, null, null) | ||
| ).count; | ||
| const records = streamDataRecordsByDataSource( | ||
| dataSourceId, | ||
| dataSourceView?.filter, | ||
| dataSourceView?.search, | ||
| ); | ||
| const batches = batchAsync(records, DATA_SOURCE_JOB_BATCH_SIZE); | ||
|
|
||
| for await (const batch of batches) { | ||
| const taggedRecords: TaggedRecord[] = batch.map((record) => { | ||
| return { | ||
| externalId: record.externalId, | ||
| json: record.json, | ||
| tag: { | ||
| name: `Mapped View: ${map.name} / ${view.name}`, | ||
| present: Boolean(record.mappedMatched), | ||
| }, | ||
| }; | ||
| }); | ||
| await adaptor.tagRecords(taggedRecords); | ||
| count += batch.length; | ||
| if (total) { | ||
| const percentComplete = Math.floor((count * 100) / total); | ||
| logger.info( | ||
| `Tagged ${count} records of ${total}, ${percentComplete}% complete`, | ||
| ); | ||
| } else { | ||
| logger.info(`Tagged ${count} records`); | ||
| } | ||
| } | ||
|
|
||
| logger.info( | ||
| `Tagged data source ${dataSourceId} with view ${view.name} (${view.id})`, | ||
| ); | ||
|
|
||
| await sendEmail( | ||
| userEmail, | ||
| "Tagging complete", | ||
| TaggingComplete({ | ||
| dataSourceName: dataSource.name, | ||
| viewName: view.name, | ||
| }), | ||
| ); | ||
|
|
||
| return true; | ||
| } catch (error) { | ||
| logger.error( | ||
| `Failed to tag records for ${dataSource.config.type} ${dataSourceId}`, | ||
| { error }, | ||
| ); | ||
| const reason = `Failed to tag records for ${dataSource.config.type} ${dataSourceId}`; | ||
| logger.error(reason, { error }); | ||
| await sendFailureEmail(userEmail, dataSource.name, view.name, reason); | ||
| throw error; | ||
| } | ||
|
|
||
| return false; | ||
| }; |
There was a problem hiding this comment.
The new email notification functionality added to this job lacks test coverage. Given that the repository has comprehensive test coverage for similar job functions (e.g., importDataSource has multiple test cases), consider adding tests for the email notification logic in tagDataSource. This should include test cases for: successful completion emails, failure emails for various error scenarios (data source not found, view not found, adaptor unavailable), and handling of email sending failures.
src/server/jobs/tagDataSource.ts
Outdated
| await sendEmail( | ||
| userEmail, | ||
| "Tagging complete", | ||
| TaggingComplete({ | ||
| dataSourceName: dataSource.name, | ||
| viewName: view.name, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Email sending operations are not wrapped in error handling. If sendFailureEmail or sendEmail throws an exception, the job will fail unexpectedly. Consider wrapping email operations in try-catch blocks and logging errors rather than allowing them to fail the entire job, since the primary job responsibility is tagging records, not sending notifications.
| ); | ||
| const reason = `Failed to tag records for ${dataSource.config.type} ${dataSourceId}`; | ||
| logger.error(reason, { error }); | ||
| await sendFailureEmail(userEmail, dataSource.name, view.name, reason); |
There was a problem hiding this comment.
If sendFailureEmail throws an exception here, it will mask the original error. Consider wrapping this email operation in a try-catch block and logging the email sending error separately, while still re-throwing the original error to maintain proper job failure handling.
| await sendFailureEmail(userEmail, dataSource.name, view.name, reason); | |
| try { | |
| await sendFailureEmail(userEmail, dataSource.name, view.name, reason); | |
| } catch (emailError) { | |
| logger.error("Failed to send failure email for tagDataSource job", { | |
| error: emailError, | |
| }); | |
| } |
| ); | ||
| const reason = `Could not get data source adaptor for source ${dataSourceId}, type ${dataSource.config.type}`; | ||
| logger.error(reason); | ||
| await sendFailureEmail(userEmail, dataSource.name, view.name, reason); |
There was a problem hiding this comment.
If sendFailureEmail throws an exception, this failure path will also fail. Consider wrapping the email operation in a try-catch block to ensure that failures to send email notifications don't prevent proper logging and return of the job failure status.
667f268 to
32ffae7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 12 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/server/emails/TaggingFailed.tsx:12
- The import statements have inconsistent formatting with extra blank lines. For consistency with TaggingComplete.tsx and other email templates in the codebase, remove the extra blank lines between imports.
import * as React from "react";
import { getAbsoluteUrl } from "@/utils/appUrl";
import { EmailLayout } from "./Layout";
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const viewId = String(args.viewId); | ||
| const userEmail = String(args.userEmail); | ||
|
|
||
| const dataSource = await findDataSourceById(dataSourceId); |
There was a problem hiding this comment.
When the data source is not found, the email is sent with the dataSourceId as the dataSourceName parameter. This will display a UUID in the email instead of a user-friendly name. Consider using a placeholder like "Unknown Data Source" or formatting the ID more clearly as "Data Source (ID: ...)" to improve the user experience.
| return false; | ||
| } | ||
|
|
||
| const view = await findMapViewById(viewId); |
There was a problem hiding this comment.
When the view is not found, the email is sent with the viewId as the viewName parameter. This will display a UUID in the email instead of a user-friendly name. Consider using a placeholder like "Unknown View" or formatting the ID more clearly as "View (ID: ...)" to improve the user experience.
No description provided.