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

Ensure Vonage delivery returns an HTTP::Response #59

Merged
merged 2 commits into from
Nov 2, 2020

Conversation

rbague
Copy link
Contributor

@rbague rbague commented Oct 30, 2020

Closes #53 by ensuring the Vonage delivery method returns the HTTP response object in its delivery method, so it can be used in an around callback (as explained here).

I've also added tests for all the delivery methods so that any new changes don't break this "feature". In the end, we'll probably want to standardize the response of the delivery action of all delivery methods (as @excid3 suggested here).

@rbague
Copy link
Contributor Author

rbague commented Oct 30, 2020

Just for mentioning it, whilst working on this, I realized that the ActionCable and Email delivery methods return "strange" values.
The ActionCable delivery method returns the response of broadcasting though action_cable, which is nil (I don't know if it would make more sense to explicitly return nil, for clarity, and in case the return value of broadcast changes in the future).
And the Email delivery returns the scheduled active_job that will send the email later on (which I don't know if it is useful at all to the user, and it may be better to return nil)

@excid3
Copy link
Owner

excid3 commented Oct 31, 2020

Looking good @rbague 👍

ActionCable - I don't think the return value matters much on that one.
Email - I actually don't know that we need to deliver_later here since we're already in a background job. It's redundant to queue up another job. What if we change it to deliver_now and use that return value?

@rbague
Copy link
Contributor Author

rbague commented Nov 2, 2020

It does make more sense to deliver the email the same moment the job is executed, instead of triggering a new one. Which also solves the "issue" of returning the mailer job, and now returns a Mail::Message object, which can be useful for debugging.

I've also updated the dummy application so emails are sent

@excid3
Copy link
Owner

excid3 commented Nov 2, 2020

This looks great. Thanks for the help on this @rbague!

@excid3 excid3 merged commit d147d49 into excid3:master Nov 2, 2020
@rbague rbague deleted the delivery-response branch November 2, 2020 14:51
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.

Get response from delivery method in callback
2 participants