Skip to content
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

Attachments in templates #108

Merged
merged 22 commits into from
Mar 30, 2017
Merged

Attachments in templates #108

merged 22 commits into from
Mar 30, 2017

Conversation

rajumsys
Copy link
Contributor

@rajumsys rajumsys commented Mar 9, 2017

WIP for #97

Here is what it does

  • Checks if the email is using template and an attachment is provided
  • If true, it prepares the substitution data, hits template's preview endpoint and get's the substituted template
  • Then it makes a transmission api call without template but use the previous rendered contents (which already has template data) + attachments

🎉

Things to do:

  • Add notes that customer should add template permission to api key

  • Notify users in case if API does not have permission

  • Add unit tests

$headers,
$attachments
$headers
, $attachments
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol, that was just to comment/uncomment quickly with least change.

@@ -35,3 +35,6 @@
add_filter('wp_mail', array($sp, 'init_sp_http_mailer'));
}
}
define('WP_DEBUG', true);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these definitely not going to merge! just for aid of development

Copy link
Contributor

@avigoldman avigoldman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$body = json_decode($response['body']);

if (property_exists($body, 'errors')) {
$this->edebug('Error in getting template data');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can not use edebug or setError here since this does not extend PHPMailer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh right. good catch. but i'll have to find a way to use it because we need use it. i think php now support mixin. so i may consider mixin this class in http mailer so that i can use whatever HTTP Mailer class has.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just pass in $this from the mailer into the method call to keep it simple.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ha ha, that's clever.

@rajumsys rajumsys force-pushed the issue97 branch 2 times, most recently from 62910c9 to b4097f3 Compare March 23, 2017 20:49
@@ -38,35 +40,60 @@ protected function mailSend($header, $body)

function sparkpost_send()
{
$this->edebug('Preparing request data');
$this->debug('Preparing request data');
Copy link
Contributor

@avigoldman avigoldman Mar 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

debug vs edebug?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's one of the ugly things i had to do. we can't call edebug from templates class

@DynamicArray
Copy link

Pulled #97 and dropped into my project. Initial test shows attachment appearing on email - success!.

}
public function render_include_attachment_field()
{
echo '<label><input type="checkbox" id="include_attachment" name="include_attachment" value="1" %s />Include Attachment</label>';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is %s in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redundant


$request_body = $this->get_request_body();

if(!$request_body) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just default $request_body and let the API handle the errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you elaborate?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I misunderstood this. Carry on.

@rajumsys rajumsys merged commit a0e5a95 into master Mar 30, 2017
@rajumsys rajumsys deleted the issue97 branch March 30, 2017 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants