From f7476eb8268576f25229e75f58001c9d2cf994b2 Mon Sep 17 00:00:00 2001 From: Tom Wey Date: Mon, 30 Dec 2024 17:12:49 +0000 Subject: [PATCH 1/7] Add some test coverage for currency range validation --- .../src/test/scala/model/CurrencyTest.scala | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 support-payment-api/src/test/scala/model/CurrencyTest.scala diff --git a/support-payment-api/src/test/scala/model/CurrencyTest.scala b/support-payment-api/src/test/scala/model/CurrencyTest.scala new file mode 100644 index 0000000000..6d0b285fa9 --- /dev/null +++ b/support-payment-api/src/test/scala/model/CurrencyTest.scala @@ -0,0 +1,22 @@ +package model + +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.must.Matchers + +class CurrencyTest extends AnyFlatSpec with Matchers { + "exceedsMaxAmount" should "return true if the amount is greater than the allowed range" in { + Currency.exceedsMaxAmount(2100, Currency.GBP) mustBe true + } + + it should "return false if the amount is below the allowed range" in { + Currency.exceedsMaxAmount(0.5, Currency.GBP) mustBe true + } + + it should "return false if the amount is within the allowed range" in { + Currency.exceedsMaxAmount(100, Currency.GBP) mustBe false + } + + it should "allow the max to be 4% larger than the value specified to allow for covering transaction cost" in { + Currency.exceedsMaxAmount(2080, Currency.GBP) mustBe false + } +} From cfc83252e407e4da41c18de53799af3e71af89c8 Mon Sep 17 00:00:00 2001 From: Tom Wey Date: Mon, 30 Dec 2024 17:14:13 +0000 Subject: [PATCH 2/7] Validate min as well as max --- .../src/main/scala/model/Currency.scala | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/support-payment-api/src/main/scala/model/Currency.scala b/support-payment-api/src/main/scala/model/Currency.scala index 61b214d58f..f3f4b94582 100644 --- a/support-payment-api/src/main/scala/model/Currency.scala +++ b/support-payment-api/src/main/scala/model/Currency.scala @@ -4,6 +4,10 @@ import enumeratum.{CirceEnum, Enum, EnumEntry} import scala.collection.immutable.IndexedSeq +case class Range(min: BigDecimal, max: BigDecimal) { + def contains(amount: BigDecimal): Boolean = (amount >= min) && (amount <= max) +} + // Models currencies supported by the API sealed trait Currency extends EnumEntry @@ -24,12 +28,15 @@ object Currency extends Enum[Currency] with CirceEnum[Currency] { case object NZD extends Currency def exceedsMaxAmount(amount: BigDecimal, currency: Currency): Boolean = { - val maxAmount = currency match { - case AUD => 16000 - case USD => 10000 - case _ => 2000 - } // Users can opt in to add 4% to cover the transaction cost - amount > maxAmount * 1.04 + val transactionCostAsPercentage = 1.04 + + val currencyRange = currency match { + case AUD => Range(min = 1, max = 16000 * transactionCostAsPercentage) + case USD => Range(min = 1, max = 10000 * transactionCostAsPercentage) + case _ => Range(min = 1, max = 2000 * transactionCostAsPercentage) + } + + !currencyRange.contains(amount) } } From a38bfc7810f09d19f731e9daf0099d82e91b0d1a Mon Sep 17 00:00:00 2001 From: Tom Wey Date: Mon, 30 Dec 2024 17:15:37 +0000 Subject: [PATCH 3/7] Better name --- support-payment-api/src/main/scala/model/Currency.scala | 2 +- .../src/main/scala/services/PaypalService.scala | 2 +- .../src/main/scala/services/StripeService.scala | 4 ++-- .../src/test/scala/model/CurrencyTest.scala | 8 ++++---- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/support-payment-api/src/main/scala/model/Currency.scala b/support-payment-api/src/main/scala/model/Currency.scala index f3f4b94582..23cde2b367 100644 --- a/support-payment-api/src/main/scala/model/Currency.scala +++ b/support-payment-api/src/main/scala/model/Currency.scala @@ -27,7 +27,7 @@ object Currency extends Enum[Currency] with CirceEnum[Currency] { case object NZD extends Currency - def exceedsMaxAmount(amount: BigDecimal, currency: Currency): Boolean = { + def isAmountOutOfBounds(amount: BigDecimal, currency: Currency): Boolean = { // Users can opt in to add 4% to cover the transaction cost val transactionCostAsPercentage = 1.04 diff --git a/support-payment-api/src/main/scala/services/PaypalService.scala b/support-payment-api/src/main/scala/services/PaypalService.scala index d2f705ea89..8ab4779f48 100644 --- a/support-payment-api/src/main/scala/services/PaypalService.scala +++ b/support-payment-api/src/main/scala/services/PaypalService.scala @@ -25,7 +25,7 @@ trait Paypal { class PaypalService(config: PaypalConfig)(implicit pool: PaypalThreadPool) extends Paypal with StrictLogging { def createPayment(createPaypalPaymentData: CreatePaypalPaymentData): PaypalResult[Payment] = { - if (model.Currency.exceedsMaxAmount(createPaypalPaymentData.amount, createPaypalPaymentData.currency)) { + if (model.Currency.isAmountOutOfBounds(createPaypalPaymentData.amount, createPaypalPaymentData.currency)) { Left(PaypalApiError.fromString("Amount exceeds the maximum allowed ")).toEitherT[Future] } else { Either diff --git a/support-payment-api/src/main/scala/services/StripeService.scala b/support-payment-api/src/main/scala/services/StripeService.scala index 754dd831b6..fad0df2570 100644 --- a/support-payment-api/src/main/scala/services/StripeService.scala +++ b/support-payment-api/src/main/scala/services/StripeService.scala @@ -45,7 +45,7 @@ class SingleAccountStripeService(config: StripeAccountConfig)(implicit pool: Str ).asJava def createCharge(data: LegacyStripeChargeRequest): EitherT[Future, StripeApiError, Charge] = { - if (model.Currency.exceedsMaxAmount(data.paymentData.amount, data.paymentData.currency)) { + if (model.Currency.isAmountOutOfBounds(data.paymentData.amount, data.paymentData.currency)) { Left(StripeApiError.fromString("Amount exceeds the maximum allowed ", Some(config.publicKey))).toEitherT[Future] } else { Future(Charge.create(getChargeParams(data), requestOptions)).attemptT @@ -79,7 +79,7 @@ class SingleAccountStripeService(config: StripeAccountConfig)(implicit pool: Str def createPaymentIntent( data: StripePaymentIntentRequest.CreatePaymentIntent, ): EitherT[Future, StripeApiError, PaymentIntent] = { - if (model.Currency.exceedsMaxAmount(data.paymentData.amount, data.paymentData.currency)) { + if (model.Currency.isAmountOutOfBounds(data.paymentData.amount, data.paymentData.currency)) { Left(StripeApiError.fromString("Amount exceeds the maximum allowed ", Some(config.publicKey))).toEitherT[Future] } else { diff --git a/support-payment-api/src/test/scala/model/CurrencyTest.scala b/support-payment-api/src/test/scala/model/CurrencyTest.scala index 6d0b285fa9..43ffa6bb51 100644 --- a/support-payment-api/src/test/scala/model/CurrencyTest.scala +++ b/support-payment-api/src/test/scala/model/CurrencyTest.scala @@ -5,18 +5,18 @@ import org.scalatest.matchers.must.Matchers class CurrencyTest extends AnyFlatSpec with Matchers { "exceedsMaxAmount" should "return true if the amount is greater than the allowed range" in { - Currency.exceedsMaxAmount(2100, Currency.GBP) mustBe true + Currency.isAmountOutOfBounds(2100, Currency.GBP) mustBe true } it should "return false if the amount is below the allowed range" in { - Currency.exceedsMaxAmount(0.5, Currency.GBP) mustBe true + Currency.isAmountOutOfBounds(0.5, Currency.GBP) mustBe true } it should "return false if the amount is within the allowed range" in { - Currency.exceedsMaxAmount(100, Currency.GBP) mustBe false + Currency.isAmountOutOfBounds(100, Currency.GBP) mustBe false } it should "allow the max to be 4% larger than the value specified to allow for covering transaction cost" in { - Currency.exceedsMaxAmount(2080, Currency.GBP) mustBe false + Currency.isAmountOutOfBounds(2080, Currency.GBP) mustBe false } } From c14059aaf8826d474274961feacc036e98f8581e Mon Sep 17 00:00:00 2001 From: Tom Wey Date: Mon, 30 Dec 2024 17:21:18 +0000 Subject: [PATCH 4/7] Tweak error message to be more accurate --- .../src/main/scala/services/PaypalService.scala | 2 +- .../src/main/scala/services/StripeService.scala | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/support-payment-api/src/main/scala/services/PaypalService.scala b/support-payment-api/src/main/scala/services/PaypalService.scala index 8ab4779f48..8de5d88c39 100644 --- a/support-payment-api/src/main/scala/services/PaypalService.scala +++ b/support-payment-api/src/main/scala/services/PaypalService.scala @@ -26,7 +26,7 @@ class PaypalService(config: PaypalConfig)(implicit pool: PaypalThreadPool) exten def createPayment(createPaypalPaymentData: CreatePaypalPaymentData): PaypalResult[Payment] = { if (model.Currency.isAmountOutOfBounds(createPaypalPaymentData.amount, createPaypalPaymentData.currency)) { - Left(PaypalApiError.fromString("Amount exceeds the maximum allowed ")).toEitherT[Future] + Left(PaypalApiError.fromString("Amount is outside the allowed range ")).toEitherT[Future] } else { Either .catchNonFatal { diff --git a/support-payment-api/src/main/scala/services/StripeService.scala b/support-payment-api/src/main/scala/services/StripeService.scala index fad0df2570..375e143a45 100644 --- a/support-payment-api/src/main/scala/services/StripeService.scala +++ b/support-payment-api/src/main/scala/services/StripeService.scala @@ -46,7 +46,7 @@ class SingleAccountStripeService(config: StripeAccountConfig)(implicit pool: Str def createCharge(data: LegacyStripeChargeRequest): EitherT[Future, StripeApiError, Charge] = { if (model.Currency.isAmountOutOfBounds(data.paymentData.amount, data.paymentData.currency)) { - Left(StripeApiError.fromString("Amount exceeds the maximum allowed ", Some(config.publicKey))).toEitherT[Future] + Left(StripeApiError.fromString("Amount is outside the allowed range ", Some(config.publicKey))).toEitherT[Future] } else { Future(Charge.create(getChargeParams(data), requestOptions)).attemptT .bimap( @@ -80,7 +80,7 @@ class SingleAccountStripeService(config: StripeAccountConfig)(implicit pool: Str data: StripePaymentIntentRequest.CreatePaymentIntent, ): EitherT[Future, StripeApiError, PaymentIntent] = { if (model.Currency.isAmountOutOfBounds(data.paymentData.amount, data.paymentData.currency)) { - Left(StripeApiError.fromString("Amount exceeds the maximum allowed ", Some(config.publicKey))).toEitherT[Future] + Left(StripeApiError.fromString("Amount is outside the allowed range ", Some(config.publicKey))).toEitherT[Future] } else { Future { From db631e26a11856eff89c7c1b4ad53997e6be00d4 Mon Sep 17 00:00:00 2001 From: Tom Wey Date: Mon, 30 Dec 2024 17:27:50 +0000 Subject: [PATCH 5/7] Fix tests --- .../src/test/scala/services/PaypalServiceSpec.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/support-payment-api/src/test/scala/services/PaypalServiceSpec.scala b/support-payment-api/src/test/scala/services/PaypalServiceSpec.scala index 2363242394..912386a533 100644 --- a/support-payment-api/src/test/scala/services/PaypalServiceSpec.scala +++ b/support-payment-api/src/test/scala/services/PaypalServiceSpec.scala @@ -114,14 +114,14 @@ class PaypalServiceSpec extends AnyFlatSpec with Matchers with MockitoSugar with it should "return an error if the payment amount exceeds Australia max" in new PaypalServiceTestFixture { val createPaypalPaymentData = CreatePaypalPaymentData(Currency.AUD, 16640.50, "url", "url") whenReady(paypalService.createPayment(createPaypalPaymentData).value) { result => - result mustBe (Left(PaypalApiError.fromString("Amount exceeds the maximum allowed "))) + result mustBe (Left(PaypalApiError.fromString("Amount is outside the allowed range "))) } } it should "return an error if the payment amount exceeds non-Australia max" in new PaypalServiceTestFixture { val createPaypalPaymentData = CreatePaypalPaymentData(Currency.GBP, 2080.50, "url", "url") whenReady(paypalService.createPayment(createPaypalPaymentData).value) { result => - result mustBe (Left(PaypalApiError.fromString("Amount exceeds the maximum allowed "))) + result mustBe (Left(PaypalApiError.fromString("Amount is outside the allowed range "))) } } From cddaf6b44e4ba624979eb13bb82a161cfd8b4a55 Mon Sep 17 00:00:00 2001 From: Tom Wey Date: Mon, 30 Dec 2024 17:30:41 +0000 Subject: [PATCH 6/7] Better test description --- support-payment-api/src/test/scala/model/CurrencyTest.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/support-payment-api/src/test/scala/model/CurrencyTest.scala b/support-payment-api/src/test/scala/model/CurrencyTest.scala index 43ffa6bb51..aa3ed13075 100644 --- a/support-payment-api/src/test/scala/model/CurrencyTest.scala +++ b/support-payment-api/src/test/scala/model/CurrencyTest.scala @@ -16,7 +16,7 @@ class CurrencyTest extends AnyFlatSpec with Matchers { Currency.isAmountOutOfBounds(100, Currency.GBP) mustBe false } - it should "allow the max to be 4% larger than the value specified to allow for covering transaction cost" in { + it should "allow the amount to be 4% larger than the nominal max to allow for covering transaction cost" in { Currency.isAmountOutOfBounds(2080, Currency.GBP) mustBe false } } From 31b55265b9f742835b243f173d2dc9fe000092d4 Mon Sep 17 00:00:00 2001 From: Tom Wey Date: Fri, 10 Jan 2025 09:25:20 +0000 Subject: [PATCH 7/7] Fix test description --- support-payment-api/src/test/scala/model/CurrencyTest.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/support-payment-api/src/test/scala/model/CurrencyTest.scala b/support-payment-api/src/test/scala/model/CurrencyTest.scala index aa3ed13075..8e1303a32d 100644 --- a/support-payment-api/src/test/scala/model/CurrencyTest.scala +++ b/support-payment-api/src/test/scala/model/CurrencyTest.scala @@ -8,7 +8,7 @@ class CurrencyTest extends AnyFlatSpec with Matchers { Currency.isAmountOutOfBounds(2100, Currency.GBP) mustBe true } - it should "return false if the amount is below the allowed range" in { + it should "return true if the amount is below the allowed range" in { Currency.isAmountOutOfBounds(0.5, Currency.GBP) mustBe true }