diff --git a/.changeset/curvy-crabs-rule.md b/.changeset/curvy-crabs-rule.md new file mode 100644 index 00000000000..0ebf2920f12 --- /dev/null +++ b/.changeset/curvy-crabs-rule.md @@ -0,0 +1,5 @@ +--- +'@shopify/theme': patch +--- + +Fix unpublished themes being marked as development themes diff --git a/packages/theme/src/cli/commands/theme/push.test.ts b/packages/theme/src/cli/commands/theme/push.test.ts index 15e263522d8..4258d0263c3 100644 --- a/packages/theme/src/cli/commands/theme/push.test.ts +++ b/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', () => { diff --git a/packages/theme/src/cli/commands/theme/push.ts b/packages/theme/src/cli/commands/theme/push.ts index 1e2c1ed0ed0..5a9d19a8add 100644 --- a/packages/theme/src/cli/commands/theme/push.ts +++ b/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 { + 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 } diff --git a/packages/theme/src/cli/services/local-storage.ts b/packages/theme/src/cli/services/local-storage.ts index dcaa0f0b3c6..93220a0f72a 100644 --- a/packages/theme/src/cli/services/local-storage.ts +++ b/packages/theme/src/cli/services/local-storage.ts @@ -24,7 +24,7 @@ function themeLocalStorage() { function developmentThemeLocalStorage() { if (!_developmentThemeLocalStorageInstance) { _developmentThemeLocalStorageInstance = new LocalStorage({ - projectName: 'shopify-cli-development-theme-conf', + projectName: 'shopify-cli-development-theme-config', }) } return _developmentThemeLocalStorageInstance diff --git a/packages/theme/src/cli/utilities/development-theme-manager.test.ts b/packages/theme/src/cli/utilities/development-theme-manager.test.ts index 929f97591c4..4fbc5279f08 100644 --- a/packages/theme/src/cli/utilities/development-theme-manager.test.ts +++ b/packages/theme/src/cli/utilities/development-theme-manager.test.ts @@ -8,6 +8,7 @@ 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} from '@shopify/cli-kit/node/themes/utils' vi.mock('@shopify/cli-kit/node/themes/api') vi.mock('../services/local-storage.js') @@ -50,14 +51,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() }) @@ -71,22 +80,47 @@ describe('DevelopmentThemeManager', () => { 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() }) }) + + 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) + }) + }) }) diff --git a/packages/theme/src/cli/utilities/unpublished-theme-manager.test.ts b/packages/theme/src/cli/utilities/unpublished-theme-manager.test.ts new file mode 100644 index 00000000000..55372121d21 --- /dev/null +++ b/packages/theme/src/cli/utilities/unpublished-theme-manager.test.ts @@ -0,0 +1,45 @@ +import {UnpublishedThemeManager} from './unpublished-theme-manager.js' +import {createTheme} from '@shopify/cli-kit/node/themes/api' +import {buildTheme} from '@shopify/cli-kit/node/themes/factories' +import {UNPUBLISHED_THEME_ROLE} from '@shopify/cli-kit/node/themes/utils' +import {beforeEach, describe, expect, test, vi} from 'vitest' + +vi.mock('@shopify/cli-kit/node/themes/api') + +describe('UnpublishedThemeManager', () => { + const storeFqdn = 'mystore.myshopify.com' + const token = 'token' + const newThemeId = 201 + + describe('create', () => { + beforeEach(() => { + vi.mocked(createTheme).mockImplementation(({name, role}) => + Promise.resolve( + buildTheme({ + id: newThemeId, + name: name!, + role: role!, + })!, + ), + ) + }) + + test('creates an unpublished theme by default', async () => { + // Given + const themeManager = buildThemeManager() + + // When + const theme = await themeManager.create() + + // Then + expect(theme.role).toBe(UNPUBLISHED_THEME_ROLE) + }) + }) + + function buildThemeManager(): UnpublishedThemeManager { + return new UnpublishedThemeManager({ + storeFqdn, + token, + }) + } +}) diff --git a/packages/theme/src/cli/utilities/unpublished-theme-manager.ts b/packages/theme/src/cli/utilities/unpublished-theme-manager.ts new file mode 100644 index 00000000000..cd1f41442c0 --- /dev/null +++ b/packages/theme/src/cli/utilities/unpublished-theme-manager.ts @@ -0,0 +1,24 @@ +import {AdminSession} from '@shopify/cli-kit/node/session' +import {ThemeManager} from '@shopify/cli-kit/node/themes/theme-manager' +import {Role, UNPUBLISHED_THEME_ROLE} from '@shopify/cli-kit/node/themes/utils' + +export class UnpublishedThemeManager extends ThemeManager { + protected context = 'Theme' + + constructor(adminSession: AdminSession) { + super(adminSession) + } + + async create(themeRole?: Role, themeName?: string) { + const role = themeRole || UNPUBLISHED_THEME_ROLE + return super.create(role, themeName) + } + + protected setTheme(themeId: string): void { + this.themeId = themeId + } + + protected removeTheme(): void { + this.themeId = undefined + } +}