Skip to content

Conversation

chaadow
Copy link
Contributor

@chaadow chaadow commented Sep 11, 2025

related to #55659
cc @fatkodima

Add test for the if deliveries.first.is_a?(Array) case

@fatkodima
Copy link
Member

Thanks, but I I think it is not currently needed.

@fatkodima fatkodima closed this Sep 11, 2025
@chaadow
Copy link
Contributor Author

chaadow commented Sep 11, 2025

@fatkodima I'm sorry but I have to insist. if you remove this line:

deliveries = deliveries.first if deliveries.first.is_a?(Array)

All tests still pass. which means my line as it stand does not hold any value theoretically.

However when I add these tests:
I have the following error

image

Could you please reconsider re-opening and merging please

mail1 = DelayedMailer.test_message(1)
mail2 = DelayedMailer.test_message(2)

ActionMailer.deliver_all_later([mail1, mail2])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fatkodima could you please tell me why this is not needed? I'm passing an array, and the splat operator transforms it into : [[ mail1, mail2 ]] so I still need to test this case to check it does not break

maybe it should be extracted into another, more generic test, test "deliver_all_later accepts an array of mailers"

because this is something that was not tested before with flatten! ( if you remove flatten! tests were still passing because you didn not add any test with an array of mailers ActionMailer.deliver_all_later([mail1, mail2])

In fact if you remove deliveries.flatten! and run the tests they still pass, which is an issue for me

@fatkodima fatkodima reopened this Sep 12, 2025
@fatkodima fatkodima merged commit 46758bd into rails:main Sep 12, 2025
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants