Added open events for transactional emails #22287
Conversation
ref BAE-383 Uses the Ghost Setting to determine whether these are tracked. These are not used in Ghost Admin but just within Mailgun. For self-hosters, this allows for better analytics in the Mailgun interface.
WalkthroughThe changes implement dynamic control over email open tracking across several modules. A new parameter, Suggested labels
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ghost/core/test/e2e-api/members/send-magic-link.test.js (1)
381-421: Tests properly verify open tracking functionality.The new test suite effectively verifies both enabling and disabling of email open tracking. The first test confirms that when
email_track_opensis set totrue, emails include theo:tracking-opens: trueheader. The second test verifies behavior when tracking is disabled.There is a TODO comment on line 415 noting that the test doesn't validate that the
o:tracking-opensheader is unset. Consider enhancing this test to explicitly verify the absence of the header when tracking is disabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ghost/core/core/server/services/newsletters/NewslettersService.js(2 hunks)ghost/core/core/server/services/newsletters/index.js(2 hunks)ghost/core/core/server/services/settings/SettingsBREADService.js(1 hunks)ghost/core/test/e2e-api/members/send-magic-link.test.js(1 hunks)ghost/magic-link/lib/MagicLink.js(3 hunks)ghost/members-api/lib/members-api.js(1 hunks)
🔇 Additional comments (7)
ghost/core/core/server/services/newsletters/index.js (2)
9-9: Good addition of settingsCache module.The addition of the
settingsCachemodule is necessary to enable dynamic control of email open tracking.
26-26: Properly injected settingsCache dependency.The
settingsCacheis correctly provided to the NewslettersService, following good dependency injection principles.ghost/magic-link/lib/MagicLink.js (1)
33-33: Added new optional parameter for dynamic open tracking.The new
getOpenTrackingEnabledparameter is well-documented and properly stored as an instance property.Also applies to: 46-46
ghost/members-api/lib/members-api.js (1)
160-160: Properly implemented open tracking setting access.The implementation correctly passes a function that retrieves the
email_track_openssetting from the settings cache to the MagicLink service.ghost/core/core/server/services/settings/SettingsBREADService.js (1)
82-82: Consistent implementation of open tracking getter.The implementation is consistent with other services, providing the same function to retrieve the email tracking setting from the settings cache.
ghost/core/core/server/services/newsletters/NewslettersService.js (2)
26-26: LGTM! Parameter addition for configurable open tracking.The
settingsCacheparameter is properly added to the constructor options.Also applies to: 31-31
88-88: Good implementation of dynamic open tracking configuration.The
getOpenTrackingEnabledmethod correctly retrieves theemail_track_openssetting from thesettingsCache, enabling dynamic control of email open tracking across the application.
| const mailObject = { | ||
| to: options.email, | ||
| subject: this.getSubject(type), | ||
| text: this.getText(url, type, options.email), | ||
| html: this.getHTML(url, type, options.email) | ||
| }); | ||
| }; | ||
|
|
||
| if (await this.getOpenTrackingEnabled()) { | ||
| mailObject['o:tracking-opens'] = true; | ||
| } | ||
|
|
||
| const info = await this.transporter.sendMail(mailObject); |
There was a problem hiding this comment.
Missing null check for getOpenTrackingEnabled.
The code doesn't check if this.getOpenTrackingEnabled is defined before calling it, which could cause runtime errors if the function is not provided when instantiating MagicLink.
Add a null check before calling the function:
- if (await this.getOpenTrackingEnabled()) {
+ if (this.getOpenTrackingEnabled && await this.getOpenTrackingEnabled()) {
mailObject['o:tracking-opens'] = true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const mailObject = { | |
| to: options.email, | |
| subject: this.getSubject(type), | |
| text: this.getText(url, type, options.email), | |
| html: this.getHTML(url, type, options.email) | |
| }); | |
| }; | |
| if (await this.getOpenTrackingEnabled()) { | |
| mailObject['o:tracking-opens'] = true; | |
| } | |
| const info = await this.transporter.sendMail(mailObject); | |
| const mailObject = { | |
| to: options.email, | |
| subject: this.getSubject(type), | |
| text: this.getText(url, type, options.email), | |
| html: this.getHTML(url, type, options.email) | |
| }; | |
| - if (await this.getOpenTrackingEnabled()) { | |
| + if (this.getOpenTrackingEnabled && await this.getOpenTrackingEnabled()) { | |
| mailObject['o:tracking-opens'] = true; | |
| } | |
| const info = await this.transporter.sendMail(mailObject); |
ref BAE-383
Uses the Ghost Setting to determine whether these are tracked. These are not used in Ghost Admin but just within Mailgun. For self-hosters, this allows for better analytics in the Mailgun interface.