🎨 Changed email verification error messages to be configurable via hostSettings#26631
Conversation
WalkthroughThis pull request implements configurable error messages for email verification workflows across the email and publishing systems. A new error code 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Set the |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #26631 +/- ##
=======================================
Coverage 73.09% 73.10%
=======================================
Files 1535 1535
Lines 121261 121268 +7
Branches 14648 14649 +1
=======================================
+ Hits 88641 88647 +6
+ Misses 31613 31593 -20
- Partials 1007 1028 +21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@JohnONolan any chance we could get a review on this? |
9larsons
left a comment
There was a problem hiding this comment.
A more general comment: I'd love for us to define the available hostSettings (and more generally, our config options) somewhere. The fact that this is totally ambiguous is troublesome and hard to reason about.
I think I'd like to see a more generic pattern for passing the messages, although the DI pattern is the right one. Perhaps something like hostSettings.messages.emailVerification.<items>?
Then DI would/could still pass in hostSettings.messages.emailVerification, or if we use other hostSettings/config, the more broad item that's appropriate.
| emailAnalyticsJobs, | ||
| domainWarmingService | ||
| domainWarmingService, | ||
| emailSendingDisabledMessage: configService.get('hostSettings:emailVerification:emailSendingDisabledMessage') |
There was a problem hiding this comment.
I dislike passing a message through the wrapper vs passing config, as this feels too 'low level' and an anti pattern if we were to have several messages.
There was a problem hiding this comment.
Good point! Switched to passing the full config service via DI, same pattern as DomainWarmingService.
| amountTriggered: amount | ||
| }); | ||
|
|
||
| if (throwOnTrigger) { |
There was a problem hiding this comment.
Currently, aside from tests, we have no callers that pass throwOnTrigger = true. This would then be dead code. Could you let me know why you updated this path?
There was a problem hiding this comment.
You're right! Reverted it back.
…stSettings Pass config service to EmailService via DI (matching DomainWarmingService pattern) instead of threading resolved message strings through the wrapper. Removed dead configurable message from VerificationTrigger's throwOnTrigger path — no production callers use throwOnTrigger = true.
af205f3 to
abed0b6
Compare
9larsons
left a comment
There was a problem hiding this comment.
Super clean! I am annoyed we pass config: configService, as I think simply passing configService is more clear... but we do this almost everywhere in core so it's better to be consistent.
Managed hosting providers other than Ghost(Pro) currently see hardcoded "support@ghost.org" and "account is currently in review" messages that don't apply to their platform. This adds hostSettings configuration keys (emailSendingDisabledMessage, emailVerificationNeededMessage) under the existing emailVerification namespace, with the current Ghost(Pro) messages kept as defaults. Also replaced fragile regex-based error detection in publish-limit modal with proper error code checking.
Got some code for us? Awesome 🎊!
Please take a minute to explain the change you're making:
Please check your PR against these items:
We appreciate your contribution! 🙏