Conversation
kevinfodness
left a comment
kevinfodness
left a comment
There was a problem hiding this comment.
A few comments and suggestions, but no blockers. 🍣
| $wrap = new \CS_REST_General( $auth ); | ||
|
|
||
| return $wrap; |
There was a problem hiding this comment.
Can we just return the new instance directly?
| $wrap = new \CS_REST_General( $auth ); | |
| return $wrap; | |
| return new \CS_REST_General( $auth ); |
| } | ||
| $auth = [ 'api_key' => $settings['api_key'] ]; | ||
|
|
||
| $wrap = new \CS_REST_Lists( |
There was a problem hiding this comment.
imo a better variable name would be $lists
There was a problem hiding this comment.
Good call! In a previous iteration I was using the Clients Wrapper, copied over from the existing Campaign Monitor provider. Forgot to change the variable when I switched to the more direct class.
| /** | ||
| * Gets the lists for the client. | ||
| * | ||
| * @TODO: Add caching that works on Pantheon and WordPress VIP. |
There was a problem hiding this comment.
A wp_cache_get/wp_cache_set would work fine on both. Can probably cache lists for 5 minutes?
| } | ||
|
|
||
| foreach ( $segments as $segment ) { | ||
| // Filter segments to only include segments for the list we're using, and exclude default segments. |
There was a problem hiding this comment.
Isn't the list ID filter already handled by providing the list ID to CS_REST_Lists above? Why do we need to filter for it again?
There was a problem hiding this comment.
Yup! This comment was left over from an earlier implementation (got all segments and then filtered them by lists) before I switched to using the Lists class/endpoint. Thanks for catching.
| * }|false The response from the API. | ||
| */ | ||
| public function create_campaign( int $newsletter_id, array $list_ids, string $campaign_id = null, string $from_name ): array|false { | ||
| // TODO: Move non-email provider code to the core plugin. |
| } | ||
| $auth = [ 'api_key' => $settings['api_key'] ]; | ||
|
|
||
| $wrap = new \CS_REST_Campaigns( $campaign_id, $auth ); |
There was a problem hiding this comment.
Same here, let's call this $campaigns
| } | ||
| $auth = [ 'api_key' => $settings['api_key'] ]; | ||
|
|
||
| $wrap = new \CS_REST_Campaigns( $campaign_id, $auth ); |
| } | ||
| $auth = [ 'api_key' => $settings['api_key'] ]; | ||
|
|
||
| $wrap = new \CS_REST_Campaigns( $campaign_id, $auth ); |
| } | ||
| $auth = [ 'api_key' => $settings['api_key'] ]; | ||
|
|
||
| $wrap = new \CS_REST_Subscribers( $list_id, $auth ); |
|
|
||
|
|
||
| /** | ||
| * Add subscriber to list |
There was a problem hiding this comment.
It's worth asking around to other teams that use this plugin whether this functionality is actually needed. I'd rather that Newsletter Builder be responsible for managing campaigns, and not be responsible for managing list members. If implementers need to do list management, they can do so separately, and this plugin should be focused on sending campaigns to existing lists/segments with existing subscriber bases to avoid trying to do too much.
2 participants
Clone Campaign Monitor provider to introduce a version which works based on list segments instead of lists.
Background
This is, apparently, Campaign Monitor's recommended workflow for sending several newsletters with different subscriber bases. In this implementation, the client maintains a single list for all subscribers and sets up segments to control which newsletters each subscriber receives. This has the advantage of allowing a single set of custom fields to be maintained for all users. In a more traditional Campaign Monitor implementation, each user + list association is unique, which means custom fields (like an access token or user preference) must be maintained multiple times for each email address.