Skip to content

Conversation

@kelostrada
Copy link
Contributor

added functions to handle adding multiple recipients

end
end

@doc """
Copy link
Owner

Choose a reason for hiding this comment

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

The docs don't match the functionality here.

Suggested change
@doc """
@doc """
Adds multiple recipients to an email list.
:ok = add_recipienst(123, ["recipient_id_1"])
"""

Copy link
Contributor

Choose a reason for hiding this comment

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

done

"""
@spec add_multiple_recipients(integer, [String.t()]) :: :ok | :error
def add_multiple_recipients(list_id, recipient_ids) when is_list(recipient_ids) do
Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer the name to be add_recipients.

Copy link
Contributor

Choose a reason for hiding this comment

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

done

def search(opts) do
query = URI.encode_query(opts)
SendGrid.get("#{@base_api_url}/search?#{query}")
|> handle_search_result
Copy link
Owner

Choose a reason for hiding this comment

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

I think this function may have been missed during the commit. I'm not seeing it in the changes.

end

# Handles the result when there are multiple persisted recipients.
defp handle_recipient_result({:ok, %{body: %{"persisted_recipients" => recipients}}}) when is_list(recipients) and length(recipients) > 1 do
Copy link
Owner

Choose a reason for hiding this comment

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

Since the list of persisted recipients may not be the length of the list passed to the request, I think that adding multiple recipients should just return the the body of the response instead. It's straightforward when just adding a single recipient.

Copy link
Contributor

Choose a reason for hiding this comment

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

done

}
]
"""
@spec add_multiple([]) :: { :ok, [String.t] } | { :ok, String.t } | { :error, list(String.t) }
Copy link
Owner

Choose a reason for hiding this comment

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

Refer to the comment about the return for this function.

Suggested change
@spec add_multiple([]) :: { :ok, [String.t] } | { :ok, String.t } | { :error, list(String.t) }
@spec add_multiple([String.t()]) :: {:ok, map()} | {:error, any()}

Copy link
Contributor

Choose a reason for hiding this comment

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

done

@lukaszsamson lukaszsamson force-pushed the feature/multiple_recipients branch from 82e5c67 to 76d7cac Compare January 30, 2020 15:03
@lukaszsamson
Copy link
Contributor

@alexgaribay I rebased this PR and applied your suggestions. Can you merge it now?

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.

3 participants