From 1bbac7bf0e9d8b98978e31873dba8414bb5ca65d Mon Sep 17 00:00:00 2001 From: Luis Almeida Date: Thu, 21 May 2026 16:07:37 +0200 Subject: [PATCH 1/3] feat(themes): wire themes:migrate to the new endpoint contract Persist `partials/*` template entries to disk, render the new `migration_report` response section as a tight per-template list with green ticks and bold cyan identifiers, surface 5xx failures as the server's `general_error` suffixed with a link to file a GitHub issue, and restructure the command to mirror `themes:publish.ts` (lib stays log-free, command owns try/catch and `CliUx.ux.action`). --- .../src/commands/themes/migrate.ts | 16 ++- .../lib/handleThemeMigrationApiError.test.ts | 99 +++++++++++++++ .../src/lib/handleThemeMigrationApiError.ts | 27 ++++ packages/zcli-themes/src/lib/migrate.test.ts | 119 +++--------------- packages/zcli-themes/src/lib/migrate.ts | 67 ++++------ .../src/lib/migrationReportToString.test.ts | 116 +++++++++++++++++ .../src/lib/migrationReportToString.ts | 35 ++++++ .../src/lib/rewriteTemplates.test.ts | 25 ++++ packages/zcli-themes/src/types.ts | 25 ++++ .../tests/functional/migrate.test.ts | 79 +++++++++++- 10 files changed, 453 insertions(+), 155 deletions(-) create mode 100644 packages/zcli-themes/src/lib/handleThemeMigrationApiError.test.ts create mode 100644 packages/zcli-themes/src/lib/handleThemeMigrationApiError.ts create mode 100644 packages/zcli-themes/src/lib/migrationReportToString.test.ts create mode 100644 packages/zcli-themes/src/lib/migrationReportToString.ts diff --git a/packages/zcli-themes/src/commands/themes/migrate.ts b/packages/zcli-themes/src/commands/themes/migrate.ts index 55a649df..390acbed 100644 --- a/packages/zcli-themes/src/commands/themes/migrate.ts +++ b/packages/zcli-themes/src/commands/themes/migrate.ts @@ -1,6 +1,10 @@ -import { Command } from '@oclif/core' +import { Command, CliUx } from '@oclif/core' import * as path from 'path' +import * as chalk from 'chalk' +import type { AxiosError } from 'axios' import migrate from '../../lib/migrate' +import migrationReportToString from '../../lib/migrationReportToString' +import handleThemeMigrationApiError from '../../lib/handleThemeMigrationApiError' export default class Migrate extends Command { static description = 'migrate theme to the latest version of the templating api' @@ -21,6 +25,14 @@ export default class Migrate extends Command { const { argv: [themeDirectory] } = await this.parse(Migrate) const themePath = path.resolve(themeDirectory) - await migrate(themePath) + try { + CliUx.ux.action.start('Migrating theme') + const report = await migrate(themePath) + CliUx.ux.action.stop('Ok') + this.log(migrationReportToString(report)) + } catch (e) { + CliUx.ux.action.stop(chalk.bold.red('!')) + handleThemeMigrationApiError(e as AxiosError, themePath) + } } } diff --git a/packages/zcli-themes/src/lib/handleThemeMigrationApiError.test.ts b/packages/zcli-themes/src/lib/handleThemeMigrationApiError.test.ts new file mode 100644 index 00000000..5d0f304e --- /dev/null +++ b/packages/zcli-themes/src/lib/handleThemeMigrationApiError.test.ts @@ -0,0 +1,99 @@ +import * as sinon from 'sinon' +import { expect } from '@oclif/test' +import * as errors from '@oclif/core/lib/errors' +import handleThemeMigrationApiError from './handleThemeMigrationApiError' +import type { AxiosError } from 'axios' + +describe('handleThemeMigrationApiError', () => { + beforeEach(() => { + sinon.restore() + }) + + it('routes template_errors through handleTemplateError', () => { + const errorStub = sinon.stub(errors, 'error').callThrough() + const e = { + response: { + status: 400, + data: { + template_errors: { + home_page: [{ description: "'articles' does not exist", line: 10, column: 6, length: 7 }] + } + } + } + } as unknown as AxiosError + + try { + handleThemeMigrationApiError(e, 'theme/path') + } catch { + const [call] = errorStub.getCalls() + const [thrown] = call.args + expect(thrown).to.contain('theme/path/templates/home_page.hbs:10:6') + expect(thrown).to.contain("'articles' does not exist") + } + }) + + it('renders the server general_error suffixed with a plain issues URL on a 5xx', () => { + const errorStub = sinon.stub(errors, 'error').callThrough() + const e = { + response: { + status: 500, + data: { general_error: 'Failed to migrate the theme.' } + } + } as unknown as AxiosError + + try { + handleThemeMigrationApiError(e, 'theme/path') + } catch { + const [call] = errorStub.getCalls() + const thrown = call.args[0] as string + expect(thrown).to.match(/^Failed to migrate the theme\./) + expect(thrown).to.contain('Please open an issue at https://github.com/zendesk/zcli/issues/new') + expect(thrown).to.not.contain('themes-migrate') + expect(thrown).to.not.contain('## What happened') + } + }) + + it('throws the general_error verbatim on a 4xx', () => { + const errorStub = sinon.stub(errors, 'error').callThrough() + const e = { + response: { + status: 400, + data: { general_error: 'Theme is already on the latest supported version for migration' } + } + } as unknown as AxiosError + + try { + handleThemeMigrationApiError(e, 'theme/path') + } catch { + const [call] = errorStub.getCalls() + expect(call.args[0]).to.equal('Theme is already on the latest supported version for migration') + } + }) + + it('falls back to the axios message when no body fields are present', () => { + const errorStub = sinon.stub(errors, 'error').callThrough() + const e = { + response: { status: 400, data: {} }, + message: 'Network error' + } as unknown as AxiosError + + try { + handleThemeMigrationApiError(e, 'theme/path') + } catch { + const [call] = errorStub.getCalls() + expect(call.args[0]).to.equal('Network error') + } + }) + + it('throws the original axios error when there is no response', () => { + const errorStub = sinon.stub(errors, 'error').callThrough() + const e = new Error('Connection refused') as AxiosError + + try { + handleThemeMigrationApiError(e, 'theme/path') + } catch { + const [call] = errorStub.getCalls() + expect(call.args[0]).to.equal(e) + } + }) +}) diff --git a/packages/zcli-themes/src/lib/handleThemeMigrationApiError.ts b/packages/zcli-themes/src/lib/handleThemeMigrationApiError.ts new file mode 100644 index 00000000..6c7699b2 --- /dev/null +++ b/packages/zcli-themes/src/lib/handleThemeMigrationApiError.ts @@ -0,0 +1,27 @@ +import type { AxiosError } from 'axios' +import { error } from '@oclif/core/lib/errors' +import type { MigrateErrorBody } from '../types' +import parseAxiosError from './parseAxiosError' +import handleTemplateError from './handleTemplateError' + +export default function handleThemeMigrationApiError (e: AxiosError, themePath: string): never { + const { message, response } = parseAxiosError(e) + + if (!response) error(e) + + const status = response.status + const body = (response.data ?? {}) as MigrateErrorBody + const { template_errors: templateErrors, general_error: generalError } = body + + if (templateErrors) { + handleTemplateError(themePath, templateErrors) + } else if (status >= 500 && generalError) { + error(`${generalError} Please open an issue at https://github.com/zendesk/zcli/issues/new so we can investigate.`) + } else if (generalError) { + error(generalError) + } else { + error(message) + } + + throw new Error('unreachable') +} diff --git a/packages/zcli-themes/src/lib/migrate.test.ts b/packages/zcli-themes/src/lib/migrate.test.ts index 03a60f0a..5940ae5d 100644 --- a/packages/zcli-themes/src/lib/migrate.test.ts +++ b/packages/zcli-themes/src/lib/migrate.test.ts @@ -9,7 +9,6 @@ import * as rewriteTemplates from './rewriteTemplates' import * as rewriteAssets from './rewriteAssets' import * as axios from 'axios' import { request } from '@zendesk/zcli-core' -import * as errors from '@oclif/core/lib/errors' import migrate from './migrate' const manifest = { @@ -32,7 +31,7 @@ describe('migrate', () => { sinon.restore() }) - it('calls the migrations endpoint with the correct payload and rewrites files', async () => { + it('calls the migrations endpoint, rewrites files, and returns the migration_report', async () => { const getManifestStub = sinon.stub(getManifest, 'default') const getTemplatesStub = sinon.stub(getTemplates, 'default') const getVariablesStub = sinon.stub(getVariables, 'default') @@ -71,6 +70,12 @@ describe('migrate', () => { ] ]) + const migrationReport = { + home_page: [ + { target: 'logo_url', strategy: 'inline', description: 'desc', test_plan: 'test' } + ] + } + requestStub.returns( Promise.resolve({ status: 200, @@ -86,12 +91,13 @@ describe('migrate', () => { }, assets: { 'category_tree.js': Buffer.from('console.log("tree")').toString('base64') - } + }, + migration_report: migrationReport } }) as axios.AxiosPromise ) - await migrate('theme/path') + const result = await migrate('theme/path') expect( requestStub.calledWith( @@ -134,103 +140,11 @@ describe('migrate', () => { 'category_tree.js': Buffer.from('console.log("tree")').toString('base64') }) ).to.equal(true) - }) - - it('throws an error when validation fails with template errors', async () => { - sinon.stub(getManifest, 'default').returns(manifest) - sinon.stub(getTemplates, 'default').returns({}) - sinon.stub(getVariables, 'default').returns([]) - sinon.stub(getAssets, 'default').returns([]) - - sinon.stub(request, 'requestAPI').throws({ - response: { - data: { - template_errors: { - home_page: [ - { - description: "'articles' does not exist", - line: 10, - column: 6, - length: 7 - } - ], - footer: [ - { - description: "'alternative_locales' does not exist", - line: 6, - column: 12, - length: 10 - } - ] - } - } - } - }) - const errorStub = sinon.stub(errors, 'error').callThrough() - - try { - await migrate('theme/path') - } catch { - const [call] = errorStub.getCalls() - const [error] = call.args - expect(error).to.contain('theme/path/templates/home_page.hbs:10:6') - expect(error).to.contain("'articles' does not exist") - expect(error).to.contain('theme/path/templates/footer.hbs:6:12') - expect(error).to.contain("'alternative_locales' does not exist") - } + expect(result).to.deep.equal(migrationReport) }) - it('throws an error when there is a general error', async () => { - sinon.stub(getManifest, 'default').returns(manifest) - sinon.stub(getTemplates, 'default').returns({}) - sinon.stub(getVariables, 'default').returns([]) - sinon.stub(getAssets, 'default').returns([]) - - sinon.stub(request, 'requestAPI').throws({ - response: { - data: { - general_error: 'Something went wrong' - } - } - }) - - const errorStub = sinon.stub(errors, 'error').callThrough() - - try { - await migrate('theme/path') - } catch { - const [call] = errorStub.getCalls() - const [error] = call.args - expect(error).to.equal('Something went wrong') - } - }) - - it('throws an error when there is a response with a message', async () => { - sinon.stub(getManifest, 'default').returns(manifest) - sinon.stub(getTemplates, 'default').returns({}) - sinon.stub(getVariables, 'default').returns([]) - sinon.stub(getAssets, 'default').returns([]) - - sinon.stub(request, 'requestAPI').throws({ - response: { - data: {} - }, - message: 'Network error' - }) - - const errorStub = sinon.stub(errors, 'error').callThrough() - - try { - await migrate('theme/path') - } catch { - const [call] = errorStub.getCalls() - const [error] = call.args - expect(error).to.equal('Network error') - } - }) - - it('throws an error when there is no response', async () => { + it('propagates AxiosError on request failure', async () => { sinon.stub(getManifest, 'default').returns(manifest) sinon.stub(getTemplates, 'default').returns({}) sinon.stub(getVariables, 'default').returns([]) @@ -239,14 +153,11 @@ describe('migrate', () => { const axiosError = new Error('Connection refused') sinon.stub(request, 'requestAPI').throws(axiosError) - const errorStub = sinon.stub(errors, 'error').callThrough() - try { await migrate('theme/path') - } catch { - const [call] = errorStub.getCalls() - const [error] = call.args - expect(error).to.equal(axiosError) + throw new Error('Should have thrown') + } catch (e) { + expect(e).to.equal(axiosError) } }) }) diff --git a/packages/zcli-themes/src/lib/migrate.ts b/packages/zcli-themes/src/lib/migrate.ts index a73673d1..d6fd1232 100644 --- a/packages/zcli-themes/src/lib/migrate.ts +++ b/packages/zcli-themes/src/lib/migrate.ts @@ -1,20 +1,14 @@ -import type { ValidationErrors } from '../types' +import type { MigrateResponse, MigrationReport } from '../types' import getManifest from './getManifest' import getTemplates from './getTemplates' import getVariables from './getVariables' import getAssets from './getAssets' -import * as chalk from 'chalk' import { request } from '@zendesk/zcli-core' -import { error } from '@oclif/core/lib/errors' -import { CliUx } from '@oclif/core' -import type { AxiosError } from 'axios' import rewriteTemplates from './rewriteTemplates' import rewriteManifest from './rewriteManifest' import rewriteAssets from './rewriteAssets' -import handleTemplateError from './handleTemplateError' -import parseAxiosError from './parseAxiosError' -export default async function migrate (themePath: string): Promise { +export default async function migrate (themePath: string): Promise { const manifest = getManifest(themePath) const templates = getTemplates(themePath) const variables = getVariables(themePath, manifest.settings) @@ -32,42 +26,25 @@ export default async function migrate (themePath: string): Promise status === 200 - }) - rewriteManifest(themePath, data.metadata.api_version) - rewriteTemplates(themePath, data.templates) - rewriteAssets(themePath, data.assets) - CliUx.ux.action.stop('Ok') - } catch (e) { - CliUx.ux.action.stop(chalk.bold.red('!')) - const { message, response } = parseAxiosError(e as AxiosError) + const { data } = await request.requestAPI('/hc/api/internal/theming/migrations', { + method: 'POST', + headers: { + 'X-Zendesk-Request-Originator': 'zcli themes:migrate' + }, + data: { + templates: { + ...templates, + assets: assetsPayload, + variables: variablesPayload, + metadata: metadataPayload + } + }, + validateStatus: (status: number) => status === 200 + }) - if (response) { - const { template_errors: templateErrors, general_error: generalError } = - response.data as { - template_errors: ValidationErrors; - general_error: string; - } - if (templateErrors) handleTemplateError(themePath, templateErrors) - else if (generalError) error(generalError) - else error(message) - } else { - error(e as AxiosError) - } - } + const response = data as MigrateResponse + rewriteManifest(themePath, response.metadata.api_version) + rewriteTemplates(themePath, response.templates) + rewriteAssets(themePath, response.assets) + return response.migration_report } diff --git a/packages/zcli-themes/src/lib/migrationReportToString.test.ts b/packages/zcli-themes/src/lib/migrationReportToString.test.ts new file mode 100644 index 00000000..b7bf9bae --- /dev/null +++ b/packages/zcli-themes/src/lib/migrationReportToString.test.ts @@ -0,0 +1,116 @@ +import { expect } from '@oclif/test' +import migrationReportToString from './migrationReportToString' +import type { MigrationReport } from '../types' + +describe('migrationReportToString', () => { + it('renders a single-line "no migrations fired" message for an empty report', () => { + expect(migrationReportToString({})).to.contain('no migrations fired') + }) + + it('renders a single-line "no migrations fired" message for a missing report', () => { + expect(migrationReportToString(undefined)).to.contain('no migrations fired') + }) + + it('opens with the green success line when there are fired migrations', () => { + const report: MigrationReport = { + article_page: [ + { target: 'a', strategy: 'inline', description: 'd', test_plan: 't' } + ] + } + + expect(migrationReportToString(report)).to.contain('Theme migrated successfully') + }) + + it('groups entries by template identifier and sorts groups', () => { + const report: MigrationReport = { + header: [ + { target: 'user_info', strategy: 'partial', description: 'desc-A', test_plan: 'test-A' } + ], + article_page: [ + { target: 'section.internal', strategy: 'inline', description: 'desc-B', test_plan: 'test-B' } + ] + } + + const string = migrationReportToString(report) + + expect(string).to.contain('article_page') + expect(string).to.contain('header') + expect(string.indexOf('article_page')).to.be.lessThan(string.indexOf('header')) + }) + + it('preserves server order within a template group', () => { + const report: MigrationReport = { + article_page: [ + { target: 'section.internal', strategy: 'inline', description: 'first', test_plan: 't1' }, + { target: 'breadcrumbs', strategy: 'partial', description: 'second', test_plan: 't2' }, + { target: 'related_articles', strategy: 'partial', description: 'third', test_plan: 't3' } + ] + } + + const string = migrationReportToString(report) + + expect(string.indexOf('first')).to.be.lessThan(string.indexOf('second')) + expect(string.indexOf('second')).to.be.lessThan(string.indexOf('third')) + }) + + it('does not render strategy labels', () => { + const report: MigrationReport = { + doc: [ + { target: 'a', strategy: 'inline', description: 'd1', test_plan: 't1' }, + { target: 'b', strategy: 'partial', description: 'd2', test_plan: 't2' }, + { target: 'c', strategy: 'prefix', description: 'd3', test_plan: 't3' } + ] + } + + const string = migrationReportToString(report) + + expect(string).to.not.contain('inline') + expect(string).to.not.contain('partial') + expect(string).to.not.contain('prefix') + }) + + it('renders the description and the test plan without a "Test:" prefix', () => { + const report: MigrationReport = { + header: [ + { + target: 'user_info', + strategy: 'partial', + description: 'The `user_info` helper is rewritten as a partial.', + test_plan: 'Verify the dropdown still opens.' + } + ] + } + + const string = migrationReportToString(report) + + expect(string).to.contain('The `user_info` helper is rewritten as a partial.') + expect(string).to.contain('Verify the dropdown still opens.') + expect(string).to.not.contain('Test:') + }) + + it('falls back to a description-only line when target is null', () => { + const report: MigrationReport = { + doc: [ + { target: null, strategy: 'prefix', description: 'Bundled normalize.css.', test_plan: 'Verify CSS resets.' } + ] + } + + const string = migrationReportToString(report) + + expect(string).to.contain('Bundled normalize.css.') + expect(string).to.contain('Verify CSS resets.') + }) + + it('does not render the template file path on the subheader line', () => { + const report: MigrationReport = { + article_page: [ + { target: 'a', strategy: 'inline', description: 'd', test_plan: 't' } + ] + } + + const string = migrationReportToString(report) + + expect(string).to.not.contain('templates/article_page.hbs') + expect(string).to.not.contain('.hbs') + }) +}) diff --git a/packages/zcli-themes/src/lib/migrationReportToString.ts b/packages/zcli-themes/src/lib/migrationReportToString.ts new file mode 100644 index 00000000..d7ec26b6 --- /dev/null +++ b/packages/zcli-themes/src/lib/migrationReportToString.ts @@ -0,0 +1,35 @@ +import * as chalk from 'chalk' +import type { MigrationReport } from '../types' + +const successLine = chalk.green('Theme migrated successfully') + +export default function migrationReportToString (report: MigrationReport | undefined): string { + if (!report || Object.keys(report).length === 0) { + return `${successLine} (no migrations fired).` + } + + const lines: string[] = [successLine, ''] + + const identifiers = Object.keys(report).sort() + for (const identifier of identifiers) { + const entries = report[identifier] + if (!entries || entries.length === 0) continue + + lines.push(` ${chalk.green('✓')} ${chalk.cyan.bold(identifier)}`) + + for (const { target, description, test_plan: testPlan } of entries) { + if (target) { + lines.push(` • ${chalk.bold(target)} — ${description}`) + } else { + lines.push(` • ${description}`) + } + lines.push(` ${testPlan}`) + } + + lines.push('') + } + + if (lines[lines.length - 1] === '') lines.pop() + + return lines.join('\n') +} diff --git a/packages/zcli-themes/src/lib/rewriteTemplates.test.ts b/packages/zcli-themes/src/lib/rewriteTemplates.test.ts index 58b04d28..70576969 100644 --- a/packages/zcli-themes/src/lib/rewriteTemplates.test.ts +++ b/packages/zcli-themes/src/lib/rewriteTemplates.test.ts @@ -50,6 +50,31 @@ describe('rewriteTemplates', () => { }).to.throw('Failed to write template file: theme/path/templates/home_page.hbs') }) + it('writes partials to a partials/ subdirectory', () => { + const mkdirSyncStub = sinon.stub(fs, 'mkdirSync') + const writeFileSyncStub = sinon.stub(fs, 'writeFileSync') + + const templates = { + 'partials/user_info': '
{{user.name}}
', + 'partials/breadcrumbs': '' + } + + rewriteTemplates('theme/path', templates) + + expect(mkdirSyncStub.firstCall.args).to.deep.equal([ + 'theme/path/templates/partials', + { recursive: true } + ]) + expect(writeFileSyncStub.firstCall.args).to.deep.equal([ + 'theme/path/templates/partials/user_info.hbs', + '
{{user.name}}
' + ]) + expect(writeFileSyncStub.secondCall.args).to.deep.equal([ + 'theme/path/templates/partials/breadcrumbs.hbs', + '' + ]) + }) + it('handles empty templates object', () => { const writeFileSyncStub = sinon.stub(fs, 'writeFileSync') diff --git a/packages/zcli-themes/src/types.ts b/packages/zcli-themes/src/types.ts index 7d5e73d4..965bdf9f 100644 --- a/packages/zcli-themes/src/types.ts +++ b/packages/zcli-themes/src/types.ts @@ -34,6 +34,31 @@ export type ValidationErrors = { [path: `templates/${string}.hbs`]: ValidationError[] } +export type MigrationStrategy = 'inline' | 'partial' | 'prefix' + +export type MigrationReportEntry = { + target: string | null, + strategy: MigrationStrategy, + description: string, + test_plan: string +} + +export type MigrationReport = { + [identifier: string]: MigrationReportEntry[] +} + +export type MigrateResponse = { + templates: Record, + metadata: { api_version: number }, + assets: Record, + migration_report: MigrationReport +} + +export type MigrateErrorBody = { + template_errors?: ValidationErrors, + general_error?: string +} + export type Brand = { id: number, name: string, diff --git a/packages/zcli-themes/tests/functional/migrate.test.ts b/packages/zcli-themes/tests/functional/migrate.test.ts index 94a69897..a2c24829 100644 --- a/packages/zcli-themes/tests/functional/migrate.test.ts +++ b/packages/zcli-themes/tests/functional/migrate.test.ts @@ -12,6 +12,7 @@ describe('themes:migrate', function () { let manifestBackup: string let templateBackup: string const migratedAssetPath = path.join(baseThemePath, 'assets/category_tree.js') + const partialsDir = path.join(baseThemePath, 'templates/partials') beforeEach(() => { fetchStub = sinon.stub(global, 'fetch') @@ -38,6 +39,10 @@ describe('themes:migrate', function () { if (fs.existsSync(migratedAssetPath)) { fs.unlinkSync(migratedAssetPath) } + // Clean up partials/ directory created by the migration + if (fs.existsSync(partialsDir)) { + fs.rmSync(partialsDir, { recursive: true, force: true }) + } }) describe('successful migration', () => { @@ -56,13 +61,24 @@ describe('themes:migrate', function () { Promise.resolve( JSON.stringify({ metadata: { - api_version: 2 + api_version: 3 }, templates: { - document_head: '{{!chat (obsolete)}}' + document_head: '{{!chat (obsolete)}}', + 'partials/user_info': '
{{user.name}}
' }, assets: { 'category_tree.js': Buffer.from('console.log("category_tree");\n').toString('base64') + }, + migration_report: { + document_head: [ + { + target: 'chat', + strategy: 'inline', + description: 'The `chat` helper is no longer rendered.', + test_plan: 'Confirm no chat-related markup is rendered.' + } + ] } }) ) @@ -71,13 +87,13 @@ describe('themes:migrate', function () { success .stdout() - .it('should migrate theme successfully and update files', async () => { + .it('should migrate theme successfully and update files', async (ctx) => { await MigrateCommand.run([baseThemePath]) const manifest = JSON.parse( fs.readFileSync(path.join(baseThemePath, 'manifest.json'), 'utf8') ) - expect(manifest.api_version).to.equal(2) + expect(manifest.api_version).to.equal(3) // Verify template was updated const template = fs.readFileSync( @@ -86,9 +102,64 @@ describe('themes:migrate', function () { ) expect(template).to.contain('{{!chat (obsolete)}}') + // Verify partial was written under templates/partials/ + const partial = fs.readFileSync( + path.join(baseThemePath, 'templates/partials/user_info.hbs'), + 'utf8' + ) + expect(partial).to.contain('{{user.name}}') + // Verify asset was written const asset = fs.readFileSync(migratedAssetPath, 'utf8') expect(asset).to.equal('console.log("category_tree");\n') + + // Verify migration report was printed + expect(ctx.stdout).to.contain('Theme migrated successfully') + expect(ctx.stdout).to.contain('document_head') + expect(ctx.stdout).to.contain('chat') + expect(ctx.stdout).to.contain('Confirm no chat-related markup is rendered.') + }) + }) + + describe('migration with internal server error', () => { + test + .env(env) + .stderr() + .do(() => { + fetchStub + .withArgs( + sinon.match({ + url: 'https://z3ntest.zendesk.com/hc/api/internal/theming/migrations', + method: 'POST' + }) + ) + .resolves({ + status: 500, + ok: false, + text: () => + Promise.resolve( + JSON.stringify({ + general_error: 'Failed to migrate the theme.' + }) + ) + }) + }) + .it('should print a short failure message and a plain issues URL', async () => { + try { + await MigrateCommand.run([baseThemePath]) + throw new Error('Should have thrown an error') + } catch (error) { + if ( + error instanceof Error && + error.message === 'Should have thrown an error' + ) { + throw error + } + const message = (error as CLIError).message + expect(message).to.contain('Failed to migrate the theme') + expect(message).to.contain('https://github.com/zendesk/zcli/issues/new') + expect(message).to.not.contain('themes-migrate') + } }) }) From 2a86683d18cc1dc044e7ce78befc727c68395e41 Mon Sep 17 00:00:00 2001 From: Luis Almeida Date: Fri, 22 May 2026 14:25:23 +0200 Subject: [PATCH 2/3] refactor(themes): tighten migration error types and handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce a dedicated `TemplateErrors` type (identifier-keyed) so the migrations endpoint contract is represented honestly, instead of piggy-backing on the path-keyed `ValidationErrors`. Drop the dead `throw new Error('unreachable')` from `handleThemeMigrationApiError` and let the function infer its return type — `error()` from `@oclif/core` already throws unconditionally on the default path. --- packages/zcli-themes/src/lib/handleTemplateError.ts | 13 ++++++------- .../src/lib/handleThemeMigrationApiError.ts | 4 +--- packages/zcli-themes/src/lib/preview.ts | 4 ++-- packages/zcli-themes/src/types.ts | 5 ++++- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/packages/zcli-themes/src/lib/handleTemplateError.ts b/packages/zcli-themes/src/lib/handleTemplateError.ts index bf000dbe..97f875b8 100644 --- a/packages/zcli-themes/src/lib/handleTemplateError.ts +++ b/packages/zcli-themes/src/lib/handleTemplateError.ts @@ -1,15 +1,14 @@ -import { ValidationError, ValidationErrors } from '../types' +import { TemplateErrors, ValidationErrors } from '../types' import validationErrorsToString from './validationErrorsToString' import * as chalk from 'chalk' import { error } from '@oclif/core/lib/errors' -export default function handleTemplateError (themePath: string, templateErrors: ValidationErrors) { +export default function handleTemplateError (themePath: string, templateErrors: TemplateErrors) { + // The theming endpoints key these by template identifier (e.g. `home_page`). + // `validationErrorsToString` expects a path-keyed shape, so we convert here. const validationErrors: ValidationErrors = {} - for (const [template, errors] of Object.entries(templateErrors)) { - // the theming endpoints return the template identifier as the 'key' instead of - // the template path. We must fix this so we can reuse `validationErrorsToString` - // and align with the job import error handling - validationErrors[`templates/${template}.hbs`] = errors as ValidationError[] + for (const [identifier, errors] of Object.entries(templateErrors)) { + validationErrors[`templates/${identifier}.hbs`] = errors } const title = `${chalk.bold('InvalidTemplates')} - Template(s) with syntax error(s)` diff --git a/packages/zcli-themes/src/lib/handleThemeMigrationApiError.ts b/packages/zcli-themes/src/lib/handleThemeMigrationApiError.ts index 6c7699b2..b77cd3f6 100644 --- a/packages/zcli-themes/src/lib/handleThemeMigrationApiError.ts +++ b/packages/zcli-themes/src/lib/handleThemeMigrationApiError.ts @@ -4,7 +4,7 @@ import type { MigrateErrorBody } from '../types' import parseAxiosError from './parseAxiosError' import handleTemplateError from './handleTemplateError' -export default function handleThemeMigrationApiError (e: AxiosError, themePath: string): never { +export default function handleThemeMigrationApiError (e: AxiosError, themePath: string) { const { message, response } = parseAxiosError(e) if (!response) error(e) @@ -22,6 +22,4 @@ export default function handleThemeMigrationApiError (e: AxiosError, themePath: } else { error(message) } - - throw new Error('unreachable') } diff --git a/packages/zcli-themes/src/lib/preview.ts b/packages/zcli-themes/src/lib/preview.ts index aaedc407..30e1823c 100644 --- a/packages/zcli-themes/src/lib/preview.ts +++ b/packages/zcli-themes/src/lib/preview.ts @@ -1,4 +1,4 @@ -import type { Flags, ValidationErrors } from '../types' +import type { Flags, TemplateErrors } from '../types' import getManifest from './getManifest' import getTemplates from './getTemplates' import getVariables from './getVariables' @@ -65,7 +65,7 @@ export default async function preview (themePath: string, flags: Flags): Promise template_errors: templateErrors, general_error: generalError } = response.data as { - template_errors: ValidationErrors, + template_errors: TemplateErrors, general_error: string } if (templateErrors) handleTemplateError(themePath, templateErrors) diff --git a/packages/zcli-themes/src/types.ts b/packages/zcli-themes/src/types.ts index 965bdf9f..06bf42ae 100644 --- a/packages/zcli-themes/src/types.ts +++ b/packages/zcli-themes/src/types.ts @@ -34,6 +34,9 @@ export type ValidationErrors = { [path: `templates/${string}.hbs`]: ValidationError[] } +export type TemplateErrors = Record + + export type MigrationStrategy = 'inline' | 'partial' | 'prefix' export type MigrationReportEntry = { @@ -55,7 +58,7 @@ export type MigrateResponse = { } export type MigrateErrorBody = { - template_errors?: ValidationErrors, + template_errors?: TemplateErrors, general_error?: string } From c029b11ffe5b26e1a4a30732f9642bc393d62968 Mon Sep 17 00:00:00 2001 From: Luis Almeida Date: Fri, 22 May 2026 14:32:14 +0200 Subject: [PATCH 3/3] fix(themes): drop extra blank line in types.ts to satisfy eslint --- packages/zcli-themes/src/types.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/zcli-themes/src/types.ts b/packages/zcli-themes/src/types.ts index 06bf42ae..72d57d1b 100644 --- a/packages/zcli-themes/src/types.ts +++ b/packages/zcli-themes/src/types.ts @@ -36,7 +36,6 @@ export type ValidationErrors = { export type TemplateErrors = Record - export type MigrationStrategy = 'inline' | 'partial' | 'prefix' export type MigrationReportEntry = {