feature(admin-panel): Add nestjs subscription service #12979

dschom · 2022-05-21T00:47:05Z

Because

We want to display account subscription data in the admin panel.

This pull request

Hooks up the Account React component to real subscription data. 
Fixes a long standing issue with Knex in the admin panel! Knex instance is now bound to BaseAuthModel and repurposed across derived classes.
Introduces a subscription module that provides the nestjs services necessary to retrieve subscription data.
Introduces a subscription service that acts as the primary point of request for subscription data.
Introduces stripe service so that stripe can be dependency injected.
Introduces firestore service so that firestore can be dependency injected.
Introduces play store service so that play store accessor can be dependency injected.
Introduces app store service so that app store accessor can be dependency injected.
Introduces subscription formatters that unify subscription dtos into standard response format.
Improves config setup to allow for local.json and secrets.json files to be used.
Introduces configuration settings so that underlying services can be accessed.
Introduces feature flags to disable queries to underlying stripe, google or apple apis.
Hoists a couple joi validators up to fxa-shared for reuse.
Breaks out an auxiliary method that can determine product ids given iapType and a plan. This exists in in stripe.ts in fxa-shared. (The previous code had zero dependence on the stripe class, which made this possible. This was primarily done for testing purposes.)
Exposes a couple more fields on AppStoreSubscriptionPurchase to support subscription formatting.
Fixes typing on MozSubscription.endedAt. Value is allowed to be null | undefined.
Adds tests and achieves 90+ percent test coverage on all new code.
Adds reusable mocks for standard services to facilitate testing.
Updates readme with info about subscriptions, configuration, feature flags, and testing.
Updates pm2.js with better defaults to support subscription service.

Issue that this pull request solves

Closes: #11157

Checklist

Put an x in the boxes that apply

My commit is GPG signed.
If applicable, I have modified or added tests which pass locally.
I have added necessary documentation (if appropriate).
I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

Other information (Optional)

I had trouble testing some of the integrations with apple app store and google pay. If someone who is doing code review can run though this flow, it'd be much appreciated. I have a high percentage of test coverage, so unless one of my mocks is incorrect, I feel pretty good about this. I also have feature flags in place around google play, apple apply app store and stripe. In the event something malfunctions, we can toggle environment variables.

fxa-bananafox · 2022-05-21T15:14:08Z

I got u dschom. 💯

fxa-bananafox · 2022-05-21T15:35:31Z

I got u dschom. 💯

dschom · 2022-05-23T16:06:57Z

packages/fxa-admin-server/src/config.ts

@@ -1,180 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public


Moved to src/config/index.ts to match other projects and keep .json config files isolated.

dschom · 2022-05-23T16:10:53Z

packages/fxa-admin-server/src/schema.gql

@@ -113,7 +113,7 @@ type MozSubscription {
  currentPeriodEnd: Float!
  currentPeriodStart: Float!
  cancelAtPeriodEnd: Boolean!
-  endAt: Float!
+  endedAt: Float


Keeping this in alignment with stripe format.

dschom · 2022-05-23T16:12:16Z

packages/fxa-admin-server/src/subscriptions/firestore.service.ts

+/**
+ * Factory for providing access to firestore
+ */
+export const FirestoreFactory: Provider<Firestore> = {


I can add a follow up item for moving these to fxa-shared/nestjs if we think they are generally useful.

dschom · 2022-05-23T16:17:50Z

packages/fxa-admin-server/src/subscriptions/subscriptions.formatters.ts

+  static toStatus(
+    subscription: MozAppSubscriptionStatusType
+  ): Stripe.Subscription.Status | null {
+    // Mappings are not 1 to 1, which makes this tricky. Going with a very minimal set of mappings,


I was a bit torn as far as what should be done here. The Jira ticket said to just provide a single status field, which works well for stripe, so I did my best to bring this into align with what was in stripe. However, in some ways it might be more clear to a support person to just expose more of the internal subscription state and then link to the apple documentation.

Good call out, I think its fair to probably start with the initial requirement for now.

If you see anything off in these mappings, please let me know. A lot of it was reading the docs and doing a best effort on interpreting them. This is my first exposure to these APIs, so any feedback here is definitely appreciated.

dschom · 2022-05-23T16:26:45Z

packages/fxa-admin-server/src/subscriptions/subscriptions.service.ts

+   * getSubscriptions when performance is of concern.
+   * @param uid - the account id
+   */
+  public async *getSubscriptionsGenerator(uid: string) {


I think we should use more generators. As of right now, this generator doesn't really provide us all that much benefit since downstream dependencies don't use them, however, extending the pattern to the underlying data access classes in fxa-shared might be a nice thing to do down the road.

dschom · 2022-05-23T16:31:33Z

packages/fxa-shared/payments/stripe.ts

-      .split(',')
-      .map((c) => c.trim().toLowerCase())
-      .filter((c) => !!c);
+    return determineIapIdentifiers(iapType, price);


This function had no dependence on the class's state (i.e. 'this' is never referenced). I broke it out to into it's own auxiliary method in order to make testing easier and improve reuse.

fxa-bananafox · 2022-05-23T16:36:04Z

I got u dschom. 💯

vbudhram

Just a couple nits on the code. I'm gonna try to give this a spin locally.

vbudhram · 2022-05-23T17:30:52Z

packages/fxa-admin-server/README.md

+
+In the event you can’t provide a secrets file, but still want to do some development work, consider using feature flags to disable subscription features accordingly.
+
+Here is an example secrets.json that would support stripe, and google play, and apple app store.


vbudhram · 2022-05-23T17:38:44Z

packages/fxa-admin-server/src/subscriptions/appstore.service.spec.ts

+  });
+
+  it('should be defined', () => {
+    expect(service).toBeDefined();


Is it possible to test getSubscriptions or is it alot of work?

I can add a mock that ensures purchaseManager.queryCurrentSubscriptionPurchases is invoked properly.

Really the test coverage should be on the purchase manager. IMO, I shouldn’t have re-test that the purchase manager is working here. Since the purchase manager and iap stuff was already in place, I sort of assumed it was already under test, but maybe it could use more test coverage. Having to add this coverage seems like a separate task, and I’d have to evaluate what’s already there.

vbudhram · 2022-05-23T17:46:13Z

packages/fxa-admin-server/src/subscriptions/stripe.service.ts

+
+    super(config, metrics, logger);
+
+    // Intentionally leaving this out for now.


Is this something you planned to tackle in follow up issue?

Ah, I am glad you brought this up! I did this earlier on development work and actually forgot that I wanted to call this out in the review. There were two reasons I did this initially.

For stripe purchase manager wasn’t needed. I only realized after your commetnd and working on the app store / play store integration that it is. I’ll add this back in asap, and try to figure out a way to get this guarded by a test case.

I was a bit worried that the listeners in the purchase manager wouldn’t function properly in the admin-server since it requires a VPN connection. I wanted to check with someone about this.

BTW, I think this also speaks to the issues I had with testing the app and play store integrations. I feel like creating that test infrastructure was beyond the scope of the task at hand. If there is something that I can just piggyback off of, that'd be great.

vbudhram · 2022-05-23T17:51:05Z

packages/fxa-admin-server/src/subscriptions/subscriptions.formatters.ts

+  static toStatus(
+    subscription: MozAppSubscriptionStatusType
+  ): Stripe.Subscription.Status | null {
+    // Mappings are not 1 to 1, which makes this tricky. Going with a very minimal set of mappings,


Good call out, I think its fair to probably start with the initial requirement for now.

packages/fxa-admin-server/src/subscriptions/test.util.ts

vbudhram

@dschom Tested locally and it looks good. One thing I noticed was that the current period start and end time have strange dates

?

dschom · 2022-05-23T21:35:50Z

@dschom Tested locally and it looks good. One thing I noticed was that the current period start and end time have strange dates ?

A good call out, I'll investigate the dates.

I also will revisit the intentionally undefined purchase manager instance that you brought up, as I am probably playing it a bit too fast and bit too loose with testing around the app store and play store purchases.

@vbudhram I just looked into the dates again. It appears this is the value stored in stripe. so the problem does not exist in this PR. If this persists in staging, I'll file a bug for it, as I think it is an issue elsewhere. Here's a screen shot of a local Firestore doc:

fxa-bananafox · 2022-05-23T23:58:37Z

I got u dschom. 💯

fxa-bananafox · 2022-05-24T00:13:57Z

I got u dschom. 💯

fxa-bananafox · 2022-05-24T01:38:43Z

I got u dschom. 💯

fxa-bananafox · 2022-05-24T14:42:32Z

I got u dschom. 💯

fxa-bananafox · 2022-05-24T15:01:51Z

I got u dschom. 💯

Because: - We want to display account subscription data in the admin panel. This Commit: - Hooks up the Account React component to real subscription data.  - Fixes a long standing issue with Knex in the admin panel! Knex instance is now bound to BaseAuthModel and repurposed across derived classes. - Introduces a subscription module that provides the nestjs services necessary to retrieve subscription data. - Introduces a subscription service that acts as the primary point of request for subscription data. - Introduces stripe service so that stripe can be dependency injected. - Introduces firestore service so that firestore can be dependency injected. - Introduces play store service so that play store accessor can be dependency injected. - Introduces app store service so that app store accessor can be dependency injected. - Introduces subscription formatters that unify subscription dtos into standard response format. - Improves config setup to allow for local.json and secrets.json files to be used. - Introduces configuration settings so that underlying services can be accessed. - Introduces feature flags to disable queries to underlying stripe, google or apple apis. - Hoists a couple joi validators up to fxa-shared for reuse. - Breaks out an auxiliary method that can determine product ids given iapType and a plan. This exists in in stripe.ts in fxa-shared. (The previous code had zero dependence on the stripe class, which made this possible. This was primarily done for testing purposes.) - Exposes a couple more fields on AppStoreSubscriptionPurchase to support subscription formatting. - Fixes typing on MozSubscription.endedAt. Value is allowed to be null | undefined. - Adds tests and achieves 90+ percent test coverage on all new code. - Adds reusable mocks for standard services to facilitate testing. - Updates readme with info about subscriptions, configuration, feature flags, and testing. - Updates pm2.js with better defaults to support subscription service.

fxa-bananafox · 2022-05-24T15:54:38Z

I got u dschom. 💯

dschom mentioned this pull request May 21, 2022

feature(admin-panel): Provide link to manage customer subscription in stripe. #12980

Merged

4 tasks

dschom force-pushed the FXA-4265-hook-up-subscriptions-data-in-admin-panel branch from 158a87b to 234433f Compare May 21, 2022 15:13

dschom force-pushed the FXA-4265-hook-up-subscriptions-data-in-admin-panel branch from b35ee6f to a35a0ba Compare May 21, 2022 15:35

dschom commented May 23, 2022

View reviewed changes

dschom marked this pull request as ready for review May 23, 2022 16:31

dschom requested a review from a team as a code owner May 23, 2022 16:31

dschom requested a review from bbangert May 23, 2022 16:32

dschom force-pushed the FXA-4265-hook-up-subscriptions-data-in-admin-panel branch from bbbb953 to 0ce86ae Compare May 23, 2022 16:35

vbudhram reviewed May 23, 2022

View reviewed changes

vbudhram approved these changes May 23, 2022

View reviewed changes

dschom force-pushed the FXA-4265-hook-up-subscriptions-data-in-admin-panel branch from 205b887 to 9478d6b Compare May 23, 2022 23:58

dschom force-pushed the FXA-4265-hook-up-subscriptions-data-in-admin-panel branch from 44e07a5 to ffedaa4 Compare May 24, 2022 00:13

dschom force-pushed the FXA-4265-hook-up-subscriptions-data-in-admin-panel branch from 461022c to 23221e0 Compare May 24, 2022 01:38

dschom force-pushed the FXA-4265-hook-up-subscriptions-data-in-admin-panel branch from 157c502 to 524d580 Compare May 24, 2022 14:42

dschom force-pushed the FXA-4265-hook-up-subscriptions-data-in-admin-panel branch from c450fab to f8fd5c0 Compare May 24, 2022 15:01

dschom force-pushed the FXA-4265-hook-up-subscriptions-data-in-admin-panel branch from 7587e39 to 0db515b Compare May 24, 2022 15:54

chore(deps): updated yarn.lock

8398eef

dschom merged commit cff8bc4 into main May 24, 2022

dschom deleted the FXA-4265-hook-up-subscriptions-data-in-admin-panel branch August 1, 2022 22:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(admin-panel): Add nestjs subscription service #12979

feature(admin-panel): Add nestjs subscription service #12979

dschom commented May 21, 2022 •

edited

Loading

fxa-bananafox bot commented May 21, 2022

fxa-bananafox bot commented May 21, 2022

dschom May 23, 2022

dschom May 23, 2022

dschom May 23, 2022

dschom May 23, 2022

vbudhram May 23, 2022

dschom May 23, 2022

dschom May 23, 2022

dschom May 23, 2022 •

edited

Loading

fxa-bananafox bot commented May 23, 2022

vbudhram left a comment

vbudhram May 23, 2022

vbudhram May 23, 2022

dschom May 23, 2022

vbudhram May 23, 2022

dschom May 23, 2022 •

edited

Loading

vbudhram May 23, 2022

vbudhram left a comment

dschom commented May 23, 2022 •

edited

Loading

fxa-bananafox bot commented May 23, 2022

fxa-bananafox bot commented May 24, 2022

fxa-bananafox bot commented May 24, 2022

fxa-bananafox bot commented May 24, 2022

fxa-bananafox bot commented May 24, 2022

fxa-bananafox bot commented May 24, 2022

		@@ -1,180 +0,0 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public


		In the event you can’t provide a secrets file, but still want to do some development work, consider using feature flags to disable subscription features accordingly.

		Here is an example secrets.json that would support stripe, and google play, and apple app store.


		super(config, metrics, logger);

		// Intentionally leaving this out for now.

feature(admin-panel): Add nestjs subscription service #12979

feature(admin-panel): Add nestjs subscription service #12979

Conversation

dschom commented May 21, 2022 • edited Loading

Because

This pull request

Issue that this pull request solves

Checklist

Screenshots (Optional)

Other information (Optional)

fxa-bananafox bot commented May 21, 2022

fxa-bananafox bot commented May 21, 2022

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

dschom May 23, 2022 • edited Loading

Choose a reason for hiding this comment

fxa-bananafox bot commented May 23, 2022

vbudhram 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

dschom May 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbudhram left a comment

Choose a reason for hiding this comment

dschom commented May 23, 2022 • edited Loading

fxa-bananafox bot commented May 23, 2022

fxa-bananafox bot commented May 24, 2022

fxa-bananafox bot commented May 24, 2022

fxa-bananafox bot commented May 24, 2022

fxa-bananafox bot commented May 24, 2022

fxa-bananafox bot commented May 24, 2022

dschom commented May 21, 2022 •

edited

Loading

dschom May 23, 2022 •

edited

Loading

dschom May 23, 2022 •

edited

Loading

dschom commented May 23, 2022 •

edited

Loading