Merge pull request #1056 from guardian/ph-20240710-1228-s+2024

SupporterPlus 2024

README.md

@@ -8,7 +8,7 @@ The price migration engine is an orchestration engine used to perform controlled
 
 - [An introduction to the general principles of price migrations](docs/price-migrations-from-first-principles.md)
 - [The journey of a cohort item](docs/the-journey-of-a-cohort-item.md)
-- [Coding conventions](docs/coding-conventions.md)
+- [Coding directives](docs/coding-directives.md)
 - [Notification windows](docs/notification-windows.md)
 - [The art of computing start dates](docs/start-date-computation.md)
 - [The art of the cap; or how to gracefully cap prices in the engine](docs/the-art-of-the-cap.md)

docs/coding-directives.md

@@ -0,0 +1,8 @@
+# Coding Directives
+
+The price migration engine doesn't have coding conventions per se. ZIO does a very good job at keeping sanity between pure and impure code, and putting adhoc code into migration specific objects (the set up of the so called "modern" migrations) helps separate the general engine logic from specific requests. We also rely on the coding expertise of contributors to simply do the right thing (including breaking rules when needed).
+
+With that said, we have the following conventions
+
+- Coding Directive #1: When using `MigrationType(cohortSpec)` to dispatch values or behaviour per migration, and unless exceptions (there are a couple in the code for when we handle exceptions or for exceptional circumstances), we will be explicit on what we want and declaring all the cases. If somebody is implementing a new migration and follows the steps Pascal presented during GW2024, then declaring a new case will happen during the [first step](https://github.com/guardian/price-migration-engine/pull/1012). The reason for this rule is that an inexperienced contributor could easily miss a place in the code where a new migration should specify behaviour. If the code compiles without prompting that decision, then the contributor might miss it. And even if the decision is to go with the "default", this needs to be explicitly specified. This convention was introduced in this pull request [pull:1022](https://github.com/guardian/price-migration-engine/pull/1022).
+

docs/migration-implementation-manual.md

@@ -6,7 +6,7 @@ In this chapter, we explain how to implement a migration from scratch.
 
 In the chapter [Price migrations from first principles](https://github.com/guardian/price-migration-engine/blob/main/docs/price-migrations-from-first-principles.md) we have discovered why some commercial entities run price migrations. In this chapter I will be describing how we implement them at the Guardian. This description is almost entirely based on Pascal's experience in implementing and monitoring them, but if one day you are responsible for an implementation you may find easier to do things slightly differently.
 
-Price migrations are not P&E projects. They are Marketing led projects. Marketing, in collaboration with Finance, will anounce that a collection of subscriptions in Zuora, often corresponding to all subscriptions of a particular product (for instance, Guardian Weekly subscriptions) will need to be migrated. The price migration engine orchestrates that operation from a systems point of view, but this happens within the larger context of operations ran by Marketing.
+Price migrations are not P&E projects. They are Marketing led projects. Marketing, in collaboration with Finance, will announce that a collection of subscriptions in Zuora, often corresponding to all subscriptions of a particular product (for instance, Guardian Weekly subscriptions) will need to be migrated. The price migration engine orchestrates that operation from a systems point of view, but this happens within the larger context of operations ran by Marketing.
 
 ## The membership-workflow update
 

docs/price-migrations-from-first-principles.md

@@ -49,7 +49,7 @@ Let's actually have a look at 3 of those schools
 ```
 Alice School   | monthly                                   | 13th
 Bob School     | quarterly (January, April, July, October) | 20th
-Charles School | Annually (March)                           | 23rd 
+Charles School | Annually (March)                          | 23rd
 ```
 
 Alice school is paying monthly on the 13th, as before. Bob School is paying on quarterly basis, on the indicated months, and does so on the 20th on the months they are paying. Charles School pays annually on the 23rd of March.

docs/the-journey-of-a-cohort-item.md

@@ -1,20 +1,20 @@
 ## The journey of a cohort item
 
-In [Price migrations from first principles](./price-migrations-from-first-principles.md) we have seen what price migrations are and the general logic behind then. In this chapter let's have a closer look at the actual steps of a price migration following the journey of a cohort item in the engine.
+In [Price migrations from first principles](./price-migrations-from-first-principles.md) we have seen what price migrations are, and the general logic behind then. In this chapter let's have a closer look at the actual steps of a price migration following the journey of a cohort item in the engine.
 
 ### What is a cohort item ?
 
 In the engine parlance a "cohort" is a set of subscription numbers that are part of a given price migration. Logically a "cohort item" would then be one of those numbers, but in fact it refers to a record in the Dynamo table where the engine maintains information about the price migration.
 
-One such cohort items seen in one of the Dynamo tables is shown below
+One such cohort item seen in one of the Dynamo tables is shown below
 
 ![](./the-journey-of-a-cohort-item/1707822328.png)
 
-The price migration may apply to a collection of subscriptions, but each subscription is price risen independently to the others, so to understand the logic of a migration we only need to follow the journey of one given subscription number.
+The price migration may apply to a collection of subscriptions, but each subscription is price risen independently to the others (and yes, a price migration could just have one subscription in it), so to understand the logic of a migration we only need to follow the journey of one given subscription number.
 
 ### Loading the subscription numbers and creating the Dynamo table.
 
-A price migration fundamentally need two ingredients 
+A price migration fundamentally need two ingredients:
 
 1. The engine has been updated with the required code to encode the specificities of the migration (for instance specific features we implemented for Newspaper2024)
 
@@ -28,14 +28,9 @@ S-00000004
 etc..
 ``` 
 
-At this point we need to have decided a name for the migration. Let's imagine we called it Newspaper2024. We locate the `price-migration-engine-prod` S3 bucket and create a directory called `Newspaper2024` and put in it two files.
+At this point we need to have decided a name for the migration. Let's imagine we called it Newspaper2024. We locate the `price-migration-engine-prod` S3 bucket and create a directory called `Newspaper2024` in which we put a file called `subscription-numbers.csv`.
 
-- salesforce-subscription-id-report.csv , which contains the list of subscription numbers
-- excluded-subscription-ids.csv , an empty file
-
-(Note: we are soon going to change those conventions, because they are legacy names which have outstayed their welcome, this will come as a PR and this documentation will be updated, but at the time those lines are written, that's the convention.)
-
-The engine will not automagically pick up those files, it needs to be instructed to do so (this will not be covered here), but the effect of the Dynamo being created and the file having been loaded is that we now have a table with records and each record simply contains a subscription number.
+The engine will not automagically pick up the file, it needs to be instructed to do so (this will not be covered here), but the effect of the Dynamo table being created and the file having been loaded is that we now have a table with records and each record simply contains a subscription number.
 
 To help with understanding of the following steps let's use a (simplified) version of the CohortItem case class in the engine Scala Code
 
@@ -71,7 +66,13 @@ CohortItem(
 
 In this stage the engine essentially is saying "I have recorded this subscription number and it's ready to be Estimated"
 
-### The Estimation Stage
+### The Estimation Stage (part 1)
+
+The first step of the Estimation stage is to run a maintenance function called `monitorDoNotProcessUntil` from the Estimation handler. This function requests items in processing stage `DoNotProcessUntil` and looks up the attribute `doNotProcessUntil`, which is a LocalDate, and check whether that date in in the past (or today). If that date is in the past, then the item is going to be migrated to processing stage `ReadyForEstimation`, otherwise the item is left untouched.
+
+The reason for the extistence of the `DoNotProcessUntil` processing stage is that it was the simplest way to handle the existence of cancellation saves and the business requirement to leave a subscription alone for a certain period of time (6 months) if, at notification state, the item carries a cancellation discount. If and when we observe such a thing, we put the item in processing stage `DoNotProcessUntil`, and then update the value of the attribute `doNotProcessUntil` to be the date until when the item needs to be left alone. On that future date (or shortly after) the item can be processed again and the way this happens is to move it to `ReadyForEstimation` waiting for it to be estimated (again).
+
+### The Estimation Stage (part 2)
 
 The estimation stage has several purposes, in fact 3 main purposes 
 
@@ -136,17 +137,35 @@ CohortItem(
 
 Then, the cohort item is going to.... sleep. It's going to sleep as long as it take for it to be at the start date minus about 40 days. This might take a few days or up to a year.
 
-### Notification and Amendment
+### Notification (part 1)
+
+In the case of migration SupporterPlus2024, the first operation is to check whether the item is under an active cancellation save. By definition, an "active" cancellation save is a cancellation save that was issued less than 6 months ago. If the item is found in that state, its processing stage become `DoNotProcessUntil` and the value of `doNotProcessUntil` is updated to become the effective date of the cancelation save plus 6 months.
+
+### Notification (part 2)
 
-Today is now `LocalDate.of(2024, 5, 10)` minus about 40 days. The engine sees a subscription in `SalesforcePriceRiceCreationComplete` stage ready to be user notified. The engine is going to perform a certain number of look ups and will send a message to a queue that will eventually be delivered to Braze (triggering the sending of an email, or sending an additional request to an external company for a letter to be printed and delivered).
+Today is now `LocalDate.of(2024, 5, 10)` minus about 40 days. The engine sees a subscription in `SalesforcePriceRiceCreationComplete` stage ready to be user notified. The engine is going to perform a certain number of lookups and will send a message to a queue that will eventually be delivered to Braze (triggering the sending of an email, or sending an additional request to an external company for a letter to be printed and delivered).
 
-The message to the user is going to mention the start date and the new estimated new price. The engine then notifies Salesforce that the user has been notified (for record keeping) and then will perform the amendment in Zuora, meaning will update Zuora with the fact that the subscription in Zuora is price risen and that the price rise is taking effect on the date that had already been decided during the estimation step. Note that this is the first and only moment that the engine performs and write operation in Zuora during the entire price rise process of that subscription. 
+The message to the user is going to mention the start date and the new estimated new price. The outcome of this step is the subscription being put in processing stage `NotificationSendComplete`.
 
-It is also important to notice that the amendment step only happened after the user notification step. This is to avoid a situation where there would be a bug in the engine or even just a long outage and causing subscriptions to be price risen in Zuora without the users having (yet) been notified. That would be illegal and put the Guardian in hot water.
+Once the item is in `NotificationSendComplete` stage the SalesforceNotificationDateUpdatehandler lambda will fire up. This operation notifies Salesforce that the user has been notified (for record keeping) and puts the item in processing stage `NotificationSendDateWrittenToSalesforce`.
 
-At this point the processing stage is `AmendmentComplete`.
+### Amendment
 
-The last step is now another Salesforce update, where we inform Salesforce that the subscription in Zuora has been edited for price rise. Then the processing stage becomes `AmendmentWrittenToSalesforce`. This completes the price rise of subscription "S-00000003".
+The item now being in processing stage `NotificationSendDateWrittenToSalesforce` the engine will perform an amendment in Zuora, meaning will update Zuora with the fact that the subscription in Zuora is price risen and that the price rise is taking effect on the date that had already been decided during the Estimation step. Note that this is the first and only moment that the engine performs a write operation in Zuora during the entire price rise process of that subscription and puts the item in `AmendmentComplete`.
+
+The next step is yet another Salesforce update, where we inform Salesforce that the subscription in Zuora has been edited for price rise. Then the processing stage becomes `AmendmentWrittenToSalesforce`. This completes the price rise of subscription "S-00000003".
+
+It is also important to notice that the amendment step only happened after the user notification step. This is a security to avoid a situation where there would be a bug in the engine or even just a long outage, causing subscriptions to be price risen in Zuora without the users having (yet) been notified. That would be illegal and put the Guardian in hot water.
+
+Last, but not least, this entire section, eg: moving through 
+
+- `SalesforcePriceRiceCreationComplete`
+- `NotificationSendComplete`
+- `NotificationSendDateWrittenToSalesforce`
+- `AmendmentComplete`
+- `AmendmentWrittenToSalesforce`
+
+happens the same day. Being independent steps it is possible to delay the next one of the sequence, but there is also value in letting in them complete the same day, so that is a customer calls a CRS, they see, in Zuora and Salesforce, an up-to-date view of what the engine was in the process of doing.
 
 ### Cancellations
 
@@ -163,3 +182,16 @@ CohortItem(
 
 Once the cohort item is in `Cancelled` state the engine will no longer touch it. Alike `AmendmentWrittenToSalesforce`, `Cancelled` is a final state for a cohort item.
 
+### Processing Lambdas
+
+The price migration engine define a state machine which is linear. The lambdas fire in a given linear order (ocassionaly the same lambda fires more than once) For reference here is the other in which the lambda fire 
+
+- CohortTableCreationHandler
+- SubscriptionIdUploadHandler
+- EstimationHandler
+- SalesforcePriceRiseCreationHandler
+- NotificationHandler
+- SalesforceNotificationDateUpdateHandler
+- AmendmentHandler
+- SalesforceAmendmentUpdateHandler
+- CohortTableDatalakeExportHandler

lambda/src/main/scala/pricemigrationengine/handlers/AmendmentHandler.scala

@@ -86,22 +86,8 @@ object AmendmentHandler extends CohortHandler {
   }
 
   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
-    //    3. Update the code to perform a negative charge back
-    //    4. Rerun the engine and check the item in Zuora
-
-    // Note that we do not apply the check to the SupporterPlus2023V1V2 migration
-    // where due to the way the prices are computed, the new price can be higher than the
+    // We do not apply the check to the SupporterPlus2023V1V2 migration, nor the SupporterPlus2024
+    // migration where, due to the way the prices are computed, the new price can be higher than the
     // estimated price (which wasn't including the extra contribution).
 
     MigrationType(cohortSpec) match {
@@ -111,6 +97,7 @@ object AmendmentHandler extends CohortHandler {
       case DigiSubs2023            => true
       case Newspaper2024           => true
       case GW2024                  => true
+      case SupporterPlus2024       => false
       case Legacy                  => true
     }
   }
@@ -195,6 +182,16 @@ object AmendmentHandler extends CohortHandler {
               GW2024Migration.priceCap
             )
           )
+        case SupporterPlus2024 =>
+          ZIO.fromEither(
+            SupporterPlus2024Migration.zuoraUpdate(
+              subscriptionBeforeUpdate,
+              startDate,
+              oldPrice,
+              estimatedNewPrice,
+              SupporterPlus2024Migration.priceCap
+            )
+          )
         case Legacy =>
           ZIO.fromEither(
             ZuoraSubscriptionUpdate