Deploy multiple webhook subscription modules #3764

gracejychang · 2024-04-20T00:33:01Z

WHY are these changes introduced?

Fixes https://github.com/Shopify/develop-app-management/issues/1720 and https://github.com/Shopify/develop-app-management/issues/1724

See https://github.com/Shopify/develop-app-management/issues/1720 for more details and testing instructions

WHAT is this pull request doing?

How to test your changes?

Post-release steps

Measuring impact

How do we know this change was effective? Please choose one:

n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
Existing analytics will cater for this addition
PR includes analytics changes to measure impact

Checklist

I've considered possible cross-platform impacts (Mac, Linux, Windows)
I've considered possible documentation changes
I've made sure that any changes to dev or deploy have been reflected in the internal flowchart.

github-actions · 2024-04-20T00:33:17Z

We detected some changes at either packages/*/src or packages/cli-kit/assets/cli-ruby/** and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

packages/app/src/cli/models/app/loader.test.ts

github-actions · 2024-04-23T19:26:46Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	71.98% (+0.2% 🔼)	7010/9739
🟡	Branches	68.85% (+0.16% 🔼)	3457/5021
🟡	Functions	71.5% (+0.28% 🔼)	1884/2635
🟡	Lines	73.28% (+0.2% 🔼)	6608/9018

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / app.test-data.ts	93.06% (+0.25% 🔼)	93.83% (-0.98% 🔻)	83.56% (+0.46% 🔼)	92.5% (+0.24% 🔼)
🟢	`...` / breakdown-extensions.ts	96.49% (+0.06% 🔼)	82.76% (-0.57% 🔻)	100%	100%

Test suite run success

1641 tests passing in 766 suites.

Report generated by 🧪jest coverage report action from 017baff

gracejychang · 2024-05-01T00:01:42Z

packages/app/src/cli/models/extensions/specifications/app_config_webhook_subscription.ts

    if (topics)
      return topics.map((topic) => {
        return {api_version, uri, topic, ...optionalFields}
      })
  })

-  return webhookSubscriptions.length > 0 ? webhookSubscriptions : []
+  return webhookSubscriptions.length > 0 ? webhookSubscriptions.filter(Boolean) : []


fancy way of filtering out undefined values we get from subscription objects that might not have topics (ie. in the case of privacy compliance webhooks)

We have some utils for this kind of operations. To filter undefineds from an array, use getArrayRejectingUndefined :)

gracejychang · 2024-05-01T00:02:56Z

packages/app/src/cli/models/extensions/specifications/app_config_webhook_subscription.ts

@@ -50,14 +50,15 @@ function transformFromWebhookSubscriptionConfig(content: object) {
  const {api_version, subscriptions = []} = webhooks

  const webhookSubscriptions = subscriptions.flatMap((subscription) => {
-    const {uri, topics, ...optionalFields} = subscription
+    // compliance_topics gets handled by privacy_compliance_webhooks
+    const {uri, topics, compliance_topics: _, ...optionalFields} = subscription


needed compliance_topics: _ to prevent compliance_topics to get pushed as part of subscription in the webhook_subscription config, which gives us errors

karenxie · 2024-05-01T15:13:12Z

packages/app/src/cli/models/extensions/extension-instance.ts

@@ -150,7 +150,7 @@ export class ExtensionInstance<TConfiguration extends BaseConfigType = BaseConfi
  }

  isUuidManaged() {
-    return !this.isAppConfigExtension && !this.specification.extensionManagedInToml
+    return !this.isAppConfigExtension && this.specification.uidStrategy === 'uuid'


should this also be !this.specification.uidStrategy === 'dynamic'?

karenxie · 2024-05-01T15:13:46Z

packages/app/src/cli/services/context/identifiers-extensions.ts

@@ -274,7 +274,9 @@ async function ensureExtensionIdsForExtensionsManagedInToml(
  developerPlatformClient: DeveloperPlatformClient,
  appId: string,
 ) {
-  const extensionsManagedInToml = localExtensionRegistrations.filter((ext) => ext.specification.extensionManagedInToml)
+  const extensionsManagedInToml = localExtensionRegistrations.filter(


we should probably rename these vars as well

github-actions · 2024-05-01T20:29:27Z

Unused types (1)

Filename	types
packages/app/src/cli/models/extensions/specification.ts	`UidStrategy`

…dInToml, it's not needed since if there are no matches, a new extension + id + uuid is created

gracejychang · 2024-05-02T00:32:19Z

packages/app/src/cli/services/context/identifiers-extensions.ts

@@ -301,12 +303,19 @@ async function ensureExtensionIdsForExtensionsManagedInToml(
      localConfigArray?.forEach((localConfigObj: any) => {
        // eslint-disable-next-line @typescript-eslint/no-explicit-any
        const hasMatch = possibleMatches?.some((possibleMatch: any) => {
-          const remoteConfigString = possibleMatch.activeVersion?.config
-          const remoteConfigObj = remoteConfigString ? JSON.parse(remoteConfigString) : ''
+          const remoteActiveVersionConfig = possibleMatch.activeVersion?.config


since we were only checking against the activeVersion (which is null in cases when we add a new subscription then run dev), we were creating new webhook-subscription drafts, so added new checks for to check against the draftVersion too

gracejychang · 2024-05-02T00:36:55Z

packages/app/src/cli/services/context/identifiers-extensions.ts

  const configContent = await extension.commonDeployConfig('')
  return Array.isArray(configContent) ? configContent : [configContent]
 }

 function mapExtensionsIdsNonUuidManaged(extensionsIdsNonUuidManaged: {[key: string]: string[]}) {
  const result: {[key: string]: string} = {}
  for (const key in extensionsIdsNonUuidManaged) {
-    if (extensionsIdsNonUuidManaged[key]!.length > 0) {
+    if (extensionsIdsNonUuidManaged[key]!.length > 1) {
+      result[key] = extensionsIdsNonUuidManaged[key]!.toString()


kind of a hack :/ basically if there's more than 1 id we turn the entire array into a string (then we turn it back into an array later when we're updating the drafts 😬 )

there might be some other downstream effects that this causes

gracejychang · 2024-05-02T00:37:15Z

packages/app/src/cli/services/dev/update-extension.ts

+  stdout,
+  stderr,
+}: UpdateExtensionDraftOptions) {
+  const registrationIdArr = registrationId.split(',')


this is such a hack lol.. basically if there are multiple registration ids, the data shape is something like:

{ webhook-subscription: '1250,1251'}

(bc I just turned the entire array into a string in mapExtensionsIdsNonUuidManaged)
So the split here turns this back into an array

[1250, 1251]

I will modify the type here to be [key: string]: string[]. Non multiples extensions will only have one element inside the array

Anyway, I have a question here.

When we deploy the subscriptions, if the local content doesn't match the content of any remote module, then we create a new registrationId (I mean there's no update, the subscriptions are only created or deleted)

This updateDraft, is triggered by the dev command when the user modifies the content of the toml, and in this case we are reusing the registrationIds

gracejychang · 2024-05-02T00:38:16Z

packages/app/src/cli/services/dev/update-extension.ts

+
+  // eslint-disable-next-line @typescript-eslint/no-misused-promises
+  registrationIdArr.forEach(async (registrationId, index) => {
+    const config = configArr[index] as object


thoughts on this? 😬 feels a bit hacky /prone to bugs - just the quickest way I could get individual subscriptions stored in config in the draft versions of webhook-subscription

isaacroldan · 2024-05-02T09:03:11Z

packages/app/src/cli/services/context/identifiers-extensions.ts

+  if (
+    specificationIdentifier === WebhookSubscriptionSpecIdentifier &&
+    remoteConfigObj.topic === localConfig.topic &&
+    remoteConfigObj.uri === localConfig.uri
+  )
+    return true
+  return false


Suggested change

if (

specificationIdentifier === WebhookSubscriptionSpecIdentifier &&

remoteConfigObj.topic === localConfig.topic &&

remoteConfigObj.uri === localConfig.uri

)

return true

return false

return specificationIdentifier === WebhookSubscriptionSpecIdentifier &&

remoteConfigObj.topic === localConfig.topic &&

remoteConfigObj.uri === localConfig.uri

alvaro-shopify · 2024-05-03T10:52:24Z

packages/app/src/cli/models/app/app.ts

@@ -318,6 +318,7 @@ export class App implements AppInterface {
    return configuration as CurrentAppConfiguration & SpecsAppConfiguration
  }

+  // karen.xie: do we need to also filter the webhook_subscription modules here?
  private filterDeclarativeWebhooksConfig() {


If we don't want to manage extension from these specs:

privacy_compliance_webhooks

webhook_subscription

You should gate those specs behind the flag Flag.DeclarativeWebhooks in partners like this one

That way the final toml schema won't include them so the extensions are not loaded (in fact, you'll receive an parsing error in case the content is there an the beta is not enabled)

I thing that you could also remove fetching the Flag.DeclarativeWebhooks because you don't use it in elsewhere

alvaro-shopify · 2024-05-03T11:44:47Z

packages/app/src/cli/services/context/identifiers-extensions.ts

+          const remoteActiveVersionConfigObj = remoteActiveVersionConfig ? JSON.parse(remoteActiveVersionConfig) : ''
+          const remoteDraftVersionConfigObj = remoteDraftVersionConfig ? JSON.parse(remoteDraftVersionConfig) : ''
+          if (
+            matchesRemoteConfigForWebhookSubscriptions(


Why do we need an explicit matchesRemoteConfigForWebhookSubscriptions?, I mean we could just check the .config regardless the type of extension if they have the uidStrategy === 'dynamic' right?.

alvaro-shopify · 2024-05-03T11:52:03Z

packages/app/src/cli/services/dev/processes/draftable-extension.ts

@@ -40,8 +41,19 @@ export const pushUpdatesForDraftableExtensions: DevProcessFunction<DraftableExte
      await extension.build({app, stdout, stderr, useTasks: false, signal, environment: 'development'})
      const registrationId = remoteExtensions[extension.localIdentifier]
      if (!registrationId) throw new AbortError(`Extension ${extension.localIdentifier} not found on remote app.`)
-      // Initial draft update for each extension
-      await updateExtensionDraft({extension, developerPlatformClient, apiKey, registrationId, stdout, stderr})
+      if (extension.specification.externalIdentifier === WebhookSubscriptionSpecIdentifier) {


I wouldn't do this inside.

I will create a function that returns the list of final extension instances before the Promise.all

The extension instances that include multipleConfig, will return a copy of the ExtensionInstance for each item, and the only difference could be the config right?

This way you don't need to deal with multiple methods

Anyway this
if (extension.specification.externalIdentifier === WebhookSubscriptionSpecIdentifier) should be if (await multipleConfigs(extension)) right?

alvaro-shopify · 2024-05-03T12:12:22Z

packages/app/src/cli/services/dev/update-extension.ts

+  stdout,
+  stderr,
+}: UpdateExtensionDraftOptions) {
+  const registrationIdArr = registrationId.split(',')


I will modify the type here to be [key: string]: string[]. Non multiples extensions will only have one element inside the array

Anyway, I have a question here.

When we deploy the subscriptions, if the local content doesn't match the content of any remote module, then we create a new registrationId (I mean there's no update, the subscriptions are only created or deleted)

This updateDraft, is triggered by the dev command when the user modifies the content of the toml, and in this case we are reusing the registrationIds

gracejychang commented Apr 20, 2024

View reviewed changes

packages/app/src/cli/models/app/loader.test.ts Show resolved Hide resolved

packages/app/src/cli/models/app/loader.test.ts Show resolved Hide resolved

karenxie marked this pull request as draft April 22, 2024 19:42

karenxie force-pushed the deploy-multiple-modules branch from 31d8d7b to 0f7bb15 Compare April 23, 2024 19:22

karenxie force-pushed the deploy-multiple-modules branch 5 times, most recently from 53ef185 to e9bc505 Compare April 25, 2024 21:33

karenxie changed the title ~~Deploy multiple modules~~ Deploy multiple webhook subscription modules Apr 26, 2024

gracejychang commented May 1, 2024

View reviewed changes

karenxie reviewed May 1, 2024

View reviewed changes

gracejychang added 5 commits May 1, 2024 16:26

deploy multiple modules if extension in toml

6c56dfd

bundleConfig deploys multiple modules and add test for it

74d6955

wip: app test data, ID extensions type

ad6d5f8

loader and loader test

55e8883

matching uuid first pass (from prototype)

256b6bf

karenxie force-pushed the deploy-multiple-modules branch from 548764f to 0b58ba2 Compare May 1, 2024 20:28

karenxie and others added 7 commits May 1, 2024 17:33

Various refactorings, test additions, and test fixes

99298cd

get rid of if/else statement in ensureExtensionIdsForExtensionsManage…

dceec82

…dInToml, it's not needed since if there are no matches, a new extension + id + uuid is created

refactor/remove matchesRemoteeConfig

0620e18

remove extensionManagedInToml and replace with uidStrategy check

7b79526

fix privacy compliance webhooks bug

d048d85

Fix linter errors

89696dc

Do not deploy webhook-subscription modules if beta is not enabled

63432d0

karenxie force-pushed the deploy-multiple-modules branch from 017baff to 63432d0 Compare May 1, 2024 21:33

fix dev bug wip

2cd6bc2

gracejychang commented May 2, 2024

View reviewed changes

isaacroldan reviewed May 2, 2024

View reviewed changes

alvaro-shopify reviewed May 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deploy multiple webhook subscription modules #3764

Deploy multiple webhook subscription modules #3764

gracejychang commented Apr 20, 2024 •

edited by karenxie

github-actions bot commented Apr 20, 2024

github-actions bot commented Apr 23, 2024 •

edited

gracejychang May 1, 2024

isaacroldan May 2, 2024

gracejychang May 1, 2024

karenxie May 1, 2024

karenxie May 1, 2024

github-actions bot commented May 1, 2024

gracejychang May 2, 2024

gracejychang May 2, 2024

gracejychang May 2, 2024

alvaro-shopify May 3, 2024

gracejychang May 2, 2024 •

edited

isaacroldan May 2, 2024

alvaro-shopify May 3, 2024

alvaro-shopify May 3, 2024 •

edited

alvaro-shopify May 3, 2024

alvaro-shopify May 3, 2024

alvaro-shopify May 3, 2024

Deploy multiple webhook subscription modules #3764

Are you sure you want to change the base?

Deploy multiple webhook subscription modules #3764

Conversation

gracejychang commented Apr 20, 2024 • edited by karenxie

WHY are these changes introduced?

WHAT is this pull request doing?

How to test your changes?

Post-release steps

Measuring impact

Checklist

github-actions bot commented Apr 20, 2024

github-actions bot commented Apr 23, 2024 • edited

Coverage report

Test suite run success

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

github-actions bot commented May 1, 2024

Unused types (1)

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

gracejychang May 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvaro-shopify May 3, 2024 • edited

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

gracejychang commented Apr 20, 2024 •

edited by karenxie

github-actions bot commented Apr 23, 2024 •

edited

gracejychang May 2, 2024 •

edited

alvaro-shopify May 3, 2024 •

edited