Adding support to wrapper for the Journeys API#97
Conversation
niawahyuni
left a comment
There was a problem hiding this comment.
thanks @taledo - looking good, just a comment on the order_field below if it can be removed
| * } | ||
| */ | ||
|
|
||
| function get_journey_recipients($since = '', $page_number = NULL, $page_size = NULL, $order_field = NULL, |
There was a problem hiding this comment.
is it posisble to remove $order_field reference from all the journey_emails endpoint (and the comment above the method)? these endspoints did not have order_field parameter (only order_direction)
cmtiml
left a comment
There was a problem hiding this comment.
Great work, some minor suggestions : )
| * @return CS_REST_Wrapper_Result A successful response will be an object of the form | ||
| * array( | ||
| * { | ||
| * 'ListID' => The id of the list |
| * 'Results' => array( | ||
| * { | ||
| * 'EmailAddress' => | ||
| * 'Date' => |
There was a problem hiding this comment.
Are we referring to the API document for the return results?
It looks like there is slight difference between the API doc and the result shown here:
https://www.campaignmonitor.com/api/journeys/#journey-email-opens
| * { | ||
| 'EmailAddress' => | ||
| 'Date' => | ||
| 'URL' => |
There was a problem hiding this comment.
| * 'EmailID' => String | ||
| * 'Name' => String | ||
| * 'Bounced' => Int | ||
| * 'Clicked' => Int |
There was a problem hiding this comment.
Slight diff here: https://www.campaignmonitor.com/api/journeys/#getting-journey-summary
There was a problem hiding this comment.
this one is meant to be Clicked - in the processing of updating the API docs
| 'refresh_token' => 'your refresh token'); | ||
| $wrap = new CS_REST_JourneyEmails('Email ID to get bounces for', $auth); | ||
| $result = $wrap->get_journey_bounces('Get bounces since', 1, 50, 'email', 'asc'); | ||
| //$result = $wrap->get_journey_bounces(date('Y-m-d', strtotime('-30 days')), page, page size, order field, order direction); |
There was a problem hiding this comment.
Thank you very much for putting a sample code, can we put it above the code to indicate the calling format?
| 'refresh_token' => 'your refresh token'); | ||
| $wrap = new CS_REST_JourneyEmails('Email ID to get recipients for', $auth); | ||
| $result = $wrap->get_journey_recipients('Get recipients since', 1, 50, 'email', 'asc'); | ||
| //$result = $wrap->get_journey_recipients(date('Y-m-d', strtotime('-30 days')), page, page size, order field, order direction); |
There was a problem hiding this comment.
Same here, can we move it above the code?
| 'refresh_token' => 'your refresh token'); | ||
| $wrap = new CS_REST_JourneyEmails('Email ID to get unsubscribes for', $auth); | ||
| $result = $wrap->get_journey_unsubscribes('Get unsubscribes since', 1, 50, 'email', 'asc'); | ||
| //$result = $wrap->get_journey_unsubscribes(date('Y-m-d', strtotime('-30 days')), page, page size, order field, order direction); |
There was a problem hiding this comment.
Same here, can we move it above the code?
|
|
||
| $this->assertIdentical($expected_result, $result); | ||
| } | ||
|
|
There was a problem hiding this comment.
Can we remove the extra lines here?
| $this->general_test('get_journey_summary', $call_options, $raw_result, $deserialised); | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
Can we remove the extra lines here?
| $this->wrapper = new CS_REST_General($this->auth, $this->protocol, $this->log_level, | ||
| $this->api_host, $this->mock_log, $this->mock_serialiser, | ||
| $this->mock_transport); | ||
|
|
There was a problem hiding this comment.
Do we need the extra lines here?
|
Pull request comments have been actioned @cmtiml @niawahyuni |
niawahyuni
left a comment
There was a problem hiding this comment.
thanks @taledo - just one minor changes below before merging/releasing them
| * 'EmailID' => The ID of the email attached to the journey | ||
| * 'Name' => The name of the email attached to the journey | ||
| * 'Bounced' => The number of recipients who bounced | ||
| * 'Clicks' => The total number of recorded clicks |
There was a problem hiding this comment.
@taledo this one is meant to be Clicked - the API docs was just updated couple of minutes ago.. sorry for the confustion
There was a problem hiding this comment.
This is also now done. I will merge! Thank you
cmtiml
left a comment
There was a problem hiding this comment.
Nice work, just need to action on the review comments from Nia, LGTM
No description provided.