Ghost icon indicating copy to clipboard operation
Ghost copied to clipboard

core: refactor bulk email service to allow non-mailgun senders.

Open markstos opened this issue 3 years ago • 2 comments

Use Case

Ghost documentation says "More bulk mailers besides Mailgun are planned". This PR helps along that goal with some foundational work.

Also, I signed up with Mailgun today, and there is no indication there is a free tier. The lowest publish tier is the $35/month "Foundation" plan. I contact support today to clarify.

Design Goal

This refactor is designed so that the Ghost does not need to support an alternate bulk email sender in the core. Alternate providers could be implemented as unsupported, third-party NPM modules.

Retrieving analytics back from the bulk email provider could be handled in the same way.

Implementation

  • Removes references to mailgun in bulk-email-processor.js
  • Defines a bulk emailer module in configuration, defaulting to Mailgun
  • Loads the bulk mailer module based on the config

Now a new bulk email service can be defined by:

  1. Creating a bulk email module that exports BATCH_SIZE, getInstance and send compatible with Ghost's mailgun.js.
  2. Upload that module to NPM.
  3. Reference that module in config at buklEmail:module

The alternate module will likely require its own API key, which can also be defined in the config system or setting system.

markstos avatar Jun 26 '22 16:06 markstos

Codecov Report

Merging #14984 (16bfb67) into main (630149e) will increase coverage by 0.00%. The diff coverage is 55.55%.

@@           Coverage Diff           @@
##             main   #14984   +/-   ##
=======================================
  Coverage   61.53%   61.53%           
=======================================
  Files         573      573           
  Lines       46430    46431    +1     
  Branches     4209     4209           
=======================================
+ Hits        28571    28572    +1     
  Misses      17813    17813           
  Partials       46       46           
Impacted Files Coverage Δ
core/server/services/bulk-email/mailgun.js 19.51% <0.00%> (-0.16%) :arrow_down:
...server/services/bulk-email/bulk-email-processor.js 25.57% <62.50%> (+0.38%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 630149e...16bfb67. Read the comment docs.

codecov[bot] avatar Jun 26 '22 16:06 codecov[bot]

After I created this PR, I discovered that Ghost uses a formal "adapter" pattern in other places for extensibility on the backend. I presume the preference would be to use the same pattern to support alternatives to the Mailgun backend. For now I've decided to use Mailgun and not pursue this, but here are some references if someone else is interested to explore this more:

adapter-manager

Here's the adapter-manager project:

https://github.com/TryGhost/Utils/blob/f2b428a139e26d8f1a0947cbc3b6f24d1834c2ea/packages/adapter-manager/README.md

registering adapters

Here you can see where adapters get registered:

https://github.com/TryGhost/Ghost/blob/main/core/server/services/adapter-manager/index.js

What appears to be happening is that a path to where a custom solution is stored gets declared under the "adapters" section in the config file. If this is not present, the internal implementation in used.

My patch here provided the same kind of logic in spirit, but this since this formal pattern is in use in other places, it seems best to use it for consistency with supporting alternative mailing backends as well.

markstos avatar Jul 07 '22 15:07 markstos

Hey @markstos, apologies for taking a while to get back to you on this PR - I've been slowly, slowly trying to catch up with all the issues and PRs across our repos after getting quite behind!

As you correctly identified, we have an adapter pattern that we would use to abstract out the bulk mail provider.

I think this is quite a desirable move for a lot of self-hosters and I'd be happy to support you or anyone else that would like to pursue laying some foundation pieces for abstracting out bulk mail and/or email analytics to work with other providers.

For now, I'll close this PR - but again I'm happy to discuss it further or offer support to make progress with this.

ErisDS avatar Aug 23 '22 15:08 ErisDS

Hey! @ErisDS would it be possible to identify what would be an acceptable end goal here? Would it include an additional adapter alongside the abstraction? Any chance there is a shortlist of desired adapters perhaps ?

Also is there any comments over the stuff that was proposed in this PR? I would love to pick it up, but want to understand a bit on more as to what was missing from the proposed changes.

jugglingjsons avatar Dec 15 '22 20:12 jugglingjsons

@jugglingjsons I like to think I had the right idea in this PR, but didn't formally follow the established adapter pattern, I doubt they want to support any other mail providers internally, so it seems fair if a new PR just implements the adapter pattern, creating a place to plug external adapters into without actually shipping other adapters. People could publish other implementations to NPM on Github. I ended up using Mailgun, so I'm not pursuing this myself.

markstos avatar Dec 16 '22 19:12 markstos