From 8b9590a5d869e81d078ff39e181b8d6c31beab40 Mon Sep 17 00:00:00 2001 From: Gray Gilmore Date: Fri, 21 Nov 2025 16:55:57 -0800 Subject: [PATCH 1/2] Add missing ThemeManager tests --- .../public/node/themes/theme-manager.test.ts | 199 ++++++++++++++++++ 1 file changed, 199 insertions(+) create mode 100644 packages/cli-kit/src/public/node/themes/theme-manager.test.ts diff --git a/packages/cli-kit/src/public/node/themes/theme-manager.test.ts b/packages/cli-kit/src/public/node/themes/theme-manager.test.ts new file mode 100644 index 00000000000..46d7eedc5a7 --- /dev/null +++ b/packages/cli-kit/src/public/node/themes/theme-manager.test.ts @@ -0,0 +1,199 @@ +import {ThemeManager} from './theme-manager.js' +import {Theme} from './types.js' +import {fetchTheme, themeCreate} from './api.js' +import {DEVELOPMENT_THEME_ROLE, UNPUBLISHED_THEME_ROLE} from './utils.js' +import {BugError} from '../error.js' +import {test, describe, expect, vi, beforeEach} from 'vitest' + +vi.mock('./api.js') +vi.mock('../../../private/node/themes/generate-theme-name.js', () => ({ + generateThemeName: vi.fn((context: string) => `${context} (test-123-hostname)`), +})) + +const session = {token: 'token', storeFqdn: 'my-shop.myshopify.com', refresh: async () => {}} + +class TestThemeManager extends ThemeManager { + protected context = 'test-context' + private storedThemeId: string | undefined + + constructor(adminSession: any) { + super(adminSession) + this.storedThemeId = undefined + } + + getStoredThemeId(): string | undefined { + return this.storedThemeId + } + + setThemeId(themeId: string | undefined): void { + this.themeId = themeId + } + + protected setTheme(themeId: string): void { + this.storedThemeId = themeId + this.themeId = themeId + } + + protected removeTheme(): void { + this.storedThemeId = undefined + this.themeId = undefined + } +} + +const mockTheme: Theme = { + id: 123, + name: 'Test Theme', + role: DEVELOPMENT_THEME_ROLE, + processing: false, + createdAtRuntime: false, +} + +describe('ThemeManager', () => { + let manager: TestThemeManager + + beforeEach(() => { + manager = new TestThemeManager(session) + }) + + describe('findOrCreate', () => { + test('returns an existing theme when one exists', async () => { + // Given + manager.setThemeId('123') + vi.mocked(fetchTheme).mockResolvedValue(mockTheme) + + // When + const result = await manager.findOrCreate() + + // Then + expect(fetchTheme).toHaveBeenCalledWith(123, session) + expect(result).toEqual(mockTheme) + expect(themeCreate).not.toHaveBeenCalled() + }) + + test('creates a new theme when one does not exist', async () => { + // Given + manager.setThemeId(undefined) + vi.mocked(themeCreate).mockResolvedValue(mockTheme) + + expect(manager.getStoredThemeId()).toBeUndefined() + + // When + const result = await manager.findOrCreate() + + // Then + expect(fetchTheme).not.toHaveBeenCalled() + expect(themeCreate).toHaveBeenCalledWith( + { + name: 'test-context (test-123-hostname)', + role: DEVELOPMENT_THEME_ROLE, + }, + session, + ) + expect(result).toEqual(mockTheme) + expect(manager.getStoredThemeId()).toBe('123') + }) + }) + + describe('fetch', () => { + test('returns undefined when no themeId is set', async () => { + // Given + manager.setThemeId(undefined) + + // When + const result = await manager.fetch() + + // Then + expect(result).toBeUndefined() + expect(fetchTheme).not.toHaveBeenCalled() + }) + + test('fetches and returns a theme when themeId is set', async () => { + // Given + manager.setThemeId('123') + vi.mocked(fetchTheme).mockResolvedValue(mockTheme) + + // When + const result = await manager.fetch() + + // Then + expect(fetchTheme).toHaveBeenCalledWith(123, session) + expect(result).toEqual(mockTheme) + }) + + test('removes theme when fetch returns undefined', async () => { + // Given + manager.setThemeId('123') + vi.mocked(fetchTheme).mockResolvedValue(undefined) + + // When + const result = await manager.fetch() + + // Then + expect(fetchTheme).toHaveBeenCalledWith(123, session) + expect(result).toBeUndefined() + expect(manager.getStoredThemeId()).toBeUndefined() + }) + }) + + describe('generateThemeName', () => { + test('generates a theme name with the provided context', () => { + // When + const result = manager.generateThemeName('my-app') + + // Then + expect(result).toBe('my-app (test-123-hostname)') + }) + }) + + describe('create', () => { + test('creates a new theme with default role and generated name', async () => { + // Given + vi.mocked(themeCreate).mockResolvedValue(mockTheme) + + // When + const result = await manager.create() + + // Then + expect(themeCreate).toHaveBeenCalledWith( + { + name: 'test-context (test-123-hostname)', + role: DEVELOPMENT_THEME_ROLE, + }, + session, + ) + expect(result).toEqual(mockTheme) + expect(manager.getStoredThemeId()).toBe('123') + }) + + test('creates a new theme with specified role and name', async () => { + // Given + const customTheme = {...mockTheme, name: 'Custom name', role: UNPUBLISHED_THEME_ROLE} + vi.mocked(themeCreate).mockResolvedValue(customTheme) + + // When + const result = await manager.create(UNPUBLISHED_THEME_ROLE, 'Custom name') + + // Then + expect(themeCreate).toHaveBeenCalledWith( + { + name: 'Custom name', + role: UNPUBLISHED_THEME_ROLE, + }, + session, + ) + expect(result).toEqual(customTheme) + expect(manager.getStoredThemeId()).toBe('123') + }) + + test('throws BugError when theme creation fails', async () => { + // Given + vi.mocked(themeCreate).mockResolvedValue(undefined) + + // When/Then + await expect(manager.create()).rejects.toThrow(BugError) + await expect(manager.create()).rejects.toThrow( + 'Could not create theme with name "test-context (test-123-hostname)" and role "development"', + ) + }) + }) +}) From 367cc05ec84a639c08fd6e47cca8f8240c47983b Mon Sep 17 00:00:00 2001 From: Gray Gilmore Date: Fri, 21 Nov 2025 17:06:47 -0800 Subject: [PATCH 2/2] Add ability to set theme name on push --- .changeset/puny-plants-allow.md | 7 +++++ .../public/node/themes/theme-manager.test.ts | 22 ++++++++++++++ .../src/public/node/themes/theme-manager.ts | 6 ++-- packages/cli/README.md | 1 + packages/cli/oclif.manifest.json | 11 +++++++ packages/theme/src/cli/commands/theme/push.ts | 5 ++++ packages/theme/src/cli/services/push.test.ts | 29 +++++++++++++++++++ packages/theme/src/cli/services/push.ts | 11 ++++--- 8 files changed, 85 insertions(+), 7 deletions(-) create mode 100644 .changeset/puny-plants-allow.md diff --git a/.changeset/puny-plants-allow.md b/.changeset/puny-plants-allow.md new file mode 100644 index 00000000000..8ffea571694 --- /dev/null +++ b/.changeset/puny-plants-allow.md @@ -0,0 +1,7 @@ +--- +'@shopify/cli-kit': minor +'@shopify/theme': minor +'@shopify/cli': minor +--- + +Add `--name` flag to `theme push` diff --git a/packages/cli-kit/src/public/node/themes/theme-manager.test.ts b/packages/cli-kit/src/public/node/themes/theme-manager.test.ts index 46d7eedc5a7..1e9e1592f2b 100644 --- a/packages/cli-kit/src/public/node/themes/theme-manager.test.ts +++ b/packages/cli-kit/src/public/node/themes/theme-manager.test.ts @@ -92,6 +92,28 @@ describe('ThemeManager', () => { expect(result).toEqual(mockTheme) expect(manager.getStoredThemeId()).toBe('123') }) + + test('always creates a new theme when name is provided', async () => { + // Given + manager.setThemeId('123') + const customTheme = {...mockTheme, name: 'Custom name'} + vi.mocked(themeCreate).mockResolvedValue(customTheme) + + // When + const result = await manager.findOrCreate('Custom name') + + // Then + expect(fetchTheme).not.toHaveBeenCalled() + expect(themeCreate).toHaveBeenCalledWith( + { + name: 'Custom name', + role: DEVELOPMENT_THEME_ROLE, + }, + session, + ) + expect(result).toEqual(customTheme) + expect(manager.getStoredThemeId()).toBe('123') + }) }) describe('fetch', () => { diff --git a/packages/cli-kit/src/public/node/themes/theme-manager.ts b/packages/cli-kit/src/public/node/themes/theme-manager.ts index 2a40aa46912..9430a236495 100644 --- a/packages/cli-kit/src/public/node/themes/theme-manager.ts +++ b/packages/cli-kit/src/public/node/themes/theme-manager.ts @@ -13,10 +13,10 @@ export abstract class ThemeManager { constructor(protected adminSession: AdminSession) {} - async findOrCreate(): Promise { - let theme = await this.fetch() + async findOrCreate(name?: string): Promise { + let theme = name ? undefined : await this.fetch() if (!theme) { - theme = await this.create() + theme = await this.create(undefined, name) } return theme } diff --git a/packages/cli/README.md b/packages/cli/README.md index 41a104121be..f34cf2c7c48 100644 --- a/packages/cli/README.md +++ b/packages/cli/README.md @@ -2330,6 +2330,7 @@ FLAGS -u, --unpublished Create a new unpublished theme and push to it. -x, --ignore=... Skip uploading the specified files (Multiple flags allowed). Wrap the value in double quotes if you're using wildcards. + --name= The name for the theme. Will always create a new theme. --no-color Disable color output. --password= Password generated from the Theme Access app. --path= The path where you want to run the command. Defaults to the current working directory. diff --git a/packages/cli/oclif.manifest.json b/packages/cli/oclif.manifest.json index d8d00d63b12..68ee7fc85d1 100644 --- a/packages/cli/oclif.manifest.json +++ b/packages/cli/oclif.manifest.json @@ -6527,6 +6527,17 @@ "name": "live", "type": "boolean" }, + "name": { + "description": "The name for the theme. Will always create a new theme.", + "env": "SHOPIFY_FLAG_NAME", + "exclusive": [ + "theme" + ], + "hasDynamicHelp": false, + "multiple": false, + "name": "name", + "type": "option" + }, "no-color": { "allowNo": false, "description": "Disable color output.", diff --git a/packages/theme/src/cli/commands/theme/push.ts b/packages/theme/src/cli/commands/theme/push.ts index 598a7d8a4c5..045ebaf6d77 100644 --- a/packages/theme/src/cli/commands/theme/push.ts +++ b/packages/theme/src/cli/commands/theme/push.ts @@ -97,6 +97,11 @@ export default class Push extends ThemeCommand { description: 'Require theme check to pass without errors before pushing. Warnings are allowed.', env: 'SHOPIFY_FLAG_STRICT_PUSH', }), + name: Flags.string({ + description: 'The name for the theme. Will always create a new theme.', + env: 'SHOPIFY_FLAG_NAME', + exclusive: ['theme'], + }), environment: themeFlags.environment, } diff --git a/packages/theme/src/cli/services/push.test.ts b/packages/theme/src/cli/services/push.test.ts index 9cb40923822..f6dce2668ac 100644 --- a/packages/theme/src/cli/services/push.test.ts +++ b/packages/theme/src/cli/services/push.test.ts @@ -258,6 +258,21 @@ describe('createOrSelectTheme', async () => { expect(setDevelopmentTheme).not.toHaveBeenCalled() }) + test('creates unpublished theme when name flag is provided', async () => { + // Given + vi.mocked(themeCreate).mockResolvedValue(buildTheme({id: 2, name: 'Custom name', role: UNPUBLISHED_THEME_ROLE})) + vi.mocked(fetchTheme).mockResolvedValue(undefined) + + const flags: PushFlags = {name: 'Custom name'} + + // When + const theme = await createOrSelectTheme(adminSession, flags) + + // Then + expect(theme).toMatchObject({name: 'Custom name'}) + expect(setDevelopmentTheme).not.toHaveBeenCalled() + }) + test('creates development theme when development flag is provided', async () => { // Given vi.mocked(themeCreate).mockResolvedValue(buildTheme({id: 1, name: 'Theme', role: DEVELOPMENT_THEME_ROLE})) @@ -272,6 +287,20 @@ describe('createOrSelectTheme', async () => { expect(setDevelopmentTheme).toHaveBeenCalled() }) + test('creates development theme when development and name flags are provided', async () => { + // Given + vi.mocked(themeCreate).mockResolvedValue(buildTheme({id: 1, name: 'Custom name', role: DEVELOPMENT_THEME_ROLE})) + vi.mocked(fetchTheme).mockResolvedValue(undefined) + const flags: PushFlags = {development: true, name: 'Custom name'} + + // When + const theme = await createOrSelectTheme(adminSession, flags) + + // Then + expect(theme).toMatchObject({role: DEVELOPMENT_THEME_ROLE, name: 'Custom name'}) + expect(setDevelopmentTheme).toHaveBeenCalled() + }) + test('creates development theme when development and unpublished flags are provided', async () => { // Given vi.mocked(themeCreate).mockResolvedValue(buildTheme({id: 1, name: 'Theme', role: DEVELOPMENT_THEME_ROLE})) diff --git a/packages/theme/src/cli/services/push.ts b/packages/theme/src/cli/services/push.ts index 1015a5af0ac..2660b5f047d 100644 --- a/packages/theme/src/cli/services/push.ts +++ b/packages/theme/src/cli/services/push.ts @@ -108,6 +108,9 @@ export interface PushFlags { /** The environment to push the theme to. */ environment?: string[] + + /** The name of the theme. Will always create a new theme. */ + name?: string } /** @@ -366,13 +369,13 @@ export async function createOrSelectTheme( flags: PushFlags, multiEnvironment?: boolean, ): Promise { - const {live, development, unpublished, theme, environment} = flags + const {live, development, unpublished, theme, environment, name} = flags if (development) { const themeManager = new DevelopmentThemeManager(session) - return themeManager.findOrCreate() - } else if (unpublished) { - const themeName = theme ?? (await promptThemeName('Name of the new theme')) + return themeManager.findOrCreate(name) + } else if (unpublished ?? name) { + const themeName = name ?? theme ?? (await promptThemeName('Name of the new theme')) return themeCreate( { name: themeName,