feat(subscriptions): guess a language tag from plan metadata #12666

chenba · 2022-04-27T13:32:28Z

Because:

we want to move localizable content strings from Stripe plan metadata
into a product configuration Firestore document dictionary keyed by a
language tag

This commit:

try to guess the language of a plan from its details with one or more
of Google Translate, the plan title, and the plan currency

Issue that this pull request solves

Closes: #12053

biancadanforth

Thanks Barry for this PR and for trying to address multiple aspects of how we detect and copy over locale information on these configs.

I tried this locally, and I got unexpected results. IIUC the Cloud Translate API is a public API, so I don't need any credentials to run the script locally. While the converter script executes successfully, the VPN and MDN Plus stage productConfig.locales are both {}, and the planConfig.locales all have the 'en' locale string, regardless of the language of the "product:details" on the Stripe Plan. Am I missing something?

I think part of the problem, at least for MDN, is that they have now added a bunch of plans with localized product details in other languages than English, so we probably want to check for the same language between product and plan config for any language and not assume English is the only case?

...erver/scripts/stripe-products-and-plans-to-firestore-documents/plan-language-tags-guesser.ts

biancadanforth · 2022-04-27T15:24:40Z

...erver/scripts/stripe-products-and-plans-to-firestore-documents/plan-language-tags-guesser.ts

+  // language, but with the special case of Switzerland (the only one at the
+  // time of writing), this is the sure way to detect for Swiss language
+  // tags.
+  const currencyToLocaleMap: { [key: string]: string } = { chf: 'CH' };


Maybe we can move this variable and comment to the top of the file? I see folks asking for GBP for example, which would be a unique way to identify English language tags. In general we may want to extend this list in the future if/when we add new currencies -- I guess if they occur prior to the final migration...

...ipts/stripe-products-and-plans-to-firestore-documents/stripe-products-and-plans-converter.ts

biancadanforth · 2022-04-27T16:12:58Z

packages/fxa-auth-server/test/scripts/stripe-products-and-plans-converter.js

@@ -320,7 +306,7 @@ describe('StripeProductsAndPlansConverter', () => {
        },
      };
      const expected = {};
-      const actual = converter.stripePlanLocalesToProductConfigLocales(
+      const actual = await converter.stripePlanLocalesToProductConfigLocales(
        planWithLocalizedData
      );
      assert.deepEqual(expected, actual);


We should probably add a test for the new, nested try..catch block?

biancadanforth · 2022-04-27T16:14:08Z

packages/fxa-auth-server/test/scripts/stripe-products-and-plans-converter.js

-      assert.isNull(actual);
-    });
-  });
-
  describe('stripePlanLocalesToProductConfigLocales', () => {


Should we add some test cases where the product config and plan config are in the same language, and therefore the locales goes on the planConfig instead of the productConfig?

Taking another look, and while I do see a new test case for 're-throws an error from Google Translation API', I don't see one for putting the localized, English strings on the planConfig?

...erver/scripts/stripe-products-and-plans-to-firestore-documents/plan-language-tags-guesser.ts

chenba · 2022-04-27T17:06:45Z

Thanks Barry for this PR and for trying to address multiple aspects of how we detect and copy over locale information on these configs.

I tried this locally, and I got unexpected results. IIUC the Cloud Translate API is a public API, so I don't need any credentials to run the script locally.

🤦 Sorry I meant to leave a comment regarding Google Translate. It's not public afaik; setup instruction is here: https://cloud.google.com/translate/docs/setup. But since you've already used GCP for other APIs (e.g. BigQuery) I think you just need to enable it.

I think part of the problem, at least for MDN, is that they have now added a bunch of plans with localized product details in other languages than English, so we probably want to check for the same language between product and plan config for any language and not assume English is the only case?

The product metadata should have just one language (at least that's how we've been encouraging people to set up their metadata), and that language should be English, which is why English is the special case. We can compare plans of the same detected language, but if they are different, there's no way to decide which get "promoted" to the product config.

I can write a script outside of this PR to see if that's something we need to handle. How does that sound? Comparing plans against each other post language detection seems like a bigger refactor. The script currently simply loop through the plans and make a decision for that single plan.

...erver/scripts/stripe-products-and-plans-to-firestore-documents/plan-language-tags-guesser.ts

biancadanforth · 2022-04-27T21:26:34Z

Sorry I meant to leave a comment regarding Google Translate. It's not public afaik; setup instruction is here: https://cloud.google.com/translate/docs/setup. But since you've already used GCP for other APIs (e.g. BigQuery) I think you just need to enable it.

Ah okay; let me try it again. I enabled it in GCP, and that's where it said it was a public API.

Also, if I didn't have the correct authn to use the API, I would expect to have hit some API errors, but the script completed successfully without throwing or logging any errors around this. Shouldn't it?

Edit: Okay sorry now I am seeing more expected behavior when I use GOOGLE_APPLICATION_CREDENTIALS. I still think there are some missing test cases and error handling here. Also I don't see any de locales in the resultant productConfig. Can you compare the results with the VPN stage product to see what is is captured and what's still missing?

biancadanforth · 2022-04-28T12:13:20Z

I can write a script outside of this PR to see if that's something we need to handle. How does that sound? Comparing plans against each other post language detection seems like a bigger refactor. The script currently simply loop through the plans and make a decision for that single plan.

Yeah maybe it isn't necessary. I wouldn't worry about it. I think it's more important to see how these new heuristics fare in copying over the data (e.g. from the VPN stage and MDN Plus stage products/plans), what is still left behind, and what to do about it.

chenba · 2022-04-28T14:51:45Z

Sorry I meant to leave a comment regarding Google Translate. It's not public afaik; setup instruction is here: https://cloud.google.com/translate/docs/setup. But since you've already used GCP for other APIs (e.g. BigQuery) I think you just need to enable it.

Ah okay; let me try it again. I enabled it in GCP, and that's where it said it was a public API. Also, if I didn't have the correct authn to use the API, I would expect to have hit some API errors, but the script completed successfully without throwing or logging any errors around this. Shouldn't it?

Hmm I don't remember that screen. It's entirely possible that I didn't pay attention. That's the first time I've seen it referred to as "public"; I'm not sure what that means. As long as I can remember, even their free API access would still require some sort of auth.

Looks like the problem is that the script continues to execute when the API key is missing.

Edit: Okay sorry now I am seeing more expected behavior when I use GOOGLE_APPLICATION_CREDENTIALS. I still think there are some missing test cases and error handling here. Also I don't see any de locales in the resultant productConfig. Can you compare the results with the VPN stage product to see what is is captured and what's still missing?

There is just no "de" plan for VPN on Stage apparently.

...ipts/stripe-products-and-plans-to-firestore-documents/stripe-products-and-plans-converter.ts

chenba · 2022-04-28T14:53:55Z

Added a new commit: b3028f7. For ease of review, I'll squash after approval.

chenba · 2022-04-28T14:55:21Z

There is just no "de" plan for VPN on Stage apparently.

There are a good number of EUR plans with no metadata at all on stage.

chenba · 2022-04-28T16:29:11Z

Here are the languages from production plans detected using the current code: de, de-CH, es, fr, fr-CH, it, it-CH, nl.

chenba · 2022-04-28T16:56:40Z

Looks like the problem is that the script continues to execute when the API key is missing.

One thing to note is that there is no missing API key error until the script encounters the first plan with its own product detail strings. The means during the course of running the script, you'd see some success message first.

I can add an explicit check for the google credentials env var also.

chenba · 2022-04-28T21:41:36Z

Added another commit (f0fbb1a) for re-throwing errors from Google Translation API and accepting a list of supported language. I think this is ready for a re-review.

biancadanforth

Thanks Barry; I think this looks pretty good now, though I suggested one more test case to add re: adding localized metadata to the planConfig instead of the productConfig in the convert method and one or two other minor suggestions.

Possible follow-up: How will this behave for tiered plans in the same, non-English locale? I'm guessing the localized data would be overwritten on the productConfig? I couldn't find an MDN plan for which this was the case, but it wasn't an exhaustive search.

biancadanforth · 2022-05-03T15:58:17Z

...erver/scripts/stripe-products-and-plans-to-firestore-documents/plan-language-tags-guesser.ts

+let locales: string[];
+
+const initLocales = (x: string[]) => {
+  locales = x;
+};


Not a blocker, but why not import supportedLanguages directly into this file instead of keeping it on the StripeProductsAndPlansConverter? I don't see that it's used anywhere in that class except to pass it into getLanguageTagFromPlanMetadata.

It's the minimal (least risky) change to support the supported languages, which would be the default list anyway, as your converter script is already importing it indirectly through the auth server configs. Being to able to set the list also make testing easier. And for a script that we hopefully will run only a few times, I didn't feel the need to update the tests because the supported languages is not a superset of the list used in the tests.

I do admit that it's not the most optimal code!

chenba · 2022-05-03T19:35:52Z

... one more test case to add re: adding localized metadata to the planConfig instead of the productConfig in the convert method ...

Unless I'm not understanding you correctly, this is what plan4 is for: https://github.com/mozilla/fxa/pull/12666/files#diff-819594cdeefeceffd955ecd631712aa1151dd0905b2d362ecba866fb48d7ddf5R423. The config object for that plan has its own en strings: https://github.com/mozilla/fxa/pull/12666/files#diff-819594cdeefeceffd955ecd631712aa1151dd0905b2d362ecba866fb48d7ddf5R432

Possible follow-up: How will this behave for tiered plans in the same, non-English locale? I'm guessing the localized data would be overwritten on the productConfig? I couldn't find an MDN plan for which this was the case, but it wasn't an exhaustive search.

Yes. And yes, there are currently no non-en plans that fit that scenario.

biancadanforth · 2022-05-03T19:46:50Z

Unless I'm not understanding you correctly, this is what plan4 is for: https://github.com/mozilla/fxa/pull/12666/files#diff-819594cdeefeceffd955ecd631712aa1151dd0905b2d362ecba866fb48d7ddf5R423. The config object for that plan has its own en strings: https://github.com/mozilla/fxa/pull/12666/files#diff-819594cdeefeceffd955ecd631712aa1151dd0905b2d362ecba866fb48d7ddf5R432

Oh yeah you're right sorry; I was looking for a new test case, and instead you rolled it into an existing test case. Thanks.

biancadanforth · 2022-05-03T19:54:21Z

Friendly reminder to update the deploy doc about enabling the Cloud Translate API in GCP for stage and prod.

Because: - we want to move localizable content strings from Stripe plan metadata into a product configuration Firestore document dictionary keyed by a language tag This commit: - try to guess the language of a plan from its details with one or more of Google Translate, the plan title, and the plan currency

chenba requested a review from biancadanforth April 27, 2022 13:32

chenba requested a review from a team as a code owner April 27, 2022 13:32

chenba force-pushed the FXA-4662-detect-plan-lang branch from 24f324c to 85f5b41 Compare April 27, 2022 13:37

biancadanforth self-assigned this Apr 27, 2022

biancadanforth suggested changes Apr 27, 2022

View reviewed changes

pdehaan reviewed Apr 27, 2022

View reviewed changes

...erver/scripts/stripe-products-and-plans-to-firestore-documents/plan-language-tags-guesser.ts Outdated Show resolved Hide resolved

chenba commented Apr 28, 2022

View reviewed changes

...ipts/stripe-products-and-plans-to-firestore-documents/stripe-products-and-plans-converter.ts Show resolved Hide resolved

chenba requested a review from biancadanforth April 28, 2022 21:41

biancadanforth approved these changes May 3, 2022

View reviewed changes

chenba force-pushed the FXA-4662-detect-plan-lang branch from f0fbb1a to fcd6d0f Compare May 4, 2022 13:09

chenba force-pushed the FXA-4662-detect-plan-lang branch from fcd6d0f to baf50fb Compare May 4, 2022 15:28

chenba merged commit fd5e7bd into main May 4, 2022

chenba deleted the FXA-4662-detect-plan-lang branch May 4, 2022 16:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(subscriptions): guess a language tag from plan metadata #12666

feat(subscriptions): guess a language tag from plan metadata #12666

chenba commented Apr 27, 2022

biancadanforth left a comment •

edited

Loading

biancadanforth Apr 27, 2022

biancadanforth Apr 27, 2022

biancadanforth Apr 27, 2022

biancadanforth May 3, 2022

chenba commented Apr 27, 2022

biancadanforth commented Apr 27, 2022 •

edited

Loading

biancadanforth commented Apr 28, 2022 •

edited

Loading

chenba commented Apr 28, 2022 •

edited

Loading

chenba commented Apr 28, 2022

chenba commented Apr 28, 2022

chenba commented Apr 28, 2022

chenba commented Apr 28, 2022

chenba commented Apr 28, 2022

biancadanforth left a comment

biancadanforth May 3, 2022

chenba May 3, 2022

chenba commented May 3, 2022

biancadanforth commented May 3, 2022

biancadanforth commented May 3, 2022

feat(subscriptions): guess a language tag from plan metadata #12666

feat(subscriptions): guess a language tag from plan metadata #12666

Conversation

chenba commented Apr 27, 2022

Issue that this pull request solves

biancadanforth left a comment • edited Loading

Choose a reason for hiding this comment

biancadanforth Apr 27, 2022

Choose a reason for hiding this comment

biancadanforth Apr 27, 2022

Choose a reason for hiding this comment

biancadanforth Apr 27, 2022

Choose a reason for hiding this comment

biancadanforth May 3, 2022

Choose a reason for hiding this comment

chenba commented Apr 27, 2022

biancadanforth commented Apr 27, 2022 • edited Loading

biancadanforth commented Apr 28, 2022 • edited Loading

chenba commented Apr 28, 2022 • edited Loading

chenba commented Apr 28, 2022

chenba commented Apr 28, 2022

chenba commented Apr 28, 2022

chenba commented Apr 28, 2022

chenba commented Apr 28, 2022

biancadanforth left a comment

Choose a reason for hiding this comment

biancadanforth May 3, 2022

Choose a reason for hiding this comment

chenba May 3, 2022

Choose a reason for hiding this comment

chenba commented May 3, 2022

biancadanforth commented May 3, 2022

biancadanforth commented May 3, 2022

biancadanforth left a comment •

edited

Loading

biancadanforth commented Apr 27, 2022 •

edited

Loading

biancadanforth commented Apr 28, 2022 •

edited

Loading

chenba commented Apr 28, 2022 •

edited

Loading