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

Update SendGrid Adapter to return ok/error tuple #572

Merged
merged 1 commit into from Feb 19, 2021

Conversation

germsvel
Copy link
Collaborator

Requires: #571

What changed?

We update the SendGridAdapter to abide by the new Adapter behaviour. Adapters must now returns an {:ok, email} or {:error, error}.

Note on implementation with throw/catch

Building SendGrid's body requires a lot of deeply nested data manipulation. Some code that is deeply nested within branching logic raises errors.

Rather than returning an error tuple at a really low level and have to bubble up the error all the way until it's returned, we opted for a simpler approach: throwing and catching the errors.

We throw and catch {:error, error} when we used to raise error.

It's possible we could do something different in the future, like bubbling up the errors and returning them without having to resort to throw and catch, but right now we want to minimize the number of changes for these raises deep in the call stack.

Raising (not returning) configuration errors

While working on this, an important question came up. Should we raise or return an error because of a missing API key? Currently we raise.

I think it's okay to raise an error in that case because it's a configuration issue. It's not the same type of error that happens when you can't build an email, deliver it, or even get a response that isn't 200. That error is a configuration issue and so the sooner we notify the user of Bamboo, the better.

We can also think about it this way: we want to return an ok/error tuple because we people want to handle email building or delivery issues. Having an API key missing isn't that kind of issue. It means the library is configured incorrectly, so it seems good to raise an error.

@germsvel germsvel added adapter Related to supported adapters breaking Potentially breaking change labels Dec 28, 2020
@lady3bean
Copy link

Circle is failing on mix test with:

== Compilation error in file lib/bamboo/adapters/send_grid_adapter.ex ==
** (CompileError) lib/bamboo/adapters/send_grid_adapter.ex:60: undefined function build_api_error/3
    (elixir) src/elixir_locals.erl:107: :elixir_locals."-ensure_no_undefined_local/3-lc$^0/1-0-"/2
    (elixir) src/elixir_locals.erl:108: anonymous fn/3 in :elixir_locals.ensure_no_undefined_local/3
    (stdlib) erl_eval.erl:680: :erl_eval.do_apply/6

Otherwise changes lgtm

@germsvel
Copy link
Collaborator Author

germsvel commented Jan 4, 2021

Thanks @lady3bean. The build is failing because this branch depends on #571 which introduced the build_api_error function. Once I merge 571, and rebase this one, the CI error should go away.

What changed?
==============

We update the `SendGridAdapter` to abide by the new Adapter behaviour.
The adapter now returns an `{:ok, email}` or `{:error, error}`.

As part of this work, we create the `build_api_error` function to
replace the `raise_api_error` function.

Note on implementation with throw/catch
---------------------------------------

Building SendGrid's body requires a lot of deeply nested data
manipulation. Nested deep within some of the branching logic was some
code that raised errors.

Rather than returning an error tuple at a really low level and have to
bubble up the error all the way til it's returned, we opted for a
simpler approach: throwing and catching the errors.

We throw and catch `{:error, error}` when we used to `raise error`.

It's possible we could do something different in the future, like
bubbling up the errors and returning them without having to resort to
`throw` and `catch`, but right now we want to minimize the number of
changes for these `raise`s deep in the call stack.

Raising (not returning) configuration errors
--------------------------------

While working on this, an important question came up. Should we raise or
return an error because of a missing API key? Currently we raise.

I think it's okay to raise an error in that case because it's a
configuration issue. It's not the same type of error that happens when
you can't build an email, deliver it, or even get a response that isn't
200. That error is a configuration issue and so the sooner we notify the
user of Bamboo, the better.

We can also think about it this way: we want to return an ok/error tuple
because we people want to handle email building or delivery issues.
Having an API key missing isn't that kind of issue. It means the library
is configured incorrectly, so it seems good to raise an error.
@germsvel germsvel changed the base branch from bamboo-2 to master February 19, 2021 14:30
@germsvel germsvel merged commit 6b68fca into master Feb 19, 2021
@germsvel germsvel deleted the gv-sendgrid-new-deliver branch February 19, 2021 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adapter Related to supported adapters breaking Potentially breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants