Use Mailchimp campaign config from format meta instead of env#1354
Use Mailchimp campaign config from format meta instead of env#1354
Conversation
|
Note: I've tested creating a dynamic recipient filter by using just the interest category and group – this works but would require more configuration, so maybe let's just stick with the saved segments for now. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
One note: I think ALL our newsletters are free at this point, so maybe the checkbox is not needed? |
annatraussnig
left a comment
annatraussnig
left a comment
There was a problem hiding this comment.
Super nice. I "tested" the publikator bit, not the E2E NL pipeline, but I assume you did.
Mini question: remembering the previous NL simplification PR, I would have expected it to be more code removed after this change (processing the CONFIGS object, etc), but I guess that's it.
| value={newsletter?.fromName} | ||
| onChange={handleChange('fromName')} | ||
| /> | ||
| <Field |
There was a problem hiding this comment.
do/can we add validation here?
There was a problem hiding this comment.
Good idea. I've been hesitant to introduce more complexity since I'm not sure if we handle form errors well in Publikator but I can have a look.
| value={newsletter?.replyTo} | ||
| onChange={handleChange('replyTo')} | ||
| /> | ||
| <Checkbox |
There was a problem hiding this comment.
as i mentioned, i think all newsletters are free and this concept has been deprecated, but check with Henning
There was a problem hiding this comment.
I'm not super sure, e.g. the DAILY and WEEKLY newsletters never were configured like this.
There was a problem hiding this comment.
i think this is a relic of pre-regwall era.
| onInputChange={onInputChange} | ||
| format={titleData?.format?.meta} | ||
| /> | ||
| <NewsletterForm editor={editor} node={node} /> |
There was a problem hiding this comment.
i would insert it much higher up in the form. It's only shown for formats and, for formats, it feels much more important than TTS or socials.
There was a problem hiding this comment.
Sounds reasonable, yes
|
|
||
| if (!response.ok) { | ||
| const json = await response.json() | ||
| console.error(json) |
There was a problem hiding this comment.
console.error intentional?
There was a problem hiding this comment.
Kind of, because our error handling is extremely inconsistent. This way, at least there's something useful in the logs (the MailChimp API response sometimes contains detailed errors). I'm also throwing the json.detail and actually am using that in Publikator again, so if something goes wrong, people may be able to tell us what happened.
3 participants
As the format meta is resolved anyway in the publish mutation, we can just put the MailChimp campaign configuration right there instead of using an env var.
This reduces the amount of API configuration necessary for adding newsletters and things like the sender name can be updated easily via Publikator. Also removes config lookup by the format's repo ID.
Newsletter settings are now editable in the Publikator Meta editor: