Enhancement/#13006 - Implement RRM REST and datastore infrastructure concerning CTAs - #13208
Conversation
…evenue Manager module.
|
Size Change: 0 B Total Size: 3.36 MB ℹ️ View Unchanged
|
nfmohit
left a comment
nfmohit
left a comment
There was a problem hiding this comment.
Thank you for your excellent work on this, @hussain-t!
I've been looking into this, and while the implementaton, I have a few concerns about the current shape that I think are worth discussing before we lock it in:
- The create API is keyed to an arbitrary
newsletterConfigpayload, rather than a generic config payload. - On the PHP side, we're building the
newsletterConfigwithout going through the model setters, so there's no allowlisted mapping of fields. - The datapoint and datastore are hard-wired to newsletter signup, which means the next CTA type would require widening the public contract again.
What do you think about treating create as a generic type + config payload, and routing type-specific validation and mapping through a CTA type handler registry (both PHP and JS)? Shared fields like displayName would stay on the common create payload, and each type would own its own config allowlist/setters (and JS validation). Adding a new type would then just mean registering a handler, rather than touching the datapoint or datastore contract.
I've put together a quick PoC of this on #13332 to illustrate what I mean (not meant to merge as-is; the tests and call sites still need updating). Happy to walk through it together if that's easier.
…' into enhancement/#13006-rrm-rest-ctas.
…ion to the type handler.
|
Thanks @nfmohit, great suggestion. I've implemented it on this branch, following your PoC: create now takes a generic A couple of small deviations from the PoC: the newsletter handler drives its allowlist from field/setter maps, so adding a field is a single entry, and it throws per-field errors ( |
nfmohit
left a comment
nfmohit
left a comment
There was a problem hiding this comment.
Thank you for addressing my feedback, @hussain-t! I've left a few additional feedback for you to consider, thank you!
|
Thanks for the review, @nfmohit; all addressed. Two notes: Making the identifiers optional flips On the JS registration points, it can be reduced to 2 with an assertion signature on |
There was a problem hiding this comment.
Thanks for addressing my feedback, @hussain-t!
I've left one comment which I'll address myself. Besides:
On the JS registration points, it can be reduced to 2 with an assertion signature on
validateConfigplus keying the registry offhandler.type, but I'd leave it for now. The 4 places today are compile-enforced, so missing one gives you atscerror at the exact spot, and I'd rather not trade that for type-level inference with no behavior change. Can revisit when a second CTA type lands.
That sounds fair to me. I've opened #13428 so that we can address this post-launch when we get the chance.
Thanks!
| * @param state Data store's state. | ||
| * @return {(Array.<Object>|undefined)} The CTAs; `undefined` if not loaded yet. | ||
| */ | ||
| getCTAs( state: CTAsState ): CTA[] | undefined { |
There was a problem hiding this comment.
Making the identifiers optional flips
requiresParamstofalseincreateFetchStore, andreceiveGetCTAsthen replaces the params with{}before the reducer runs, so the publication ID can't reach it. CTAs are now stored as a flat list for the configured publication, asfetchPublicationStoreReducerCallbackdoes for publications.
The drawback I see with this approach is that the first successful fetch becomes the list for every publication. A later getCTAs({ publicationID }) for another publication will not refetch, and createCTA for publication A can append onto publication B’s list.
I realize that the express setup will usually use the saved publication, but that is still a broken cache key for a selector whose contract is per-publication.
We could still key the list by publication ID, falling back to the saved publicationID when none is passed. Stamping { ctas, params } from the get control callback is enough for the reducer to key the list after receiveGetCTAs wipes params.
This does complicate the implementation a bit, but it may be worth it as it would be more future-proof.
Given the time constraint, I'll go ahead and implement this. Let me know if you have any thoughts, thanks!
…ports accordingly.
nfmohit
left a comment
nfmohit
left a comment
There was a problem hiding this comment.
LGTM ✅
Note: I've verified that the failing E2E test in CI is unrelated to the changes in this PR.
| // Conditionally resolve settings so that the fetch reducer has | ||
| // the publication ID to key the list by. | ||
| const settingsResolution = maybeResolveSettings( registry, params ); | ||
|
|
||
| if ( settingsResolution ) { | ||
| yield commonActions.await( settingsResolution ); | ||
| } |
There was a problem hiding this comment.
This is a slightly out-of-scope change.
The selector falls back to state.settings.publicationID. If neither an ID nor settings were present, it always returned undefined. The old resolver treated that as “not loaded” and fetched again, so a no-arg getPublication() could loop even after the publication was in state.publications.
This change resolves the settings first to fix that issue.
There was a problem hiding this comment.
While working on this, I realized that we're necessarily duplicating the fallback mechanism on both the client and server sides.
I've opened #13429 to refactor this and relocate the mechanism to the client side, either as a nice-to-have or post-launch.
CC: @hussain-t and @JakePT for comments.
2 participants
Summary
Related issue(s):
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist