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

Adds preliminary support for Klarna #1059

Closed
wants to merge 48 commits into from
Closed
Show file tree
Hide file tree
Changes from 37 commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
dc22957
Adds preliminary support for Klarna
Mar 4, 2014
226c14c
Adds integrations helper support for cart items
Mar 4, 2014
9a3c9a8
Skip unimplemented Klarna notification tests for now
Mar 4, 2014
01b41b9
Format item tax rates as two-place decimals
Mar 8, 2014
9529a0f
Expect regular, non-Hash cart item objects
Mar 17, 2014
5f5ea74
Fixes merchant_digest and tests
Mar 19, 2014
a5bbde4
Part-way through refactoring to use #line_item
Mar 21, 2014
f53f4f6
Klarna module now supports #line_items
Mar 21, 2014
4fc3ab7
Remove refactored-out code
Mar 21, 2014
0f3fbd1
Do not show the shared secret to buyers
Mar 21, 2014
7959954
Remove now-unused cart_items
Mar 21, 2014
1a38ed0
Adding notification stuff
Mar 28, 2014
97c28f8
No Pry for now
Apr 2, 2014
19b7283
Verify notification requests + use new payload format
Apr 16, 2014
1c3909a
Add support for shipping lines
Apr 16, 2014
510645a
Fix up Klarna module test
Apr 16, 2014
3a19e38
Better street address
Apr 16, 2014
162c413
Fix Klarna return URL
Apr 16, 2014
3320f6c
Returns from Klarna are always a success
Apr 16, 2014
bed5868
Rm silly comments
Apr 16, 2014
892cd20
Remove Klarna Return class (not needed)
Apr 21, 2014
bb03426
Clarify required field
Apr 21, 2014
141296e
Move setting test mode up and delete unneeded street address set
Apr 21, 2014
a252f16
Refactor Klarna Notification verifier to #acknowledge
Apr 21, 2014
289a231
Refactor Klarna Notification
Apr 21, 2014
d3cf30f
Remove Klarna Notification#test? since its already implemented
Apr 21, 2014
feca1c7
Refactor out Klarna.digest
Apr 21, 2014
fb3aff9
Assign purchase_country the actual purchasing country
Apr 21, 2014
1f865ed
Refactor out valid_helper
Apr 21, 2014
4d9b9cd
Normalize test values
Apr 21, 2014
0fbf607
Implement #sign_fields on helper
Apr 21, 2014
3dde048
Refactor out CartItem and example_cart_item
Apr 21, 2014
9653ff3
Normalize shared secret
Apr 21, 2014
320b3c7
Add support for customer emails
Apr 22, 2014
57b5241
Make #gross x.00 formatted
Apr 22, 2014
beb6887
Update Klarna to production URL
Apr 28, 2014
d877732
Inline valid? method
Apr 28, 2014
116cf51
Inline street_address
Apr 28, 2014
5560cf1
Correct indentation
Apr 28, 2014
71cd3ce
Fix formatting
Apr 28, 2014
9a3ce57
Split test out
Apr 28, 2014
d521276
Add signature test
Apr 28, 2014
09de314
Add calculated tax rates
Apr 29, 2014
9d99fc1
Actually implement tax rates properly
Apr 29, 2014
95d5880
Default to using sv-se as fallback locale
Apr 29, 2014
cae9e8b
Remove minitest/autorun
Apr 29, 2014
2c57418
whitespace
Apr 29, 2014
b53abb4
Do tax calculation math correctly
Apr 29, 2014
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 13 additions & 12 deletions lib/active_merchant/billing/integrations/helper.rb
Expand Up @@ -19,16 +19,16 @@ def self.inherited(subclass)

def initialize(order, account, options = {})
options.assert_valid_keys([:amount, :currency, :test, :credential2, :credential3, :credential4, :country, :account_name, :transaction_type, :authcode, :notify_url, :return_url, :redirect_param, :forward_url])
@fields = {}
@raw_html_fields = []
@test = options[:test]
self.order = order
self.account = account
self.amount = options[:amount]
self.currency = options[:currency]
self.credential2 = options[:credential2]
self.credential3 = options[:credential3]
self.credential4 = options[:credential4]
@fields = {}
@raw_html_fields = []
@test = options[:test]
self.order = order
self.account = account
self.amount = options[:amount]
self.currency = options[:currency]
self.credential2 = options[:credential2]
self.credential3 = options[:credential3]
self.credential4 = options[:credential4]
Copy link
Contributor

Choose a reason for hiding this comment

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

These used to be aligned?

self.notify_url = options[:notify_url]
self.return_url = options[:return_url]
self.redirect_param = options[:redirect_param]
Expand Down Expand Up @@ -100,8 +100,9 @@ def lookup_country_code(name_or_code, format = country_format)

def method_missing(method_id, *args)
method_id = method_id.to_s.gsub(/=$/, '').to_sym
# Return and do nothing if the mapping was not found. This allows
# For easy substitution of the different integrations

# Return and do nothing if the mapping was not found. This allows
# for easy substitution of the different integrations
return if mappings[method_id].nil?

mapping = mappings[method_id]
Expand Down
71 changes: 71 additions & 0 deletions lib/active_merchant/billing/integrations/klarna.rb
@@ -0,0 +1,71 @@
require File.dirname(__FILE__) + '/klarna/helper.rb'
require File.dirname(__FILE__) + '/klarna/notification.rb'
require 'digest'

module ActiveMerchant #:nodoc:
module Billing #:nodoc:
module Integrations #:nodoc:
module Klarna
mattr_accessor :service_url
self.service_url = 'https://api.hostedcheckout.io/api/v1/checkout'

def self.notification(post_body, options = {})
Notification.new(post_body, options)
end

def self.return(query_string, options = {})
Return.new(query_string, options)
end

def self.cart_items_payload(fields, cart_items)
check_required_fields!(fields)

payload = fields['purchase_country'].to_s +
fields['purchase_currency'].to_s +
fields['locale'].to_s

cart_items.each do |item|
payload << item[:type].to_s +
item[:reference].to_s +
item[:quantity].to_s +
item[:unit_price].to_s +
item.fetch(:discount_rate, nil).to_s
end

payload << fields['merchant_id'].to_s +
fields['merchant_terms_uri'].to_s +
fields['merchant_checkout_uri'].to_s +
fields['merchant_base_uri'].to_s +
fields['merchant_confirmation_uri'].to_s

payload
end

def self.sign(fields, cart_items, shared_secret)
payload = cart_items_payload(fields, cart_items)

digest(payload, shared_secret)
end

def self.digest(payload, shared_secret)
Digest::SHA256.base64digest(payload + shared_secret.to_s)
end

private

def self.check_required_fields!(fields)
%w(purchase_country
purchase_currency
locale
merchant_id
merchant_terms_uri
merchant_checkout_uri
merchant_base_uri
merchant_confirmation_uri).each do |required_field|
raise ArgumentError, "Missing required field #{required_field}" if fields[required_field].nil?
end
end
end
end
end
end
91 changes: 91 additions & 0 deletions lib/active_merchant/billing/integrations/klarna/helper.rb
@@ -0,0 +1,91 @@
module ActiveMerchant #:nodoc:
module Billing #:nodoc:
module Integrations #:nodoc:
module Klarna
class Helper < ActiveMerchant::Billing::Integrations::Helper
mapping :currency, 'purchase_currency'
mapping :return_url, 'merchant_confirmation_uri'
mapping :notify_url, 'merchant_push_uri'
mapping :cancel_return_url, ['merchant_terms_uri', 'merchant_checkout_uri', 'merchant_base_uri']
mapping :account, 'merchant_id'
Copy link
Contributor

Choose a reason for hiding this comment

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

there is no mapping for customer or billing address?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It isn’t necessary since I used add_field instead.

Would you prefer I used mappings? I find them to be clumsy and indirect in comparison with add_field

Copy link
Contributor

Choose a reason for hiding this comment

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

That's where I'm getting confused. You're using the details from shipping_address, which could differ from the customer and billing_address that we'd send it. I'm just wondering how those aren't being mapped to anything on Klarna's side and we're just throwing them away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’m following up with Klarna on this issue, but here’s what I’ve got:

Here’s an example HTML form that I copy/pasted from a working forward.html.erb page: https://gist.github.com/edward/115f37ba310a2b6f91bb

Here’s a doc covering the fields that we send Klarna’s HPP: https://hpp-staging-eu.herokuapp.com/api/doc#!/checkout/POST--version-checkout---format-_post_0

Here’s an example form-maker thing that lets you construct a Klarna-valid form and send it to their checkout: https://hpp-staging-eu.herokuapp.com/example/checkout-form/advanced – I’m unable to make it pass an email, phone number, etc. to Klarna.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it blow up on their end when we try?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the customer info is all valid, there is no effect.

If the customer info indicates that there’s a customer trying to purchase something from an unsupported country, it blows up.

mapping :customer, email: 'shipping_address_email'

def initialize(order, account, options = {})
super
@shared_secret = options[:credential2]

add_field('platform_type', application_id)
add_field('test_mode', test?)
end

def line_item(item)
@line_items ||= []
@line_items << item

i = @line_items.size - 1

add_field("cart_item-#{i}_type", item.fetch(:type, ''))
add_field("cart_item-#{i}_reference", item.fetch(:reference, ''))
add_field("cart_item-#{i}_name", item.fetch(:name, ''))
add_field("cart_item-#{i}_quantity", item.fetch(:quantity, ''))
add_field("cart_item-#{i}_unit_price", item.fetch(:unit_price, ''))
add_field("cart_item-#{i}_discount_rate", item.fetch(:discount_rate, ''))
add_field("cart_item-#{i}_tax_rate", item.fetch(:tax_rate, ''))

@fields
end

def billing_address(billing_fields)
country = billing_fields[:country]

add_field('purchase_country', country)
add_field('locale', guess_locale_based_on_country(country))
end

def shipping_address(shipping_fields)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could these be done via the standard mappings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ll look again – I felt this was more readable so I went this way first

add_field('shipping_address_given_name', shipping_fields[:first_name])
add_field('shipping_address_family_name', shipping_fields[:last_name])

street_address = [shipping_fields[:address1], shipping_fields[:address2]].join(', ')
add_field('shipping_address_street_address', street_address)

add_field('shipping_address_postal_code', shipping_fields[:zip])
add_field('shipping_address_city', shipping_fields[:city])
add_field('shipping_address_country', shipping_fields[:country])
add_field('shipping_address_phone', shipping_fields[:phone])
end

def form_fields
sign_fields

super
end

def sign_fields
merchant_digest = Klarna.sign(@fields, @line_items, @shared_secret)
add_field('merchant_digest', merchant_digest)
end

private

def street_address
Copy link
Contributor

Choose a reason for hiding this comment

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

not used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks

[address1, address2].compact.join(', ')
end

def guess_locale_based_on_country(country_code)
case country_code
when /no/i
"nb-no"
when /fi/i
"fi-fi"
when /se/i
"sv-se"
else
raise StandardError, "Unable to guess locale based on country #{country_code}"
Copy link
Contributor

Choose a reason for hiding this comment

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

What about just defaulting to something?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd still prefer a sane default vs an exception

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you suggest a default or an approach to determining one?

This really is exceptional behaviour here – would we not want to fail fast on something like this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Umm, which country has the biggest population? ¯(°_o)/¯ idunnolol
Does Klarna enforce that the purchase_country is one of its supported countries NO, FI, or SE?? If not, could a shop not be can be in US, but be using Klarna because it sells to a country Klarna supports and uses that gateway. If we raise here, customers would never be able to make a purchase on this shop. (Also, isn't purchase_country supposed to be where the purchase is being made, not the merchants country likes its been set?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Sweden has the biggest population at 9 million, but this thing will likely be supporting Germany soon at 81 million
  • The thing enforcing purchase_country is Shopify – based on the billing address, we show payment options that work for that country, right? This Shopify-supplied country code is passed to this method.

It really is an exceptional case, but I can set it to 'sv-se' so as to not bail out.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're currently setting purchase_country to @options[:country] though, that's the shops country, not the customers country.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make a reasonable guess using geoip?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You’re right – that’s my mistake; I’ll change this to actually be the country where the purchase is happening.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bslobodin no, guessing locales is a super tricky thing.

I really do feel like this is an exceptional case. I’d rather set it to be 'sv-se' since the worst-case scenario there is that it will at least work, but might make the buyer guess some fields.

I’m going to see if I can also set this locale to be 'uk-en' or 'us-en' and see what happens. These docs show an English-language screen but this doc does not mention English as being a supported locale.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried setting it to values like 'us-en', 'uk-en', 'se-en' and the HPP just crashes.

I’m asking Klarna.

end
end
end
end
end
end
end
98 changes: 98 additions & 0 deletions lib/active_merchant/billing/integrations/klarna/notification.rb
@@ -0,0 +1,98 @@
require 'net/http'
require 'json'

module ActiveMerchant #:nodoc:
module Billing #:nodoc:
module Integrations #:nodoc:
module Klarna
class Notification < ActiveMerchant::Billing::Integrations::Notification
Copy link
Contributor

Choose a reason for hiding this comment

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

This still needs to be complete, or removed if Klarna doesn't support Notifications.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. This is still a WIP; confirming this behaviour now – just wanted to get the rest of it sorted first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ve filled this in. Yay notifications!

def initialize(post, options = {})
super
@shared_secret = @options[:credential2]
end

def complete?
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, in Shopify’s OffSitePaymentGateway

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked and couldn't find any?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad – I was thinking of something else. I’ll remove this.

Should we change the template generated by AM to not provide this method stub?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see where it’s used now – it was in the default generated test suite.

Since it’s pretty much the closest thing I have to an expected API, I filled it in. I’ll update this and the test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah gotcha, maybe leave it then if it's part of some convention.

status == 'Complete'
end

def item_id
params["reservation"]
end

def transaction_id
params["reference"]
end

def received_at
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, in the generated test suite. (Same answer for the rest of these comments. I’ll remove them.)

Should we remove this from the generated test suite?

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that you mention it, I don't know what the right thing to do is. I know we don't use some of these, but perhaps others have grown to expect them. I'm still leaning towards smallest possible footprint for initial implementation and if someone needs these in the future, PRs are welcomed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’m cool with removing them, but I feel like if we’re going to do that, then we should update the generators, or else we’re just perpetuating this issue and making the next gateway implementor’s job more complex.

@odorcicd what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I spoke with Denis in person and his recommendation was to let it be and leave it in.

Should we decide to remove it, it should be removed from the generators too, but we should consult with the Spree guys who are also contributors to AM.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

params["completed_at"]
end

def payer_email
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure – was filling in the generated template and was asking myself the same question.

Is there a particular API to which I should be writing to or should I just stick to what Shopify needs?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO it makes sense to only have methods that are called, if other users of AM find a need to extract these from the notification, that can be done in a separate PR, but at least then we'll know it's being used somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. We should probably remove these from the generated test suite to streamline implementations. I’ll make a note.

params["billing_address"]["email"]
end

def receiver_email
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Same as others)

params["shipping_address"]["email"]
end

def currency
params["purchase_currency"]
end

def gross
amount = Float(gross_cents) / 100
sprintf("%.2f", amount)
end

def gross_cents
params["cart"]["total_price_including_tax"]
end

def status
case params['status']
when 'checkout_complete'
'Complete'
else
params['status']
Copy link
Contributor

Choose a reason for hiding this comment

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

We generally expect this to return one of ['Completed', 'Pending', 'Failed']. I suppose other values from params['status'] will just cause the notification to be ignored, so perhaps that's reasonable then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I felt odd about writing this too. I figured that since I don’t actually know what statuses map to Pending or Failed, I shouldn’t guess here.

end
end

def acknowledge(authcode = nil)
Verifier.new(@options[:authorization_header], @raw, @shared_secret).verify
end

private

def parse(post)
@raw = post.to_s
@params = JSON.parse(post)
end

class Verifier
attr_reader :header, :payload, :digest, :shared_secret
def initialize(header, payload, shared_secret)
@header, @payload, @shared_secret = header, payload, shared_secret

@digest = extract_digest
end

def verify
digest_matches?
end

private

def extract_digest
match = header.match(/^Klarna (?<digest>.+)$/)
match && match[:digest]
end

def digest_matches?
Klarna.digest(payload, shared_secret) == digest
end
end
end
end
end
end
end
8 changes: 4 additions & 4 deletions test/test_helper.rb
Expand Up @@ -10,6 +10,8 @@
end

require 'test/unit'
require 'minitest/autorun'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like I should probably remove this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this staying, and if so, what are the implications for the other bits in AM?


require 'money'
require 'mocha/version'
if(Mocha::VERSION.split(".")[1].to_i < 12)
Expand Down Expand Up @@ -61,7 +63,6 @@ class SimpleTestGateway < ActiveMerchant::Billing::Gateway
class SubclassGateway < SimpleTestGateway
end


module ActiveMerchant
module Assertions
AssertionClass = RUBY_VERSION > '1.9' ? MiniTest::Assertion : Test::Unit::AssertionFailedError
Expand All @@ -72,7 +73,7 @@ def assert_field(field, value)
end
end

# Allows the testing of you to check for negative assertions:
# Allows testing of negative assertions:
#
# # Instead of
# assert !something_that_is_false
Expand All @@ -91,7 +92,7 @@ def assert_false(boolean, message = nil)
end
end

# A handy little assertion to check for a successful response:
# An assertion of a successful response:
#
# # Instead of
# assert response.success?
Expand Down Expand Up @@ -231,7 +232,6 @@ def symbolize_keys(hash)
end

module ActionViewHelperTestHelper

def self.included(base)
base.send(:include, ActiveMerchant::Billing::Integrations::ActionViewHelper)
base.send(:include, ActionView::Helpers::FormHelper)
Expand Down