Updated welcome email renderer to use custom design settings#27094
Updated welcome email renderer to use custom design settings#27094
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request introduces feature-flagged email design customization for member welcome emails. The renderer service now accepts optional 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ghost/core/core/server/services/member-welcome-emails/member-welcome-email-renderer.js (1)
219-243:⚠️ Potential issue | 🟠 MajorResolve wrapper-level URLs before passing them into the template.
linkReplacer.replace()only normalizes URLs inside the Lexical body.header_imageandfooter_contentare injected afterwards, so any relative/content/images/...or#/portal/...URLs stored inemail_design_settingswill go out as broken relative URLs in the final email.🔧 Suggested fix
const managePreferencesUrl = new URL('#/portal/account/newsletters', siteSettings.url).href; const year = new Date().getFullYear(); - const headerImage = useDesignCustomization ? (designSettings.header_image || null) : null; + const headerImage = useDesignCustomization && designSettings.header_image + ? new URL(designSettings.header_image, siteSettings.url).href + : null; + const footerContent = useDesignCustomization && designSettings.footer_content + ? await linkReplacer.replace(designSettings.footer_content, (url) => url, {base: siteSettings.url}) + : null; const showHeaderTitle = useDesignCustomization ? designSettings.show_header_title !== false : false; const showBadge = useDesignCustomization ? designSettings.show_badge !== false : false; const html = this.#wrapperTemplate({ content: contentWithAbsoluteLinks, emailTitle: subjectWithReplacements, subject: subjectWithReplacements, - footerContent: useDesignCustomization ? designSettings.footer_content : null, + footerContent, hasHeaderContent: Boolean(headerImage || showHeaderTitle), headerImage,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/member-welcome-emails/member-welcome-email-renderer.js` around lines 219 - 243, The wrapper-level assets (header_image and footer_content) are not being URL-normalized before passing into this.#wrapperTemplate, so relative URLs stored in designSettings will remain broken; update the code to resolve those before templating by converting headerImage to an absolute URL (e.g., if headerImage is a string use new URL(headerImage, siteSettings.url).href or equivalent) and run linkReplacer.replace on designSettings.footer_content (and any other wrapper HTML like managePreferencesUrl if it can be relative) so footer_content and headerImage are passed into this.#wrapperTemplate as absolute/normalized URLs instead of raw relative values.
🧹 Nitpick comments (1)
ghost/core/test/integration/services/member-welcome-emails.test.js (1)
441-467: Restore the shared design-settings row inside this test.This suite seeds
default-automated-emailonce inbefore. Mutating it inline makes the test order-dependent if more cases are added below later, and it leaves dirty state behind if an assertion throws before suite teardown.♻️ Suggested cleanup
const memberWelcomeEmailService = require('../../../core/server/services/member-welcome-emails/service'); memberWelcomeEmailService.init(); await memberWelcomeEmailService.api.loadMemberWelcomeEmails(); + const originalDesignSettings = await db.knex('email_design_settings') + .where('id', defaultEmailDesignSettingId) + .first('footer_content', 'show_badge'); + + try { await db.knex('email_design_settings') .where('id', defaultEmailDesignSettingId) .update({ footer_content: '<p>Fresh footer content</p>', show_badge: false }); await memberWelcomeEmailService.api.send({ member: { email: 'fresh-design@example.com', name: 'Fresh Design', uuid: '77777777-7777-4777-8777-777777777777' }, memberStatus: 'free' }); sinon.assert.calledOnce(mailService.GhostMailer.prototype.send); const sendCall = mailService.GhostMailer.prototype.send.firstCall; assert.ok(sendCall.args[0].html.includes('Fresh footer content</p>')); assert.equal(sendCall.args[0].html.includes('https://ghost.org/?via=pbg-newsletter'), false); + } finally { + await db.knex('email_design_settings') + .where('id', defaultEmailDesignSettingId) + .update(originalDesignSettings); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/integration/services/member-welcome-emails.test.js` around lines 441 - 467, This test mutates the shared email_design_settings row (identified by defaultEmailDesignSettingId) and leaves global state dirty; before updating via db.knex(...).update(...) in the test, read and store the original row (e.g., const original = await db.knex('email_design_settings').where('id', defaultEmailDesignSettingId').first()) and then after the assertion restore it (or use a try/finally around the update/send/assert block) by writing the original values back with db.knex(...).where('id', defaultEmailDesignSettingId').update(original) so the shared design-settings row is always restored even if an assertion throws. Ensure the code references the same defaultEmailDesignSettingId and the memberWelcomeEmailService.api.send call remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@ghost/core/core/server/services/member-welcome-emails/member-welcome-email-renderer.js`:
- Around line 219-243: The wrapper-level assets (header_image and
footer_content) are not being URL-normalized before passing into
this.#wrapperTemplate, so relative URLs stored in designSettings will remain
broken; update the code to resolve those before templating by converting
headerImage to an absolute URL (e.g., if headerImage is a string use new
URL(headerImage, siteSettings.url).href or equivalent) and run
linkReplacer.replace on designSettings.footer_content (and any other wrapper
HTML like managePreferencesUrl if it can be relative) so footer_content and
headerImage are passed into this.#wrapperTemplate as absolute/normalized URLs
instead of raw relative values.
---
Nitpick comments:
In `@ghost/core/test/integration/services/member-welcome-emails.test.js`:
- Around line 441-467: This test mutates the shared email_design_settings row
(identified by defaultEmailDesignSettingId) and leaves global state dirty;
before updating via db.knex(...).update(...) in the test, read and store the
original row (e.g., const original = await
db.knex('email_design_settings').where('id',
defaultEmailDesignSettingId').first()) and then after the assertion restore it
(or use a try/finally around the update/send/assert block) by writing the
original values back with db.knex(...).where('id',
defaultEmailDesignSettingId').update(original) so the shared design-settings row
is always restored even if an assertion throws. Ensure the code references the
same defaultEmailDesignSettingId and the memberWelcomeEmailService.api.send call
remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5cb03195-69b1-4cba-9a68-4ead4d315990
⛔ Files ignored due to path filters (1)
ghost/core/test/integration/services/__snapshots__/member-welcome-emails-snapshot.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
ghost/core/core/server/services/member-welcome-emails/email-templates/wrapper.hbsghost/core/core/server/services/member-welcome-emails/member-welcome-email-renderer.jsghost/core/core/server/services/member-welcome-emails/service.jsghost/core/test/integration/services/member-welcome-emails-snapshot.test.jsghost/core/test/integration/services/member-welcome-emails.test.jsghost/core/test/unit/server/services/member-welcome-emails/member-welcome-email-renderer.test.jsghost/core/test/unit/server/services/member-welcome-emails/service.test.js
|
|
||
| before(async function () { | ||
| await testUtils.setup('default')(); | ||
| configUtils.set('labs:welcomeEmailsDesignCustomization', false); |
There was a problem hiding this comment.
note: this ensures that existing snapshot tests in this file run with the labs flag disabled. As long as there are no changes to the pre-existing snapshots, we can be confident that this doesn't regress any behavior for current welcome emails.
|
|
||
| describe('labs flag on', function () { | ||
| beforeEach(function () { | ||
| configUtils.set('labs:welcomeEmailsDesignCustomization', true); |
There was a problem hiding this comment.
note: this new describe block runs the same tests as the pre-existing snapshots, but with the labs flag enabled
cmraible
force-pushed
the
chris-ny-1197-use-design-settings-from-the-database-when-rendering-welcome
branch
from
d6f48ef to
3ae338c
Compare
cmraible
changed the base branch from
main
to
disable-welcome-emails-customization-in-tests
cmraible
force-pushed
the
chris-ny-1197-use-design-settings-from-the-database-when-rendering-welcome
branch
from
bd3200d to
3afa747
Compare
|
| const emailStylesSource = fs.readFileSync( | ||
| path.join(__dirname, '../email-service/email-templates/partials/styles.hbs'), | ||
| 'utf8' | ||
| ); | ||
| this.Handlebars.registerPartial('styles', emailStylesSource); |
There was a problem hiding this comment.
I'm not 100% sure this is the right approach here. Basically this uses the styles.hbs from newsletters directly, which includes some cruft that we don't need in welcome emails. It works, but I'm not sure how I feel about this architecturally - maybe we don't want to couple welcome emails and newsletters in this way?
|
Closing in favor of #27191 |
1 participant
closes https://linear.app/ghost/issue/NY-1197/use-design-settings-from-the-database-when-rendering-welcome-emails
Don't be scared of the +2000 lines - it's mostly snapshots