add free two months plan and allow for supporter plus monthly #2304

johnduffell · 2024-06-04T10:34:19Z

This PR is part of the cancellation save work for supporter plus.

This adds links between supporter plus monthly to the newly added 2 months free plan.

The means that people calling the endpoint will be offered the appropriate save period.

Tested in CODE on A-S00894132 and it previewed and added the 100% discount correctly
https://apisandbox.zuora.com/platform/subscriptions/8ad093fb8fe33df8018fe7cfb31a5054

I've also added a couple of tests for the "happy" cases, I wasn't sure whether to try to cover too much especially in the IT test.

johnduffell · 2024-06-04T10:35:31Z

handlers/discount-api/src/productToDiscountMapping.ts

-const catalog: {
-	[K in 'CODE' | 'PROD']: {
-		[K in Product]: { [K in BillingPeriod]: string };
-	};
-} = {


I had to remove the type because if it wanted all the billing periods for all products.

If I made them optional, the code lower down didn't compile because it assumed any might be undefined.

If I remove the type completely, it infers the exact shape and compiles as appropriate.

You could type it more strongly using the types in the product-catalog module:

import type { ProductBillingPeriod } from '@modules/product-catalog/productCatalog'; export const catalog: { [S in 'CODE' | 'PROD']: { [P in 'DigitalSubscription' | 'SupporterPlus']: { [BP in ProductBillingPeriod<P>]: string; }; }; } = { CODE: { DigitalSubscription: { Month: '2c92c0f84bbfec8b014bc655f4852d9d', Quarter: '2c92c0f84bbfec58014bc6a2d43a1f5b', Annual: '2c92c0f94bbffaaa014bc6a4212e205b', }, SupporterPlus: { Month: '8ad08cbd8586721c01858804e3275376', Annual: '', }, }, PROD: { DigitalSubscription: { Month: '2c92a0fb4edd70c8014edeaa4eae220a', Quarter: '2c92a0fb4edd70c8014edeaa4e8521fe', Annual: '2c92a0fb4edd70c8014edeaa4e972204', }, SupporterPlus: { Month: '8a128ed885fc6ded018602296ace3eb8', Annual: '', }, }, };

But then you would have to provide a value for all the billing periods which each product supports and it looks like you don't want to add one for S+ annual, so up to you if you think this is worthwhile. I suspect probably not.

thanks for the pointer, in this case I think the type just needs to be inferred as then it only compiles if everything it actually uses statically is present. Since this is not used dynamically, that should be enough.
The one that needs to be right and isn't checked by the compiler is the one that is keyed off the ID in the subscription, but that's just business logic that we need to get right.

johnduffell · 2024-06-04T10:36:19Z

handlers/discount-api/src/productToDiscountMapping.ts

+		cancellationFree2Mo: {
+			productRatePlanId: '8a1299c28fb956e8018fe2c0e12c3ae4',
+			name: 'Cancellation Save Discount - Free for 2 months',
+			upToPeriods: 2,
+			upToPeriodsType: 'Months',
+		},


https://www.zuora.com/apps/Product.do?method=view&id=2c92a0ff5345f9200153559c6d2a3385

johnduffell · 2024-06-04T10:36:30Z

handlers/discount-api/src/productToDiscountMapping.ts

+		cancellationFree2Mo: {
+			productRatePlanId: '8ad081dd8fd3d9df018fe2b6a7bc379d',
+			name: 'Cancellation Save Discount - Free for 2 months',
+			upToPeriods: 2,
+			upToPeriodsType: 'Months',
+		},


https://apisandbox.zuora.com/apps/Product.do?method=view&id=2c92c0f85721ff7c01572940171363db

johnduffell · 2024-06-04T10:36:58Z

handlers/discount-api/src/productToDiscountMapping.ts

+		supporterPlus: {
+			Month: '8a128ed885fc6ded018602296ace3eb8',
+		},


https://www.zuora.com/apps/Product.do?method=view&id=8a12865b8219d9b4018221061563643f
Supporter Plus V2 - Monthly

johnduffell · 2024-06-05T10:03:23Z

handlers/discount-api/src/discountEndpoint.ts

@@ -104,10 +104,10 @@ const getDiscountPreview = async (
 		discount.productRatePlanId,
 	);

-	if (!previewResponse.success || previewResponse.invoiceItems.length != 2) {
+	if (!previewResponse.success || previewResponse.invoiceItems.length < 2) {


supporter plus has 2 charges (base plus contribution)
I wasn't sure how valuable this check was/what it's protecting us from? but I kept it in a loose form. Should I keep it in the limit 2->3 or is minimum 2 ok? Or remove completely?

Not strictly related to your change but what do you think about pulling that 2 out to a const to give it a name?

Suggested change

if (!previewResponse.success || previewResponse.invoiceItems.length < 2) {

const supporterPlusChargeCount = 2;

if (!previewResponse.success || previewResponse.invoiceItems.length < supporterPlusChargeCount) {

funny thing is it only supported digi sub before which has one charge.
So the 2 referred to the single digi sub charge plus the new discount charge.

Now it can be 3 as we have S+ with two charges plus one new discount charge.

Maybe we could look at the subscription and count the charges, and then check for n+1 items in the preview, but I'm not 100% sure what that check was intended to assert. Perhaps it's just checking that the new charge is being planned to trigger on the right date, otherwise it wouldn't appear in the invoice?

@rupertbates any thoughts?

I think it's just following the principle of 'set out your expectations and if the situation you're in doesn't match them fail at that point rather than trying to fudge it'. Seems like a reasonable thing to do to me but if it becomes overly complicated when you add S+ into the mix I wouldn't be too upset to lose it.

I see, so fail fast and loudly principle rather than leaving zuora in a mess. I will think about whether to count the charges (on non removed rate plans) later.

johnduffell · 2024-06-05T10:04:28Z

handlers/discount-api/test/fixtures/request-bodies/digitalSub-subscribe-body-old-price.ts

@@ -57,7 +58,7 @@ export const digiSubSubscribeBody = (
 					RatePlanData: [
 						{
 							RatePlan: {
-								ProductRatePlanId: '2c92c0f84bbfec8b014bc655f4852d9d',
+								ProductRatePlanId: catalog.CODE.digiSub.Month,


I updated the existing code to use the const, just so it's a little clearer what's going on.

johnduffell · 2024-06-05T10:05:30Z

handlers/discount-api/test/fixtures/request-bodies/supporterplus-subscribe-body-tier2.ts

@@ -0,0 +1,69 @@
+import { zuoraDateFormat } from '@modules/zuora/common';


this file is mostly a copy of the above one, rather than adding a parameter, but there's definitely scope for more reuse.

johnduffell · 2024-06-05T10:06:41Z

handlers/discount-api/test/helpers.ts

+	subscribeBody: {
+		subscribes: {
+			Account: {
+				IdentityId__c: string;
+				InvoiceTemplateId: string;
+				AutoPay: boolean;
+				PaymentTerm: string;
+				CreatedRequestId__c: string;
+				Name: string;
+				sfContactId__c: string;
+				Batch: string;
+				PaymentGateway: string;
+				Currency: string;
+				BcdSettingOption: string;
+				BillCycleDay: number;
+				CrmId: string;
+			};
+			SubscribeOptions: { GenerateInvoice: boolean; ProcessPayments: boolean };
+			SubscriptionData: {
+				RatePlanData: {
+					RatePlan: { ProductRatePlanId: string };
+					SubscriptionProductFeatureList: any[];
+				}[];
+				Subscription: {
+					ContractEffectiveDate: string;
+					ContractAcceptanceDate: string;
+					TermStartDate: string;
+					AutoRenew: boolean;
+					RenewalTerm: number;
+					InitialTerm: number;
+					ReaderType__c: string;
+					TermType: string;
+					CreatedRequestId__c: string;
+					InitialTermPeriodType: string;
+				};
+			};
+			PaymentMethod: {
+				BankTransferAccountName: string;
+				Type: string;
+				BankTransferAccountNumber: string;
+				FirstName: string;
+				PaymentGateway: string;
+				BankTransferType: string;
+				Country: string;
+				BankCode: string;
+				LastName: string;
+			};
+			BillToContact: {
+				FirstName: string;
+				Country: string;
+				LastName: string;
+				WorkEmail: string;
+			};
+		}[];


this type is a bit wild but it doesn't seem to exist in any defined form, it's just from the data. I wasn't sure what to do here?

If you felt adventurous you could upgrade this to use the orders api and define the types outlined here:
https://developer.zuora.com/v1-api-reference/api/operation/POST_Order/
in
modules/zuora/orders.ts

Not in this PR though obviously!

tjmw

Looks really good!

I left a few minor comments/suggestions to take or leave, no blockers though!

tjmw · 2024-06-06T10:15:05Z

handlers/discount-api/src/discountEndpoint.ts

@@ -104,10 +104,10 @@ const getDiscountPreview = async (
 		discount.productRatePlanId,
 	);

-	if (!previewResponse.success || previewResponse.invoiceItems.length != 2) {
+	if (!previewResponse.success || previewResponse.invoiceItems.length < 2) {


Not strictly related to your change but what do you think about pulling that 2 out to a const to give it a name?

Suggested change

if (!previewResponse.success || previewResponse.invoiceItems.length < 2) {

const supporterPlusChargeCount = 2;

if (!previewResponse.success || previewResponse.invoiceItems.length < supporterPlusChargeCount) {

tjmw · 2024-06-06T11:57:14Z

handlers/discount-api/test/eligibilityChecker.test.ts

+	const eligibilityChecker = new EligibilityChecker(catalog);
+	expect(
+		dayjs(
+			eligibilityChecker.getNextBillingDateIfEligible(
+				billingPreview,
+				getDiscountableRatePlan(sub),
+			),
+		).format('YYYY-MM-DD'),
+	).toEqual('2024-07-04');


This is totally subjective so feel free to ignore! I'm a fan of the arrange-act-assert pattern in tests, just to make clear what's happening at each phase. What do you think about this slight refactoring?

Suggested change

const eligibilityChecker = new EligibilityChecker(catalog);

expect(

dayjs(

eligibilityChecker.getNextBillingDateIfEligible(

billingPreview,

getDiscountableRatePlan(sub),

),

).format('YYYY-MM-DD'),

).toEqual('2024-07-04');

// Arrange

// ..other setup

const eligibilityChecker = new EligibilityChecker(catalog);

// Act

const nextDate = eligibilityChecker.getNextBillingDateIfEligible(

billingPreview,

getDiscountableRatePlan(sub),

);

// Assert

expect(dayjs(nextDate).format('YYYY-MM-DD')).toEqual('2024-07-04');

yep I'm totally on board with that, I never heard that way of describing it but I do try to follow it, it was a just a bit copy and paste at the moment.
I will check the other tests and keep them all consistent with your suggestion.

although that's an interesting one for the tests that are expected to throw - because catching an exception is not referentially transparent, it's not easy to pull out the "Act" and the "Assert".
Try is a very scala-y thing so I'm guessing that or similar is not the done thing in TS?

how about this? #2309

although that's an interesting one for the tests that are expected to throw - because catching an exception is not referentially transparent, it's not easy to pull out the "Act" and the "Assert".

Yeah totally, I definitely find it harder to follow AAA in these cases.

Try is a very scala-y thing so I'm guessing that or similar is not the done thing in TS?

True, it's not a language feature, but you do occasionally see abstractions which attempt to provide something similar, it's not very common.

tjmw · 2024-06-06T11:58:47Z

handlers/discount-api/test/previewDiscountIntegration.test.ts

+test('Supporter Plus subscriptions are eligible', async () => {
+	const zuoraClient = await ZuoraClient.create(stage);
+
+	console.log('Creating a new S+ subscription');


Should this console.log still be here?

I think so, but I haven't quite got my head around the logging standards in TS yet. The logging does seem very spammy in the tests. This was copied from the "creating a digital subscription" code and follows the pattern.

There is a lot of logging in the code which actually runs in the lambdas so that you can tell what's happening in Cloudwatch logs, I'd usually remove logging from tests though.

I would like to find a way to lower the logging level when running tests but I haven't really looked into it too closely and it didn't look trivial to do.

On a separate point, I'm not sure how useful all these integration tests are. I was experimenting with setting up a new subscription for every test run, carrying out the action under test and then cancelling the subs, but I think it was of limited use ultimately because you need subs which are in a number of different states to test effectively and that's not really possible to do like this.

fair enough well it's another string to the bow. I was a bit surprised there were no "end to end" style integration tests that pass in the raw payload and expect the output.

I wonder if you've seen my PR here ? : #2307
we can use it as stated to collect requests in PROD for a short time period, but we could also use it to capture tests that we do manually in CODE (and even run locally in DEV as integration tests) and store them permanently. Perhaps not quite as easy to read and update as normal tests, but we could improve that.

johnduffell · 2024-06-10T09:19:28Z

thanks for the comments and suggestions, one comment warranted more work at this stage and it's waiting for review in this PR: #2309

add free two months plan and allow for supporter plus monthly

ecd1a1c

johnduffell commented Jun 4, 2024

View reviewed changes

johnduffell marked this pull request as draft June 4, 2024 10:55

allow more than 2 invoice items when previewing discount

847bd75

Base automatically changed from jd-remove-unused-fields to main June 4, 2024 11:10

add automated tests plus tidyup

45f7154

johnduffell marked this pull request as ready for review June 5, 2024 10:01

johnduffell commented Jun 5, 2024

View reviewed changes

johnduffell requested a review from rupertbates June 5, 2024 10:08

tjmw approved these changes Jun 6, 2024

View reviewed changes

johnduffell mentioned this pull request Jun 6, 2024

test improvements #2309

Merged

johnduffell merged commit e31a434 into main Jun 10, 2024
42 checks passed

johnduffell deleted the jd-supporterplus-free-save branch June 10, 2024 09:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add free two months plan and allow for supporter plus monthly #2304

add free two months plan and allow for supporter plus monthly #2304

johnduffell commented Jun 4, 2024 •

edited

Loading

johnduffell Jun 4, 2024

rupertbates Jun 6, 2024

johnduffell Jun 6, 2024

johnduffell Jun 4, 2024

johnduffell Jun 4, 2024

johnduffell Jun 4, 2024 •

edited

Loading

johnduffell Jun 5, 2024

tjmw Jun 6, 2024

johnduffell Jun 6, 2024

rupertbates Jun 6, 2024

johnduffell Jun 6, 2024

johnduffell Jun 5, 2024

johnduffell Jun 5, 2024

johnduffell Jun 5, 2024

rupertbates Jun 6, 2024 •

edited

Loading

tjmw left a comment

tjmw Jun 6, 2024

tjmw Jun 6, 2024

johnduffell Jun 6, 2024

johnduffell Jun 6, 2024

johnduffell Jun 6, 2024

tjmw Jun 10, 2024

tjmw Jun 6, 2024

johnduffell Jun 6, 2024

rupertbates Jun 6, 2024

rupertbates Jun 6, 2024

johnduffell Jun 6, 2024

johnduffell commented Jun 10, 2024

	if (!previewResponse.success \|\| previewResponse.invoiceItems.length < 2) {
	const supporterPlusChargeCount = 2;
	if (!previewResponse.success \|\| previewResponse.invoiceItems.length < supporterPlusChargeCount) {

		@@ -0,0 +1,69 @@
		import { zuoraDateFormat } from '@modules/zuora/common';

add free two months plan and allow for supporter plus monthly #2304

add free two months plan and allow for supporter plus monthly #2304

Conversation

johnduffell commented Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnduffell Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rupertbates Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

tjmw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnduffell commented Jun 10, 2024

johnduffell commented Jun 4, 2024 •

edited

Loading

johnduffell Jun 4, 2024 •

edited

Loading

rupertbates Jun 6, 2024 •

edited

Loading