From 9b66da7b29d61d2d7c2b5da9ae00748d0f9e763c Mon Sep 17 00:00:00 2001 From: Alessio Gaeta Date: Thu, 20 Jun 2019 11:03:27 +0200 Subject: [PATCH 1/2] Stop tampering with user data A setter should not change a field value just to avoid validation issues: if data are invalid, they should be explicitely fixed in the data source. --- .../billing/plugin/adyen/client/model/PaymentInfo.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/main/java/org/killbill/billing/plugin/adyen/client/model/PaymentInfo.java b/src/main/java/org/killbill/billing/plugin/adyen/client/model/PaymentInfo.java index 4eb352e4..92a0d234 100644 --- a/src/main/java/org/killbill/billing/plugin/adyen/client/model/PaymentInfo.java +++ b/src/main/java/org/killbill/billing/plugin/adyen/client/model/PaymentInfo.java @@ -218,8 +218,7 @@ public String getHouseNumberOrName() { } public void setHouseNumberOrName(final String houseNumberOrName) { - // Adyen needs houseNumberOrName to not be blank in US https://docs.adyen.com/developers/api-reference/common-api/address - this.houseNumberOrName = houseNumberOrName != null ? houseNumberOrName : ""; + this.houseNumberOrName = houseNumberOrName; } public String getCity() { From 8fcdc1c6bce888ed5796979124e60188ea523009 Mon Sep 17 00:00:00 2001 From: Alessio Gaeta Date: Thu, 20 Jun 2019 11:06:27 +0200 Subject: [PATCH 2/2] Fix billing address validation As per Adyen documentation, the billing address per se is optional [1], but when sending it, the country is always mandatory, while the remaining fields must either be sent all or none [2] (although in our experience the ZIP always has been treated as optional by Adyen: maybe it is mandatory only for US and CA). This commit makes the address validation compliant (altough still only validates the data presence, not the *values* formal correctness). [1] https://docs.adyen.com/api-reference/payments-api/paymentrequest/ [2] https://docs.adyen.com/api-reference/common-api/address/ --- .../converter/PaymentInfoConverter.java | 52 ++++++++--- .../converter/PaymentInfoConverterTest.java | 92 +++++++++++++++++++ 2 files changed, 130 insertions(+), 14 deletions(-) create mode 100644 src/test/java/org/killbill/billing/plugin/adyen/client/payment/converter/PaymentInfoConverterTest.java diff --git a/src/main/java/org/killbill/billing/plugin/adyen/client/payment/converter/PaymentInfoConverter.java b/src/main/java/org/killbill/billing/plugin/adyen/client/payment/converter/PaymentInfoConverter.java index 64c60f85..cb4836f1 100644 --- a/src/main/java/org/killbill/billing/plugin/adyen/client/payment/converter/PaymentInfoConverter.java +++ b/src/main/java/org/killbill/billing/plugin/adyen/client/payment/converter/PaymentInfoConverter.java @@ -74,28 +74,52 @@ private void setShopperInteraction(final PaymentInfo paymentInfo, final PaymentR } private void setBillingAddress(final PaymentInfo paymentInfo, final PaymentRequest paymentRequest) { - final Address address = new Address(); - address.setStreet(paymentInfo.getStreet()); - address.setHouseNumberOrName(paymentInfo.getHouseNumberOrName()); - address.setCity(paymentInfo.getCity()); - address.setPostalCode(paymentInfo.getPostalCode()); - address.setStateOrProvince(paymentInfo.getStateOrProvince()); - + final String street = paymentInfo.getStreet(); + final String houseNumberOrName = paymentInfo.getHouseNumberOrName(); + final String city = paymentInfo.getCity(); + final String postalCode = paymentInfo.getPostalCode(); + final String stateOrProvince = paymentInfo.getStateOrProvince(); + final String country = paymentInfo.getCountry(); final String adjustedCountry; - if ("UK".equalsIgnoreCase(paymentInfo.getCountry())) { + if ("UK".equalsIgnoreCase(country)) { // Passing UK will result in: validation 134 Billing address problem (Country UK invalid) adjustedCountry = "GB"; - } else if ("QC".equalsIgnoreCase(paymentInfo.getCountry())) { + } else if ("QC".equalsIgnoreCase(country)) { // Passing QC will result in: validation 134 Billing address problem (Country QC invalid) adjustedCountry = "CA"; } else { - adjustedCountry = paymentInfo.getCountry(); + adjustedCountry = country; } - address.setCountry(adjustedCountry); - // Required by Adyen: house number and city - // house number can be passed in the street or in the houseNumberOrName field - if ((address.getStreet() != null || address.getHouseNumberOrName() != null) && address.getCity() != null) { + // Adyen validation docs: + // - https://docs.adyen.com/api-reference/payments-api/paymentrequest/ + // - https://docs.adyen.com/developers/api-reference/common-api/address + // TL;DR: the billing address per se is optional, but when sending it, the country is always mandatory, + // while the remaining fields must either be sent all or none (although in our experience the ZIP always has + // been treated as optional by Adyen: maybe it is mandatory only for US and CA) + // TODO? Introduce some data validation + final boolean stateProvinceValid = !("US".equals(adjustedCountry) || "CA".equals(adjustedCountry)) + || stateOrProvince != null; + + final boolean addressComplete = street != null && houseNumberOrName != null && city != null + && postalCode != null && stateProvinceValid; + + final boolean addressEmpty = street == null && houseNumberOrName == null && city == null && postalCode == null + && stateOrProvince == null; + + final boolean addressValid = adjustedCountry != null && (addressComplete || addressEmpty); + + // TODO: validate the right format for fields + + if (addressValid) { + final Address address = new Address(); + address.setStreet(street); + address.setHouseNumberOrName(houseNumberOrName); + address.setCity(city); + address.setPostalCode(postalCode); + address.setStateOrProvince(stateOrProvince); + address.setCountry(adjustedCountry); + paymentRequest.setBillingAddress(address); } } diff --git a/src/test/java/org/killbill/billing/plugin/adyen/client/payment/converter/PaymentInfoConverterTest.java b/src/test/java/org/killbill/billing/plugin/adyen/client/payment/converter/PaymentInfoConverterTest.java new file mode 100644 index 00000000..0157c994 --- /dev/null +++ b/src/test/java/org/killbill/billing/plugin/adyen/client/payment/converter/PaymentInfoConverterTest.java @@ -0,0 +1,92 @@ +/* + * Copyright 2014-2019 Groupon, Inc + * Copyright 2014-2019 The Billing Project, LLC + * + * The Billing Project licenses this file to you under the Apache License, version 2.0 + * (the "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +package org.killbill.billing.plugin.adyen.client.payment.converter; + +import java.lang.reflect.Field; +import java.util.ArrayList; +import java.util.Collection; +import java.util.Iterator; +import java.util.Map; + +import org.killbill.adyen.payment.PaymentRequest; +import org.killbill.billing.plugin.adyen.client.model.PaymentInfo; +import org.killbill.billing.plugin.adyen.client.model.paymentinfo.Card; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Parameters; +import org.testng.annotations.Test; + +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Maps; + +import static org.testng.Assert.*; + +public class PaymentInfoConverterTest { + + @DataProvider + public static Object[][] paymentInfoProvider() { + // Testing only address values, by now + // Map initialization in Java is even more verbose... + return new String[][]{ + {"street:address1", "houseNumberOrName:address2", "city:city", "postalCode:00000", "stateOrProvince:SOP", "country:CO", "Y"}, // All fields present + {"country:CO", "Y"}, // Only Country + + {"street:address1", "houseNumberOrName:address2", "city:city", "postalCode:00000", "stateOrProvince:SOP", "N"}, // Missing Country + + {"houseNumberOrName:address2", "city:city", "postalCode:00000", "stateOrProvince:SOP", "country:CO", "N"}, // Missing address field + {"street:address1", "city:city", "postalCode:00000", "stateOrProvince:SOP", "country:CO", "N"}, + {"street:address1", "houseNumberOrName:address2", "postalCode:00000", "stateOrProvince:SOP", "country:CO", "N"}, + {"street:address1", "houseNumberOrName:address2", "city:city", "stateOrProvince:SOP", "country:CO", "N"}, + + {"street:address1", "houseNumberOrName:address2", "city:city", "postalCode:00000", "country:CO", "Y"}, // No State for Country other than US/CA + {"street:address1", "houseNumberOrName:address2", "city:city", "postalCode:00000", "country:US", "N"}, // Missing State for US/CA + {"street:address1", "houseNumberOrName:address2", "city:city", "postalCode:00000", "country:CA", "N"}, + + // TODO? New test cases if/when data validation is introduced + }; + } + + @Test(dataProvider = "paymentInfoProvider") + public void testConvertPaymentInfoToPaymentRequest(final String[] paramsValue) + throws NoSuchFieldException, IllegalAccessException { + + final Card pi = buildPaymentInfo(paramsValue); + final boolean expectedAddrPresent = "Y".equals(paramsValue[paramsValue.length - 1]); + + final PaymentRequest pr = new PaymentInfoConverter().convertPaymentInfoToPaymentRequest(pi); + final boolean addrPresent = pr.getBillingAddress() != null; + + assertEquals(addrPresent, expectedAddrPresent); + + } + + private static Card buildPaymentInfo(final String[] values) + throws IllegalAccessException, NoSuchFieldException { + + final Card pi = new Card(); + + for (int i = 0; i < values.length - 1; i++) { + final String[] pair = values[i].split(":", 2); + + final Field field = PaymentInfo.class.getDeclaredField(pair[0]); + field.setAccessible(true); + field.set(pi, "null".equals(pair[1]) ? null : pair[1]); + } + + return pi; + } +}