[Theme] Add error handling when development theme role gets mismatched

packages/theme/src/cli/commands/theme/push.test.ts

@@ -1,18 +1,22 @@
-import Push from './push.js'
+import Push, {ThemeSelectionOptions, createOrSelectTheme} from './push.js'
 import {DevelopmentThemeManager} from '../../utilities/development-theme-manager.js'
+import {UnpublishedThemeManager} from '../../utilities/unpublished-theme-manager.js'
 import {ensureThemeStore} from '../../utilities/theme-store.js'
 import {findOrSelectTheme} from '../../utilities/theme-selector.js'
 import {push} from '../../services/push.js'
-import {describe, vi, expect, test} from 'vitest'
+import {FilterProps} from '../../utilities/theme-selector/filter.js'
+import {describe, vi, expect, test, beforeEach} from 'vitest'
 import {Config} from '@oclif/core'
 import {execCLI2} from '@shopify/cli-kit/node/ruby'
 import {AdminSession, ensureAuthenticatedThemes} from '@shopify/cli-kit/node/session'
 import {Theme} from '@shopify/cli-kit/node/themes/types'
 import {buildTheme} from '@shopify/cli-kit/node/themes/factories'
 import {renderConfirmationPrompt, renderTextPrompt} from '@shopify/cli-kit/node/ui'
+import {DEVELOPMENT_THEME_ROLE, LIVE_THEME_ROLE, UNPUBLISHED_THEME_ROLE} from '@shopify/cli-kit/node/themes/utils'
 
 vi.mock('../../services/push.js')
 vi.mock('../../utilities/development-theme-manager.js')
+vi.mock('../../utilities/unpublished-theme-manager.js')
 vi.mock('../../utilities/theme-store.js')
 vi.mock('../../utilities/theme-selector.js')
 vi.mock('@shopify/cli-kit/node/ruby')
@@ -37,64 +41,105 @@ describe('Push', () => {
       expect(execCLI2).not.toHaveBeenCalled()
       expect(push).toHaveBeenCalled()
     })
+  })
 
-    test('should pass theme selection flags to FindOrSelectTheme', async () => {
+  describe('createOrSelectTheme', async () => {
+    beforeEach(() => {
+      vi.mocked(DevelopmentThemeManager.prototype.findOrCreate).mockResolvedValue(
+        buildTheme({id: 1, name: 'Theme', role: DEVELOPMENT_THEME_ROLE})!,
+      )
+      vi.mocked(UnpublishedThemeManager.prototype.create).mockResolvedValue(
+        buildTheme({id: 2, name: 'Unpublished Theme', role: UNPUBLISHED_THEME_ROLE})!,
+      )
+      vi.mocked(findOrSelectTheme).mockImplementation(
+        async (_session: AdminSession, options: {header?: string; filter: FilterProps}) => {
+          if (options.filter.live) {
+            return buildTheme({id: 3, name: 'Live Theme', role: LIVE_THEME_ROLE})!
+          } else if (options.filter.theme) {
+            return buildTheme({id: 4, name: options.filter.theme, role: DEVELOPMENT_THEME_ROLE})!
+          } else {
+            return buildTheme({id: 5, name: 'Theme', role: DEVELOPMENT_THEME_ROLE})!
+          }
+        },
+      )
+    })
+
+    test('creates unpublished theme when unpublished flag is provided', async () => {
       // Given
-      const theme = buildTheme({id: 1, name: 'Theme', role: 'development'})!
-      vi.mocked(findOrSelectTheme).mockResolvedValue(theme)
+      const flags: ThemeSelectionOptions = {unpublished: true}
+      // When
+      const theme = await createOrSelectTheme(adminSession, flags)
+
+      // Then
+      expect(theme).toMatchObject({role: UNPUBLISHED_THEME_ROLE})
+    })
 
+    test('creates development theme when development flag is provided', async () => {
+      // Given
+      const flags: ThemeSelectionOptions = {development: true}
       // When
-      await runPushCommand(['--live', '--development', '-t', '1'], path, adminSession)
+      const theme = await createOrSelectTheme(adminSession, flags)
 
       // Then
-      expect(findOrSelectTheme).toHaveBeenCalledWith(adminSession, {
-        header: 'Select a theme to push to:',
-        filter: {
-          live: true,
-          development: true,
-          theme: '1',
-        },
-      })
+      expect(theme).toMatchObject({role: DEVELOPMENT_THEME_ROLE})
     })
 
-    test('should create an unpublished theme when the `unpublished` flag is provided', async () => {
+    test('returns live theme when live flag is provided', async () => {
       // Given
-      const theme = buildTheme({id: 1, name: 'Theme', role: 'development'})!
-      vi.mocked(DevelopmentThemeManager.prototype.create).mockResolvedValue(theme)
+      const flags: ThemeSelectionOptions = {live: true, 'allow-live': true}
 
       // When
-      await runPushCommand(['--unpublished', '--theme', 'test_theme'], path, adminSession)
+      const theme = await createOrSelectTheme(adminSession, flags)
 
       // Then
-      expect(DevelopmentThemeManager.prototype.create).toHaveBeenCalledWith('unpublished', 'test_theme')
-      expect(findOrSelectTheme).not.toHaveBeenCalled()
+      expect(theme).toMatchObject({role: LIVE_THEME_ROLE})
     })
 
-    test("should render confirmation prompt if 'allow-live' flag is not provided and selected theme role is live", async () => {
+    test('creates development theme when development and unpublished flags are provided', async () => {
       // Given
-      const theme = buildTheme({id: 1, name: 'Theme', role: 'live'})!
-      vi.mocked(findOrSelectTheme).mockResolvedValue(theme)
-      vi.mocked(renderConfirmationPrompt).mockResolvedValue(true)
+      const flags: ThemeSelectionOptions = {development: true, unpublished: true}
 
       // When
-      await runPushCommand([], path, adminSession)
+      const theme = await createOrSelectTheme(adminSession, flags)
+
+      // Then
+      expect(theme).toMatchObject({role: DEVELOPMENT_THEME_ROLE})
+    })
+
+    test("renders confirmation prompt if 'allow-live' flag is not provided and selected theme role is live", async () => {
+      // Given
+      const flags: ThemeSelectionOptions = {live: true}
+
+      // When
+      await createOrSelectTheme(adminSession, flags)
 
       // Then
       expect(renderConfirmationPrompt).toHaveBeenCalled()
     })
 
-    test('should render text prompt if unpublished flag is provided and theme flag is not provided', async () => {
+    test('renders text prompt if unpublished flag is provided and theme flag is not provided', async () => {
       // Given
-      const theme = buildTheme({id: 1, name: 'Theme', role: 'development'})!
-      vi.mocked(renderTextPrompt).mockResolvedValue('test_name')
-      vi.mocked(DevelopmentThemeManager.prototype.create).mockResolvedValue(theme)
+      const flags = {unpublished: true}
 
       // When
-      await runPushCommand(['--unpublished'], path, adminSession)
+      await createOrSelectTheme(adminSession, flags)
 
       // Then
       expect(renderTextPrompt).toHaveBeenCalled()
     })
+
+    test('returns undefined if confirmation prompt is rejected', async () => {
+      // Given
+      const flags = {live: true}
+
+      vi.mocked(renderConfirmationPrompt).mockResolvedValue(false)
+
+      // When
+      const theme = await createOrSelectTheme(adminSession, flags)
+
+      // Then
+      expect(theme).toBeUndefined()
+    })
   })
 
   describe('run with CLI 2 implementation', () => {

packages/theme/src/cli/commands/theme/push.ts

@@ -7,10 +7,11 @@ import {showEmbeddedCLIWarning} from '../../utilities/embedded-cli-warning.js'
 import {push} from '../../services/push.js'
 import {hasRequiredThemeDirectories} from '../../utilities/theme-fs.js'
 import {currentDirectoryConfirmed} from '../../utilities/theme-ui.js'
+import {UnpublishedThemeManager} from '../../utilities/unpublished-theme-manager.js'
 import {Flags} from '@oclif/core'
 import {globalFlags} from '@shopify/cli-kit/node/cli'
 import {execCLI2} from '@shopify/cli-kit/node/ruby'
-import {ensureAuthenticatedThemes} from '@shopify/cli-kit/node/session'
+import {AdminSession, ensureAuthenticatedThemes} from '@shopify/cli-kit/node/session'
 import {useEmbeddedThemeCLI} from '@shopify/cli-kit/node/context/local'
 import {RenderConfirmationPromptOptions, renderConfirmationPrompt} from '@shopify/cli-kit/node/ui'
 import {UNPUBLISHED_THEME_ROLE, promptThemeName} from '@shopify/cli-kit/node/themes/utils'
@@ -149,31 +150,12 @@ export default class Push extends ThemeCommand {
       return
     }
 
-    const developmentThemeManager = new DevelopmentThemeManager(adminSession)
-
     if (!flags.stable) {
-      const {live, development, unpublished, path, nodelete, theme, publish, json, force, ignore, only} = flags
-
-      let selectedTheme: Theme
-      if (unpublished) {
-        const themeName = theme || (await promptThemeName('Name of the new theme'))
-        selectedTheme = await developmentThemeManager.create(UNPUBLISHED_THEME_ROLE, themeName)
-      } else {
-        selectedTheme = await findOrSelectTheme(adminSession, {
-          header: 'Select a theme to push to:',
-          filter: {
-            live,
-            unpublished,
-            development,
-            theme,
-          },
-        })
-      }
+      const {path, nodelete, publish, json, force, ignore, only} = flags
 
-      if (selectedTheme.role === 'live' && !flags['allow-live']) {
-        if (!(await confirmPushToLiveTheme(adminSession.storeFqdn))) {
-          return
-        }
+      const selectedTheme: Theme | undefined = await createOrSelectTheme(adminSession, flags)
+      if (!selectedTheme) {
+        return
       }
 
       await push(selectedTheme, adminSession, {
@@ -191,6 +173,8 @@ export default class Push extends ThemeCommand {
 
     showEmbeddedCLIWarning()
 
+    const developmentThemeManager = new DevelopmentThemeManager(adminSession)
+
     const targetTheme = await (flags.development
       ? developmentThemeManager.findOrCreate()
       : developmentThemeManager.fetch())
@@ -214,6 +198,41 @@ export default class Push extends ThemeCommand {
   }
 }
 
+export interface ThemeSelectionOptions {
+  live?: boolean
+  development?: boolean
+  unpublished?: boolean
+  theme?: string
+  'allow-live'?: boolean
+}
+
+export async function createOrSelectTheme(
+  adminSession: AdminSession,
+  flags: ThemeSelectionOptions,
+): Promise<Theme | undefined> {
+  const {live, development, unpublished, theme} = flags
+
+  if (development) {
+    const themeManager = new DevelopmentThemeManager(adminSession)
+    return themeManager.findOrCreate()
+  } else if (unpublished) {
+    const themeName = theme || (await promptThemeName('Name of the new theme'))
+    const themeManager = new UnpublishedThemeManager(adminSession)
+    return themeManager.create(UNPUBLISHED_THEME_ROLE, themeName)
+  } else {
+    if (live && !flags['allow-live'] && !(await confirmPushToLiveTheme(adminSession.storeFqdn))) {
+      return
+    }
+    return findOrSelectTheme(adminSession, {
+      header: 'Select a theme to push to:',
+      filter: {
+        live,
+        theme,
+      },
+    })
+  }
+}
+
 async function confirmPushToLiveTheme(store: string) {
   const message = `Push theme files to the published theme on ${store}?`
 

packages/theme/src/cli/utilities/development-theme-manager.test.ts

@@ -2,12 +2,14 @@ import {
   DevelopmentThemeManager,
   NO_DEVELOPMENT_THEME_ID_SET,
   DEVELOPMENT_THEME_NOT_FOUND,
+  UNEXPECTED_ROLE_VALUE,
 } from './development-theme-manager.js'
 import {getDevelopmentTheme, setDevelopmentTheme, removeDevelopmentTheme} from '../services/local-storage.js'
 import {createTheme, fetchTheme} from '@shopify/cli-kit/node/themes/api'
 import {buildTheme} from '@shopify/cli-kit/node/themes/factories'
 import {beforeEach, describe, expect, vi, test} from 'vitest'
 import {Theme} from '@shopify/cli-kit/node/themes/types'
+import {DEVELOPMENT_THEME_ROLE, UNPUBLISHED_THEME_ROLE} from '@shopify/cli-kit/node/themes/utils'
 
 vi.mock('@shopify/cli-kit/node/themes/api')
 vi.mock('../services/local-storage.js')
@@ -19,7 +21,7 @@ describe('DevelopmentThemeManager', () => {
   const newThemeId = 201
   const onlyLocallyExistingId = 404
   const themeTestDatabase: {[id: number]: Theme | undefined} = {
-    [existingId]: {id: existingId} as Theme,
+    [existingId]: {id: existingId, role: DEVELOPMENT_THEME_ROLE} as Theme,
     [onlyLocallyExistingId]: undefined,
   }
   let localDevelopmentThemeId: string | undefined
@@ -39,6 +41,7 @@ describe('DevelopmentThemeManager', () => {
         })!,
       ),
     )
+    themeTestDatabase[existingId]!.role = DEVELOPMENT_THEME_ROLE
   })
 
   function buildDevelopmentThemeManager() {
@@ -50,14 +53,22 @@ describe('DevelopmentThemeManager', () => {
 
   describe('find', () => {
     test('should throw Abort if no ID is locally stored', async () => {
+      // Given
       localDevelopmentThemeId = undefined
+
+      // When
+      // Then
       await expect(() => buildDevelopmentThemeManager().find()).rejects.toThrowError(NO_DEVELOPMENT_THEME_ID_SET)
       expect(removeDevelopmentTheme).not.toHaveBeenCalled()
     })
 
     test('should remove locally stored ID and throw Abort if API could not return theme', async () => {
+      // Given
       const theme = onlyLocallyExistingId.toString()
       localDevelopmentThemeId = theme
+
+      // When
+      // Then
       await expect(() => buildDevelopmentThemeManager().find()).rejects.toThrowError(DEVELOPMENT_THEME_NOT_FOUND(theme))
       expect(removeDevelopmentTheme).toHaveBeenCalledOnce()
     })
@@ -67,26 +78,77 @@ describe('DevelopmentThemeManager', () => {
       localDevelopmentThemeId = theme
       await expect(buildDevelopmentThemeManager().find()).resolves.toEqual(themeTestDatabase[existingId])
     })
+
+    test('should remove locally stored ID and throw Abort if API returns theme with unexpected role', async () => {
+      // Given
+      themeTestDatabase[existingId]!.role = UNPUBLISHED_THEME_ROLE
+      const theme = existingId.toString()
+      localDevelopmentThemeId = theme
+      const developmentThemeManager = buildDevelopmentThemeManager()
+
+      // When
+      // Then
+      await expect(() => developmentThemeManager.find()).rejects.toThrowError(UNEXPECTED_ROLE_VALUE)
+      expect(removeDevelopmentTheme).toHaveBeenCalledOnce()
+    })
   })
 
   describe('findOrCreate', () => {
     test('should not create a new development theme if API returns theme with locally stored ID', async () => {
+      // Given
       const theme = existingId.toString()
       localDevelopmentThemeId = theme
+
+      // When
+      // Then
       expect((await buildDevelopmentThemeManager().findOrCreate()).id.toString()).toEqual(theme)
     })
 
     test('should create a new development theme if no ID is locally stored', async () => {
+      // Given
       localDevelopmentThemeId = undefined
+
+      // When
+      // Then
       expect((await buildDevelopmentThemeManager().findOrCreate()).id.toString()).toEqual(newThemeId.toString())
       expect(removeDevelopmentTheme).not.toHaveBeenCalled()
     })
 
     test('should create a new development theme if locally existing ID points to nowhere', async () => {
+      // Given
       const theme = onlyLocallyExistingId.toString()
       localDevelopmentThemeId = theme
+
+      // When
+      // Then
       expect((await buildDevelopmentThemeManager().findOrCreate()).id.toString()).toEqual(newThemeId.toString())
       expect(removeDevelopmentTheme).toHaveBeenCalledOnce()
     })
+
+    test('should remove locally stored ID and throw Abort if API returns theme with unexpected role', async () => {
+      // Given
+      themeTestDatabase[existingId]!.role = UNPUBLISHED_THEME_ROLE
+      const theme = existingId.toString()
+      localDevelopmentThemeId = theme
+      const developmentThemeManager = buildDevelopmentThemeManager()
+
+      // When
+      // Then
+      await expect(() => developmentThemeManager.findOrCreate()).rejects.toThrowError(UNEXPECTED_ROLE_VALUE)
+      expect(removeDevelopmentTheme).toHaveBeenCalledOnce()
+    })
+  })
+
+  describe('create', () => {
+    test('should create a new development theme', async () => {
+      // Given
+      const developmentThemeManager = buildDevelopmentThemeManager()
+
+      // When
+      const theme = await developmentThemeManager.create()
+
+      // Then
+      expect(theme.role).toBe(DEVELOPMENT_THEME_ROLE)
+    })
   })
 })