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

Fix LocalAdapter.deliver/2 return type #589

Merged
merged 1 commit into from
Mar 10, 2021
Merged

Fix LocalAdapter.deliver/2 return type #589

merged 1 commit into from
Mar 10, 2021

Conversation

SamuelWillis
Copy link
Contributor

@SamuelWillis SamuelWillis commented Mar 9, 2021

Fixes issue #588

Fix the LocalAdapter.deliver/2 to return a {:ok, result} tuple when
the adapter is configured with the open_email_in_browser_url option.

This does not include a test as I could not come up with a way to test the local adapter with out it attempting a System.cmd call and trying to open a file/browser window while running the tests.

Any suggestions there would be appreciated.

I was looking at adding the test to the Bamboo.Adapters.LocalAdapterTest like the following:

  test "sent emails has emails that were delivered synchronously" do
    email = new_email(subject: "This is my email")

    config = %{
      open_email_in_browser_url: ""
    }

    {:ok, _response} = email |> LocalAdapter.deliver(config)

    assert [%Bamboo.Email{subject: "This is my email"}] = SentEmail.all()
  end

The above results in a message being logged while running the tests: "The file $filename does not exist."

Fix the `LocalAdapter.deliver/2` to return a `{:ok, result}` tuple when
the adapter is configured with the `open_email_in_browser_url` option.
@germsvel
Copy link
Collaborator

Thanks for opening this PR @SamuelWillis. I think there might be a good way to test this — probably something like https://github.com/thoughtbot/bamboo/pull/590/files has (a similar PR for this same issue) — but I'm okay getting this in as is, and I'll see if we can follow up with a test in #590.

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