There should be no unused return values for Keyword functions.

https://codeclimate.com/github/aviabird/gringotts/lib/gringotts/gateways/cams.ex#issue_5a3a55fffc08350001000043

Currently, we have named workers which can only be one at a time and can be set in the config passed from the application.

This is only good when we have to run a worker at a time.

If we have to support multiple workers then either we have to move away from the named workers or use something like gproc etc which support that using ETS etc.

This was introduced in first_pass PR merge.

Some refs
https://www.amberbit.com/blog/2016/5/13/process-name-registration-in-elixir/ https://github.com/uwiger/gproc
https://m.alphasights.com/process-registry-in-elixir-a-practical-example-4500ee7c0dcc
https://medium.com/elixirlabs/registry-in-elixir-1-4-0-d6750fb5aeb
http://codeloveandboards.com/blog/2016/03/20/supervising-multiple-genserver-processes/

Space missing after comma

https://codeclimate.com/github/aviabird/gringotts/lib/gringotts/gateways/cams.ex#issue_5a3a55fffc0835000100003d

Integration tests must test each request's life; for example, there should be a test for these:

• authorize -> capture -> refund
• authorize -> void

The optional arguments are silently ignored. Need lots of functions that properly format the opts contents and send it to MONEI. Reference

• billing
• cart
• custom
• customer
• invoice_id
• transaction_id
• category
• merchant
• shipping
• shipping_customer

More

• Use the Address struct for *_address.

Currently, we are passing currency as a string "USD" in the application config :global_config.

This can be improved to use atoms like :usd or something which can be driven and controlled by the library instead of a plane string.

Support non PCI-DSS merchants

Most gateways provide some kind of client-side SDK or JS libs to allow client browsers to direct the payment data directly to the gateway, allowing even PCI-DSS non-compliant merchants to use their API and services.

Out of the gateways currently supported (v1.0.2), only Stripe implementation provides a checkout feature.

Discussion

1. Should we add a new checkout method in the Public API?

T0d0

Add checkout for these gateways:

• Stripe
• MONEI
• Paymill
• WireCard
• AuthorizeNet
• Trexle

The merchant should have the flexibility to switch between test and the production url.
This is done so that they can use the test url to check their sandbox account, while the production url would be used for the main account.

The opts field will have the following key-value pair. Depending on this test or production url is set.
[ { mode: "test" } ]

File has the variable name before the pattern while most of the files have the variable name after the pattern when naming parameter pattern matches

https://codeclimate.com/github/aviabird/gringotts/lib/gringotts/gateways/bogus.ex#issue_5a3a55fffc08350001000038

Currently, the support is to set the mode as "test" or "prod" for 'sandbox' or 'live' account in the global config of application with respect to a gateway. By default if no mode is set then the "test" mode is used.

config :gringotts, :global_config,
  mode: :test 

But, this configuration should be made mandatory and a compile time exception should be raised if the field is not set.

Integration tests must test each request's life; for example, there should be a test for these:

• authorize -> capture -> refund
• authorize -> void

Support all optional fields provided by Paymill

Library spits out a weird error message when the key name in the application config does not match the required_config of the gateways in lib.

** (exit) exited in: GenServer.call(:payment_worker, {:authorize, Gringotts.Gateways.Stripe, 5, nil, [currency: "usd"]}, 5000)
   ** (EXIT) an exception was raised:
       ** (ArgumentError) argument error
           :erlang.byte_size(nil)
           (gringotts) lib/gringotts/gateways/stripe.ex:321: Gringotts.Gateways.Stripe.commit/4
           (gringotts) lib/gringotts/worker.ex:26: Gringotts.Worker.handle_call/3
           (stdlib) gen_server.erl:636: :gen_server.try_handle_call/4
           (stdlib) gen_server.erl:665: :gen_server.handle_msg/6
           (stdlib) proc_lib.erl:247: :proc_lib.init_p_do_apply/3
   (elixir) lib/gen_server.ex:774: GenServer.call/3

We can definitely improve this by showing an error message which specifies that we have used wrong key names which are not present in the use Gringotts.Adapter, required_config: [:secret_key, :default_currency] of the gateway file.

Examples and the tests do not have the tax, shipping and duty fields in the opts. Though they are optional for ANet, the implementation assumes they are present in opts (so they are mandatory right now).

When they are absent, the implementation spits out empty tags which ANet doesn't like:

<tax>
    </>
    <name/>
    <description/>
</tax>
<duty>
    </>
    <name/>
    <description/>
</duty>
<shipping>
    </>
    <name/>
    <description/>
</shipping>

The error:

{:error,
 %Gringotts.Response{authorization: nil, avs_result: nil, cvc_result: nil,
  error_code: "E00003", fraud_review: nil,
  message: "Name cannot begin with the '>' character, hexadecimal value 0x3E. Line 36, position 6.",
  params: %{"ErrorResponse" => %{"messages" => %{"message" => %{"code" => "E00003",
          "text" => "Name cannot begin with the '>' character, hexadecimal value 0x3E. Line 36, position 6."},
        "resultCode" => "Error"}}}, status_code: 200, success: false}}

Let's run all tests together, excluding those that are explicitly marked :skip.

script:
    - mix coveralls.travis --include integration

--trace flag is redundant as traces are already enabled in the test_helper.exs

The condition of if should not be wrapped in parentheses.

https://codeclimate.com/github/aviabird/gringotts/lib/gringotts/gateways/cams.ex#issue_5a3a55fffc0835000100003a

There is no whitespace around parentheses/brackets most of the time, but here there is.

https://codeclimate.com/github/aviabird/gringotts/lib/gringotts/gateways/paymill.ex#issue_5a3a55fffc08350001000052

Currently, gringotts support only credit card based payment for Authorize.Net.
The support for customer payment profile id based payment for functions purchase, capture, refund and void is yet not provided.

Cond statements should contain at least two conditions besides true.

https://codeclimate.com/github/aviabird/gringotts/lib/gringotts/gateways/authorize_net.ex#issue_5a3a55fffc0835000100002b

Review test cases for correct implementation

Config related-error reporting was introduced in #7 and currently happens at runtime, before every call to Gringotts API.
We should validate the config at compile time to reduce the overhead.

Review test cases for correct implementation

Refer stripe.ex

{api_key, ""} passing this parameter as empty string does not make sense here.

defp commit(method, path, params, opts) do
    config = Keyword.fetch!(opts, :config)
    # TODO: credentials should be investigated why it is {api_key, ""}
    # Did to mimic the earlier behaviour.
    method
      |> http("#{@base_url}/#{path}", params, credentials: {config[:api_key], ""})
      |> respond
  end

Please do necessary investigation for this one and fix it.

Edit:

• Remove http and money utils from Base
• Remove random* methods from Bogus

Review test cases for correct implementation.

In wiredcard gateway inside authorize method we are trying to manipulate the options depending on type of payment method using method

check_and_return_payment_method(payment_method)

however same could be done by using pattern matching feature of elixir by creating two identical functions with params with diff type and pattern match the input params based on it.

The Authorize.Net gateway only supports USD for now.
Create the functionality to support other currencies.

In Stripe Gateway, in card_params and address_params functions I think the struct is unnecessarily converted to map inorder to access the values. which could be easliy done using [dot] operator.
E.g:
card.name
Please go through the link to see how to access values in struct.
Get a value from a struct

defp card_params(%CreditCard{} = card) do
   card = Map.from_struct(card)

   [ "card[name]": card[:name],
     "card[number]": card[:number],
     "card[exp_year]": card[:year],
     "card[exp_month]": card[:month],
     "card[cvc]": card[:verification_code]
   ]   
 end

Current state of Response

We can add arbitrary fields in a Response thanks to Response.new().

Improvements

1. [Deprecate] The :success field seems redundant as all API calls return the usual {:ok, Response} or {:error, Response}
2. [Rename] The token/ID returned for each transaction should go into a key named :id instead of :authorization IMO.
3. [Addition] We should keep a separate key for a token named :token (:authorization is also fine).
4. [Format] We should also create a list of error atoms which are uniform for all gateways This will help users to pattern match on these atoms. Curently we have just 2 tags: :ok and :error, but we could,
   • return tuples with different kinds of tags, like :ok, :error, :declined, :invalid, etc.
   • return just :ok and :error tuples, but also add a reason or error_reason field:

   error_reason: {:server_error, "reason"} | {:invalid_format, "reason"} | {:network_error, "reason"}`

   where "reason" is the error msg string returned by the gateway.
5. [Rename] :params to :raw
   • What should be the format for this? JSON (map), raw string, keyword-list?
   • We could make this a tuple, {:html, String.t}, {:json, Map.t} (already decoded), {:xml, ??}
6. [Format] :avs_result is a tuple, a map would be better as the keys would be explicit,

   %{address: string, zip_code: string}

TL;DR

New definition:

Update: This is WIP here

[Recommendation]

It's clear that using floating point numbers for representing money is a bad idea, and using plain integers fairly limits the application since there exist currencies with more than 2 decimal places. This rules out supporting money as it uses integers.

Which kind of monie then?

ex_money and monetized both use Decimals to represent money, but ex_money offers some more features than monetized, like:

• ISO 4127 aware formatting.
• Explicit rounding obeys the rounding rules as defined by the Unicode consortium in its CLDR repository and implemented by the hex package ex_cldr. These rules define the number of fractional digits for a currency and the rounding increment where appropriate.
• Provides simple utilities for financial accounting (a plus point for merchants -- merchants are more likely to adopt ex_money).
• Provides a way to perform currency conversion without writing extra code.

Who benefits from ex_money

There's not much to gain for Gringotts, all it does is format the amount (as documented in the gateway or as per ISO4127) to construct a correct POST request for the gateway. The real benefit is to merchants already using ex_money, or planning to do so. They won't have to convert their super awesome Money structs into floats/ints/string and be sure that Gringotts does not cause them any loss.

What will this break?

• All methods accepting an amount parameter will now accept only Money.t.

  amount = ~M[12.34]USD # or Money.new("USD", 12.34)
  Gringotts.purchase(:payment_worker, Gringotts.Gateways.XYZ, amount, card, opts)

• [Discuss] Should we deprecate usage of float/integer or should we completely remove support for it?
  • There's no point in accepting floats and converting internally to Money.t only to convert them back into plain strings in the end.
  • Working with integers will make it impossible to unify our API across gateways. Some gateways like to work in 1/100th units some in units, so if we allow integers, we'll have to decide if Gringotts works in 1/100th units or unit, then enforce that in our API. Let's just use Money.t instead as our unambiguous "protocol".
• [Resolution] The ayes have it, the ayes have it: Remove support float and integer completely!

Migration tips (for merchants)

Any merchant doing even trivial financial accounting in their app would be using some Money lib.

• Merchants still using integers or floats should consider using a Money lib, preferably ex_money.
• Create a Money.t struct from whatever your representation is and pass that in Gringott's API methods.
• We could work on @kipcole9's suggestion of implementing a protocol and accept any Money lib that implements it. But how do we go about doing that?

TL;DR

Let's use ex_money or a protocol and track it's integration into Gringotts here.

doctests will help us catch any stale examples in the documentation, in case the examples break due to changes in code.

The following need to be resolved for doctests to work:

1. The outputs also have to be written down in the documentation

• Typical response objects are huge (will take up a lot of space).
• Responses are unique and can't be tested against.

2. Our examples invoke calls on the :payment_worker but there's no gateway configuration in the app.
3. We might be able to mock the remotes during the doctests too, but I'm not sure how. Mocking is important because all doctests will fail otherwise, because the remote responses are unique each time and we can't write the responses down in the docs.

Currently, we are using the gateway expected case styling in the gateway config block in the application code.

This introduces a lot of disparity in between the gateway configs.

Proposal:-

We have a common style and we create a map at gateway level in library to interface gateway keys with config application level keys.

Changes to be made in all the existing gateways;

• Map all the application level config keys to the gateway specific keys and then send the request.
• any other changes?.. developing...

This would ensure that we have a consistent API for config.

File has the variable name before the pattern while most of the files have the variable name after the pattern when naming parameter pattern matches

https://codeclimate.com/github/aviabird/gringotts/lib/gringotts/gateways/monei.ex#issue_5a3a55fffc0835000100004b

Modules should have a @moduledoc tag.

https://codeclimate.com/github/aviabird/gringotts/lib/gringotts/gateways/paymill.ex#issue_5a3a55fffc0835000100004c

Review test cases for correct implementation

When a network time out happens we need to handle that gracefully.

AuthorizeNet converts Decimal to float, defeating the purpose of the protocol.

The certain requirements which are common to all the gateways e.g. currency can be kept in the global config.
These should be overridden if something else is set by the gateway.
Example
Global configuration

config :gringotts, :global_config,
    mode: :test
    default_currency: 'USD'

Gateway specific configuration

# Keep the `key` name same as the adapter name
config :gringotts, Gringotts.Gateways.Stripe,
    adapter: Gringotts.Gateways.Stripe,
    api_key: "sk_test_vIX41hayC0BKrPWQerLuOMld",
    default_currency: "EUR"

Updated Checks and modifications needed.(25th Jan)

• All the gateways require changes for this feature; sometimes there is just one url for both test and live, some have just one.
• Add this in the main documentation and on the getting started guide.

Updates(25th Jan):

We are just adding test mode in the global_config.

Abstract/extract following modules.

Checklist

• 1. Response struct creation for all the gateways to return a common API. assigned to @SagarKarwande #1
• 2. Request module for wrapping all the requests
• 3. SSL requests also which need a certificate in the application to make a call(need to research on this more).

order_id is not received is response after successful transaction.
Need to formatted properly.

Review test cases for correct implementation

The current version of the library supports config in which the values have to be hard coded

config :Gringotts, Gringotts.Gateways.Stripe,
        adapter: Gringotts.Gateways.Stripe,
        api_key: "sk_test_vIX41hC0sdfBKrPWQerLuOMld",
        default_currency: "USD"

But, the values for the config should be picked from the system environment variables.

config :Gringotts, Gringotts.Gateways.Stripe,
        adapter: System.get_env("ADAPTER"),
        api_key: System.get_env("API_KEY"),

Currently, we are passing the worker name in the Kuber.Hex.purchase(.,.,.) This is not the ideal behaviour.

Ideally, the application should have no knowledge of the workers etc. It should be managed at the lib level etc.

gringotts.new

Generates just a gateway implementation file, it should also create:

• test/gateways/<gateway>_test.exs
• test/integration/gateways/<gateway>_test.exs

Which mocking library do we endorse, bypass or mox (I'm strongly against mock)?

Test coverage

The task has no coverage, take inspiration from phx.new.

When the api takes more time to respond, the Genserver timeouts.

One options is to wait for the request to complete using

call(worker, {:authorize, gateway, amount, card, opts}, :infinity)

There is third filed which whcih specifies time in milliseconds for the handle_call of genserver. Default is 5s

standard we have followed in all the calls is id, amount, config but this is not followed in the refund method of the worker.ex file.

Make changes in this format as per the standard.

This would require change in all the gateway files.

Integration tests must test each request's life; for example, there should be a test for these:

• authorize -> capture -> refund
• authorize -> void

Additions to the protocol

to_integer (for lack of a better name)

@spec to_integer(Money.t) :: {ISO4217 :: String.t, amount :: integer, exponent :: integer}

The exponent is not used and is just being made available to the application if need be.

This is useful for gateways that require amount as integer (like cents instead of dollars)
Examples:

usd_price = Money.new(4.1234, :USD)
Gringotts.Money.to_integer(usd_price) #=> {"USD", 412, -2}
bhd_price = Money.new(4.1234, :BHD)
Gringotts.Money.to_integer(bhd_price) #=> {"BHD", 4123, -3}
# the Bahraini dinar is divided into 1000 fils unlike the dollar which is divided in 100 cents

to_string

@spec to_string(Money.t) :: {ISO4217 :: String.t, amount :: String.t}

Returns a tuple of ISO4217 currency code and the amount as string. The amount must match this regex: ~r[\d+\.\d\d{n}] where n+1 should match the required precision for the currency. Gringotts cannot and will not validate this of course.

Examples:

usd_price = Money.new(4.1234, :USD)
Gringotts.Money.to_string(usd_price) #=> {"USD", 4.12}
bhd_price = Money.new(4.1234, :BHD)
Gringotts.Money.to_string(bhd_price) #=> {"BHD", 4.123}

Large numbers should be written with underscores: 20_204

https://codeclimate.com/github/aviabird/gringotts/lib/gringotts/gateways/paymill.ex#issue_5a3d1818f6edd90001000052