Introducing the Money protocol #71

oyeb · 2018-01-02T19:34:13Z

Building on the discussion in #62, this PR introduces a protocol to replace amount argument, and removes the need to specify currency in config or opts.
There's a fallback implementation for Any which enables us to support ex_money and monetized right out of the box.

Todo

Please push commits to resolve the following:

Fix examples in README
Possibly rename the protocol method amount to value... as an amount can be seen to have a value in a specific currency.
Add integration tests for a money lib.
Add ex_money as dependency and implement the protocol for it.

pkrawat1 · 2018-01-03T09:25:07Z

test/gateways/monei_test.exs

@@ -196,6 +197,12 @@ defmodule Gringotts.Gateways.MoneiTest do
    then = Enum.map(all, &Gateway.respond({:ok, &1}))
    assert Keyword.keys(then) == [:ok, :error, :error, :error]
  end
+
+  defp amount(amount, currency \\ "USD") do
+    %{amount: Decimal.new(amount), currency: currency}


@oyeb instead of using a simple map, use ex_money struct example here, put ex_money as only a dependency for test environment.

Or you also write 3 test cases for testing 3 existing libs by passing money structs of these libs showing how we accept all the libraries in elixir.

cc @kartikjagdale @arjun289 @SagarKarwande

@pkrawat1 agree with you on this.

pkrawat1 · 2018-01-03T09:26:56Z

test/integration/gateways/monei_test.exs

@@ -62,4 +62,5 @@ defmodule Gringotts.Integration.Gateways.MoneiTest do
    assert config[:adapter] == Gringotts.Gateways.Monei
  end

+  defp amount(amount, currency \\ "EUR"), do: %{amount: Decimal.new(amount), currency: currency}


same here as above comment, modify test cases to show it works with dependencies? what do you think?

Actually compatibility with external libs can be tested in a single file I am thinking.

Thoughts ? @kartikjagdale @arjun289 @oyeb

Not without adding them as dependencies that are available in test environment.

@oyeb agree here with you...

So do we agree to add ex_money and monteized to our dependencies?

Yes, also keep the any fallback just in case. We need more documentation stating out of box support for these libs. @oyeb

pkrawat1 · 2018-01-03T09:27:34Z

mix.exs

@@ -74,8 +75,14 @@ defmodule Gringotts.Mixfile do
    [
      main: "Gringotts",
      logo: "images/lg.png",
-      source_url: "https://github.com/aviabird/gringotts"
+      source_url: "https://github.com/aviabird/gringotts",
+      groups_for_modules: groups_for_modules()


Cheers for this! Nothing to be done here 👍

pkrawat1 · 2018-01-03T09:29:32Z

lib/gringotts/gateways/monei.ex

@@ -252,22 +242,18 @@ defmodule Gringotts.Gateways.Monei do
  The following session shows how one would process a payment in one-shot,
  without (pre) authorization.

-      iex> opts = [currency: "EUR"] # The default currency is EUR, and this is just for an example.
+      iex> amount = %{amount: 40, currency: "EUR"}


I still don't understand why were we passing opts = [currency: "EUR"] in the first place? It was available all along in the gateway configs and hence in the gateway files in the lib.

Thoughts: @oyeb @kartikjagdale @arjun289 @Zeus999 ?

@ashish173 in gateway configs, it was the default_currency, to fallback to this currency if someone doesn't specify currency.

I had implemented just 1 optional param, currency. So it was the only thing that I could show an example for 😄
I've always been against having default_currency in gateway/lib-global configs. Thankfully, with this PR all that will go away.

@oyeb why u r against with default_currency? what problems you find in that?

@pkrawat1 agree with @ashish173 here that user should be able to pass a default currency through options.

@kartikjagdale I don't think there is a need for that, if the user is passing a currency in Money struct, then what is the use of of passing default currency in options. it should be handled by our application only.

All amounts should necessarily, explicitly have an associated currency. Pls explain the need for a default on #62.

yes, now we don't have the need for the default currency.

Okay, I've read all the comments above and for the sake of organisation I am listing some points which would benefit from having default_currency.

In the application Instead of using "USD"strings in all the requests we can have a single source of truth for currency per gateway(assuming we have multiple gateways) or a global setting for default_currency this way we will be able to use that to create our request params for lib.

This lead to better organisation per gateway. Please note that we don't use this config directly in the library. We rely on what is send in the request.

cc: @oyeb @kartikjagdale @Zeus999 @arjun289

pkrawat1 · 2018-01-03T09:30:56Z

lib/gringotts/gateways/monei.ex

@@ -30,7 +30,6 @@ defmodule Gringotts.Gateways.Monei do
  | ----                | ---    | ----            |
  | `billing_address`   |        | Not implemented |
  | `cart`              |        | Not implemented |
-  | `currency`          |        | **Implemented** |


@oyeb why did we remove this one? these params show what goes in the payments gateway and this PR should not have any change in the effect? Am I missing something here?

currency is no longer an optional arg. It's explicitly available with the amount.

okay, I think I misunderstood that! 👍

pkrawat1 · 2018-01-03T09:33:02Z

lib/gringotts/gateways/monei.ex

@@ -107,7 +106,7 @@ defmodule Gringotts.Gateways.Monei do
  aliases to it (to save some time):
  ```
  iex> alias Gringotts.{Response, CreditCard, Gateways.Monei}
-  iex> opts = [currency: "EUR"] # The default currency is EUR, and this is just for an example.
+  iex> amount = %{amount: 40, currency: "EUR"}


Doing this is okay, for testing purpose developer can use a Map but in case of code follow my comments in this PR it would awesome if we can somehow make it explicit that we support 2-3 money libs out there, maybe a bunch of grouped test cases just thinking?

Thoughts? @oyeb @kartikjagdale @arjun289 @SagarKarwande @Zeus999

I'd love to do that! And I'd love to add integration tests that make sure ex_money and monetized are compliant.
All that's stopping me is that I'll have to add them as an optional or only: :test dependency.

And if we are adding them as dependency, I suggest we get them as optional: :true instead of only: :test as that will let us explicitly implement the protocol for them (which is currently happening via the fall back to any)

@oyeb it will awesome if we have integration test for ex_money and monetized scenarios.

@oyeb Yes, let's add them as optional and implement the protocol, make sure we have a fallback in any case.

chandradot99 · 2018-01-03T10:17:40Z

lib/gringotts.ex

@@ -92,10 +101,8 @@ defmodule Gringotts do
        country: "US",
        postal_code: "11111"
      }


payment map is been divided into CreditCard and Address struct. so @oyeb this example needs to be change accordingly and now we are passing Address into the opts keyword.

What's a payment map? I'm not familiar with the recent changes in stripe (and also AFK), so I'll appreciate it if you can push a commit to this branch with the corrections.

@oyeb ok I will make the required changes.

@Zeus999 I don't understand how is this change flowing backwards from gateway to gringotts.ex, care to give more info.

@pkrawat1 I think, the one who added the docs in gringotts.ex copied the docs from stripe.ex.

@Zeus999 I added that 🥇 anyways @oyeb please make sure this is fixed in a way such that it does not depend on the gateway! @oyeb @Zeus999 collaborate on this.

@pkrawat1 I am fixing this.

kartikjagdale

LGTM 👍

oyeb · 2018-01-05T17:11:05Z

lib/gringotts/gateways/monei.ex

    params =
      [
        paymentType: "PA",
-        amount: :erlang.float_to_binary(amount, decimals: 2),
-        currency: currency(opts)
+        amount: amount |> Money.amount |> Decimal.to_float |> :erlang.float_to_binary(decimals: 2),


We could rename the protocol method Money.amount to Money.value perhaps, then this would read:

amount: amount |> Money.value |> Decimal.to_float |> :erlang.float_to_binary(decimals: 2)

Writing Money.value(amount) is better than writing Money.amount(amount), no?

oyeb · 2018-01-07T17:57:22Z

Task 3

I've hit a wall with writing integration tests for the money libs. The problem is that all these libs have a "naked" Money module/type, so if I add them to our deps(), the module gets redefined, as expected!

Moreover, there's no way for me to refer to the Money type from ex_money differently from the Money type from money (note that there's always only 1 Money type at the end (of compilation)).

oyeb · 2018-01-07T18:02:59Z

Task 4

Because the module is naked, I can't implement the protocol for them in Gringotts. But we can send a PR to their projects, where they can implement our protocol. They won't have to add our lib as a dependency. It'll look something like:

# Add this in their money.ex
if Code.ensure_compiled?(Gringotts.Money) do
 defimpl value(money) do
    ...
 end
 defimpl currency(money) do
    ...
 end
end

If we ever change our protocol, we send PRs. If they make a breaking change later, it's their responsibility to mend it.

* Since amounts are Decimal.t, we depend on the decimal lib. - but it's marked as optional, so our users will have to explicitly add it to their deps * Added a group for gateways in the docs, for more features of ex_docs see how other use it: https://github.com/kipcole9/money/blob/v1.1.0/mix.exs

* no need of currency in opts, removed @default_currency * updates tests (even integration) to use a map for amount

There's much t0d0 in here. * Documented the amount section

* Changed `amount` -> `value`

* We'll just be keeping ex_money in tests, as all Money libs keep their modules "naked". See this thread: https://elixirforum.com/t/working-with-modules-with-same-name/11364

* Re-ordered methods, they are now alphabetic. * docs are not tied to any specific gateway.

kipcole9 · 2018-01-14T22:10:48Z

lib/gringotts/money.ex

+end
+
+defimpl Gringotts.Money, for: Any do
+  def currency(money), do: Map.get(money, :currency)


May I suggest calling to_string on the map result which would then normalise from libs (like ex_money) that use an atom for currency code?

def currency(money), do: Map.get(money, :currency) |> to_string

or to ensure uppercase per your definitions:

def currency(money), do: Map.get(money, :currency) |> to_string |> String.upcase

I think this is covered in 1076a9f (in this PR)
Both money and ex_money use upcase atoms, so the implementation for Money (not Any) will fire and give us a string. monetized uses upcase strings, so there's no need to do an upcase.

kipcole9 · 2018-01-14T22:14:43Z

lib/gringotts/money.ex

+
+defimpl Gringotts.Money, for: Any do
+  def currency(money), do: Map.get(money, :currency)
+  def amount(money), do: Map.get(money, :amount)


Perhaps to enforce the use of Decimals this might be:

def amount(%{amount: %Decimal{} = amount}), do: amount

We'll definitely introduce this when we add the to_float_string and to_integer methods to the protocol.

Building on the discussion in #62, this PR introduces a protocol to replace amount argument, and removes the need to specify currency in config or opts. Supporting `ex_money` and `monetized` money libs out of the box. Usage:- Money can now be passed like this while calling the public API methods. money = %{amount: Decimal.new(2017.18), currency: "USD"} Conversation for this happened here on elixir forum and https://elixirforum.com/t/gringotts-a-complete-payment-library-for-elixir-and-phoenix-framework/11054

oyeb added the discussion label Jan 2, 2018

ashish173 added the PR state: WIP label Jan 2, 2018

pkrawat1 reviewed Jan 3, 2018

View reviewed changes

chandradot99 reviewed Jan 3, 2018

View reviewed changes

kartikjagdale approved these changes Jan 3, 2018

View reviewed changes

pkrawat1 assigned oyeb Jan 3, 2018

oyeb commented Jan 5, 2018

View reviewed changes

oyeb force-pushed the money-protocol branch 2 times, most recently from 184944c to d10e347 Compare January 8, 2018 10:52

oyeb mentioned this pull request Jan 9, 2018

Replace floating point numbers with a Money lib #62

Closed

oyeb added 7 commits January 10, 2018 13:06

[money-protocol] Adapted Monei

c5852b6

* no need of currency in opts, removed @default_currency * updates tests (even integration) to use a map for amount

Updated public API docs

f73b3d7

There's much t0d0 in here. * Documented the amount section

Fixed readme examples, added table

8ff2f83

* Changed `amount` -> `value`

Adds protocol implementation, tests for ex_money

1cd73a1

* We'll just be keeping ex_money in tests, as all Money libs keep their modules "naked". See this thread: https://elixirforum.com/t/working-with-modules-with-same-name/11364

Adds implementation for Monetized.Money

1076a9f

[docs] Changed Public API docs

f2cf398

* Re-ordered methods, they are now alphabetic. * docs are not tied to any specific gateway.

oyeb force-pushed the money-protocol branch from 4a82a57 to f2cf398 Compare January 10, 2018 07:36

oyeb added enhancement PR action: review and removed PR state: WIP labels Jan 10, 2018

pkrawat1 added 3 commits January 10, 2018 20:34

added money usage for case when not using standard supported money libs

d13e353

modified docs

8ce97ac

modified example

8592af6

pkrawat1 merged commit d5fda24 into dev Jan 10, 2018

pkrawat1 deleted the money-protocol branch January 10, 2018 19:12

kipcole9 reviewed Jan 14, 2018

View reviewed changes

pkrawat1 mentioned this pull request Jan 25, 2018

keep common requirements in global config #49

Open

2 tasks

oyeb mentioned this pull request Jan 25, 2018

Currency list we are passing in config as string can be atoms #55

Closed

ashish173 mentioned this pull request Apr 22, 2018

Release 1.1.0 #157

Merged

Introducing the Money protocol #71

Introducing the Money protocol #71

Conversation

oyeb commented Jan 2, 2018 • edited Loading

Todo

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

chandradot99 Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkrawat1 Jan 3, 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

oyeb Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chandradot99 Jan 3, 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

kartikjagdale 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 commented Jan 7, 2018 • edited Loading

Task 3

oyeb commented Jan 7, 2018 • edited Loading

Task 4

kipcole9 Jan 14, 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

oyeb commented Jan 2, 2018 •

edited

Loading

chandradot99 Jan 3, 2018 •

edited

Loading

pkrawat1 Jan 3, 2018 •

edited

Loading

oyeb Jan 3, 2018 •

edited

Loading

chandradot99 Jan 3, 2018 •

edited

Loading

oyeb commented Jan 7, 2018 •

edited

Loading

oyeb commented Jan 7, 2018 •

edited

Loading

kipcole9 Jan 14, 2018 •

edited

Loading