Fix: Campaign subject is now interpolated with contact variables#397
Fix: Campaign subject is now interpolated with contact variables#397BenoitPrmt wants to merge 2 commits intousesend:mainfrom
Conversation
|
@BenoitPrmt is attempting to deploy a commit to the kmkoushik's projects Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR extracts contact variable replacement into a shared utility and applies per-contact interpolation to email subjects. campaign-service removes the inline replacement logic and uses replaceContactVariables when creating suppressed and non-suppressed emails. EmailQueueService now conditionally resolves and persists an interpolated subject at send-time for campaign-linked emails. A new test suite validates built-in and custom variables, fallback parsing, dotted lookups, nullable built-ins, and unknown-variable behavior. Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/web/src/server/utils/contact-variable-replacement.unit.test.ts (2)
6-6: 💤 Low valueImport should use the
~/alias per coding guidelines.-} from "./contact-variable-replacement"; +} from "~/server/utils/contact-variable-replacement";As per coding guidelines: "Use the
~/alias for imports from src in apps/web."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/server/utils/contact-variable-replacement.unit.test.ts` at line 6, Update the import in contact-variable-replacement.unit.test.ts to use the project src alias instead of a relative path: replace the current import from "./contact-variable-replacement" with the aliased module path (e.g. "~/server/utils/contact-variable-replacement") so the test imports the same module via the ~/ alias per coding guidelines.
23-59: ⚡ Quick winAdd tests for null built-in fields with fallback and
contact.prefix notation.Two meaningful edge cases are missing:
contact.firstName(orlastName) isnull— the fallback should be used. This is important becausefirstName/lastNameare nullable in Prisma, and the branch at line 113 (if (contactValue && contactValue.length > 0)) relies on the falsy-null guard to emit the fallback.{{contact.firstName}}notation — the regex supports(?:contact\.)?prefix but this path is untested.🧪 Suggested additional tests
+ it("uses fallback when built-in field is null", () => { + const contactWithNullName = { ...baseContact, firstName: null }; + expect( + replaceContactVariables( + "Hello {{firstName,fallback=Friend}}", + contactWithNullName as Contact, + [...BUILT_IN_CONTACT_VARIABLES], + ), + ).toBe("Hello Friend"); + }); + + it("replaces variables with contact. prefix notation", () => { + expect( + replaceContactVariables("Hello {{contact.firstName}}", baseContact, [ + ...BUILT_IN_CONTACT_VARIABLES, + ]), + ).toBe("Hello Benoît"); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/server/utils/contact-variable-replacement.unit.test.ts` around lines 23 - 59, Add unit tests covering nullable built-in fields and the optional contact. prefix: create a contact fixture with firstName (and/or lastName) set to null and assert replaceContactVariables returns the provided fallback when using "{{firstName,fallback=you}}" and also assert the same behavior when using the prefixed form "{{contact.firstName,fallback=you}}"; reference the replaceContactVariables function and BUILT_IN_CONTACT_VARIABLES/ baseContact fixture to locate where to add tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/web/src/server/utils/contact-variable-replacement.unit.test.ts`:
- Line 6: Update the import in contact-variable-replacement.unit.test.ts to use
the project src alias instead of a relative path: replace the current import
from "./contact-variable-replacement" with the aliased module path (e.g.
"~/server/utils/contact-variable-replacement") so the test imports the same
module via the ~/ alias per coding guidelines.
- Around line 23-59: Add unit tests covering nullable built-in fields and the
optional contact. prefix: create a contact fixture with firstName (and/or
lastName) set to null and assert replaceContactVariables returns the provided
fallback when using "{{firstName,fallback=you}}" and also assert the same
behavior when using the prefixed form "{{contact.firstName,fallback=you}}";
reference the replaceContactVariables function and BUILT_IN_CONTACT_VARIABLES/
baseContact fixture to locate where to add tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d3c1841-43c3-44ee-b5bb-93c33d19b626
📒 Files selected for processing (4)
apps/web/src/server/service/campaign-service.tsapps/web/src/server/service/email-queue-service.tsapps/web/src/server/utils/contact-variable-replacement.tsapps/web/src/server/utils/contact-variable-replacement.unit.test.ts
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
1 participant
Closes #396
Campaign subject is now interpolated with contact variables
Summary by cubic
Fixes campaign email subjects so contact variables interpolate per recipient with fallbacks, preventing raw placeholders from being sent. Addresses #396.
Bug Fixes
campaign-serviceand again at send time inemail-queue-service; persist the updated subject when it changes.email,firstName,lastName), contact book variables,contact.prefix, andfallback=with optional whitespace; leave unknown variables unchanged.Refactors
server/utils/contact-variable-replacement; removed duplicate logic and expanded unit tests.Written for commit 0e55612. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests