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

Adapter: consistent returns #125

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions lib/apus/adapter.ex
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
defmodule Apus.Adapter do
@moduledoc """
An adapter provides a way to communicate with a third-party SMS service
using a consistent API.
"""

@callback deliver(%Apus.Message{}, %{}) :: any
@callback handle_config(map) :: map
@doc "Send the SMS message."
@callback deliver(%Apus.Message{}, config :: map) :: {:ok, Apus.Message.t()} | {:error, any}

@doc "Initialize adapter configuration."
@callback handle_config(config :: map) :: config :: map
Copy link
Member

Choose a reason for hiding this comment

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

Possible typo on the return type here?

I think this should be :: map instead of :: config :: map?

end
7 changes: 6 additions & 1 deletion lib/apus/adapters/local_adapter.ex
Original file line number Diff line number Diff line change
@@ -1,13 +1,18 @@
defmodule Apus.LocalAdapter do
@moduledoc """
Deliver messages to a local inbox, available to view if
`Apus.SentMessagesViewerPlug` is installed in your router.
"""

@behaviour Apus.Adapter

alias Apus.SentMessages

def deliver(message, _config) do
SentMessages.push(message)
case SentMessages.push(message) do
:ok -> {:ok, message}
error -> {:error, error}
end
Comment on lines +12 to +15
Copy link
Member

Choose a reason for hiding this comment

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

The SentMessages.push function always returns the same result so I don't think we need the case here.

Can we remove the case and update SentMessages.push to return {:ok, message}?

end

def handle_config(config), do: config
Expand Down
6 changes: 5 additions & 1 deletion lib/apus/adapters/test_adapter.ex
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
defmodule Apus.TestAdapter do
@moduledoc """
Adapter for use with automated testing.
"""

@behaviour Apus.Adapter
Expand All @@ -15,7 +16,10 @@ defmodule Apus.TestAdapter do
end

def deliver(message, _config) do
send(self(), {:delivered_message, message})
case send(self(), {:delivered_message, message}) do
{:delivered_message, _} -> {:ok, message}
error -> {:error, error}
end
Comment on lines +19 to +22
Copy link
Member

Choose a reason for hiding this comment

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

The send function always returns the given input so we shouldn't need the case here.

As we can hardcode the response, maybe something like this would be clearer?

send(self(), {:delivered_message, message})

{:ok, message}

end

def handle_config(config) do
Expand Down
2 changes: 0 additions & 2 deletions lib/apus/sent_messages.ex
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,5 @@ defmodule Apus.SentMessages do
Agent.update(__MODULE__, fn messages ->
[message | messages]
end)

message
Copy link
Member

Choose a reason for hiding this comment

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

Related to the comment above on the local_adapter, could we change this to {:ok, message} to keep this clearer?

end
end
2 changes: 1 addition & 1 deletion test/lib/apus/adapters/local_adapter_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ defmodule Apus.LocalAdapterTest do
test "deliver/2 should deliver a message" do
message = Message.new(from: "+15551234567", to: "+15557654321", body: "Hello there")

LocalAdapter.deliver(message, %{})
{:ok, %Apus.Message{}} = LocalAdapter.deliver(message, %{})

assert SentMessages.all() == [message]
end
Expand Down
2 changes: 1 addition & 1 deletion test/lib/apus/adapters/plivo_adapter_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ defmodule Apus.PlivoAdapterTest do
message = Message.new(from: "+15551234567", to: "+15557654321", body: "Hello there")

use_cassette "plivo_sms_from_success", match_requests_on: [:request_body] do
{:ok, pl_message} = PlivoAdapter.deliver(message, config)
{:ok, %Apus.Message{} = pl_message} = PlivoAdapter.deliver(message, config)

assert pl_message.from == "+15551234567"
assert pl_message.to == "+15557654321"
Expand Down
2 changes: 1 addition & 1 deletion test/lib/apus/adapters/test_adapter_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ defmodule Apus.TestAdapterTest do
test "deliver/2 should deliver a message" do
message = Message.new(from: "+15551234567", to: "+15557654321", body: "Hello there")

TestAdapter.deliver(message, %{})
{:ok, %Apus.Message{}} = TestAdapter.deliver(message, %{})

assert_received {:delivered_message, ^message}
end
Expand Down
2 changes: 1 addition & 1 deletion test/lib/apus/adapters/twilio_adapter_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ defmodule Apus.TwilioAdapterTest do
message = Message.new(from: "+15551234567", to: "+15557654321", body: "Hello there")

use_cassette "twilio_sms_from_success", match_requests_on: [:request_body] do
{:ok, %{} = tw_message} = TwilioAdapter.deliver(message, config)
{:ok, %Apus.Message{} = tw_message} = TwilioAdapter.deliver(message, config)

assert tw_message.from == "+15551234567"
assert tw_message.to == "+15557654321"
Expand Down