-
Notifications
You must be signed in to change notification settings - Fork 5
new guardian subscription model #3381
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
base: main
Are you sure you want to change the base?
Conversation
handlers/product-switch-api/runManual/realSubscriptions/downloadRealSubscriptions.ts
Fixed
Show fixed
Hide fixed
0dba696 to
54954c5
Compare
| @@ -0,0 +1,7 @@ | |||
| /** | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
runManual/realSusbcriptions/ is the package of code to extract and redact PROD subscriptions from the logs, and let us test that they can all be parsed by the subscription parser.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| @@ -0,0 +1,242 @@ | |||
| import { existsSync, mkdirSync, writeFileSync } from 'fs'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this script downloads and redacts the subscription data, it's converted from a (shorter) scala version by copilot with a bit of hand tidying up so it's a little bit rough.
I feel like it's less important because it's self contained manually run code, but I'm happy to tidy it up to production standard if necessary.
It takes less than a minute to run, mostly limited by making the AWS calls.
| const zuoraSubscription = zuoraSubscriptionSchema.parse(subscriptionData); | ||
| const guardianSubscription = | ||
| guardianSubscriptionParser.toGuardianSubscription(zuoraSubscription); | ||
| const filter = | ||
| SubscriptionFilter.activeNonEndedSubscriptionFilter(callDate); | ||
| const filteredSubscription = | ||
| filter.filterSubscription(guardianSubscription); | ||
| const flattenedSubscription = | ||
| getSinglePlanFlattenedSubscriptionOrThrow(filteredSubscription); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at the moment it just checks they can be parsed, it doesn't check any of the values make sense, which is about all we can do given we don't know much about the underlying subscriptions.
I think this is a pretty good start though.
| ratePlan, | ||
| discountRatePlans, | ||
| } satisfies GuardianSubscription; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it says the below file is a "Large diff" then open it up anyway, as it's an important file.
| const billingPeriodMonths: number = { | ||
| const billingPeriodMap: Record<string, number> = { | ||
| Month: 1, | ||
| Quarter: 3, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quarter was never supported, it was just for the type checker. Now OneTime also exists, so I'm breaking the link (this will be fixed in the next PR to use a better type)
| @@ -0,0 +1,28 @@ | |||
| import type { GuardianRatePlan } from '../../../src/guardianSubscription/guardianSubscriptionParser'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not an actual test, it's just making sure that the type checker narrows the type if we check the productKey and/or productRatePlanKey so we can do things like ratePlan.ratePlanCharges.Contribution.price to get the contribution amount without finds or getIfDefineds
| test('throws when subscription status is not Active', async () => { | ||
| const now = dayjs(); | ||
| const subscription = makeSubscriptionWithSingleCharge('Month', 10); | ||
| subscription.status = 'Suspended'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suspended doesn't exist, I think this must have been meant to be cancelled
| it('should skip cancellation for inactive subscription (Expired)', async () => { | ||
| const mockSubscription = createMockSubscription('Expired'); | ||
|
|
||
| const result = await cancelSubscriptionService( | ||
| mockLogger, | ||
| mockZuoraClient, | ||
| mockSubscription, | ||
| ); | ||
|
|
||
| expect(result).toBe(false); | ||
| expect(mockLogger.log).toHaveBeenCalledWith( | ||
| 'Subscription already inactive (Expired), skipping cancellation', | ||
| ); | ||
| expect(cancelSubscription).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should skip cancellation for inactive subscription (Suspended)', async () => { | ||
| const mockSubscription = createMockSubscription('Suspended'); | ||
|
|
||
| const result = await cancelSubscriptionService( | ||
| mockLogger, | ||
| mockZuoraClient, | ||
| mockSubscription, | ||
| ); | ||
|
|
||
| expect(result).toBe(false); | ||
| expect(mockLogger.log).toHaveBeenCalledWith( | ||
| 'Subscription already inactive (Suspended), skipping cancellation', | ||
| ); | ||
| expect(cancelSubscription).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expired exists but wouldn't come back for a "get subscription by sub name" call, and Suspended doesn't exist. There's a test above for Cancelled anyway.
| @@ -107,17 +107,49 @@ export type Product<P extends ProductKey> = ProductCatalog[P]; | |||
|
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the types in this file had to be slightly complicated to make narrowing work correctly.
I'm not totally sure of the ins and outs, but it does seem to work.
| validateOrThrow<P extends ProductKey>( | ||
| targetGuardianProductName: P, | ||
| productRatePlanKey: string, | ||
| ): GuardianCatalogKeys<P> { | ||
| const ratePlans = this.catalogData[targetGuardianProductName].ratePlans; | ||
| if (!this.hasRatePlan(productRatePlanKey, ratePlans)) { | ||
| throw new Error( | ||
| `Unsupported rate plan key: ${String( | ||
| productRatePlanKey, | ||
| )} for product ${targetGuardianProductName}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you pass a product and rate plan key, it checks that they are valid in combination and return an object to hold both in a type safe way, this is needed so it can check that the requested product switch actually exists at run time (as we can't do it at compile time with implicits!)
| export const metricsSchema = z.object({ | ||
| totalInvoiceBalance: z.number(), | ||
| currency: z.string(), | ||
| currency: z.enum(CurrencyValues), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked bigquery and there only are the 6 well known currencies in zuora, so this should be safe
| accountNumber: z.string(), | ||
| subscriptionNumber: z.string(), | ||
| status: z.string(), | ||
| status: z.enum(['Active', 'Cancelled']), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is slightly more risky as we have a lot of "expired" subscription versions, however if you fetch by sub name, you always get the latest i.e. non expired version.
handlers/product-switch-api/src/guardianSubscription/group/buildZuoraProductIdToKey.ts
Outdated
Show resolved
Hide resolved
handlers/product-switch-api/runManual/realSusbcriptions/README.md
Outdated
Show resolved
Hide resolved
...lers/product-switch-api/src/guardianSubscription/group/groupSubscriptionByZuoraCatalogIds.ts
Outdated
Show resolved
Hide resolved
...lers/product-switch-api/src/guardianSubscription/group/groupSubscriptionByZuoraCatalogIds.ts
Outdated
Show resolved
Hide resolved
| ratePlans: ZuoraSubscription['ratePlans'], | ||
| ): IndexedZuoraSubscriptionRatePlansByProduct { | ||
| const doubleGroupedRatePlans: IndexedZuoraSubscriptionRatePlansByProduct = | ||
| mapValues(this.byProductAndRatePlan(ratePlans), (x) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to make this more readable? Maybe use meaningful variable names? My brains shuts down as soon as I look at it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed an updated version, actually by grouping charges first, then grouping by rateplan/product ids, it saves us having to dig down to do the charges afterwards.
...lers/product-switch-api/src/guardianSubscription/group/groupSubscriptionByZuoraCatalogIds.ts
Outdated
Show resolved
Hide resolved
...lers/product-switch-api/src/guardianSubscription/group/groupSubscriptionByZuoraCatalogIds.ts
Outdated
Show resolved
Hide resolved
# Conflicts: # modules/arrayFunctions.ts # modules/objectFunctions.ts
handlers/product-switch-api/src/guardianSubscription/subscriptionFilter.ts
Outdated
Show resolved
Hide resolved
0ac651a to
4b857aa
Compare
ea81c8e to
14004bc
Compare
...rs/product-switch-api/src/guardianSubscription/reprocessRatePlans/guardianRatePlanBuilder.ts
Outdated
Show resolved
Hide resolved
...rs/product-switch-api/src/guardianSubscription/reprocessRatePlans/guardianRatePlanBuilder.ts
Outdated
Show resolved
Hide resolved
| const ratePlans: ProductCatalog[P]['ratePlans'] = | ||
| this.catalogData[productKey].ratePlans; | ||
| return ratePlans[productRatePlanKey]; | ||
| return ratePlans[productRatePlanKey] as ProductRatePlan<P, PRP>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This used to typecheck but now we're having to say as ProductRatePlan why is that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes I didn't like that but I couldn't find a way around it.
it's because I had to change the type of ProductRatePlan from what you'd expect ProductCatalog[P]['ratePlans'][PRP] to a conditional type that proves that PRP is a valid child of P:
P extends ProductKey
? PRP extends keyof ProductCatalog[P]['ratePlans']
? ProductCatalog[P]['ratePlans'][PRP]
: never
: neverIf I don't do that, then if you have a ProductRatePlan without an exact type, it just has no accessible properties and collapses to never. So you can't get at the charges etc. without casting at that point. Hence why I did it as early as possible.
This (by my understanding) is if P is Contribution|SubscriptionCard and PRP is Annual|Weekend, it finds that it could be an invalid combination and collapses it to never. Doing that funny trick lets it remove all the invalid combinations from the union before it gets to that point. However it can't prove that the two types are compatible.
I think typescript basically falls down as it turns everything into unions that don't have any context, then gets in a mess.
I think in the old memsub we used implicits for keeping the context, I did wonder if we can wrap the rate plan key inside the product key somehow to replicate that, but I couldn't think how it would work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I got to pretty much the same conclusion after chatting to copilot for a bit 👍
handlers/product-switch-api/src/guardianSubscription/subscriptionFilter.ts
Outdated
Show resolved
Hide resolved
...lers/product-switch-api/src/guardianSubscription/group/groupSubscriptionByZuoraCatalogIds.ts
Outdated
Show resolved
Hide resolved
680981d to
ad8572a
Compare
precursor to #3323
Background
It's common in our lambdas to need to know what kind of subscription we are working on. Conventionally we loop through the ratePlans and charges array, looking for the product*id values that match what we are looking for (that are read or hard coded from the catalog).
We then need to check the lastChangeType and effective dates accordingly to make sure we're not looking at something that's a historic rate plan.
This works reasonably well for identifying a couple of products, but it leads to repetition between lambdas and also inconsistency.
At the moment we are looking to expand product switching across multiple products, and soon we will be trying to fetch all products for MMA account overview via the lambdas, meaning we need to be able to deserialise all types of subscription consistently.
The changes overview
This PR introduces an augmented "guardian subscription" model on top of the basic ZuoraSubscription, to reduce the amount of searching through zuora subscriptions we have to do in various lambdas.
At the moment it stays close to the ZuoraSubscription model, and augments it, rather than changing to more guardian and product specific terminology. That means we stick with
contractEffectiveDateandeffectiveStartDaterather than trying to usetrialEndDateandfirstDeliveryDateetc. I think it's good to keep it relatively consistent but there's no reason it can't diverge in future either generally or on a per product basis.See the readme in the package: https://github.com/guardian/support-service-lambdas/blob/jd/gu-subscription-model/handlers/product-switch-api/src/guardianSubscription/README.md
How it works
First it processes the Zuora catalog:
This makes it ready to join with a subscription.
Next it processes a standard ZuoraSubscription from the getSubscription call
i. rateplans by product id and product rate plan id (nested)
i. group the charges by product rate plan charge id
i. determine the productKey and productRatePlanKey and add them to the rate plan
i. fetch the product and productRatePlan objects and add them to the rate plan
i. fetch each chargeKey, and replace
ratePlanChargesarray to be an object keyed off the chargeKeyi. the ratePlan goes under ratePlans property (replacing the original)
i. fetch the product and rateplan from the zuora catalog and add them to the rate plan
i. fetch each charge name from the zuora catalog and key the ratePlanCharges off that.
i. attach the catalog charge object to the associated charge.
i. move the ratePlan under
productsNotInCatalogproperty (fromratePlans)i. go through the rate plans removing those that are lastChangeType=Remove
i. If the subscription is Active - go through the charges removing any that are after their effectiveEndDate
i. If the subscription is Cancelled - remove any that have an effectiveEndDate before the termEndDate (i.e. keep whatever that was active at the very end)
Now we have a "guardian" subscription
Example "guardian" subscription
This will be used in the above PR to enable generic product switching.
Testing
I added some unit tests, but it's hard to think of all the different states of subscriptions that enter the system.
Since this has to be able to parse all subscriptions that call product-switch-api, I thought the best solution was to test it against them.
As such I implemented a novel form of testing where it searches the cloudwatch logs for zuora subscription responses, parses out the JSON body, and feeds it through the parsers, checking that it doesn't fail.
This did find a few edge cases such as backdated cancelled subscriptions, and subscriptions with a current introductory discount, directly followed by a cancellation save discount, which I have fixed.
Next steps
This is not yet in use - it will be in use for all product switches once the follow on PR is shipped.
We could in future add this as an wrapper similar to
withMMAIdentityCheck[1], if we want to reduce clutter in the business logic of the lambdas. Then the handlers would get a valid guardian subscription object for free.[1] https://github.com/search?q=repo%3Aguardian%2Fsupport-service-lambdas%20withMMAIdentityCheck&type=code