[monei] Implements optional/extra params #79
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
arjun289
approved these changes
Jan 15, 2018
lib/gringotts/gateways/monei.ex
Outdated
| | Key | Remark | Status | | ||
| | ---- | --- | ---- | | ||
| | `billing` | Address of the customer, which can be used for AVS risk check | **Implemented** | | ||
| | `cart` | | Not implemented | |
There was a problem hiding this comment.
Why no description for cart, when all other keys have a description?
arjun289
reviewed
Jan 15, 2018
| # MONEI supports payment by card, bank account and even something obscure: virtual account | ||
| # opts has the auth keys. | ||
| # MONEI supports payment by card, bank account and even something obscure: | ||
| # virtual account opts has the auth keys. |
There was a problem hiding this comment.
It would be good if you provide a link to the part of Monei documentation which describes this. The various supported payment methods.
There was a problem hiding this comment.
Link added in the real docs.
arjun289
reviewed
Jan 15, 2018
| @@ -389,27 +442,40 @@ defmodule Gringotts.Gateways.Monei do | |||
| end | |||
|
|
|||
| # Makes the request to MONEI's network. | |||
| @spec commit(atom, String.t(), keyword, map) :: {:ok | :error, Response} | |||
| @spec commit(atom, String.t(), keyword, keyword) :: {:ok | :error, Response} | |||
There was a problem hiding this comment.
spec for a private method, is it required?
There was a problem hiding this comment.
It won't appear in the docs anyways. Dialyzer can use it too.
arjun289
reviewed
Jan 15, 2018
lib/gringotts/gateways/monei.ex
Outdated
| defp verification_result(%{"result" => result} = data) do | ||
| {address, zip_code} = @avs_code_translator[result["avsResponse"]] | ||
| code = result["code"] | ||
|
|
||
| results = [ | ||
| token = data["registrationId"] |
There was a problem hiding this comment.
The parsing job in this function can be delegated to a separate private function you can return a tuple containing the result and only do verification of the result here.
{token, common,code} = som_priv_function()
oyeb
force-pushed
the
iss36-monei-opts
branch
from
d3ba40a
to
fc1c728
Compare
|
@oyeb could you resolve the conflicts on this PR. |
* Added EUR to the supported currencies.
* Devised a nice way to handle extra params. - This can handle validation and works in a fail-fast manner
* Removed duplicate code that extracted auth_info from opts * Part of the params to Monei are built in the methods, all extra params are built and validated in commit. * Fixed base_url and version
* Added examples of these in docs and integration tests
* Credo complains that expand_params is too complex as there is too much branching in it. I'm opening an issue for it.
* Added tests on register
* All optional params covered * changed markdown link-style to improve plain-text doc flow. * corrected inline examples
* Also stripped whitespace
* MONEI requires all amounts to follow this regex
[0-9]{1,8}(\.[0-9]{2})? so we no longer support TND (Tunisian Dollar) as
it requires 3 decimal places.
* The inclusion was (partially) implicitly tested in the integration tests, but I moved it to more visible mock tests. * Ran the formatter on implementation and both tests * Fixed a hard to catch typo
oyeb
force-pushed
the
iss36-monei-opts
branch
from
218ee33
to
1fbf8e5
Compare
pkrawat1
pushed a commit
that referenced
this pull request
Jan 25, 2018
* Validates currency, test for unsupported currency * Added EUR to the supported currencies. * Refactored expansion and validation of extras * Removed duplicate code that extracted auth_info from opts * Part of the params to Monei are built in the methods, all extra params are built and validated in commit. * [extra-params] All optional params covered: billing, shipping and merchant, invoice_id, category, transaction_id, custom, register, customer * Added examples of these in docs and integration tests * The inclusion was (partially) implicitly tested in the integration tests, but I moved it to more visible mock tests.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #36