Merge pull request #4758 from Shopify/unify-extension-migration-code

Simplify code around migrating extensions
Shopify · Nov 4, 2024 · 31e46ef · 31e46ef
2 parents a5fcadc + 601dfb1
commit 31e46ef
Show file tree

Hide file tree

Showing 10 changed files with 140 additions and 498 deletions.
diff --git a/packages/app/src/cli/services/context/identifiers-extensions.test.ts b/packages/app/src/cli/services/context/identifiers-extensions.test.ts
@@ -21,13 +21,14 @@ import {
   testSingleWebhookSubscriptionExtension,
   testAppAccessConfigExtension,
 } from '../../models/app/app.test-data.js'
-import {getUIExtensionsToMigrate, migrateExtensionsToUIExtension} from '../dev/migrate-to-ui-extension.js'
+import {migrateExtensionsToUIExtension} from '../dev/migrate-to-ui-extension.js'
 import {OrganizationApp} from '../../models/organization.js'
 import {ExtensionInstance} from '../../models/extensions/extension-instance.js'
 import {DeveloperPlatformClient, Flag} from '../../utilities/developer-platform-client.js'
 import {ExtensionCreateSchema} from '../../api/graphql/extension_create.js'
 import appPOSSpec from '../../models/extensions/specifications/app_config_point_of_sale.js'
 import appWebhookSubscriptionSpec from '../../models/extensions/specifications/app_config_webhook_subscription.js'
+import {getModulesToMigrate} from '../dev/migrate-app-module.js'
 import {beforeEach, describe, expect, vi, test, beforeAll} from 'vitest'
 import {AbortSilentError} from '@shopify/cli-kit/node/error'
 import {setPathValue} from '@shopify/cli-kit/common/object'
@@ -180,7 +181,7 @@ vi.mock('./prompts', async () => {
 vi.mock('./id-matching')
 vi.mock('./id-manual-matching')
 vi.mock('../dev/migrate-to-ui-extension')
-
+vi.mock('../dev/migrate-app-module')
 beforeAll(async () => {
   EXTENSION_A = await testUIExtension({
     directory: 'EXTENSION_A',
@@ -291,7 +292,7 @@ beforeAll(async () => {
 })
 
 beforeEach(() => {
-  vi.mocked(getUIExtensionsToMigrate).mockReturnValue([])
+  vi.mocked(getModulesToMigrate).mockReturnValue([])
 })
 
 describe('matchmaking returns more remote sources than local', () => {
@@ -825,7 +826,7 @@ describe('ensureExtensionsIds: Migrates extension', () => {
       {local: EXTENSION_A, remote: REGISTRATION_A},
       {local: EXTENSION_A_2, remote: REGISTRATION_A_2},
     ]
-    vi.mocked(getUIExtensionsToMigrate).mockReturnValueOnce(extensionsToMigrate)
+    vi.mocked(getModulesToMigrate).mockReturnValueOnce(extensionsToMigrate)
 
     // When / Then
     await expect(() =>
@@ -853,7 +854,7 @@ describe('ensureExtensionsIds: Migrates extension', () => {
       {local: EXTENSION_A, remote: REGISTRATION_A},
       {local: EXTENSION_A_2, remote: REGISTRATION_A_2},
     ]
-    vi.mocked(getUIExtensionsToMigrate).mockReturnValueOnce(extensionsToMigrate)
+    vi.mocked(getModulesToMigrate).mockReturnValueOnce(extensionsToMigrate)
     vi.mocked(extensionMigrationPrompt).mockResolvedValueOnce(true)
     const opts = options([EXTENSION_A, EXTENSION_A_2])
     const remoteExtensions = [REGISTRATION_A, REGISTRATION_A_2]

diff --git a/packages/app/src/cli/services/context/identifiers-extensions.ts b/packages/app/src/cli/services/context/identifiers-extensions.ts
@@ -4,12 +4,18 @@ import {EnsureDeploymentIdsPresenceOptions, LocalSource, RemoteSource} from './i
 import {extensionMigrationPrompt, matchConfirmationPrompt} from './prompts.js'
 import {createExtension} from '../dev/create-extension.js'
 import {IdentifiersExtensions} from '../../models/app/identifiers.js'
-import {getUIExtensionsToMigrate, migrateExtensionsToUIExtension} from '../dev/migrate-to-ui-extension.js'
-import {getFlowExtensionsToMigrate, migrateFlowExtensions} from '../dev/migrate-flow-extension.js'
-import {getMarketingActivtyExtensionsToMigrate} from '../dev/migrate-marketing-activity-extension.js'
+import {migrateExtensionsToUIExtension} from '../dev/migrate-to-ui-extension.js'
+import {migrateFlowExtensions} from '../dev/migrate-flow-extension.js'
 import {AppInterface} from '../../models/app/app.js'
 import {DeveloperPlatformClient} from '../../utilities/developer-platform-client.js'
-import {getPaymentsExtensionsToMigrate, migrateAppModules} from '../dev/migrate-app-module.js'
+import {
+  FlowModulesMap,
+  getModulesToMigrate,
+  MarketingModulesMap,
+  migrateAppModules,
+  PaymentModulesMap,
+  UIModulesMap,
+} from '../dev/migrate-app-module.js'
 import {ExtensionSpecification} from '../../models/extensions/specification.js'
 import {ExtensionInstance} from '../../models/extensions/extension-instance.js'
 import {SingleWebhookSubscriptionType} from '../../models/extensions/specifications/app_config_webhook_schemas/webhooks_schema.js'
@@ -26,25 +32,17 @@ export async function ensureExtensionsIds(
   options: EnsureDeploymentIdsPresenceOptions,
   {
     extensionRegistrations: initialRemoteExtensions,
-    dashboardManagedExtensionRegistrations: dashboardOnlyExtensions,
+    dashboardManagedExtensionRegistrations: dashboardExtensions,
   }: AppWithExtensions,
 ) {
   let remoteExtensions = initialRemoteExtensions
-  const validIdentifiers = options.envIdentifiers.extensions ?? {}
+  const identifiers = options.envIdentifiers.extensions ?? {}
   const localExtensions = options.app.allExtensions.filter((ext) => ext.isUUIDStrategyExtension)
 
-  const uiExtensionsToMigrate = getUIExtensionsToMigrate(localExtensions, remoteExtensions, validIdentifiers)
-  const flowExtensionsToMigrate = getFlowExtensionsToMigrate(localExtensions, dashboardOnlyExtensions, validIdentifiers)
-  const marketingActivityExtensionsToMigrate = getMarketingActivtyExtensionsToMigrate(
-    localExtensions,
-    dashboardOnlyExtensions,
-    validIdentifiers,
-  )
-  const paymentsExtensionsToMigrate = getPaymentsExtensionsToMigrate(
-    localExtensions,
-    dashboardOnlyExtensions,
-    validIdentifiers,
-  )
+  const uiExtensionsToMigrate = getModulesToMigrate(localExtensions, remoteExtensions, identifiers, UIModulesMap)
+  const flowExtensionsToMigrate = getModulesToMigrate(localExtensions, dashboardExtensions, identifiers, FlowModulesMap)
+  const paymentsToMigrate = getModulesToMigrate(localExtensions, dashboardExtensions, identifiers, PaymentModulesMap)
+  const marketingToMigrate = getModulesToMigrate(localExtensions, dashboardExtensions, identifiers, MarketingModulesMap)
 
   if (uiExtensionsToMigrate.length > 0) {
     const confirmedMigration = await extensionMigrationPrompt(uiExtensionsToMigrate)
@@ -63,33 +61,33 @@ export async function ensureExtensionsIds(
     const newRemoteExtensions = await migrateFlowExtensions(
       flowExtensionsToMigrate,
       options.appId,
-      dashboardOnlyExtensions,
+      dashboardExtensions,
       options.developerPlatformClient,
     )
     remoteExtensions = remoteExtensions.concat(newRemoteExtensions)
   }
 
-  if (marketingActivityExtensionsToMigrate.length > 0) {
-    const confirmedMigration = await extensionMigrationPrompt(marketingActivityExtensionsToMigrate, false)
+  if (marketingToMigrate.length > 0) {
+    const confirmedMigration = await extensionMigrationPrompt(marketingToMigrate, false)
     if (!confirmedMigration) throw new AbortSilentError()
     const newRemoteExtensions = await migrateAppModules(
-      marketingActivityExtensionsToMigrate,
+      marketingToMigrate,
       options.appId,
       'marketing_activity',
-      dashboardOnlyExtensions,
+      dashboardExtensions,
       options.developerPlatformClient,
     )
     remoteExtensions = remoteExtensions.concat(newRemoteExtensions)
   }
 
-  if (paymentsExtensionsToMigrate.length > 0) {
-    const confirmedMigration = await extensionMigrationPrompt(paymentsExtensionsToMigrate, false)
+  if (paymentsToMigrate.length > 0) {
+    const confirmedMigration = await extensionMigrationPrompt(paymentsToMigrate, false)
     if (!confirmedMigration) throw new AbortSilentError()
     const newRemoteExtensions = await migrateAppModules(
-      paymentsExtensionsToMigrate,
+      paymentsToMigrate,
       options.appId,
       'payments_extension',
-      dashboardOnlyExtensions,
+      dashboardExtensions,
       options.developerPlatformClient,
     )
     remoteExtensions = remoteExtensions.concat(newRemoteExtensions)
@@ -98,7 +96,7 @@ export async function ensureExtensionsIds(
   const matchExtensions = await automaticMatchmaking(
     localExtensions,
     remoteExtensions,
-    validIdentifiers,
+    identifiers,
     options.developerPlatformClient,
   )
 
@@ -124,7 +122,7 @@ export async function ensureExtensionsIds(
   return {
     validMatches,
     extensionsToCreate,
-    dashboardOnlyExtensions,
+    dashboardOnlyExtensions: dashboardExtensions,
   }
 }
 

diff --git a/packages/app/src/cli/services/dev/migrate-app-module.test.ts b/packages/app/src/cli/services/dev/migrate-app-module.test.ts
@@ -1,11 +1,10 @@
-import {getPaymentsExtensionsToMigrate, migrateAppModules} from './migrate-app-module.js'
+import {getModulesToMigrate, migrateAppModules} from './migrate-app-module.js'
 import {LocalSource, RemoteSource} from '../context/identifiers.js'
 import {testDeveloperPlatformClient} from '../../models/app/app.test-data.js'
 import {describe, expect, test} from 'vitest'
 
 function getLocalExtension(attributes: Partial<LocalSource> = {}) {
   return {
-    type: 'payments_extension',
     localIdentifier: 'my-action',
     configuration: {
       name: 'my-action',
@@ -17,30 +16,55 @@ function getLocalExtension(attributes: Partial<LocalSource> = {}) {
 function getRemoteExtension(attributes: Partial<RemoteSource> = {}) {
   return {
     uuid: '1234',
-    type: 'payments_app_credit_card',
     title: 'a-different-extension',
     ...attributes,
   } as unknown as RemoteSource
 }
+const defaultMap = {
+  payments_extension: [
+    'payments_app',
+    'payments_app_credit_card',
+    'payments_app_custom_credit_card',
+    'payments_app_custom_onsite',
+    'payments_app_redeemable',
+  ],
+  marketing_activity: ['marketing_activity_extension'],
+}
 
-describe('getPaymentsExtensionsToMigrate()', () => {
-  const defaultIds = {
-    'my-action': '1234',
-  }
+const defaultIdentifiers = {
+  'module-A': '1234',
+  'module-B': '',
+}
 
-  test('matching my remote title and localIdentifier', () => {
+describe('getModulesToMigrate()', () => {
+  test('returns an empty array if no extensions are provided', () => {
+    const toMigrate = getModulesToMigrate([], [], {}, defaultMap)
+    expect(toMigrate).toStrictEqual([])
+  })
+
+  test('matching by remote title and localIdentifier, without defaultIdentifiers', () => {
     // Given
-    const localExtension = getLocalExtension({type: 'payments_extension', localIdentifier: 'my-action'})
-    const remoteExtension = getRemoteExtension({type: 'payments_app_credit_card', title: 'my-action', uuid: 'yy'})
+    const localExtension = getLocalExtension({type: 'payments_extension', localIdentifier: 'module-A'})
+    const localExtensionB = getLocalExtension({type: 'marketing_activity', localIdentifier: 'module-B'})
+    const remoteExtension = getRemoteExtension({type: 'payments_app_credit_card', title: 'module-A', uuid: 'yy'})
+    const remoteExtensionB = getRemoteExtension({type: 'marketing_activity_extension', title: 'module-B', uuid: 'xx'})
 
     // When
-    const toMigrate = getPaymentsExtensionsToMigrate([localExtension], [remoteExtension], defaultIds)
+    const toMigrate = getModulesToMigrate(
+      [localExtension, localExtensionB],
+      [remoteExtension, remoteExtensionB],
+      {},
+      defaultMap,
+    )
 
     // Then
-    expect(toMigrate).toStrictEqual([{local: localExtension, remote: remoteExtension}])
+    expect(toMigrate).toStrictEqual([
+      {local: localExtension, remote: remoteExtension},
+      {local: localExtensionB, remote: remoteExtensionB},
+    ])
   })
 
-  test('matching my remote title and localIdentifier by truncating the title', () => {
+  test('matching by truncated remote title and localIdentifier, without defaultIdentifiers', () => {
     // Given
     const localExtension = getLocalExtension({
       type: 'payments_extension',
@@ -49,62 +73,62 @@ describe('getPaymentsExtensionsToMigrate()', () => {
     const remoteExtension = getRemoteExtension({
       type: 'payments_app_credit_card',
       title: 'Ten Chars Ten Chars Ten Chars Ten Chars Ten 123456789',
-      uuid: 'yy',
     })
 
     // When
-    const toMigrate = getPaymentsExtensionsToMigrate([localExtension], [remoteExtension], defaultIds)
+    const toMigrate = getModulesToMigrate([localExtension], [remoteExtension], {}, defaultMap)
 
     // Then
     expect(toMigrate).toStrictEqual([{local: localExtension, remote: remoteExtension}])
   })
 
-  test('matching my local and remote IDs', () => {
+  test('matching different title and localIdentifier, using uuid', () => {
     // Given
-    const localExtension = getLocalExtension({type: 'payments_extension', localIdentifier: 'my-action'})
-    const remoteExtension = getRemoteExtension({type: 'payments_app_credit_card', title: 'remote', uuid: '1234'})
+    const localExtension = getLocalExtension({type: 'payments_extension', localIdentifier: 'module-A'})
+    const remoteExtension = getRemoteExtension({
+      type: 'payments_app_credit_card',
+      title: 'random-title',
+      uuid: defaultIdentifiers['module-A'],
+    })
 
     // When
-    const toMigrate = getPaymentsExtensionsToMigrate([localExtension], [remoteExtension], defaultIds)
+    const toMigrate = getModulesToMigrate([localExtension], [remoteExtension], defaultIdentifiers, defaultMap)
 
     // Then
     expect(toMigrate).toStrictEqual([{local: localExtension, remote: remoteExtension}])
   })
 
-  test('does not return extensions where local.type is not payments_extension', () => {
+  test('does not return modules where local.type is not included in defaultMap', () => {
     // Given
     const localExtension = getLocalExtension({type: 'checkout_ui_extension'})
     const remoteExtension = getRemoteExtension({type: 'payments_app_credit_card'})
 
     // When
-    const toMigrate = getPaymentsExtensionsToMigrate([localExtension], [remoteExtension], defaultIds)
+    const toMigrate = getModulesToMigrate([localExtension], [remoteExtension], defaultIdentifiers, defaultMap)
 
     // Then
     expect(toMigrate).toStrictEqual([])
   })
 
-  test('does not return extensions where remote.type is not payments_app_credit_card', () => {
+  test('does not return modules where remote.type is not included in defaultMap', () => {
     // Given
     const localExtension = getLocalExtension({type: 'payments_extension'})
-    const remoteExtension = getRemoteExtension({type: 'PRODUCT_SUBSCRIPTION_UI_EXTENSION'})
+    const remoteExtension = getRemoteExtension({type: 'marketing_activity_extension'})
 
     // When
-    const toMigrate = getPaymentsExtensionsToMigrate([localExtension], [remoteExtension], defaultIds)
+    const toMigrate = getModulesToMigrate([localExtension], [remoteExtension], defaultIdentifiers, defaultMap)
 
     // Then
     expect(toMigrate).toStrictEqual([])
   })
 
   test('if neither title/name or ids match, does not return any extensions', () => {
     // Given
-    const localExtension = getLocalExtension({type: 'payments_extension'})
-    const remoteExtension = getRemoteExtension({
-      type: 'payments_app_credit_card',
-      uuid: '5678',
-    })
+    const localExtension = getLocalExtension({type: 'payments_extension', localIdentifier: 'module-A'})
+    const remoteExtension = getRemoteExtension({type: 'payments_app_credit_card', title: 'doesnt-match', uuid: '0000'})
 
     // When
-    const toMigrate = getPaymentsExtensionsToMigrate([localExtension], [remoteExtension], defaultIds)
+    const toMigrate = getModulesToMigrate([localExtension], [remoteExtension], defaultIdentifiers, defaultMap)
 
     // Then
     expect(toMigrate).toStrictEqual([])