Cannot deliver new inbound email via form

[juanmanuelramallo]

Summary

Steps to reproduce

1. Install ActionMailbox in your app
2. Visit /rails/conductor/action_mailbox/inbound_emails as specified in here https://guides.rubyonrails.org/action_mailbox_basics.html#working-with-action-mailbox-in-development
3. Click on "New inbound email by form"
4. Fill the form and submit

Expected

The inbound email is created successfully

Actual

A NoMethodError is raised:

NoMethodError in Rails::Conductor::ActionMailbox::InboundEmailsController#create
undefined method `original_filename' for "":String

Other Information

>> mail_params[:attachments]
=> [""]

This happens because of the hidden field tag the multiple select field uses under the hood. An easy fix for this problem is to exclude the empty string from the collection.

[ghiculescu]

Thanks for this. Could you please add a test to prevent regressions? It would go here https://github.com/rails/rails/blob/main/actionmailbox/test/controllers/rails/action_mailbox/inbound_emails_controller_test.rb

[juanmanuelramallo]

@ghiculescu I added an empty string as part of the attachments collection in the test that exercises attachments, aiming to copy the behavior we see when submitting the form via a browser.

Let me know whether you'd rather have a new test case for this or just updating an existing test case is fine.

Thank you!

[ghiculescu]

Looks good to me, as long as the test would fail without the fix.

[juanmanuelramallo]

Test failed, I was unable to run the test locally (m1 hell) couldn't install the mysql2 gem. I'll try setting up a virtual host and check again.

[juanmanuelramallo]

Minor update:

All attachments are casted to strings whenever I add an empty string to the attachments array.

This is not the case via the browser, when I upload an attachment I can see that the elements inside the attachments array are not casted to strings (this is from the web console in the error page)

>> mail_params[:attachments]
=> ["", #<ActionDispatch::Http::UploadedFile:0x00000001561f1948 @tempfile=#<Tempfile:/var/folders/23/kj43cdvn7t97fjlrd33tfbg80000gn/T/RackMultipart20211227-27798-di69ea.jpeg>, @original_filename="friendlyreminder.jpeg", @content_type="image/jpeg", @headers="Content-Disposition: form-data; name=\"mail[attachments][]\"; filename=\"friendlyreminder.jpeg\"\r\nContent-Type: image/jpeg\r\n">]

However, when using binding.irb in the test code we can see that all elements are strings:

irb(#<Rails::Conductor::ActionMailbox::InboundEmailsController:0x0000562403e394a0>):001:0> mail_params[:attachments]
=> ["", "#<Rack::Test::UploadedFile:0x0000562403e7f6a8>", "#<Rack::Test::UploadedFile:0x0000562403e7e690>"]

[juanmanuelramallo]

@ghiculescu Here's why #44008 (comment) I couldn't make the test work for that particular scenario.

So, for simplicity I ended up moving this test to another scenario that wasn't using attachments at all. Test fails without the change and passes with it.

Let me know what you think, happy to help.

[ghiculescu]

@juanmanuelramallo could you squash your commits?

[andrew2005]

Fixes #43928.

[juanmanuelramallo]

@ghiculescu rebased with latest main and squashed all commits

[drnic]

Is this PR something that will go into a Rails 7.0.X patch or held off for Rails 7.1 later this year/next year? Sorry to ask rails release question in the ticket.

[juanmanuelramallo]

Closing this as it is already solved via #45103