Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SagePay]Implemented authorize function for sagepay #131

Closed
wants to merge 20 commits into from
Closed

Conversation

Anwar0902
Copy link

In this we generated merchant_session_key and card_identifier to
complete the authorization with all details provided in 'opts' option

@@ -0,0 +1,362 @@
defmodule Gringotts.Gateways.Sagepay do
Copy link
Contributor

Choose a reason for hiding this comment

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

CamelCase module, also their brand is written in that case too: SagePay.




## Instructions!
Copy link
Contributor

Choose a reason for hiding this comment

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

This was an instruction, read once then remove. We cannot merge it in this state can we?

# and with merchant_session_key, card_identifiier ,shipping address of a
# customer, and other details and converting the map into keyword list

defp card_details(amount, merchant_key, card_key, opts) do
Copy link
Contributor

Choose a reason for hiding this comment

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

This function does a lot more than just build card details. Would be better to split it into at least card_details and other_details.

},
"vendorTxCode" => opts[:vendorTxCode],
"amount" => amount,
"currency" => opts[:currency],
Copy link
Contributor

Choose a reason for hiding this comment

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

The currency is not passed via opts! amount is of type Money.t (a struct, defined in the Money library), and it contains the currency.
Read #62 for more info.

"description" => opts[:description],
"customerFirstName" => opts[:customerFirstName],
"customerLastName" => opts[:customerLastName],
"billingAddress" => %{
Copy link
Contributor

Choose a reason for hiding this comment

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

This should have been fetched from an Address struct, not a simple map.

# Parses sagepay's response and returns a `Gringotts.Response` struct
# in a `:ok`, `:error` tuple.

defp format_response(response) do
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. This function does not return a Response struct, though @spec of authorize suggests that it would return a Response tuple.
  2. Please use one of the respond clauses from below.

{"Content-type", "application/json"}
]

body = commit(:post, "transactions", card_params, card_header)
Copy link
Contributor

Choose a reason for hiding this comment

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

This binding serves no purpose.

# Function generate_mkey generate a merchant_session_key that will exist only for 400 seconds
# and for 3 wrong card identifiers

defp generate_mkey(opts) do
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO genereate_merchant_key is a better name here.

Copy link
Contributor

@oyeb oyeb left a comment

Choose a reason for hiding this comment

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

Good work on the docs! ❤️


## Scope of this module

> It's unlikely that your first iteration will support all features of the
> gateway, so list down those items that are missing.
* SagePay does not process money in cents, and the `amount` is converted to integer.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is "rendered" in the authorize request if I wish to make a payment for $3.14, "314" or "3.14"?

> It's unlikely that your first iteration will support all features of the
> gateway, so list down those items that are missing.
* SagePay does not process money in cents, and the `amount` is converted to integer.
* SagePay supports payments from various cards and banks.

## Supported currencies and countries
Copy link
Contributor

Choose a reason for hiding this comment

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

Supported currencies

Could you also add a link to the docs from where you found this info?

[home]: https://www.sagepay.co.uk/
[example]: https://github.com/aviabird/gringotts_example
[iex-docs]: https://hexdocs.pm/iex/IEx.html#module-the-iex-exs-file
[sagepay.iex.exs]: https://github.com/Anwar0902/graph/blob/master/sagepay.iex.exs
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️ 💛 💜 Thanks for making a .iex.exs! There are many ways of sharing it with others, and I highly recommend making a gist instead of a file in a repo.


## Note

> If there's anything noteworthy about this operation, it comes here.
* The merchantSessionKey expires after 400 seconds and can only be used to create one successful card identifier.
* It will also expire and be removed after 3 failed attempts to create a card identifier.
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Who will expire? the session key or the transaction ID?
  2. It'll be great if you add a link to SagePay docs for this!

})
},
"vendorTxCode" => opts[:vendorTxCode],
"amount" => Kernel.trunc(String.to_float(value)),
Copy link
Contributor

Choose a reason for hiding this comment

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

You perhaps want to use Money.to_integer/1. It'll be easier to figure this out if you look at activemerchant implementation of SagePay.

@@ -111,39 +103,70 @@ defmodule Gringotts.Gateways.Sagepay do

alias Gringotts.{Money, CreditCard, Response}
@url "https://pi-test.sagepay.com/api/v1/"

# SagePay supports payment only by Merchant Authorization Id and Merchant name
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this very important comment into the @doc.

## The `opts` argument

Most `Gringotts` API calls accept an optional `keyword` list `opts` to supply
[optional arguments][extra-arg-docs] for transactions with the MONEI

Choose a reason for hiding this comment

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

monei??

Copy link
Contributor

@oyeb oyeb left a comment

Choose a reason for hiding this comment

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

  1. Please remove implementation of capture/3
  2. Make lot of use of backticks in docs, especially for argument names, keys of a keyword list, atoms.

@@ -170,22 +168,43 @@ defmodule Gringotts.Gateways.SagePay do
end

@doc """
Captures a pre-authorized `amount`.
Captures a pre-authorized amount.
Copy link
Contributor

Choose a reason for hiding this comment

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

The backticks were rightly placed. Why did you remove them?


## Note

> If there's anything noteworthy about this operation, it comes here.
> For example, does the gateway support partial, multiple captures?
* Deferred transactions are not sent to the bank for completion until you capture them using the capture instruction.
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. What's a deferred transaction?
  2. You can release...
    what's a release?


## Example

> A barebones example using the bindings you've suggested in the `moduledoc`.
The following example shows how one would (partially) capture a previously
authorized a payment worth 100 by referencing the obtained authorization id.
Copy link
Contributor

Choose a reason for hiding this comment

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

100 pounds or 100 £, units are important (this is not 100 pennies, for example).

@@ -313,12 +340,15 @@ defmodule Gringotts.Gateways.SagePay do
{"Content-type", "application/json"}
]

{:ok, merchant_key} = commit(:post, "merchant-session-keys", vender_name, merchant_header)
{:ok, merchant_key} =
commit_for_key(:post, "merchant-session-keys", vender_name, merchant_header)
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: vendor_name

|> respond
end

defp commit_for_key(:post, endpoint, params, opts) do
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there is only 1 call site for this function, we can move the logic there.

Copy link
Author

Choose a reason for hiding this comment

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

Since the logic of commit_for_key is totally different from the commit. So, I think we can't move.

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad! I didn't realise that commit_for_key is called from 2 functions. However, format_response can be moved inside commit_for_key.

| --------------- |
| [`merchant_id`] |
| [`vendor`] |
| [`vendorTxcode`] |
Copy link
Contributor

@oyeb oyeb Mar 27, 2018

Choose a reason for hiding this comment

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

  1. Please follow the elixir style for these atoms (snake_case).
  2. Why are these surrounded by []?
  3. If there's only going to be billingAddress (and no other addresses), then let's call it address instead.

id: body["transactionId"],
status_code: 201,
message: body["statusDetail"],
raw: body |> Poison.encode!()
Copy link
Contributor

Choose a reason for hiding this comment

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

You were forced to do this just because you didn't use a different var name on L426 😝

@Anwar0902 Anwar0902 changed the title Implemented authorize function for sagepay [SagePay]Implemented authorize function for sagepay Mar 29, 2018
Copy link
Contributor

@oyeb oyeb left a comment

Choose a reason for hiding this comment

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

Please go through your docs and add backticks wherever applicable:

  1. argument name
  2. atom, especially when it is a key in opts


* The merchant_session_key expires after 400 seconds and can only be used to create one successful card identifier.
* It will also expire and be removed after 3 failed attempts to create a card identifier.
* vendor_tx_code Code in opts should always be unique.
Copy link
Contributor

Choose a reason for hiding this comment

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

Use bacticks pls, you've missed them in some more places

a_url = @url <> endpoint

response = HTTPoison.post(a_url, params, opts)
format_response(response)
Copy link
Contributor

Choose a reason for hiding this comment

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

Move the logic in format_response here, as it is called only from here.

"save" => false
}
},
"vendorTxCode" => opts[:vendorTxCode],
Copy link
Contributor

Choose a reason for hiding this comment

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

Though you've updated the moduledoc with snake_case atoms, you haven't updated the code, this should have been opts[:vendor_tx_code]


@moduletag :integration

setup_all do
Copy link
Contributor

Choose a reason for hiding this comment

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

This block is not required, the application config is fetched by Gringotts which is not used here.

"description" => opts[:description],
"customerFirstName" => opts[:customerFirstName],
"customerLastName" => opts[:customerLastName],
"billingAddress" => %{
Copy link
Contributor

Choose a reason for hiding this comment

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

Your docs should say that a billing_address, if provided, should be an Address struct.

Copy link
Contributor

@oyeb oyeb left a comment

Choose a reason for hiding this comment

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

Please make sure your branch is up-to-date with dev:
Use git rebase:

  1. git checkout dev
  2. git pull
  3. git checkout sagepay
  4. git rebase -i dev
    If git rebase turns up a lot of conflicts, we will allow a shortcut just this one time, instead of last step (above) do:
    git merge dev

After doing this, please remove all compile time warnings in both dev and test env by:

  1. dev environment:
  • mix clean gringotts
  • mix compile, and look for warnings from sagepay
  1. test environment:
  • MIX_ENV=test mix clean gringotts
  • MIX_ENV=test mix compile

Also run credo and fix all sagepay related warnings:
mix credo --strict OR mix credo --strict --format=oneline | grep sagepay


## Example

The following example shows how one would (pre) authorize a payment of 42£ on
a sample `card`.

iex> amount = Money.new(42, :GBP)
iex> address = %{ address1: "407 St. John Street", city: "London", postalCode: "EC1V 4AB", country: "GB"}
iex> address = %{ street1: "407 St.", street2: "John Street", city: "London", postalCode: "EC1V 4AB", country: "GB"}
Copy link
Contributor

Choose a reason for hiding this comment

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

address = %Address{street1: ...}

opts = @opts ++ [vendorTxCode: "demotransaction-" <> random_code]
assert {:ok, response} = SagePay.authorize(@amount, @card, opts)
test "successful response with valid params", %{opts: opts} do
use_cassette "SagePay/authorize_with_valid_params" do
Copy link
Contributor

Choose a reason for hiding this comment

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

use_cassette "sagepay/*" do

@codecov-io
Copy link

codecov-io commented Mar 31, 2018

Codecov Report

Merging #131 into dev will decrease coverage by 6.6%.
The diff coverage is 2.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #131      +/-   ##
==========================================
- Coverage   77.33%   70.73%   -6.61%     
==========================================
  Files          14       15       +1     
  Lines         375      410      +35     
==========================================
  Hits          290      290              
- Misses         85      120      +35
Impacted Files Coverage Δ
lib/gringotts/response.ex 100% <ø> (ø) ⬆️
lib/gringotts/gateways/paymill.ex 100% <ø> (ø) ⬆️
lib/gringotts/gateways/sagepay.ex 0% <0%> (ø)
lib/gringotts/gateways/monei.ex 97.95% <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 7e7a5d3...253847a. Read the comment docs.

Copy link
Contributor

@oyeb oyeb left a comment

Choose a reason for hiding this comment

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

Please take a look at #150 and make the necessary correction.

iex> amount = Money.new(42, :GBP)
iex> address = %Address{ street1: "407 St.", street2: "John Street", city: "London", postalCode: "EC1V 4AB", country: "GB"}
iex> card = %Gringotts.CreditCard{number: "4929000005559",month: 3,year: 20,first_name: "SAM",last_name: "JONES",verification_code: "123",brand: "VISA"}
iex> opts = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the :config key from the example, this will be automatically added by Gringotts.get_and_validate_config/1


## Supported currencies and countries

:AUD, :CAD, :CHF, :CYP, :DKK, :EUR, :GBP, :HKD, :INR, :JPY,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the colons.

merchant_key = generate_merchant_key(opts)

card = card_params(card)
card_identifiier = generate_card_identifier(card, merchant_key)
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: card_identifier


case response do
{:ok, %HTTPoison.Response{body: body}} -> {:ok, body |> Poison.decode!()}
_ -> %{"error" => "something went wrong, please try again later"}
Copy link
Contributor

Choose a reason for hiding this comment

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

This error message is never propagated to the Response. If the session key/card identifier cannot be generated, a call to authorize should fail "fast" (we should not attempt to do any further computation).

# Function `generate_merchant_key` generate a `merchant_session_key` that will exist only for 400
# seconds and for 3 wrong `card_identifiers`.

defp generate_merchant_key(opts) do
Copy link
Contributor

Choose a reason for hiding this comment

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

The caller should be able to pass in a merchant key in opts, and in such a case we should simply use it.

Copy link
Author

Choose a reason for hiding this comment

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

This is not possible because its mandatory to create merchant key every time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Typically, a capture follows immediately after a successful authorization - hence the capture can use the same key (as the key is invalidated only after 400 seconds).
We can get to this when we implement capture.

| `vendor` | Name of the merchant. |
| `vendor_tx_code` | vendor_tx_code is a unique code for every transaction in SagePay. |
| `transaction_type` | SagePay allows four transactions type:- Deferred, Payment, Repeat, Refund. |
| `customer_first_name` | First name of a customer. |
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be shortened to first_name and last_name resp.

"customerLastName" => opts[:customer_last_name],
"billingAddress" => %{
"address1" => full_address,
"city" => opts[:billing_address].city,
Copy link
Contributor

Choose a reason for hiding this comment

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

The moduledoc was updated to simply address (L38), but not in code.

a sample `card`.

iex> amount = Money.new(42, :GBP)
iex> address = %Address{ street1: "407 St.", street2: "John Street", city: "London", postalCode: "EC1V 4AB", country: "GB"}
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: postalCode is not defined in Address.t

[gs]: file:///home/anwar/gringotts/doc/Gringotts.Gateways.SagePay.html#
[example]: https://github.com/aviabird/gringotts_example
[iex-docs]: https://hexdocs.pm/iex/IEx.html#module-the-iex-exs-file
[sagepay.iex.exs]: https://github.com/Anwar0902/graph/blob/master/sagepay.iex.exs
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this to a GitHub gist.

customer_last_name: "Jones",
billing_address: address
]
iex> {:ok, auth_result} = Gringotts.authorize(Gringotts.Gateways.SagePay, amount, card, opts)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the response I received:

iex(14)> Poison.decode! auth_result.raw
%{
  "3DSecure" => %{"status" => "CardNotEnrolled"},
  "amount" => %{"saleAmount" => 42, "surchargeAmount" => 0, "totalAmount" => 42},
  "avsCvcCheck" => %{
    "address" => "NotProvided",
    "postalCode" => "NotProvided",
    "securityCode" => "NotProvided",
    "status" => "NotChecked"
  },
  "bankAuthorisationCode" => "999777",
  "bankResponseCode" => "00",
  "currency" => "GBP",
  "paymentMethod" => %{
    "card" => %{
      "cardIdentifier" => "3248A1A6-5A44-470B-BC92-83536C486971",
      "cardType" => "Visa",
      "expiryDate" => "0320",
      "lastFourDigits" => "5559",
      "reusable" => false
    }
  },
  "retrievalReference" => 17717235,
  "status" => "Ok",
  "statusCode" => "0000",
  "statusDetail" => "The Authorisation was Successful.",
  "transactionId" => "EF99769B-EFE0-68EA-17B8-F38F6476B511",
  "transactionType" => "Deferred"
}
  1. The example supplies the (billing) address to SagePay but the avsCvcCheck indicates otherwise. Please get this to work. Even if SagePay does not do the AVS check, you should format this into Response.fraud_review field.
  2. The paymentMethod indicates that this card is NOT reusable. Surely there must be some way to generate a reusable card_identifier?
  3. Please make use of Response.token field to return the card_identifier to the caller.

Copy link
Author

Choose a reason for hiding this comment

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

For point 3:-for the transaction a unique merchant key pair with a unique card identifier.We can't use the card identifier for future transaction.So there is no need to store it.

Copy link

@KillerBeer KillerBeer Apr 24, 2018

Choose a reason for hiding this comment

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

curl https://pi-test.sagepay.com/api/v1/transactions \
-H "Authorization: Basic aEpZeHN3N0hMYmo0MGNCOHVkRVM4Q0RSRkxodUo4RzU0TzZyRHBVWHZFNmhZRHJyaWE6bzJpSFNyRnliWU1acG1XT1FNdWhzWFA1MlY0ZkJ0cHVTRHNocktEU1dzQlkxT2lONmh3ZDlLYjEyejRqNVVzNXU="  \
-H "Content-type: application/json" \
-X POST \
-d '{
    "transactionType": "Payment",
    "paymentMethod": {
        "card": {
            "merchantSessionKey": "<merchant-session-key>",
            "cardIdentifier": "<card-identifier>",
            "save": false
        }
    },
    "vendorTxCode": "demotransaction-<unique-suffix>",
    "amount": 10000,
    "currency": "GBP",
    "description": "Demo Payment",
    "apply3DSecure": "UseMSPSetting",
    "customerFirstName": "Sam",
    "customerLastName": "Jones",
    "billingAddress": {
        "address1": "407 St. John Street",
        "city": "London",
        "postalCode": "EC1V 4AB",
        "country": "GB"
    },
    "entryMethod": "Ecommerce"
}'

When we used this on postman after creating merchant key and card identifier the response was

{
    "statusCode": "2007",
    "statusDetail": "Please redirect your customer to the ACSURL to complete the 3DS Transaction",
    "transactionId": "D6146D7B-033E-9E2A-C52C-87A7D3E107EA",
    "acsUrl": "https://test.sagepay.com/mpitools/accesscontroler?action=pareq",
    "paReq": "eJxVUsluwjAQvfcrIj4gXrJA0WAE5dAIaKOCVKk3y7FKVLJgJ03o19fOUoovnvc8ejPzxrBss7PzLZVOi3wxIS6eLNkDHE9Kys1BilpJBnupNf+UTposJhSTgPrBo4+9aUiIF4Q+mTCIV2/ywmAQYkbHJYBGaBSUOPG8YsDFZR29MN8PvTAANEDIpIo2jBLcn/4G1NOQ80yyg2kh5teNzIqVEEWdV4C6B+iAurIZDQGNAGp1Zk3TuLb1kl9dUbj1FyBLA7o1FNc20kamTRN2rFpSbn8+NEqa3fakd++4vkTJ61o3C0A2AxJeSWZsmGGf+g55nFM896aAOh54Zusz0vc/IChtkdXd038KjM9K5mIcYUQg27LIpcmggP5iSKQWnR2O8cPZx5EpbilAt2Genq3XojL2EWtzF1m91HhDMfY6QQsA2Vw0bBANyzbR3Sf4BaPUtJU=",
    "status": "3DAuth"
}

so this asked to go to 3D secure site and to avs-check than asked not to do 3D secure procedure so we left that part .

in transaction body we have not used "apply3DSecure": "UseMSPSetting" we have done that for without 3D secure

oyeb and others added 12 commits June 7, 2018 14:18
We allow the gateway to return multiple tokens, as a `keyword`.
renamed zip_code -> postal_code
In this we generated merchant_session_key and card_identifier to
complete the authorization with all details provided in 'opts' option
change card configuration as per the Gringotts.CreditCard
change amount structure as per the Gringotts.Money
Implement respond function for response generation as per Gringotts.Response
Struct for billing address as per Gringotts.Address
Improvement in Documentation
Implemented capture function to coplete the transaction
Generation of docs for capture function
Generation og test cases
Correction in respond function
Improvement in Integration test cases for SagePay
Improvement in docs
changed billing_address to address
Changed the naming convention of keys
remove the decode code for body in sagepay
Used backtick for :atoms and keys in opts
Changed camel case for parameters to snake case
Merged commit_for_keys and format_response into format_response
Anwar0902 and others added 8 commits June 7, 2018 18:14
Used use_cassette for test cases
use_cassette for test case is written
Correct the name sagepay in use_cassette
removed extra spaces and changes in HTTPoision
unwanted variables and functions removed.
Add transaction type payment and deffered
Change opts structure
Remove the colon from the currency
* AVS and CVC checks are enforced
* `transaction_type` is no longer fetched from `opts`
* The session key and card ID are both returned under `tokens` field
* Improved test setup and case
@oyeb
Copy link
Contributor

oyeb commented Jun 7, 2018

Work is done. This is slated for v1.3.x
Thanks @Anwar0902 and @KillerBeer 🎉

@oyeb oyeb closed this Jun 7, 2018
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.

None yet

5 participants