Skip to content

feat(scripts): remove subscriptions check from inactive deletions#18223

Merged
chenba merged 1 commit intomainfrom
fxa-inactives-remove-direct-subplat-checks
Jan 15, 2025
Merged

feat(scripts): remove subscriptions check from inactive deletions#18223
chenba merged 1 commit intomainfrom
fxa-inactives-remove-direct-subplat-checks

Conversation

@chenba
Copy link
Copy Markdown
Contributor

@chenba chenba commented Jan 14, 2025

Because:

  • we want SubPlat to exclude account in the same fashion as other RPs, by providing a list
  • checking for a record in the accountCustomers table was not the correct way of checking for an active Stripe subscription

This commit:

  • remove the subscription part of the active status check in the scripts

@chenba chenba requested a review from a team as a code owner January 14, 2025 20:34
@chenba chenba force-pushed the fxa-inactives-remove-direct-subplat-checks branch from 35a289b to fd697a1 Compare January 14, 2025 21:16
@@ -157,10 +153,6 @@ const init = async () => {
Container.set(CurrencyHelper, currencyHelper);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the CurrencyHelper and StripeHelper still needed if not checking subscriptions?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to above questions. Also authFirestore references, do we use that for anything else?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for catching that. I will check. The thing with Container is that one cannot tell if something else in the dependency tree will reach in to grab something.

Copy link
Copy Markdown
Contributor

@LZoog LZoog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM besides maybe a couple of other things I think could be removed? OK with me if you want other eyes too from someone more involved in the inactive delete stuff. 😛

@@ -157,10 +153,6 @@ const init = async () => {
Container.set(CurrencyHelper, currencyHelper);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to above questions. Also authFirestore references, do we use that for anything else?

Because:
 - we want SubPlat to exclude account in the same fashion as other RPs,
   by providing a list
 - checking for a record in the `accountCustomers` table was not the
   correct way of checking for an active Stripe subscription

This commit:
 - remove the subscription part of the active status check in the
   scripts
@chenba chenba force-pushed the fxa-inactives-remove-direct-subplat-checks branch from fd697a1 to 61f67d9 Compare January 15, 2025 17:19
@chenba chenba merged commit 5c89a0e into main Jan 15, 2025
@chenba chenba deleted the fxa-inactives-remove-direct-subplat-checks branch January 15, 2025 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants