Added email analytics adapter support with Mailgun implementation #25252
Conversation
ref TryGhost#22771 This establishes the foundation for multi-provider email support using Ghost's existing AdapterManager pattern. The EmailProviderBase class defines the contract that all email provider adapters must follow, enabling community-developed providers to be published as npm packages. - Created EmailProviderBase with required send() method - Registered email adapter type in AdapterManager - Added comprehensive tests validating base class contract
|
Warning Rate limit exceeded@danielraffel has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 44 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (17)
WalkthroughAdds an adapter-based email analytics system: a new EmailAnalyticsBase class defines the required fetchLatest() contract; a MailgunEmailAnalyticsProvider adapter implements that contract in a new adapters path; the legacy EmailAnalyticsProviderMailgun implementation and its tests were removed. Adapter manager now registers an Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
- Moved require('@tryghost/errors') to top of EmailProviderBase
- Removed duplicate @returns JSDoc tag
- Updated AdapterManager JSDoc to include 'email' adapter type
- Renamed test suite from 'integration with AdapterManager' to 'module exports'
![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/unit/server/adapters/email-analytics/mailgun/index.test.js (1)
217-224: Consider adding test for undefined options.While the test at lines 217-224 covers empty options object
{}, consider adding a test case for whenoptionsisundefinedto fully verify the optional parameter contract.Add this test case:
it('should work when options is undefined', async function () { await adapter.fetchLatest(batchHandler); const mailgunOptions = mailgunClient.fetchEvents.firstCall.args[0]; mailgunOptions.event.should.equal('delivered OR opened OR failed OR unsubscribed OR complained'); mailgunOptions.tags.should.equal('bulk-email'); should.not.exist(mailgunOptions.begin); should.not.exist(mailgunOptions.end); const options = mailgunClient.fetchEvents.firstCall.args[2]; should.not.exist(options.maxEvents); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
ghost/core/core/server/adapters/email-analytics/EmailAnalyticsBase.js(1 hunks)ghost/core/core/server/adapters/email-analytics/mailgun/index.js(1 hunks)ghost/core/core/server/services/adapter-manager/config.js(1 hunks)ghost/core/core/server/services/adapter-manager/index.js(1 hunks)ghost/core/core/server/services/email-analytics/EmailAnalyticsProviderMailgun.js(0 hunks)ghost/core/core/server/services/email-analytics/EmailAnalyticsServiceWrapper.js(2 hunks)ghost/core/test/unit/server/adapters/email-analytics/EmailAnalyticsBase.test.js(1 hunks)ghost/core/test/unit/server/adapters/email-analytics/mailgun/index.test.js(1 hunks)ghost/core/test/unit/server/services/email-analytics/EmailAnalyticsProviderMailgun.test.js(0 hunks)
💤 Files with no reviewable changes (2)
- ghost/core/core/server/services/email-analytics/EmailAnalyticsProviderMailgun.js
- ghost/core/test/unit/server/services/email-analytics/EmailAnalyticsProviderMailgun.test.js
🧰 Additional context used
📓 Path-based instructions (2)
ghost/core/core/server/**
📄 CodeRabbit inference engine (AGENTS.md)
Backend core logic should reside under ghost/core/core/server/
Files:
ghost/core/core/server/services/adapter-manager/config.jsghost/core/core/server/services/email-analytics/EmailAnalyticsServiceWrapper.jsghost/core/core/server/adapters/email-analytics/EmailAnalyticsBase.jsghost/core/core/server/adapters/email-analytics/mailgun/index.jsghost/core/core/server/services/adapter-manager/index.js
ghost/core/core/server/services/**
📄 CodeRabbit inference engine (AGENTS.md)
Backend services should be implemented under ghost/core/core/server/services/
Files:
ghost/core/core/server/services/adapter-manager/config.jsghost/core/core/server/services/email-analytics/EmailAnalyticsServiceWrapper.jsghost/core/core/server/services/adapter-manager/index.js
🧠 Learnings (1)
📚 Learning: 2025-10-06T14:19:50.385Z
Learnt from: sam-lord
PR: TryGhost/Ghost#24966
File: ghost/core/test/unit/server/services/mail/GhostMailer.test.js:330-421
Timestamp: 2025-10-06T14:19:50.385Z
Learning: The GhostMailer class in `ghost/core/core/server/services/mail/GhostMailer.js` does not accept external Mailgun-specific parameters (like `o:tag`) from callers. Tags are generated internally via the `getTags()` method. This design keeps the class generic and avoids mixing transport-specific concerns into the public interface.
Applied to files:
ghost/core/core/server/adapters/email-analytics/mailgun/index.js
🧬 Code graph analysis (7)
ghost/core/test/unit/server/adapters/email-analytics/EmailAnalyticsBase.test.js (2)
ghost/core/test/unit/server/adapters/email-analytics/mailgun/index.test.js (2)
should(1-1)EmailAnalyticsBase(4-4)ghost/core/core/server/adapters/email-analytics/mailgun/index.js (1)
EmailAnalyticsBase(3-3)
ghost/core/core/server/services/adapter-manager/config.js (1)
ghost/core/core/server/services/adapter-manager/index.js (1)
adapterServiceConfig(29-29)
ghost/core/core/server/services/email-analytics/EmailAnalyticsServiceWrapper.js (1)
ghost/core/core/server/services/adapter-manager/index.js (1)
adapterManager(6-13)
ghost/core/core/server/adapters/email-analytics/EmailAnalyticsBase.js (3)
ghost/core/core/server/adapters/email-analytics/mailgun/index.js (1)
EmailAnalyticsBase(3-3)ghost/core/test/unit/server/adapters/email-analytics/EmailAnalyticsBase.test.js (1)
EmailAnalyticsBase(2-2)ghost/core/test/unit/server/adapters/email-analytics/mailgun/index.test.js (1)
EmailAnalyticsBase(4-4)
ghost/core/test/unit/server/adapters/email-analytics/mailgun/index.test.js (2)
ghost/core/test/unit/server/adapters/email-analytics/EmailAnalyticsBase.test.js (1)
EmailAnalyticsBase(2-2)ghost/core/core/server/adapters/email-analytics/mailgun/index.js (1)
EmailAnalyticsBase(3-3)
ghost/core/core/server/adapters/email-analytics/mailgun/index.js (2)
ghost/core/core/server/adapters/email-analytics/EmailAnalyticsBase.js (1)
errors(1-1)ghost/core/core/server/services/email-analytics/EmailAnalyticsProviderMailgun.js (1)
EmailAnalyticsProviderMailgun(7-60)
ghost/core/core/server/services/adapter-manager/index.js (1)
ghost/core/core/server/services/adapter-manager/config.js (1)
adapterServiceConfig(11-11)
🔇 Additional comments (12)
ghost/core/core/server/services/adapter-manager/config.js (1)
28-35: LGTM! Clean default configuration pattern.The email-analytics adapter default configuration follows the established pattern for storage and scheduling adapters. The empty mailgun object appropriately defers configuration to runtime injection.
ghost/core/core/server/services/email-analytics/EmailAnalyticsServiceWrapper.js (1)
46-58: Clean adapter-manager integration.The migration from direct require to adapter-manager based provider retrieval is well-implemented. Runtime config injection maintains the existing dependency injection pattern while enabling future provider extensibility.
ghost/core/test/unit/server/adapters/email-analytics/EmailAnalyticsBase.test.js (1)
1-66: Comprehensive base class test coverage.The test suite thoroughly validates the EmailAnalyticsBase contract including constructor behavior, config handling, requiredFns enforcement, and error handling for unimplemented methods.
ghost/core/core/server/adapters/email-analytics/mailgun/index.js (3)
25-45: Good constructor validation and initialization.The constructor properly validates required dependencies and initializes the Mailgun client with appropriate configuration. Tag handling supports both default and custom tags as expected.
86-88: Clean delegation to MailgunClient.The private wrapper method appropriately delegates to the MailgunClient with proper parameter forwarding.
1-19: Well-documented adapter implementation.The eslint exception for filename is appropriately documented, and the class structure with constants and JSDoc provides clear context for the adapter's purpose and behavior.
ghost/core/core/server/adapters/email-analytics/EmailAnalyticsBase.js (2)
16-22: Solid base class constructor.The use of
Object.definePropertyto create a non-writablerequiredFnsarray enforces the adapter contract effectively, and the default config handling is clean.
24-52: Excellent documentation and contract enforcement.The comprehensive JSDoc with usage examples clearly defines the adapter contract, and the default implementation properly enforces that subclasses must implement
fetchLatest().ghost/core/test/unit/server/adapters/email-analytics/mailgun/index.test.js (1)
1-248: Comprehensive test coverage for Mailgun adapter.The test suite thoroughly validates constructor behavior, option handling, timestamp conversion, event filtering, tag management, and adapter contract compliance. Well-structured with appropriate mocking.
ghost/core/core/server/services/adapter-manager/index.js (3)
19-19: Clean adapter registration.The email-analytics adapter registration follows the established pattern and properly references the base class for contract enforcement.
22-28: Well-documented API extension.The updated documentation clearly explains the new runtimeConfig parameter and its purpose for dependency injection, with updated examples including the new email-analytics adapter.
33-37: Runtime config merging works correctly for the current use case.The shallow merge using
Object.assignis appropriate for dependency injection where runtime config provides top-level objects (config, settings). The precedence order correctly allows runtime config to override file-based config.Note: If future adapters require deep merging of nested configuration objects, this implementation would need to be enhanced. For the current dependency injection use case, shallow merging is sufficient.
|
Fixed all issues: Critical fix:
Code quality improvements: All changes pushed in commit a02259f. |
ref TryGhost#21684 Implements Mailgun as an email provider adapter to work with Ghost's AdapterManager system. This enables future extensibility for other email providers (SendGrid, SES, Postmark) while maintaining full backward compatibility with existing Mailgun functionality. Key changes: - Created Mailgun adapter at adapters/email/mailgun/index.js extending EmailProviderBase - Removed legacy MailgunEmailProvider from services/email-service/ (single source of truth) - Enhanced AdapterManager to support runtime config injection for dependency injection - Added resetCacheFor() method to AdapterManager for proper encapsulation - Added safeguard to resetCacheFor() that throws for unknown adapter types (matches getAdapter pattern) - Fixed caching to clear cache when runtime config provided (prevents stale dependencies) - Updated EmailServiceWrapper to use adapter pattern - Added email adapter default configuration (defaults to Mailgun) - Comprehensive test coverage: 23 adapter tests + 2 resetCacheFor tests (all passing) Technical notes: - Adapter file must be index.js for Node.js module resolution (email/mailgun → email/mailgun/index.js) - Runtime config uses resetCacheFor() to maintain encapsulation vs direct instanceCache access - resetCacheFor() throws NotFoundError for unknown types to catch bugs early (consistent with getAdapter) - Maintains all existing Mailgun features: batch sending, recipient variables, delivery scheduling, error handling
- Use replaceAll() for global token replacement - Add guards for undefined replacements and replacement definitions - Default options parameter and destructure recipients/replacementDefinitions - Redact PII from error details (limit to 2000 chars) - Add defensive check for response.id - Wrap error handler in try-catch for safety - Remove redundant logging.info() call - Add mailgunClient interface validation in constructor
ref TryGhost#21684 Implements email analytics as an adapter to work with Ghost's AdapterManager system. This enables future extensibility for other email analytics providers while maintaining full backward compatibility with existing Mailgun analytics. Key changes: - Created EmailAnalyticsBase at adapters/email-analytics/EmailAnalyticsBase.js - Registered 'email-analytics' adapter type with AdapterManager - Implemented Mailgun analytics adapter at adapters/email-analytics/mailgun/index.js - Removed legacy EmailAnalyticsProviderMailgun from services/email-analytics/ - Updated EmailAnalyticsServiceWrapper to use adapter pattern - Enhanced AdapterManager to support runtime config injection - Added email-analytics adapter default configuration (defaults to Mailgun) - Comprehensive test coverage with 24 passing tests (7 base + 17 Mailgun) Technical notes: - Adapter file must be index.js for Node.js module resolution - Runtime config (config, settings) injected via AdapterManager - Maintains all existing Mailgun analytics features: event fetching, pagination, custom tags - fetchLatest() interface supports batch processing of email events (delivered, opened, failed, etc.)
- Moved require('@tryghost/errors') to top of mailgun adapter
- Added optional chaining for options.begin, options.end, options.maxEvents
- Added test case for undefined options parameter
danielraffel
force-pushed
the
adapter/email-analytics
branch
from
a02259f to
02da44f
Compare
1 participant
Context
Part 3 of 4-PR series to extract email functionality into adapters (#22771).
Series: #25250 → #25251 → PR3 → #25253
Changes
Creates EmailAnalyticsBase adapter and implements Mailgun analytics provider using the adapter pattern.
The adapter handles fetching email events (delivered, opened, failed, unsubscribed, complained) from Mailgun's API with support for filtering, pagination, and batch processing.
Removes legacy analytics provider in favor of adapter implementation.
Next: PR4 completes the series with email suppression adapter.