Merge pull request #3798 from Shopify/jmeng/devthemes

[Themes][Bugfix] - Fix unpublished themes being marked as development themes

.changeset/curvy-crabs-rule.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme': patch
+---
+
+Fix unpublished themes being marked as development themes

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')
@@ -48,64 +52,135 @@ describe('Push', () => {
       expect(execCLI2).not.toHaveBeenCalled()
       expect(push).toHaveBeenCalled()
     })
+  })
+
+  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('should pass theme selection flags to FindOrSelectTheme', async () => {
+    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})
+      expect(UnpublishedThemeManager.prototype.create).toHaveBeenCalled()
+    })
 
+    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})
+      expect(DevelopmentThemeManager.prototype.findOrCreate).toHaveBeenCalled()
     })
 
-    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)
+      const flags: ThemeSelectionOptions = {development: true, unpublished: true}
+
+      // When
+      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}
       vi.mocked(renderConfirmationPrompt).mockResolvedValue(true)
 
       // When
-      await runPushCommand([], path, adminSession)
+      const theme = await createOrSelectTheme(adminSession, flags)
 
       // Then
+      expect(theme?.role).toBe(LIVE_THEME_ROLE)
       expect(renderConfirmationPrompt).toHaveBeenCalled()
     })
 
-    test('should render text prompt if unpublished flag is provided and theme flag is not provided', async () => {
+    test("renders confirmation prompt if 'allow-live' flag is not provided and live theme is specified via theme flag", 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: ThemeSelectionOptions = {theme: '3'}
+      vi.mocked(findOrSelectTheme).mockResolvedValue(buildTheme({id: 3, name: 'Live Theme', role: LIVE_THEME_ROLE})!)
+      vi.mocked(renderConfirmationPrompt).mockResolvedValue(true)
 
       // When
-      await runPushCommand(['--unpublished'], path, adminSession)
+      const theme = await createOrSelectTheme(adminSession, flags)
+
+      // Then
+      expect(theme?.role).toBe(LIVE_THEME_ROLE)
+      expect(renderConfirmationPrompt).toHaveBeenCalled()
+    })
+
+    test('returns undefined if live theme confirmation prompt is not confirmed', async () => {
+      // Given
+      const flags: ThemeSelectionOptions = {live: true}
+      vi.mocked(renderConfirmationPrompt).mockResolvedValue(false)
+
+      // When
+      const theme = await createOrSelectTheme(adminSession, flags)
+
+      // Then
+      expect(theme).toBeUndefined()
+    })
+
+    test('renders text prompt if unpublished flag is provided and theme flag is not provided', async () => {
+      // Given
+      const flags = {unpublished: true}
+
+      // When
+      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,13 +7,14 @@ 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'
+import {LIVE_THEME_ROLE, Role, UNPUBLISHED_THEME_ROLE, promptThemeName} from '@shopify/cli-kit/node/themes/utils'
 import {cwd, resolvePath} from '@shopify/cli-kit/node/path'
 import {Theme} from '@shopify/cli-kit/node/themes/types'
 
@@ -149,31 +150,12 @@ export default class Push extends ThemeCommand {
       return
     }
 
-    const developmentThemeManager = new DevelopmentThemeManager(adminSession)
-
     if (!flags.stable && !flags.password) {
-      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,14 +198,55 @@ export default class Push extends ThemeCommand {
   }
 }
 
-async function confirmPushToLiveTheme(store: string) {
-  const message = `Push theme files to the published theme on ${store}?`
+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 {
+    const selectedTheme = await findOrSelectTheme(adminSession, {
+      header: 'Select a theme to push to:',
+      filter: {
+        live,
+        theme,
+      },
+    })
 
-  const options: RenderConfirmationPromptOptions = {
-    message,
-    confirmationMessage: 'Yes, confirm changes',
-    cancellationMessage: 'Cancel',
+    if (await confirmPushToTheme(selectedTheme.role as Role, flags['allow-live'], adminSession.storeFqdn)) {
+      return selectedTheme
+    }
   }
+}
+
+async function confirmPushToTheme(themeRole: Role, allowLive: boolean | undefined, storeFqdn: string) {
+  if (themeRole === LIVE_THEME_ROLE) {
+    if (allowLive) {
+      return true
+    }
 
-  return renderConfirmationPrompt(options)
+    const options: RenderConfirmationPromptOptions = {
+      message: `Push theme files to the ${themeRole} theme on ${storeFqdn}?`,
+      confirmationMessage: 'Yes, confirm changes',
+      cancellationMessage: 'Cancel',
+    }
+
+    return renderConfirmationPrompt(options)
+  }
+  return true
 }

packages/theme/src/cli/services/local-storage.ts

@@ -24,7 +24,7 @@ function themeLocalStorage() {
 function developmentThemeLocalStorage() {
   if (!_developmentThemeLocalStorageInstance) {
     _developmentThemeLocalStorageInstance = new LocalStorage<DevelopmentThemeLocalStorageSchema>({
-      projectName: 'shopify-cli-development-theme-conf',
+      projectName: 'shopify-cli-development-theme-config',
     })
   }
   return _developmentThemeLocalStorageInstance