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

Allow custom filename when adding an attachment #496

Merged
merged 3 commits into from
Nov 7, 2024

Conversation

loomchild
Copy link
Contributor

Description

When adding an attachment to an email, the filename that will be sent is determined by the provided file path. This is problematic when using Tempfile because part of the path will be random. I propose accepting an optional filename parameter in FileUtils.attach_file_request_builder.

License

I confirm that this contribution is made under the terms of the MIT license and that I have the authority necessary to make this contribution on behalf of its copyright owner.

@@ -92,8 +92,8 @@ def self.handle_message_payload(request_body)
# Build the request to attach a file to a message/draft object.
# @param file_path [String] The path to the file to attach.
# @return [Hash] The request that will attach the file to the message/draft
def self.attach_file_request_builder(file_path)
filename = File.basename(file_path)
def self.attach_file_request_builder(file_path, filename = nil)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively, it could also be named parameter: filename: nil.

@loomchild
Copy link
Contributor Author

@SubashPradhan could you please tell me what do you think about this PR, is there a chance of merging it?

@SubashPradhan
Copy link
Contributor

@SubashPradhan could you please tell me what do you think about this PR, is there a chance of merging it?

@loomchild Thanks for looking into this, while sending an attachment, you can configure the custom file_name on the attachments field itself.

Have you tried the above approach, example of custom filename would look as below, in the below example the filename will be reflected as different_name.jpeg in the email.

 file_path = "/path/file_name.jpeg"
  base64_content = file_to_base64(file_path)
  buffer = Base64.decode64(base64_content)
  stream = StringIO.new(buffer)

  request_body = {
    subject: "Hello",
    to: [{ name: "My Friend", email: "[email protected]" }],
    body: "Hello, World!",
    attachments: [{
      content: stream,
      content_id: 'image_1_Xf0apsUfZ',
      filename: 'different_name.jpeg',
      size: 370,
      content_type: 'image/jpeg'
    }]
  }

Can you tell me if this is something you were aiming for? Thanks

@loomchild
Copy link
Contributor Author

loomchild commented Nov 6, 2024

No. I don't want to load the entire attachment to memory. It also won't work for files > 3M per your documentation, since they require special multi-part request to Nylas API and simple Base64 embedded in JSON won't do.

That's why I want to use attach_file_request_builder helper provided by Nylas. Obviously, I can avoid using it altogether and just copy-paste your code to our app, or link a file to a different name to make the helper happy (that's what I am currently doing). But I am trying to avoid that hassle and use standard Tempfile.

That's why I am suggesting a simple improvement in your SDK, but if you are not interested, feel free to drop it.

@SubashPradhan SubashPradhan self-requested a review November 7, 2024 08:48
@SubashPradhan
Copy link
Contributor

No. I don't want to load the entire attachment to memory. It also won't work for files > 3M per your documentation, since they require special multi-part request to Nylas API and simple Base64 embedded in JSON won't do.

That's why I want to use attach_file_request_builder helper provided by Nylas. Obviously, I can avoid using it altogether and just copy-paste your code to our app, or link a file to a different name to make the helper happy (that's what I am currently doing). But I am trying to avoid that hassle and use standard Tempfile.

That's why I am suggesting a simple improvement in your SDK, but if you are not interested, feel free to drop it.

I see, thanks for the explanation, we should be good to merge the doc comment is added.

Copy link
Contributor

@SubashPradhan SubashPradhan left a comment

Choose a reason for hiding this comment

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

Thanks for contributing 🎉

@loomchild
Copy link
Contributor Author

Sure, thanks for approving 🙂. Looking forward to the next release.

@SubashPradhan SubashPradhan merged commit f016363 into nylas:main Nov 7, 2024
5 of 6 checks passed
@SubashPradhan SubashPradhan mentioned this pull request Nov 12, 2024
@loomchild
Copy link
Contributor Author

Unfortunately it doesn't work for attachments > 3MB (unfortunately I didn't test it earlier).
I created a follow-up: #502

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.

2 participants