Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use graphQL to delete theme files #4799

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
/* eslint-disable @typescript-eslint/consistent-type-definitions */
import * as Types from './types.js'

import {TypedDocumentNode as DocumentNode} from '@graphql-typed-document-node/core'

export type ThemeFilesDeleteMutationVariables = Types.Exact<{
themeId: Types.Scalars['ID']['input']
files: Types.Scalars['String']['input'][] | Types.Scalars['String']['input']
}>

export type ThemeFilesDeleteMutation = {
themeFilesDelete?: {
deletedThemeFiles?: {filename: string}[] | null
userErrors: {
filename?: string | null
code?: Types.OnlineStoreThemeFilesUserErrorsCode | null
message: string
}[]
} | null
}

export const ThemeFilesDelete = {
kind: 'Document',
definitions: [
{
kind: 'OperationDefinition',
operation: 'mutation',
name: {kind: 'Name', value: 'themeFilesDelete'},
variableDefinitions: [
{
kind: 'VariableDefinition',
variable: {kind: 'Variable', name: {kind: 'Name', value: 'themeId'}},
type: {kind: 'NonNullType', type: {kind: 'NamedType', name: {kind: 'Name', value: 'ID'}}},
},
{
kind: 'VariableDefinition',
variable: {kind: 'Variable', name: {kind: 'Name', value: 'files'}},
type: {
kind: 'NonNullType',
type: {
kind: 'ListType',
type: {kind: 'NonNullType', type: {kind: 'NamedType', name: {kind: 'Name', value: 'String'}}},
},
},
},
],
selectionSet: {
kind: 'SelectionSet',
selections: [
{
kind: 'Field',
name: {kind: 'Name', value: 'themeFilesDelete'},
arguments: [
{
kind: 'Argument',
name: {kind: 'Name', value: 'themeId'},
value: {kind: 'Variable', name: {kind: 'Name', value: 'themeId'}},
},
{
kind: 'Argument',
name: {kind: 'Name', value: 'files'},
value: {kind: 'Variable', name: {kind: 'Name', value: 'files'}},
},
],
selectionSet: {
kind: 'SelectionSet',
selections: [
{
kind: 'Field',
name: {kind: 'Name', value: 'deletedThemeFiles'},
selectionSet: {
kind: 'SelectionSet',
selections: [
{kind: 'Field', name: {kind: 'Name', value: 'filename'}},
{kind: 'Field', name: {kind: 'Name', value: '__typename'}},
],
},
},
{
kind: 'Field',
name: {kind: 'Name', value: 'userErrors'},
selectionSet: {
kind: 'SelectionSet',
selections: [
{kind: 'Field', name: {kind: 'Name', value: 'filename'}},
{kind: 'Field', name: {kind: 'Name', value: 'code'}},
{kind: 'Field', name: {kind: 'Name', value: 'message'}},
{kind: 'Field', name: {kind: 'Name', value: '__typename'}},
],
},
},
{kind: 'Field', name: {kind: 'Name', value: '__typename'}},
],
},
},
],
},
},
],
} as unknown as DocumentNode<ThemeFilesDeleteMutation, ThemeFilesDeleteMutationVariables>
19 changes: 19 additions & 0 deletions packages/cli-kit/src/cli/api/graphql/admin/generated/types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,25 @@ export type OnlineStoreThemeFilesUpsertFileInput = {
filename: Scalars['String']['input']
}

/** Possible error codes that can be returned by `OnlineStoreThemeFilesUserErrors`. */
export type OnlineStoreThemeFilesUserErrorsCode =
/** Access denied. */
| 'ACCESS_DENIED'
/** There are files with the same filename. */
| 'DUPLICATE_FILE_INPUT'
/** Error. */
| 'ERROR'
/** The file is invalid. */
| 'FILE_VALIDATION_ERROR'
/** The input value should be less than or equal to the maximum value allowed. */
| 'LESS_THAN_OR_EQUAL_TO'
/** The record with the ID used as the input value couldn't be found. */
| 'NOT_FOUND'
/** There are theme files with conflicts. */
| 'THEME_FILES_CONFLICT'
/** This action is not available on your current plan. Please upgrade to access theme editing features. */
| 'THEME_LIMITED_PLAN'

/** The input fields for Theme attributes to update. */
export type OnlineStoreThemeInput = {
/** The new name of the theme. */
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
mutation themeFilesDelete($themeId: ID!, $files: [String!]!) {
themeFilesDelete(themeId: $themeId, files: $files) {
deletedThemeFiles {
filename
}
userErrors {
filename
code
message
}
}
}
62 changes: 25 additions & 37 deletions packages/cli-kit/src/public/node/themes/api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,15 @@ import {
fetchChecksums,
bulkUploadThemeAssets,
AssetParams,
deleteThemeAsset,
deleteThemeAssets,
} from './api.js'
import {Operation} from './types.js'
import {ThemeDelete} from '../../../cli/api/graphql/admin/generated/theme_delete.js'
import {ThemeUpdate} from '../../../cli/api/graphql/admin/generated/theme_update.js'
import {ThemePublish} from '../../../cli/api/graphql/admin/generated/theme_publish.js'
import {GetThemeFileChecksums} from '../../../cli/api/graphql/admin/generated/get_theme_file_checksums.js'
import {ThemeFilesUpsert} from '../../../cli/api/graphql/admin/generated/theme_files_upsert.js'
import {ThemeFilesDelete} from '../../../cli/api/graphql/admin/generated/theme_files_delete.js'
import {OnlineStoreThemeFileBodyInputType} from '../../../cli/api/graphql/admin/generated/types.js'
import {GetThemes} from '../../../cli/api/graphql/admin/generated/get_themes.js'
import {GetTheme} from '../../../cli/api/graphql/admin/generated/get_theme.js'
Expand Down Expand Up @@ -318,63 +319,51 @@ describe('themePublish', () => {
}
})

describe('deleteThemeAsset', () => {
describe('deleteThemeAssets', () => {
test('deletes a theme asset', async () => {
// Given
const id = 123
const key = 'snippets/product-variant-picker.liquid'

vi.mocked(restRequest).mockResolvedValue({
json: {message: 'snippets/product-variant-picker.liquid was succesfully deleted'},
status: 200,
headers: {},
vi.mocked(adminRequestDoc).mockResolvedValue({
themeFilesDelete: {
deletedThemeFiles: [{filename: key}],
userErrors: [],
},
})

// When
const output = await deleteThemeAsset(id, key, session)
const output = await deleteThemeAssets(id, [key], session)

// Then
expect(restRequest).toHaveBeenCalledWith('DELETE', `/themes/${id}/assets`, session, undefined, {'asset[key]': key})
expect(output).toBe(true)
expect(adminRequestDoc).toHaveBeenCalledWith(ThemeFilesDelete, session, {
themeId: `gid://shopify/OnlineStoreTheme/${id}`,
files: [key],
})
expect(output).toEqual([{key, success: true, operation: 'DELETE'}])
})

test('returns true when attemping to delete an nonexistent asset', async () => {
test('returns success when attempting to delete nonexistent assets', async () => {
// Given
const id = 123
const key = 'snippets/product-variant-picker.liquid'

vi.mocked(restRequest).mockResolvedValue({
json: {},
status: 200,
headers: {},
vi.mocked(adminRequestDoc).mockResolvedValue({
themeFilesDelete: {
deletedThemeFiles: [{filename: key}],
userErrors: [],
},
})

// When
const output = await deleteThemeAsset(id, key, session)
const output = await deleteThemeAssets(id, [key], session)

// Then
expect(restRequest).toHaveBeenCalledWith('DELETE', `/themes/${id}/assets`, session, undefined, {'asset[key]': key})
expect(output).toBe(true)
})

test('throws an AbortError when the server responds with a 403', async () => {
// Given
const id = 123
const key = 'config/settings_data.json'
const message = 'You are not authorized to edit themes on "my-shop.myshopify.com".'

vi.mocked(restRequest).mockResolvedValue({
json: {message},
status: 403,
headers: {},
expect(adminRequestDoc).toHaveBeenCalledWith(ThemeFilesDelete, session, {
themeId: `gid://shopify/OnlineStoreTheme/${id}`,
files: [key],
})

// When
const deletePromise = () => deleteThemeAsset(id, key, session)

// Then
await expect(deletePromise).rejects.toThrow(new AbortError(message))
expect(restRequest).toHaveBeenCalledWith('DELETE', `/themes/${id}/assets`, session, undefined, {'asset[key]': key})
expect(output).toEqual([{key, success: true, operation: 'DELETE'}])
})
})

Expand All @@ -383,7 +372,6 @@ describe('themeDelete', () => {
test(`deletes a theme with graphql with a ${sessionType} session`, async () => {
// Given
const id = 123
const name = 'store theme'

vi.mocked(adminRequestDoc).mockResolvedValue({
themeDelete: {
Expand Down
47 changes: 42 additions & 5 deletions packages/cli-kit/src/public/node/themes/api.ts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While 🎩 I found we run into a nil error here (see observe logs). I guess the CLI (or at least in dev) doesn't have an associated user 🤔

Are we ready to look and 🎩 this PR again or are we waiting on this to be resolved?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be resolved now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I top hatted when using --password and was able to successfully delete theme files.

Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
ThemeFilesUpsert,
ThemeFilesUpsertMutation,
} from '../../../cli/api/graphql/admin/generated/theme_files_upsert.js'
import {ThemeFilesDelete} from '../../../cli/api/graphql/admin/generated/theme_files_delete.js'
import {
OnlineStoreThemeFileBodyInputType,
OnlineStoreThemeFilesUpsertFileInput,
Expand Down Expand Up @@ -142,11 +143,47 @@ export async function fetchThemeAssets(id: number, filenames: Key[], session: Ad
}
}

export async function deleteThemeAsset(id: number, key: Key, session: AdminSession): Promise<boolean> {
const response = await request('DELETE', `/themes/${id}/assets`, session, undefined, {
'asset[key]': key,
})
return response.status === 200
export async function deleteThemeAssets(id: number, filenames: Key[], session: AdminSession): Promise<Result[]> {
const batchSize = 50
const results: Result[] = []

for (let i = 0; i < filenames.length; i += batchSize) {
const batch = filenames.slice(i, i + batchSize)
// eslint-disable-next-line no-await-in-loop
const {themeFilesDelete} = await adminRequestDoc(ThemeFilesDelete, session, {
themeId: composeThemeGid(id),
files: batch,
})

if (!themeFilesDelete) {
unexpectedGraphQLError('Failed to delete theme assets')
}

const {deletedThemeFiles, userErrors} = themeFilesDelete

if (deletedThemeFiles) {
deletedThemeFiles.forEach((file) => {
results.push({key: file.filename, success: true, operation: Operation.Delete})
})
}

if (userErrors.length > 0) {
userErrors.forEach((error) => {
if (error.filename) {
results.push({
key: error.filename,
success: false,
operation: Operation.Delete,
errors: {asset: [error.message]},
})
} else {
unexpectedGraphQLError(`Failed to delete theme assets: ${error.message}`)
}
})
}
}

return results
}

export async function bulkUploadThemeAssets(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {reconcileJsonFiles} from './theme-reconciliation.js'
import {REMOTE_STRATEGY, LOCAL_STRATEGY} from './remote-theme-watcher.js'
import {fakeThemeFileSystem} from '../theme-fs/theme-fs-mock-factory.js'
import {deleteThemeAsset, fetchThemeAssets} from '@shopify/cli-kit/node/themes/api'
import {deleteThemeAssets, fetchThemeAssets} from '@shopify/cli-kit/node/themes/api'
import {buildTheme} from '@shopify/cli-kit/node/themes/factories'
import {Checksum, ThemeAsset, ThemeFileSystem} from '@shopify/cli-kit/node/themes/types'
import {DEVELOPMENT_THEME_ROLE} from '@shopify/cli-kit/node/themes/utils'
Expand Down Expand Up @@ -143,7 +143,7 @@ describe('reconcileJsonFiles', () => {
)

// Then
expect(deleteThemeAsset).toHaveBeenCalledWith(developmentTheme.id, assetToBeDeleted.key, adminSession)
expect(deleteThemeAssets).toHaveBeenCalledWith(developmentTheme.id, [assetToBeDeleted.key], adminSession)
expect(defaultThemeFileSystem.files.get('templates/asset.json')).toBeUndefined()
})
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import {batchedRequests} from '../batching.js'
import {MAX_GRAPHQL_THEME_FILES} from '../../constants.js'
import {outputDebug} from '@shopify/cli-kit/node/output'
import {AdminSession} from '@shopify/cli-kit/node/session'
import {deleteThemeAsset, fetchThemeAssets} from '@shopify/cli-kit/node/themes/api'
import {deleteThemeAssets, fetchThemeAssets} from '@shopify/cli-kit/node/themes/api'
import {Checksum, ThemeFileSystem, ThemeAsset, Theme} from '@shopify/cli-kit/node/themes/types'
import {renderInfo, renderSelectPrompt} from '@shopify/cli-kit/node/ui'

Expand Down Expand Up @@ -179,9 +179,13 @@ async function performFileReconciliation(
)
})

const deleteRemoteFiles = remoteFilesToDelete.map((file) => deleteThemeAsset(targetTheme.id, file.key, session))
const deleteRemoteFiles = deleteThemeAssets(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be so much faster!

targetTheme.id,
remoteFilesToDelete.map((file) => file.key),
session,
)

await Promise.all([...deleteLocalFiles, ...downloadRemoteFiles, ...deleteRemoteFiles])
await Promise.all([...deleteLocalFiles, ...downloadRemoteFiles, deleteRemoteFiles])
}

async function partitionFilesByReconciliationStrategy(
Expand Down
Loading
Loading