Merge pull request #928 from guardian/ph-20230916-2156-price-corrections

Enforced estimated price
guardian · Sep 25, 2023 · b89c764 · b89c764
2 parents c37434f + e03191c
commit b89c764
Show file tree

Hide file tree

Showing 9 changed files with 48 additions and 92 deletions.
diff --git a/lambda/src/main/scala/pricemigrationengine/handlers/AmendmentHandler.scala b/lambda/src/main/scala/pricemigrationengine/handlers/AmendmentHandler.scala
@@ -79,18 +79,15 @@ object AmendmentHandler extends CohortHandler {
     }
   }
 
-  private def checkFinalPriceVersusEstimatedNewPrice(
-      cohortSpec: CohortSpec,
-      item: CohortItem,
-      estimatedNewPrice: BigDecimal,
-      newPrice: BigDecimal
-  ): ZIO[Zuora, Failure, Unit] = {
+  private def shouldPerformFinalPriceCheck(cohortSpec: CohortSpec): Boolean = {
     // Date: 8 Sept 2023
     // This function is introduced as part of a multi stage update of the
     // engine to detect and deal with situations where the final price is higher than the
     // estimated new price, which has happened with Quarterly Guardian Weekly subscriptions
     // The ultimate aim is to update the engine to deal with those situations automatically,
     // but in this step we simply error and will alarm at the next occurrence of this situation.
+    // (see the code in `doAmendment`)
+
     // When that situation occurs, a good course of action will be to
     //    1. Revert the amendment in Zuora
     //    2. Reset the cohort item in the dynamo table
@@ -102,18 +99,8 @@ object AmendmentHandler extends CohortHandler {
     // estimated price (which wasn't including the extra contribution).
 
     MigrationType(cohortSpec) match {
-      case SupporterPlus2023V1V2MA => ZIO.succeed(())
-      case _ => {
-        if (newPrice > estimatedNewPrice) {
-          ZIO.fail(
-            AmendmentDataFailure(
-              s"[e9054daa] Item $item has gone through the amendment step but has failed the final price check. Estimated price was ${estimatedNewPrice}, but the final price was ${newPrice}"
-            )
-          )
-        } else {
-          ZIO.succeed(())
-        }
-      }
+      case SupporterPlus2023V1V2MA => false
+      case _                       => true
     }
   }
 
@@ -221,7 +208,7 @@ object AmendmentHandler extends CohortHandler {
                 subscriptionBeforeUpdate,
                 invoicePreviewBeforeUpdate,
                 startDate,
-                PriceCap.priceCorrectionFactorForPriceCap(oldPrice, estimatedNewPrice)
+                Some(PriceCap(oldPrice, estimatedNewPrice))
               )
           )
       }
@@ -242,7 +229,16 @@ object AmendmentHandler extends CohortHandler {
           )
         )
 
-      _ <- checkFinalPriceVersusEstimatedNewPrice(cohortSpec, item, estimatedNewPrice, newPrice)
+      _ <-
+        if (shouldPerformFinalPriceCheck(cohortSpec: CohortSpec) && (newPrice > estimatedNewPrice)) {
+          ZIO.fail(
+            AmendmentDataFailure(
+              s"[e9054daa] Item ${item} has gone through the amendment step but has failed the final price check. Estimated price was ${estimatedNewPrice}, but the final price was ${newPrice}"
+            )
+          )
+        } else {
+          ZIO.succeed(())
+        }
 
       whenDone <- Clock.instant
     } yield SuccessfulAmendmentResult(

diff --git a/lambda/src/main/scala/pricemigrationengine/handlers/NotificationHandler.scala b/lambda/src/main/scala/pricemigrationengine/handlers/NotificationHandler.scala
@@ -176,7 +176,7 @@ object NotificationHandler extends CohortHandler {
         case SupporterPlus2023V1V2MA => true
         case Legacy                  => false
       }
-      cappedEstimatedNewPriceWithCurrencySymbol = s"${currencySymbol}${PriceCap.cappedPrice(oldPrice, estimatedNewPrice, forceEstimated)}"
+      cappedEstimatedNewPriceWithCurrencySymbol = s"${currencySymbol}${PriceCap(oldPrice, estimatedNewPrice, forceEstimated)}"
 
       _ <- logMissingEmailAddress(cohortItem, contact)
 

diff --git a/lambda/src/main/scala/pricemigrationengine/handlers/SalesforcePriceRiseCreationHandler.scala b/lambda/src/main/scala/pricemigrationengine/handlers/SalesforcePriceRiseCreationHandler.scala
@@ -91,7 +91,7 @@ object SalesforcePriceRiseCreationHandler extends CohortHandler {
         Some(subscription.Name),
         Some(subscription.Buyer__c),
         Some(oldPrice),
-        Some(PriceCap.cappedPrice(oldPrice, estimatedNewPrice, forceEstimated)),
+        Some(PriceCap(oldPrice, estimatedNewPrice, forceEstimated)),
         Some(priceRiseDate),
         Some(subscription.Id)
       )

diff --git a/lambda/src/main/scala/pricemigrationengine/model/PriceCap.scala b/lambda/src/main/scala/pricemigrationengine/model/PriceCap.scala
@@ -14,25 +14,11 @@ object PriceCap {
    */
 
   private val priceCappingMultiplier = 1.2 // old price + 20%
-  def cappedPrice(oldPrice: BigDecimal, estimatedNewPrice: BigDecimal, forceEstimated: Boolean = false): BigDecimal = {
+  def apply(oldPrice: BigDecimal, estimatedNewPrice: BigDecimal, forceEstimated: Boolean = false): BigDecimal = {
     if (forceEstimated) {
       estimatedNewPrice
     } else {
       List(estimatedNewPrice, oldPrice * priceCappingMultiplier).min
     }
   }
-
-  def priceCorrectionFactorForPriceCap(
-      oldPrice: BigDecimal,
-      estimatedNewPrice: BigDecimal,
-      forceEstimated: Boolean = false
-  ): BigDecimal = {
-    if (
-      forceEstimated || estimatedNewPrice == 0 || estimatedNewPrice.compareTo(oldPrice * priceCappingMultiplier) <= 0
-    ) {
-      1
-    } else {
-      (oldPrice * priceCappingMultiplier) / estimatedNewPrice
-    }
-  }
 }
diff --git a/lambda/src/main/scala/pricemigrationengine/model/ZuoraSubscriptionUpdate.scala b/lambda/src/main/scala/pricemigrationengine/model/ZuoraSubscriptionUpdate.scala
@@ -35,7 +35,7 @@ object ZuoraSubscriptionUpdate {
       subscription: ZuoraSubscription,
       invoiceList: ZuoraInvoiceList,
       effectiveDate: LocalDate,
-      priceCorrectionFactor: BigDecimal
+      enforcedPrice: Option[BigDecimal]
   ): Either[AmendmentDataFailure, ZuoraSubscriptionUpdate] = {
 
     val activeRatePlans = (for {
@@ -57,8 +57,8 @@ object ZuoraSubscriptionUpdate {
       activeRatePlans
         .map(
           if (isZoneABC.nonEmpty)
-            AddZuoraRatePlan.fromRatePlanGuardianWeekly(account, catalogue, effectiveDate, priceCorrectionFactor)
-          else AddZuoraRatePlan.fromRatePlan(pricingData, effectiveDate, priceCorrectionFactor)
+            AddZuoraRatePlan.fromRatePlanGuardianWeekly(account, catalogue, effectiveDate, enforcedPrice)
+          else AddZuoraRatePlan.fromRatePlan(pricingData, effectiveDate, enforcedPrice)
         )
         .sequence
         .map { add =>
@@ -89,12 +89,12 @@ object AddZuoraRatePlan {
   def fromRatePlan(
       pricingData: ZuoraPricingData,
       contractEffectiveDate: LocalDate,
-      priceCorrectionFactor: BigDecimal
+      enforcedPrice: Option[BigDecimal]
   )(
       ratePlan: ZuoraRatePlan
   ): Either[AmendmentDataFailure, AddZuoraRatePlan] = {
     for {
-      chargeOverrides <- ChargeOverride.fromRatePlan(pricingData, ratePlan, priceCorrectionFactor)
+      chargeOverrides <- ChargeOverride.fromRatePlan(pricingData, ratePlan, enforcedPrice)
     } yield AddZuoraRatePlan(
       productRatePlanId = ratePlan.productRatePlanId,
       contractEffectiveDate,
@@ -106,7 +106,7 @@ object AddZuoraRatePlan {
       account: ZuoraAccount,
       catalogue: ZuoraProductCatalogue,
       contractEffectiveDate: LocalDate,
-      priceCorrectionFactor: BigDecimal
+      enforcedPrice: Option[BigDecimal]
   )(
       ratePlan: ZuoraRatePlan
   ): Either[AmendmentDataFailure, AddZuoraRatePlan] =
@@ -121,7 +121,7 @@ object AddZuoraRatePlan {
           fromRatePlanCharge(
             chargePair.chargeFromProduct,
             chargePair.chargeFromSubscription,
-            priceCorrectionFactor
+            enforcedPrice
           )
         )
         .sequence
@@ -154,18 +154,18 @@ object ChargeOverride {
   def fromRatePlan(
       pricingData: ZuoraPricingData,
       ratePlan: ZuoraRatePlan,
-      priceCorrectionFactor: BigDecimal
+      enforcedPrice: Option[BigDecimal]
   ): Either[AmendmentDataFailure, Seq[ChargeOverride]] =
     (for {
       ratePlanCharge <- ratePlan.ratePlanCharges
       productRatePlanCharge <- pricingData.get(ratePlanCharge.productRatePlanChargeId).toSeq
-    } yield fromRatePlanCharge(productRatePlanCharge, ratePlanCharge, priceCorrectionFactor)).sequence
+    } yield fromRatePlanCharge(productRatePlanCharge, ratePlanCharge, enforcedPrice)).sequence
       .map(_.flatten)
 
   def fromRatePlanCharge(
       productRatePlanCharge: ZuoraProductRatePlanCharge,
       ratePlanCharge: ZuoraRatePlanCharge,
-      priceCorrectionFactor: BigDecimal
+      enforcedPrice: Option[BigDecimal]
   ): Either[AmendmentDataFailure, Option[ChargeOverride]] =
     for {
       billingPeriod <- ratePlanCharge.billingPeriod.toRight(
@@ -191,6 +191,6 @@ object ChargeOverride {
       )
 
     } yield {
-      Some(ChargeOverride(productRatePlanCharge.id, billingPeriod, price * priceCorrectionFactor))
+      Some(ChargeOverride(productRatePlanCharge.id, billingPeriod, enforcedPrice.getOrElse(price)))
     }
 }
diff --git a/lambda/src/test/scala/pricemigrationengine/handlers/NotificationHandlerTest.scala b/lambda/src/test/scala/pricemigrationengine/handlers/NotificationHandlerTest.scala
@@ -30,7 +30,7 @@ class NotificationHandlerTest extends munit.FunSuite {
   // The estimated new price is the price without cap
   private val estimatedNewPrice = BigDecimal(15.00)
   test("For membership test, we need the estimatedNewPrice to be higher than the capped price") {
-    assert(PriceCap.cappedPrice(oldPrice, estimatedNewPrice) < estimatedNewPrice)
+    assert(PriceCap(oldPrice, estimatedNewPrice) < estimatedNewPrice)
   }
 
   // The price that is displayed to the customer is capped using the old price as base

diff --git a/...src/test/scala/pricemigrationengine/handlers/SalesforcePriceRiseCreationHandlerTest.scala b/...src/test/scala/pricemigrationengine/handlers/SalesforcePriceRiseCreationHandlerTest.scala
@@ -24,7 +24,7 @@ class SalesforcePriceRiseCreationHandlerTest extends munit.FunSuite {
   // To make the membership price rise test meaningful, this should actually be higher than the capped price.
   private val estimatedNewPrice = BigDecimal(15.00)
   test("For membership test, we need the estimatedNewPrice to be higher than the capped price") {
-    assert(PriceCap.cappedPrice(oldPrice, estimatedNewPrice) < estimatedNewPrice)
+    assert(PriceCap(oldPrice, estimatedNewPrice) < estimatedNewPrice)
   }
 
   private val currentTime = Instant.parse("2020-05-21T15:16:37Z")
@@ -141,7 +141,7 @@ class SalesforcePriceRiseCreationHandlerTest extends munit.FunSuite {
     assertEquals(createdPriceRises(0).Current_Price_Today__c, Some(oldPrice))
     assertEquals(
       createdPriceRises(0).Guardian_Weekly_New_Price__c,
-      Some(PriceCap.cappedPrice(oldPrice, estimatedNewPrice))
+      Some(PriceCap(oldPrice, estimatedNewPrice))
     )
     assertEquals(createdPriceRises(0).Price_Rise_Date__c, Some(startDate))
 
@@ -336,7 +336,7 @@ class SalesforcePriceRiseCreationHandlerTest extends munit.FunSuite {
     assertEquals(updatedPriceRises(0).Current_Price_Today__c, Some(oldPrice))
     assertEquals(
       updatedPriceRises(0).Guardian_Weekly_New_Price__c,
-      Some(PriceCap.cappedPrice(oldPrice, estimatedNewPrice))
+      Some(PriceCap(oldPrice, estimatedNewPrice))
     )
     assertEquals(updatedPriceRises(0).Price_Rise_Date__c, Some(startDate))
 

diff --git a/lambda/src/test/scala/pricemigrationengine/model/PriceCapTest.scala b/lambda/src/test/scala/pricemigrationengine/model/PriceCapTest.scala
@@ -6,38 +6,12 @@ class PriceCapTest extends munit.FunSuite {
     val oldPrice = BigDecimal(100)
     val cappedPrice = BigDecimal(120)
     val uncappedPrice = BigDecimal(156)
-    assertEquals(cappedPrice, PriceCap.cappedPrice(oldPrice, uncappedPrice))
+    assertEquals(cappedPrice, PriceCap(oldPrice, uncappedPrice))
   }
 
   test("The price capping works correctly in case of force estimated") {
     val oldPrice = BigDecimal(100)
-    val cappedPrice = BigDecimal(120)
     val uncappedPrice = BigDecimal(156)
-    assertEquals(uncappedPrice, PriceCap.cappedPrice(oldPrice, uncappedPrice, true))
-  }
-
-  test("The price correction factor is computed correctly") {
-    val oldPrice = BigDecimal(100)
-    val estimatedNewPrice = BigDecimal(240)
-    val correctionFactor = BigDecimal(0.5)
-    // The capped price is 120, half of the estimated new price, hence a correction factor of 0.5
-    assertEquals(correctionFactor, PriceCap.priceCorrectionFactorForPriceCap(oldPrice, estimatedNewPrice))
+    assertEquals(uncappedPrice, PriceCap(oldPrice, uncappedPrice, true))
   }
-
-  test("The price correction factor is computed correctly in case of force estimated") {
-    val oldPrice = BigDecimal(100)
-    val estimatedNewPrice = BigDecimal(240)
-    val correctionFactor = BigDecimal(1)
-    // The capped price is 120, half of the estimated new price, hence a correction factor of 0.5
-    assertEquals(correctionFactor, PriceCap.priceCorrectionFactorForPriceCap(oldPrice, estimatedNewPrice, true))
-  }
-
-  test("Correction factor in case of 0 estimated new price") {
-    val oldPrice = BigDecimal(100)
-    val estimatedNewPrice = BigDecimal(1)
-    val correctionFactor = BigDecimal(1)
-    // The correction factor in case of an estimated new price of zero is conventionally set to 1
-    assertEquals(correctionFactor, PriceCap.priceCorrectionFactorForPriceCap(oldPrice, estimatedNewPrice))
-  }
-
 }
diff --git a/lambda/src/test/scala/pricemigrationengine/model/ZuoraSubscriptionUpdateTest.scala b/lambda/src/test/scala/pricemigrationengine/model/ZuoraSubscriptionUpdateTest.scala
@@ -16,7 +16,7 @@ class ZuoraSubscriptionUpdateTest extends munit.FunSuite {
       subscription = Fixtures.subscriptionFromJson(s"$fixtureSet/Subscription.json"),
       invoiceList = Fixtures.invoiceListFromJson(s"$fixtureSet/InvoicePreview.json"),
       date,
-      1
+      None
     )
     assertEquals(
       update,
@@ -48,7 +48,7 @@ class ZuoraSubscriptionUpdateTest extends munit.FunSuite {
     )
   }
 
-  test("Zuora amendment correctly creates a charge override from a non trivial price correction factor") {
+  test("Zuora amendment correctly creates a charge override from an enforced priced") {
     val fixtureSet = "GuardianWeekly/CappedPriceIncrease3"
     val date = LocalDate.of(2022, 12, 19)
     val update = ZuoraSubscriptionUpdate.updateOfRatePlansToCurrent(
@@ -57,7 +57,7 @@ class ZuoraSubscriptionUpdateTest extends munit.FunSuite {
       subscription = Fixtures.subscriptionFromJson(s"$fixtureSet/Subscription.json"),
       invoiceList = Fixtures.invoiceListFromJson(s"$fixtureSet/InvoicePreview.json"),
       date,
-      0.5
+      Some(37.20)
     )
     assertEquals(
       update,
@@ -98,7 +98,7 @@ class ZuoraSubscriptionUpdateTest extends munit.FunSuite {
       subscription = Fixtures.subscriptionFromJson(s"$fixtureSet/Subscription.json"),
       invoiceList = Fixtures.invoiceListFromJson(s"$fixtureSet/InvoicePreview.json"),
       date,
-      1
+      None
     )
     assertEquals(
       update,
@@ -141,7 +141,7 @@ class ZuoraSubscriptionUpdateTest extends munit.FunSuite {
       subscription = Fixtures.subscriptionFromJson(s"$fixtureSet/Subscription.json"),
       invoiceList = Fixtures.invoiceListFromJson(s"$fixtureSet/InvoicePreview.json"),
       date,
-      1
+      None
     )
     assertEquals(
       update,
@@ -179,7 +179,7 @@ class ZuoraSubscriptionUpdateTest extends munit.FunSuite {
       subscription = Fixtures.subscriptionFromJson(s"$fixtureSet/Subscription.json"),
       invoiceList = Fixtures.invoiceListFromJson(s"$fixtureSet/InvoicePreview.json"),
       date,
-      1
+      None
     )
     assertEquals(
       update,
@@ -247,7 +247,7 @@ class ZuoraSubscriptionUpdateTest extends munit.FunSuite {
       subscription = Fixtures.subscriptionFromJson(s"$fixtureSet/Subscription.json"),
       invoiceList = Fixtures.invoiceListFromJson(s"$fixtureSet/InvoicePreview.json"),
       date,
-      1
+      None
     )
     assertEquals(
       update,
@@ -290,7 +290,7 @@ class ZuoraSubscriptionUpdateTest extends munit.FunSuite {
       subscription = Fixtures.subscriptionFromJson(s"$fixtureSet/Subscription.json"),
       invoiceList = Fixtures.invoiceListFromJson(s"$fixtureSet/InvoicePreview.json"),
       date,
-      1
+      None
     )
     assertEquals(
       update,
@@ -333,7 +333,7 @@ class ZuoraSubscriptionUpdateTest extends munit.FunSuite {
       subscription = Fixtures.subscriptionFromJson(s"$fixtureSet/Subscription.json"),
       invoiceList = Fixtures.invoiceListFromJson(s"$fixtureSet/InvoicePreview.json"),
       date,
-      1
+      None
     )
     assertEquals(
       update,
@@ -371,7 +371,7 @@ class ZuoraSubscriptionUpdateTest extends munit.FunSuite {
       subscription = Fixtures.subscriptionFromJson(s"$fixtureSet/Subscription.json"),
       invoiceList = Fixtures.invoiceListFromJson(s"$fixtureSet/InvoicePreview.json"),
       date,
-      1
+      None
     )
     assertEquals(
       update,
@@ -434,7 +434,7 @@ class ZuoraSubscriptionUpdateTest extends munit.FunSuite {
       subscription = Fixtures.subscriptionFromJson(s"$fixtureSet/Subscription.json"),
       invoiceList = Fixtures.invoiceListFromJson(s"$fixtureSet/InvoicePreview.json"),
       date,
-      1
+      None
     )
     assertEquals(
       update,
@@ -477,7 +477,7 @@ class ZuoraSubscriptionUpdateTest extends munit.FunSuite {
       subscription = Fixtures.subscriptionFromJson(s"$fixtureSet/Subscription.json"),
       invoiceList = Fixtures.invoiceListFromJson(s"$fixtureSet/InvoicePreview.json"),
       date,
-      1
+      None
     )
     assertEquals(
       update,