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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: handle multiple attachments file #76

Closed
wants to merge 1 commit into from

Conversation

BoutValentin
Copy link
Contributor

Hi 馃憢,

I'm currently using mail_form in a small project. In my use case, I have to handle multiple attachments in email. When I use your current last release of the code, multiple attachments are not being send. If I use only on attachment, everything works fine.

As I investigate,I found out that when multiple attachments file are use, the send method respond with an array of elements ActionDispatch::Http::UploadedFile. To handle this use case I made a fork of your projects and make small change in the deliver file. It seems to work as expected in Rails 6.1.4.1 with Ruby 3.0.0.

To be honest, I completely understand if this use case doesn't interested you and can stay with my current change in my own repo. I'm also a full beginner on the RoR frameworks and Ruby development and open to any discussion to improve the code I just propose to you.

Nice Day to everyone,
Thanks 鉂わ笍

@BoutValentin BoutValentin changed the title Fix: handle multiple attachements file Fix: handle multiple attachments file Aug 24, 2021
@czepesch
Copy link

@BoutValentin thank you very much for that fix, I've installed mail_from using your repository and was able to send two files. Finally! <3

carlosantoniodasilva added a commit that referenced this pull request Nov 4, 2022
When setting up a form uploading multiple files through a single
attachment, mail form was discarding all attachments and sending none,
even though it could consider the form as delivered successfully.

Now we will detect multiple files and add each of them individually.

Note: the reason we are not checking for something like
`respond_to?(:each)` and instead are doing a specific `is_a?(Array)`
check, is because a single uploaded file from Rack (which we use in
our tests) also responds to the `each` method (or rather, it delegates
to the tempfile internal instance used for the upload). It appears that
the Rails uploaded file does not do it, but for increased compatibility
I decided to go with this implementation. The previous one worked
because it tried both to add multiple files and handle a single file
separately, so either one of them would work. This implementation makes
an upfront decision about one or the other path, by performing the
`is_a?` check.  I don't love these types of checks, but this seemed the
most straightforward way of doing it while keeping the code easier to
follow.

Closes #76
@carlosantoniodasilva
Copy link
Member

@BoutValentin thanks for your PR, and apologies for the long delay here. I cherry-picked it into a new branch #78 and tweaked it a bit, adding a test as well. It seems to be working fine.

I'll be pushing it to master soon, if you or @czepesch are still able to give it a shot, feel free to try it then and let me know how it goes.

carlosantoniodasilva added a commit that referenced this pull request Nov 4, 2022
When setting up a form uploading multiple files through a single
attachment, mail form was discarding all attachments and sending none,
even though it could consider the form as delivered successfully.

Now we will detect multiple files and add each of them individually.

Note: the reason we are not checking for something like
`respond_to?(:each)` and instead are doing a specific `is_a?(Array)`
check, is because a single uploaded file from Rack (which we use in
our tests) also responds to the `each` method (or rather, it delegates
to the tempfile internal instance used for the upload). It appears that
the Rails uploaded file does not do it, but for increased compatibility
I decided to go with this implementation. The previous one worked
because it tried both to add multiple files and handle a single file
separately, so either one of them would work. This implementation makes
an upfront decision about one or the other path, by performing the
`is_a?` check.  I don't love these types of checks, but this seemed the
most straightforward way of doing it while keeping the code easier to
follow.

Closes #76
@czepesch
Copy link

czepesch commented Feb 7, 2023

@carlosantoniodasilva hello,
I checked. Can't send attachment when file field set to multiple: true. When I remove multiple, I'm able to send one file.

Maybe I'm missing something. It works for you?

@carlosantoniodasilva
Copy link
Member

@czepesch it worked when I originally implemented it 馃槃... are you using the latest version v1.10.0, or maybe the main branch directly?

@czepesch
Copy link

czepesch commented Feb 7, 2023

@carlosantoniodasilva main branch at first, and now checked with exactly "1.10.0"
files: [] in permit params
attribute :files, attachment: true in the model
f.file_field :files, :type => "file", multiple: true in the form
should it be like this?

@carlosantoniodasilva
Copy link
Member

@czepesch that's pretty much it... here's the diff on a sample app of the two changes I had to make:

diff --git a/app/controllers/contacts_controller.rb b/app/controllers/contacts_controller.rb
index ff2cf7d..0246d0f 100644
--- a/app/controllers/contacts_controller.rb
+++ b/app/controllers/contacts_controller.rb
@@ -4,7 +4,7 @@ class ContactsController < ApplicationController
   end

   def create
-    contact_params = params.require(:contact).permit(:name, :email, :message, :file)
+    contact_params = params.require(:contact).permit(:name, :email, :message, :file => [])
     @contact = Contact.new(contact_params)

     if @contact.deliver
diff --git a/app/views/contacts/new.html.erb b/app/views/contacts/new.html.erb
index 4d10a96..411742b 100644
--- a/app/views/contacts/new.html.erb
+++ b/app/views/contacts/new.html.erb
@@ -18,7 +18,7 @@
   </div>
   <div>
     <%= f.label :file %><br/>
-    <%= f.file_field :file %>
+    <%= f.file_field :file, multiple: true %>
   </div>

   <div>

That sent both files I uploaded successfully:

----==_mimepart_63e278569ca58_70401a4080d5
Content-Type: text/html;
 charset=UTF-8
Content-Transfer-Encoding: 7bit

<h4 style="text-decoration:underline">Contact</h4>


  <p><b>Name:</b>
  Test</p>

  <p><b>Email:</b>
  carlos@test.com</p>

  <p><b>Message:</b>
  ZOMG</p>


----==_mimepart_63e278569ca58_70401a4080d5
Content-Type: text/plain;
 charset=UTF-8;
 filename=file1.txt
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename=file1.txt

This is file 1

----==_mimepart_63e278569ca58_70401a4080d5
Content-Type: text/plain;
 charset=UTF-8;
 filename=file2.txt
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename=file2.txt

This is file 2

----==_mimepart_63e278569ca58_70401a4080d5--

@czepesch
Copy link

czepesch commented Feb 7, 2023

hm... I just sent two files successfully too, but I don't remember changing anything, just ctl+Z to where it has been.. anyway.
Thanks!

@grgatzby
Copy link

Hi everyone,
I am a newby in development, but was so happy to find this post on multiple files.

In my small project, I have used the same model (namely : the Contact model) and form for two different purposes:

  • one is a "contact us" feature where the user may attach multiple files (thanks to this post 馃), and
  • the other one handles a specific user request, where one file attachment is required (and one file only, which turned out handy for me).
    I could not see any means of using "required : true" together with "multiple: true", since when no file is attached, the output is an empty array [ ] which always satisfies 'required : true';

My turnaround was to :
1/ do an if/else test in the html view as per below:

<% if one_required_file_feature %>
  <!--- FORM FIELD: one attachment only -->
  <div>
    <%= f.file_field :files,
      class: 'btn buttons-shape',
      accept: 'text/csv,application/msword,application/vnd.ms-excel,application/pdf',
      required: true
    %>
  </div>
<% else %>
  <!--- FORM FIELD: multi files attachment for Contact us -->
  <div>
    <%= f.file_field :files,
      multiple: true,
      class: 'btn buttons-shape',
      accept: 'image/png,image/gif,image/jpeg,application/pdf'
    %>
  </div>
<% end %>

The first branch of my if else does not yield an array, and therefore properly handles the 'require: true' test

2/ tweak the strong params to handle both potential outputs, as per below
from
params.require(:contact).permit(:subject, :name, ... , files: [])
to
params.require(:contact).permit(:subject, :name, ... , :files, files: [])

3/ to be on the safe side I added 'validate: { presence: true }' in the model:
attribute :files, attachment: true, validate: { presence: true }

I would be keen to get some ideas on how to force one attachment with 'multiple: true,'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants