Attachment helper for base64 encoding #505

spelcaster · 2017-10-18T00:27:57Z

Added the method readFile to read a file and convert its content to
base64;
It's now possible create an Attachment with a raw file;

spelcaster · 2017-10-18T00:31:41Z

//sample file
const fs = require('fs');  
const sgMail = require('@sendgrid/mail');

const {
  classes: {
    Attachment
  }
} = require('@sendgrid/helpers');

let attachment = new Attachment({
  filename: 'indexes.txt',
  type: 'plain/text',
  disposition: 'attachment',
  contentId: 'mytext',
  filePath: '/home/hugocarmo/indexes.js',
  //content: fs.readFileSync('/home/hugocarmo/indexes.js'), // will work
  //content: 'w6cK', // will work
});

sgMail.setApiKey(process.env.SENDGRID_API_KEY);
const msg = {
  to: '[email protected]',
  from: '[email protected]',
  subject: 'Hello attachment',
  html: '<p>Here’s an attachment for you!</p>',
  attachments: [
    attachment
  ],
};

console.log(msg);

adamreisnz · 2017-10-18T00:51:19Z

packages/helpers/classes/attachment.js

@@ -39,17 +40,29 @@ class Attachment {
    data = toCamelCase(data);

    //Extract properties from data
-    const {content, filename, type, disposition, contentId} = data;
+    const {content, filename, type, disposition, contentId, file} = data;


I'm worried that file vs. filename will be a bit confusing. Maybe instead of passing a file object, we can have two separate new settings filePath and fileEncoding.

(The filename setting should probably be deprecated and introduce a new one fileName to be consistent)

adamreisnz · 2017-10-18T00:53:24Z

packages/helpers/classes/attachment.js

-    const {content, filename, type, disposition, contentId} = data;
+    const {content, filename, type, disposition, contentId, file} = data;
+
+    let fileContent = !file ? content : this.readFile(file);


File content is immutable so can be const. I'm not sure about the logic either though. If content is passed, should that take priority over a file? Maybe a warning if both are passed?

I think that the file should take priority over content, and warn the user if both is passed is a good idea.

Is there any example of how to give the user a warning here?

Actually we don't show warnings anywhere really, just throw errors. Maybe an error is suitable, because you are not expected to pass both values. Then, we also don't need to worry about what takes priority.

adamreisnz · 2017-10-18T00:54:34Z

packages/helpers/classes/attachment.js

    this.setFilename(filename);
    this.setType(type);
    this.setDisposition(disposition);
    this.setContentId(contentId);
  }

  /**
+   * Read a file and return its content as base64
+   */
+  readFile(file) {


If we use a separate setting for the encoding you can pass it in here, with a default value of utf8, e.g.

readFile(path, encoding = 'utf8') { ... }

adamreisnz · 2017-10-18T00:56:35Z

packages/helpers/classes/attachment.js

+  readFile(file) {
+    let encoding = !file.encoding ? 'utf8' : file.encoding;
+    let data = fs.readFileSync(file.path);
+    let buff = Buffer.from(data, encoding);


All let's are constant and I'm wondering if we should return the buffer, instead of converting to string at this point? That way you can use the helper function to read files and get buffers and manipulate them if needed before passing to the attachment.

Also, we can update the Attachment class to detect if a Buffer is passed and automatically convert to string.

Thanks for the tip.
Should I convert the method readFile to a helper function? Do you think that another helper function to convert a string to Buffer is necessary?

According to fs.readFileSync documentation if the encoding is passed, then a string is returned, otherwise it'll return a buffer.

Perhaps in this case the buffer conversion using an encoding is completely unnecessary, just if the user want to get a string from the buffer, then the encoding would be useful.

Might not need to expose it, as it's basically just using readFileSync. Let's pass the encoding straight to readFileSync, so we won't need to convert to buffer.

adamreisnz · 2017-10-20T09:23:33Z

packages/helpers/classes/attachment.js

+
+    if ((typeof content !== 'undefined') && (typeof filePath !== 'undefined')) {
+      throw new Error(
+        'The props \'content\' and \'filePath\' cannot be used together.'


Could you use template strings to make it a bit cleaner without the backslashes? E.g.

`The props 'content' and 'filePath' cannot be used together.`

adamreisnz · 2017-10-20T09:25:10Z

packages/helpers/classes/attachment.js

+      return;
+    }
+
+    this.setContent(content);


Could use a one liner for all of this:

this.setContent(filePath ? this.readFile(filePath) : content);

adamreisnz · 2017-10-20T09:26:21Z

packages/helpers/classes/attachment.js

      return;
-    }
-    if (typeof content !== 'string') {
+    } else if (contentType === 'object'


Could you please install ESLint for your code editor and run it on your code to ensure the code style is the same to the rest of the code base?

Sure, I was using the StandardJS

Have you used ESLint on your changes? The code style is still different, e.g. else if's and else's should start on a new line.

adamreisnz · 2017-10-20T09:27:09Z

packages/helpers/classes/attachment.js

+    } else if (content instanceof Buffer) {
+      this.content = content.toString('base64');
+      return;
+    } else if (contentType !== 'string') {


Could you write matching unit tests for all these new content type cases?

spelcaster · 2017-10-28T11:39:43Z

@adamreisnz the changes were made, I'm sorry for the long response time, last week I was so busy.

adamreisnz

Thanks for the changes, it's almost done just some code style issues to keep things consistent. ESLint can auto-fix these for you if you run it from the command line or in your editor 👍

adamreisnz · 2017-10-30T22:16:58Z

@thinkingserious @mbernier I'm happy with this, assuming it works as intended and has been tested!

spelcaster · 2017-10-30T22:19:00Z

cool! thanks for your review @adamreisnz :)

mbernier · 2017-11-03T15:26:47Z

@spelcaster

We have not been able to merge your Pull Request, but because you are awesome - we wanted to make sure you could still get a SendGrid Hacktoberfest shirt.

Please go fill out our swag form before Nov 5th and we will send the shirt! (We know that you might have tried this before and it didn’t work, sorry about that!)

You have till Nov 5th to fill out this form in order to get the Hacktoberfest shirt!

Thank you for contributing during Hacktoberfest! We hope to see you in the repos soon! Just so you know, we always give away a SendGrid shirt for your first ever non-Hacktoberfest PR that gets merged.

msgadi · 2017-12-23T11:57:35Z

@spelcaster I have tried your above code and tried to upload a 10mb pdf file. But It does not work. Again it does not work with a simple text file.

I have set the type to 'application/pdf' then too it does not work. Any help will be appriciated.
Refer the below code.

 ```//sample file
  const fs = require('fs');
 const sgMail = require('@sendgrid/mail');

 const {
     classes: {
         Attachment
     }
 } = require('@sendgrid/helpers');

 let attachment = new Attachment({
     filename: 'test.pdf',
     type: 'application/pdf',
     disposition: 'attachment',
     contentId: 'testingpdf',
     filePath: './attachments/test.pdf',
     //contentType:'application/pdf'
     //content: fs.readFileSync('./attachments/test.pdf'), // will work
     //content: 'w6cK', // will work
 });

 sgMail.setApiKey(process.env.SENDGRID_API_KEY);
 const msg = {
     to: '[email protected]',
     from: '[email protected]',
     subject: 'Hello attachment',
     html: '<p>Here’s an attachment for you!</p>',
     attachments: [
         attachment
     ],
 };

 sgMail.send(msg);

spelcaster · 2017-12-23T12:34:22Z

@mgadirocks Have you tried with absolute path? See https://github.com/sendgrid/sendgrid-nodejs/pull/505/files#diff-de4f2f8e9851d3bd62c6424afddcf90fR58

clee · 2018-02-14T23:14:31Z

LGTM

thinkingserious · 2018-10-22T20:59:44Z

Hi @spelcaster,

I'm ready to merge this now, thanks for your patience!

Could you please take a moment to fix the merge conflicts?

Thank you!

With Best Regards,

Elmer

- If the content is a Buffer and the disposition is 'attachment', then the Buffer will be encoded to a base64 string; - If a filePath is given, then file will be read and its content will be used, the rule to convert the file content to base64 is the same described above; - It's not allowed to use content and filePath together; - Added unit test for the new cases;

spelcaster · 2018-10-23T01:45:55Z

@thinkingserious I've squashed the commits into only 1 and resolved the conflict. Could you verify if the rule to encode the buffer into a base64 string is correct?

Thanks...

thinkingserious · 2018-11-29T01:15:04Z

Hello @spelcaster,

Thanks again for the PR!

We appreciate your contribution and look forward to continued collaboration. Thanks!

Team SendGrid DX

thinkingserious added the status: code review request requesting a community code review or review from Twilio label Oct 18, 2017

spelcaster mentioned this pull request Oct 18, 2017

Attachment helper for base64 encoding #504

Closed

adamreisnz suggested changes Oct 18, 2017

View reviewed changes

adamreisnz suggested changes Oct 20, 2017

View reviewed changes

thinkingserious added difficulty: medium fix is medium in difficulty hacktoberfest labels Oct 28, 2017

adamreisnz suggested changes Oct 30, 2017

View reviewed changes

adamreisnz approved these changes Oct 30, 2017

View reviewed changes

clee added status: code review complete and removed status: code review request requesting a community code review or review from Twilio labels Feb 14, 2018

thinkingserious added type: twilio enhancement feature request on Twilio's roadmap status: ready for deploy code ready to be released in next deploy and removed status: code review complete labels Feb 27, 2018

thinkingserious removed the hacktoberfest label Sep 28, 2018

thinkingserious added status: waiting for feedback waiting for feedback from the submitter and removed status: ready for deploy code ready to be released in next deploy labels Oct 22, 2018

spelcaster force-pushed the i504 branch from 78609a3 to 34fcd13 Compare October 23, 2018 01:42

thinkingserious merged commit 5587afb into sendgrid:master Nov 29, 2018

snyk-bot mentioned this pull request Dec 5, 2019

[Snyk] Upgrade @sendgrid/mail from 6.3.1 to 6.4.0 dyna-dot/fireship.io#5

Open

snyk-bot mentioned this pull request Feb 7, 2020

[Snyk] Upgrade @sendgrid/mail from 6.4.0 to 6.5.0 nip10/tvshows-manager#27

Open

snyk-bot mentioned this pull request Mar 19, 2020

[Snyk] Upgrade @sendgrid/mail from 6.4.0 to 6.5.4 HansHammel/resttest#14

Merged

This was referenced Apr 17, 2020

[Snyk] Upgrade @sendgrid/mail from 6.4.0 to 6.5.5 WekanCompany/leapjs#1

Closed

[Snyk] Upgrade @sendgrid/mail from 6.3.1 to 6.5.5 miasmos/portfolio-server#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attachment helper for base64 encoding #505

Attachment helper for base64 encoding #505

spelcaster commented Oct 18, 2017

spelcaster commented Oct 18, 2017 •

edited

Loading

adamreisnz Oct 18, 2017

adamreisnz Oct 18, 2017

spelcaster Oct 18, 2017

spelcaster Oct 18, 2017

adamreisnz Oct 18, 2017

adamreisnz Oct 18, 2017

adamreisnz Oct 18, 2017

spelcaster Oct 18, 2017

adamreisnz Oct 18, 2017

adamreisnz Oct 20, 2017

adamreisnz Oct 20, 2017

adamreisnz Oct 20, 2017

spelcaster Oct 20, 2017

adamreisnz Oct 30, 2017 •

edited

Loading

adamreisnz Oct 20, 2017

spelcaster Oct 20, 2017

spelcaster commented Oct 28, 2017

adamreisnz left a comment

adamreisnz commented Oct 30, 2017

spelcaster commented Oct 30, 2017

mbernier commented Nov 3, 2017

msgadi commented Dec 23, 2017

spelcaster commented Dec 23, 2017

clee commented Feb 14, 2018

thinkingserious commented Oct 22, 2018

spelcaster commented Oct 23, 2018

thinkingserious commented Nov 29, 2018

Attachment helper for base64 encoding #505

Attachment helper for base64 encoding #505

Conversation

spelcaster commented Oct 18, 2017

spelcaster commented Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamreisnz Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spelcaster commented Oct 28, 2017

adamreisnz left a comment

Choose a reason for hiding this comment

adamreisnz commented Oct 30, 2017

spelcaster commented Oct 30, 2017

mbernier commented Nov 3, 2017

You have till Nov 5th to fill out this form in order to get the Hacktoberfest shirt!

msgadi commented Dec 23, 2017

spelcaster commented Dec 23, 2017

clee commented Feb 14, 2018

thinkingserious commented Oct 22, 2018

spelcaster commented Oct 23, 2018

thinkingserious commented Nov 29, 2018

spelcaster commented Oct 18, 2017 •

edited

Loading

adamreisnz Oct 30, 2017 •

edited

Loading