[Mercadopago] Implemented authorize/3 #133

subpal · 2018-03-21T09:59:32Z

Implemented authorize
Added docs
Used Money and CreditCard datatype.

xC-Stony · 2018-03-22T04:23:40Z

lib/gringotts/gateways/mercadopago.ex

+  ## The `opts` argument
+
+  Most `Gringotts` API calls accept an optional `keyword` list `opts` to supply
+  optional arguments for transactions with the MERCADOAPGO


typo "MERCADOAPGO"

xC-Stony · 2018-03-22T04:32:27Z

lib/gringotts/gateways/mercadopago.ex

+  * `capture/3` _an_ amount.
+  * `void/2` a pre-authorization.
+
+  ## Note


Not needed as you haven't written anything under it

xC-Stony · 2018-03-22T04:35:30Z

lib/gringotts/gateways/mercadopago.ex

+
+  @spec authorize(Money.t(), CreditCard.t(), keyword) :: {:ok | :error, Response}
+  def authorize(amount , %CreditCard{} = card, opts) do
+    # commit(args, ...)


Insignificant comment

xC-Stony · 2018-03-22T04:51:00Z

lib/gringotts/gateways/mercadopago.ex

+  defp get_customer_id(opts) do
+    body = %{"email": opts[:email]} |> Poison.encode!
+    headers = [{"content-type", "application/json"}, {"accept", "application/json"}]
+    response = HTTPoison.post!("#{@base_url}/v1/customers?access_token=#{opts[:config][:access_token]}", body, headers, [])


I think you should use

HTTPoison.post

rather than

HTTPoison.post!

as the latter will raise an exception in case it fails.

Also I would suggest you to handle all request via the commit function

xC-Stony · 2018-03-22T05:04:02Z

lib/gringotts/gateways/mercadopago.ex

+      "expirationMonth": card.month,
+      "cardNumber": card.number,
+      "securityCode": card.verification_code,
+      "cardholder": %{


Can be rewritten as `

"cardholder": CreditCard.full_name(card)

`

xC-Stony

Some minor changes are needed.

ravirocx · 2018-03-22T05:35:27Z

lib/gringotts/gateways/mercadopago.ex

+  import Poison, only: [decode: 1]
+
+  alias Gringotts.{CreditCard,
+                   Response}


...can be in single line...

oyeb · 2018-03-22T06:18:18Z

lib/gringotts/gateways/mercadopago.ex

+  def authorize(amount , %CreditCard{} = card, opts) do
+    # commit(args, ...)
+    {_, value, _, _} = Money.to_integer_exp(amount)
+    if(is_nil(opts[:customer_id])) do


Use Keyword.has_key? instead.

oyeb · 2018-03-22T06:21:39Z

lib/gringotts/gateways/mercadopago.ex

+                "type": "mercadopago",
+                "id": opts[:order_id]
+              },
+      "installments": 1,


I haven't read the docs, but I don't think this should be hard-coded.

oyeb · 2018-03-22T08:02:56Z

lib/gringotts/gateways/mercadopago.ex

@@ -0,0 +1,376 @@
+defmodule Gringotts.Gateways.Mercadopago do
+  @moduledoc """
+  [MERCADOPAGO][home] gateway implementation.


Use the gateway branding style when you write docs, to my best knowledge they've named their product as mercadopago.

RajeetKaushal · 2018-03-22T09:21:58Z

lib/gringotts/gateways/mercadopago.ex

+      - To save you time, we recommend [cloning our example
+      repo][example] that gives you a pre-configured sample app ready-to-go.
+          + You could use the same config or update it the with your "secrets"
+          as described [above](#module-registering-your-monei-account-at-mercadopago).


This is because of a bug in the template file (templates/gateway.eex) used by gringotts.new. Could I interest you in submitting a tiny PR with the fix for #136?

RajeetKaushal · 2018-03-22T09:37:05Z

lib/gringotts/gateways/mercadopago.ex

+    token_id = get_token_id(card, opts)
+    opts = opts ++ [token_id: token_id]
+
+    body = get_authorize_body(value, card, opts, opts[:token_id], opts[:customer_id]) |> Poison.encode!()


If you are sending opts as an argument, what is the need to send opts[:token_id] and opts[:customer_id] separately along with it?

RajeetKaushal · 2018-03-22T09:43:40Z

lib/gringotts/gateways/mercadopago.ex

+    body = get_token_body(card) |> Poison.encode!()
+    headers = [{"content-type", "application/json"}, {"accept", "application/json"}]
+    token = HTTPoison.post!("#{@base_url}/v1/card_tokens/#{opts[:customer_id]}?public_key=#{opts[:config][:public_key]}", body, headers, [])
+    %HTTPoison.Response{body: body} = token


IMO, no need to use token variable. You can directly match them

RajeetKaushal

Minor changes needed !

oyeb

Please rename your functions, many of them start with get_.
- get_authorize_body -> authorize_params
- get_token_id -> create_customer_token or create_customer
- get_token_body -> create_card_token or create_card
- and so on...
Please remove implementation of capture

oyeb · 2018-03-27T07:33:38Z

lib/gringotts/gateways/mercadopago.ex

+
+  [credentials]: https://www.mercadopago.com/mlb/account/credentials?type=basic
+
+  * mercadopago does not process money in cents, and the `amount` is rounded to 2


This should be under a new section, perhaps ## Note

It was mistake. For mercadopago nothing is given how payments are processed.

I understand nothing may be specified, but you'll have to figure it out yourselves. Look at the implementation in activemerchant, see if they have converted the amount argument into cents or used as is. Take hint from the various SDKs they (mercadopago) have implemented.

oyeb · 2018-03-27T07:34:29Z

lib/gringotts/gateways/mercadopago.ex

+
+  ## Supported currencies and countries
+
+  > mercadoapgo supports the currencies listed here :


Why is this under a quote block >? Please add a link to the docs where you found this info, and remove the colons.

oyeb · 2018-03-27T07:36:24Z

lib/gringotts/gateways/mercadopago.ex

+      - To save you time, we recommend [cloning our example
+      repo][example] that gives you a pre-configured sample app ready-to-go.
+          + You could use the same config or update it the with your "secrets"
+          as described [above](#module-registering-your-mercadopago-account-at-mercadopago).


#module-registering-your-mercadopago-account-at-gringotts
If you build the docs using mix docs, you'll see that this turns into an internal link. It should link to the section at line 33.

oyeb · 2018-03-27T07:38:35Z

lib/gringotts/gateways/mercadopago.ex

+  * `void/2` a pre-authorization.
+  ## Note
+
+  > For a new customer, `customer_id` field should be ignored. Otherwise it should be provided.


No need to keep this in a quote block. The quote blocks generated by gringotts.new are meant to be replaced, they were added as hints for the module author.

oyeb · 2018-03-27T07:39:54Z

lib/gringotts/gateways/mercadopago.ex

+  The authorization validates the `card` details with the banking network,
+  places a hold on the transaction `amount` in the customer’s issuing bank.
+
+  mercadoapgo's `authorize` returns a map containing authorization ID string which can be used to:


... returns an authorization ID (available in the Response.id field)...

oyeb · 2018-03-27T07:50:21Z

lib/gringotts/gateways/mercadopago.ex

+    end
+  end
+
+  defp respond({:error, %HTTPoison.Error{id: _, reason: _}}, opts) do


Use the reason, see how it's done in other gateways.

oyeb · 2018-03-27T07:50:57Z

lib/gringotts/gateways/mercadopago.ex

+                "type": "mercadopago",
+                "id": opts[:order_id]
+                },
+      "installments": 1,


opts[:installments] || 1

oyeb · 2018-03-27T07:55:49Z

lib/gringotts/gateways/mercadopago.ex

+  ## Note
+  mercadopago allows partial captures also. However, you can make a partial capture to a payment only **once**.
+
+  > The authorization will be valid for 7 days. If you do not capture it by that time, it will be cancelled.


No blockquotes pls, maybe use bullets.

oyeb · 2018-03-27T07:58:23Z

lib/gringotts/gateways/mercadopago.ex

+  defp auth(amount, %CreditCard{} = card, opts, token_id, capture) do
+    {_, value, _, _} = Money.to_integer_exp(amount)
+    opts = opts ++ [token_id: token_id]
+    url = "#{@base_url}/v1/payments?access_token=#{opts[:config][:access_token]}"


Notice that URL and header construction is duplicated atleast 3 times. It's better to move this to commit along with the respond(opts). By moving both of these to commit, you'll realize this function looks very empty. That's good, because you can now move the logic up in authorize -- big gains in readability.

Don't eagerly split a function, that doesn't necessarily increase "modularity". If your private/helper function is getting used by ONLY one method, then you should consider moving that logic into the caller.

You'll have to figure out what to do about that capture argument.

We cannot move url into commit, because it if different for different cases. We have moved the headers and respond function call in commit itself.

You can (partially)! Notice that HTTPoison.request/5 takes many options, one of them being the key :params.

auth(amount, %CreditCard{} = card, opts, token_id, capture) do {_, value, _, _} = Money.to_integer_exp(amount) opts = opts ++ [token_id: token_id] url = "/v1/payments" url_params = [access_token: opts[:config][:access_token]] body = authorize_params(value, card, opts, opts[:token_id], opts[:customer_id], capture) |> Poison.encode! commit(:post, url, body, params: url_params) end defp commit(method, path, body, opts) do headers = [{"content-type", "application/json"}, {"accept", "application/json"}] HTTPoison.request(method, path, body, headers, opts) |> respond() end

oyeb · 2018-03-27T07:59:02Z

lib/gringotts/gateways/mercadopago.ex

+    url = "#{@base_url}/v1/customers?access_token=#{opts[:config][:access_token]}"
+    body = %{"email": opts[:email]} |> Poison.encode!
+    headers = [{"content-type", "application/json"}, {"accept", "application/json"}]
+    {state, res} = commit(:post, url, body, headers) |> respond(opts)


Use a case here.

oyeb · 2018-03-29T11:35:42Z

lib/gringotts/gateways/mercadopago.ex

@@ -258,8 +257,14 @@ defmodule Gringotts.Gateways.Mercadopago do
    end
  end

-  defp respond({:error, %HTTPoison.Error{id: _, reason: _}}, opts) do
-    {:error, "Network Error"}
+  defp respond({:error, %HTTPoison.Error{} = error}, opts) do


opts is never used.

oyeb · 2018-03-29T11:35:48Z

lib/gringotts/gateways/mercadopago.ex

+  defp auth(amount, %CreditCard{} = card, opts, token_id, capture) do
+    {_, value, _, _} = Money.to_integer_exp(amount)
+    opts = opts ++ [token_id: token_id]
+    url = "#{@base_url}/v1/payments?access_token=#{opts[:config][:access_token]}"


You can (partially)! Notice that HTTPoison.request/5 takes many options, one of them being the key :params.

auth(amount, %CreditCard{} = card, opts, token_id, capture) do {_, value, _, _} = Money.to_integer_exp(amount) opts = opts ++ [token_id: token_id] url = "/v1/payments" url_params = [access_token: opts[:config][:access_token]] body = authorize_params(value, card, opts, opts[:token_id], opts[:customer_id], capture) |> Poison.encode! commit(:post, url, body, params: url_params) end defp commit(method, path, body, opts) do headers = [{"content-type", "application/json"}, {"accept", "application/json"}] HTTPoison.request(method, path, body, headers, opts) |> respond() end

oyeb · 2018-03-29T11:37:44Z

lib/gringotts/gateways/mercadopago.ex

@@ -151,8 +153,9 @@ defmodule Gringotts.Gateways.Mercadopago do
  # network request in here, and parse it using another private method called
  # `respond`.
  #@spec commit(_, _, _, _) :: {:ok | :error, Response}
-  defp commit(method, path, body, headers) do
-    HTTPoison.request(method, path, body, headers, [])
+  defp commit(method, path, body, opts) do


This function doesn't actually use opts as of now. If you pass opts to the request, there's some benefit.

Could you please add the right @spec?

Created all the helper functions, body for HTTPoison requests, response methods.

1. Completed the docs 2. Used predefined CreditCard and Money data type 3. Added brand in CreditCard

1. Implemented authorize 2. Completed docs 3. Used CreditCard and Money datatype

1. Used commit for HTTPoison requests. 2. Handled network and arguement errors. 3. Updated docs

1. functions are renamed 2. if else replaced with case 3. docs updated

1. changed if else to case 2. changed function name 3. updated docs

1. Added default value of `installments` in function `authorize_params/6` as 1. 2. Added `headers` and `respond`function call in commit function itself. 3. Used `reason` in `respond` function, in case of network error. 4. Removed block quotes '>'. 5. Added money processing note.

1. Removed unnecessary `opts` from arguements of error `respond` function. 2. Used fifth arguement in `HTTPoison.Request` to send part of url as keyword list. 3. Added @SPEC for `commit` function.

oyeb

Please remove the test files from this branch.

oyeb · 2018-04-09T07:43:00Z

lib/gringotts/gateways/mercadopago.ex

+
+  @spec authorize(Money.t(), CreditCard.t(), keyword) :: {:ok | :error, Response}
+  def authorize(amount , %CreditCard{} = card, opts) do
+    if Keyword.has_key?(opts, :customer_id) do


We can move this check inside create_customer, since create_customer recieves the same opts.

def authorize(amount , %CreditCard{} = card, opts) do with {:ok, customer_id} <- create_customer(opts), {:ok, card_token} <- create_token(card, opts) do {_, value, _, _} = Money.to_integer_exp(amount) url_params = [access_token: opts[:config][:access_token]] body = authorize_params(value, card, opts, card_token, customer_id, false) |> Poison.encode! commit(:post, "/v1/payments", body, opts, params: url_params ) end end

If you wish you could even write an else block. Here are the with/1 docs.

This way we can get rid of auth_token/4 as well as auth/5. Please refrain from writing so many helper functions.

oyeb · 2018-04-09T07:43:44Z

lib/gringotts/gateways/mercadopago.ex

+    commit(:post, "/v1/payments", body, opts, params: url_params )
+  end
+
+  defp create_customer(opts) do


You can move that if here to check if the caller supplied a customer_id in opts

oyeb · 2018-04-09T07:45:07Z

lib/gringotts/gateways/mercadopago.ex

+                "last_name": card.last_name
+                },
+      "order": %{
+                "type": "mercadopago",


What are other options for the order.type? Is it always "mercadopago"?

oyeb · 2018-04-09T07:47:11Z

lib/gringotts/gateways/mercadopago.ex

+    Mention `customer_id`.  
+      iex> amount = Money.new(42, :BRL)
+      iex> card = %Gringotts.CreditCard{first_name: "Hermione", last_name: "Granger", number: "4509953566233704", year: 2099, month: 12, verification_code: "123", brand: "VISA"}
+      iex> opts = [email: "hermione@granger.com", order_id: 123125, customer_id: "308537342-HStv9cJCgK0dWU", payment_method_id: "visa", config: %{access_token: YOUR_ACCESS_TOKEN, public_key: YOUR_PUBLIC_KEY}]


Please remove the config key from the examples. Sorry for not making that clear in an earlier comment 😓

codecov-io · 2018-04-18T14:42:08Z

Codecov Report

Merging #133 into dev will decrease coverage by 8.4%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##              dev     #133      +/-   ##
==========================================
- Coverage   64.16%   55.75%   -8.41%     
==========================================
  Files          14       15       +1     
  Lines         466      504      +38     
==========================================
- Hits          299      281      -18     
- Misses        167      223      +56

Impacted Files	Coverage Δ
lib/gringotts/gateways/mercadopago.ex	`0% <0%> (ø)`
lib/gringotts.ex	`0% <0%> (-100%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4082cb...9f9d3d7. Read the comment docs.

…nto mercadopago

2. Fetched order_type from opts. 3. Removed config from opts in examples.

oyeb

Please use credo to analyze your module, just run mix credo --strict in the project root and fix all mercadopago related issues.
Remeber that credo has the explain command which is extremely useful.

oyeb · 2018-04-18T18:20:31Z

lib/gringotts/gateways/mercadopago.ex

+  | ----                     | ---                                                                                     |
+  | `email`                  | Email of the customer. Type - string .                                                  |
+  | `order_id`               | Order id issued by the merchant. Type- integer.                                         |
+  | `payment_method_id`      | Payment network operators, eg. `visa`, `mastercard`. Type- string.                      |


payment_method_id is no longer required in opts

customer_id need not be nil in case of a new customer, it can simply be skipped. Please re-word the sentence.

order_type and installments are undocumented.

oyeb · 2018-04-18T18:23:02Z

lib/gringotts/gateways/mercadopago.ex

+  The authorization validates the `card` details with the banking network,
+  places a hold on the transaction `amount` in the customer’s issuing bank.
+
+  mercadoapgo's `authorize` returns authorization ID(available in the `Response.id` field) :


We must document what's returned in Response.token here.

oyeb · 2018-04-18T18:24:49Z

lib/gringotts/gateways/mercadopago.ex

+  # `required_config` list
+  use Gringotts.Adapter, required_config: [:public_key, :access_token]
+
+  import Poison, only: [decode: 1]


This is an unused import, remove it. It shows up as a compile-time warning.

oyeb · 2018-04-18T18:30:30Z

lib/gringotts/gateways/mercadopago.ex

+    Mention `customer_id`.  
+      iex> amount = Money.new(42, :BRL)
+      iex> card = %Gringotts.CreditCard{first_name: "Hermione", last_name: "Granger", number: "4509953566233704", year: 2099, month: 12, verification_code: "123", brand: "VISA"}
+      iex> opts = [email: "hermione@granger.com", order_id: 123125, customer_id: "308537342-HStv9cJCgK0dWU", payment_method_id: "visa"]


Is it necessary to pass :email even though we have the customer's ID?

Please mask the :customer_id with some string like "hermione's_customer_id".

2. Added docs for Response.token. 3. Removed unused import. 4. Masked customer id in example. 5. Reduced the arity of authorize_params

oyeb · 2018-04-22T10:38:09Z

Closing as this PR has been approved! 🎉

mandarvaze · 2020-04-18T09:38:02Z

Although this PR was approved, it was never merged to dev
Superceded by #185

xC-Stony self-requested a review March 21, 2018 10:50

xC-Stony reviewed Mar 22, 2018

View reviewed changes

xC-Stony requested changes Mar 22, 2018

View reviewed changes

xC-Stony assigned aashishsingh2803 and subpal Mar 22, 2018

aashishsingh2803 added feature PR state: WIP labels Mar 22, 2018

ravirocx reviewed Mar 22, 2018

View reviewed changes

oyeb requested changes Mar 22, 2018

View reviewed changes

subpal removed the PR state: WIP label Mar 22, 2018

oyeb reviewed Mar 22, 2018

View reviewed changes

RajeetKaushal reviewed Mar 22, 2018

View reviewed changes

RajeetKaushal suggested changes Mar 22, 2018

View reviewed changes

radewoosh self-requested a review March 22, 2018 10:04

subpal force-pushed the mercadopago branch from 819c84e to 61f09ba Compare March 26, 2018 15:51

oyeb requested changes Mar 27, 2018

View reviewed changes

xC-Stony approved these changes Mar 27, 2018

View reviewed changes

oyeb requested changes Mar 29, 2018

View reviewed changes

subpal added 6 commits March 31, 2018 17:18

Initial Commit \n Implemented Authorize

a444179

Implemented authorize

ea9c912

Created all the helper functions, body for HTTPoison requests, response methods.

Refined authorize function

2127fe9

1. Completed the docs 2. Used predefined CreditCard and Money data type 3. Added brand in CreditCard

Implemented authorize

d7d74b9

1. Implemented authorize 2. Completed docs 3. Used CreditCard and Money datatype

jgkg

b776dc9

Modification in authorize/3, and implemented capture

4a99128

1. Used commit for HTTPoison requests. 2. Handled network and arguement errors. 3. Updated docs

subpal added 7 commits March 31, 2018 17:18

Modification in authorize/3, and implemented capture

a6b4298

1. Used commit for HTTPoison requests. 2. Handled network and arguement errors. 3. Updated docs

Modification in authorize/3, and implemented capture

94f5b1c

1. Used commit for HTTPoison requests. 2. Handled network and arguement errors. 3. Updated docs

Modification in authorize/3, and implemented capture

1d46184

1. Used commit for HTTPoison requests. 2. Handled network and arguement errors. 3. Updated docs

Updated authorize/3

5956d5b

1. functions are renamed 2. if else replaced with case 3. docs updated

Updated authorize/3

fd135db

1. changed if else to case 2. changed function name 3. updated docs

Updated authorize/3

5eabe34

1. Removed unnecessary `opts` from arguements of error `respond` function. 2. Used fifth arguement in `HTTPoison.Request` to send part of url as keyword list. 3. Added @SPEC for `commit` function.

subpal force-pushed the mercadopago branch from 04aef9e to 5eabe34 Compare March 31, 2018 11:51

oyeb requested changes Apr 9, 2018

View reviewed changes

Applied elixir formatter.

8083ad1

subpal added 6 commits April 18, 2018 21:13

Delete mercadopago_test.exs

1e8f5a3

Delete mercadopago_mock.exs

f076aab

Delete mercadopago_test.exs

e88f1f6

Removed config from opts in examples.

a04b408

Merge branch 'mercadopago' of https://github.com/aviabird/gringotts i…

4694ac9

…nto mercadopago

1. Modified authorize, removed helper functions.

1fa2019

2. Fetched order_type from opts. 3. Removed config from opts in examples.

subpal force-pushed the mercadopago branch from f73c51c to 1fa2019 Compare April 18, 2018 17:06

oyeb requested changes Apr 18, 2018

View reviewed changes

1. Added docs for order_type and installment field of opts.

9f9d3d7

2. Added docs for Response.token. 3. Removed unused import. 4. Masked customer id in example. 5. Reduced the arity of authorize_params

oyeb approved these changes Apr 22, 2018

View reviewed changes

oyeb closed this Apr 22, 2018

mandarvaze unassigned aashishsingh2803 Apr 18, 2020


		[credentials]: https://www.mercadopago.com/mlb/account/credentials?type=basic

		* mercadopago does not process money in cents, and the `amount` is rounded to 2


		## Supported currencies and countries

		> mercadoapgo supports the currencies listed here :

[Mercadopago] Implemented authorize/3 #133

[Mercadopago] Implemented authorize/3 #133

Conversation

subpal commented Mar 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xC-Stony left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RajeetKaushal left a comment

Choose a reason for hiding this comment

oyeb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oyeb Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oyeb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Apr 18, 2018 • edited Loading

Codecov Report

oyeb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oyeb commented Apr 22, 2018

mandarvaze commented Apr 18, 2020

oyeb Mar 28, 2018 •

edited

Loading

codecov-io commented Apr 18, 2018 •

edited

Loading