-
Notifications
You must be signed in to change notification settings - Fork 58
feat(emails): unified email registry + DataViews list #4727
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
+1,184
−104
Closed
Changes from all commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
5662da7
chore(deps): update php-loremipsum source URL
kmwilkerson a1ebea4
feat(emails): unified email registry + DataViews list (NPPD-945, slic…
kmwilkerson 01fbe6d
feat(emails): refine DataViews UI per PRD mockup (NPPD-945)
kmwilkerson df2a242
Merge trunk into nppd-945-unified-emails-newspack-slice
kmwilkerson 7de56e8
fix(emails): apply design polish, status dots, grid default, and test…
kmwilkerson 1bae2ad
test(emails): update PHPUnit test to use renamed 'recommended' field …
kmwilkerson e16b00a
fix(emails): stabilize sort with registry-index tiebreaker
kmwilkerson f8f0ee7
fix(emails): add visually-hidden h1 for a11y (NPPD-945)
kmwilkerson 2c30737
test(emails): cover api_get_email_settings response shape (NPPD-945)
kmwilkerson 9693f85
test(emails): cover DataViews action callbacks (NPPD-945)
kmwilkerson ff6ae9e
lint(emails): collapse visually-hidden h1 to single line (NPPD-945)
kmwilkerson a4ae443
fix(emails): address PR review feedback (NPPD-945)
kmwilkerson 90b09bf
Restore Reset action on all Newspack-managed emails - NPPD-1528
kmwilkerson ebb58fb
fix(emails): use inline-block for Badge so it shrink-wraps in grid view
kmwilkerson db4113d
fix(emails): address thomasguillot review findings on slice 1
kmwilkerson d66fa5f
fix(emails): prettier formatting on Notice component
kmwilkerson c5791f0
fix(emails): address round-2 review feedback (NPPD-945)
kmwilkerson c755a1d
Make get_email_registry() filterable for external email integrations …
kmwilkerson 19c1dce
Merge branch 'trunk' into nppd-945-unified-emails-newspack-slice
dkoo 31bd102
fix(emails): address review feedback (NPPD-1527)
kmwilkerson 434a617
refactor(emails): use useWizardApiFetch for loading states
kmwilkerson File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Discussion needed: the email registry
I understand that the plan to integrate WooCommerce transactional emails into our UI is planned for a future PR, but I'd like to take some time to discuss the implementation of the email registry here. Maybe I'm missing something, but at first glance the idea of the registry seems a little more complex than it needs to be. Am I understanding the purpose correctly?
newspack_email_configsfilter andEmails::get_email_configs()methodIf the normalization of data is the key requirement here, then rather than introducing the registry with a new type of data schema, why not define and extend the data schema based on the existing Newspack email configs? What that would look like:
WooCommerce_Emailsclass to get configs for WooCommerce emails would define the configs for the Woo emails we want, based on the same config schema as Newspack emails. I also wonder if there's a WooCommerce function somewhere that would get all available transactional emails, which we could use instead of hard-coding the emails we want (which may or may not exist in the way we expect). Then this method could transform the data from whatever format WooCommerce provides into our own schema.Emails::get_email_configs()method, we would define a "default" config object that contains all of the possible keys with reasonable default values, then fetch email configs for both Newspack and Woo emailsThis to me seems to be a simpler and more easily maintainable way to combine Newspack and Woo emails with a single data schema based on how Newspack emails are already configured. WDYT?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good architectural callout — agreed the registry adds a parallel schema where extending the existing config schema would be cleaner. I'd like to defer this refactor to a focused follow-up PR rather than restructure slice 1's data layer at this stage. The follow-up would cover:
Filed as NPPD-1550. Happy to do it in this PR instead if you'd prefer — let me know which way you want to go.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is such a major architectural change, I'd rather not merge to
trunkas-is with the parallel schema. Let's do this:newspack_email_configsschema.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before I start, one architecture question - the spec sketches three options for how WC emails plug into the unified config schema:
Option A — Your suggestion in full. A new
WooCommerce_Emails::get_email_configs()method produces config-schema entries for the WC emails we surface (withsource='woocommerce',woo_email_id,chip, plugin gating), registered via thenewspack_email_configsfilter. Single unified schema across Newspack and WC. Tradeoff: we lose direct access toWC_Emailinstance methods (get_subject(),get_recipient(),enable()/disable()) — we'd need accessor wrappers or store enough metadata in the config to drive the UI without the live instance.Option B — Keep slice 2's current path: live
WC_Emailresolution viaWC()->mailer()->get_emails(), extended throughEmails::serialize_email()(or a parallelserialize_wc_email()) to produce the same output shape. Tradeoff: no parallel registry, but two code paths into the same response.Option C — Hybrid. WC emails get config-style entries for discovery + UI metadata, but the toggle endpoint still resolves to the live
WC_Emailforenable()/disable(). Unified schema for reads; WC stays source of truth for writes.Option A is cleanest if we're committed to a single schema, but it asks us to wrap or duplicate enough of
WC_Email's API to drive activate/deactivate and recipient resolution without the live instance — adding surface area we'd otherwise get for free. C preserves the schema win where it matters (API response, UI driving off it) without re-implementing WC's write path.Which do you want me to build against?