-
Notifications
You must be signed in to change notification settings - Fork 255
Improve missing theme scope error message #7652
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@shopify/cli-kit': patch | ||
| --- | ||
|
|
||
| Improve the error shown when theme commands use an Admin API token that is missing required theme access scopes. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,7 @@ import {AdminSession} from '../session.js' | |
| import {AbortError} from '../error.js' | ||
| import {outputDebug} from '../output.js' | ||
| import {recordTiming, recordEvent, recordError} from '../analytics.js' | ||
| import {ClientError} from 'graphql-request' | ||
|
|
||
| export type ThemeParams = Partial<Pick<Theme, 'name' | 'role' | 'processing' | 'src'>> | ||
| export type AssetParams = Pick<ThemeAsset, 'key'> & Partial<Pick<ThemeAsset, 'value' | 'attachment'>> | ||
|
|
@@ -90,7 +91,11 @@ export async function fetchThemes(session: AdminSession): Promise<Theme[]> { | |
| variables: {after}, | ||
| responseOptions: {handleErrors: false}, | ||
| preferredBehaviour: THEME_API_NETWORK_BEHAVIOUR, | ||
| }).catch((error: unknown) => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we apply this same logic to
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (BOT COMMENT) 🎨 Code Style: Every other Suggestion: Consider restructuring as: let response
try {
response = await adminRequestDoc({
query: GetThemes,
session,
variables: {after},
responseOptions: {handleErrors: false},
preferredBehaviour: THEME_API_NETWORK_BEHAVIOUR,
})
} catch (error) {
abortIfMissingThemeAccessScope(error)
throw error
} |
||
| abortIfMissingThemeAccessScope(error) | ||
| throw error | ||
| }) | ||
|
|
||
| if (!response.themes) { | ||
| unexpectedGraphQLError('Failed to fetch themes') | ||
| } | ||
|
|
@@ -606,6 +611,37 @@ function unexpectedGraphQLError(message: string): never { | |
| throw recordError(new AbortError(message)) | ||
| } | ||
|
|
||
| function abortIfMissingThemeAccessScope(error: unknown): void { | ||
| if (!(error instanceof ClientError)) return | ||
|
|
||
| const requiredAccess = getRequiredAccessForAccessDeniedError(error) | ||
| if (!requiredAccess) return | ||
|
|
||
| const tryMessage = [ | ||
| 'If you authenticated with a custom app Admin API access token, open the custom app in your Shopify admin,', | ||
| 'add the required theme access scopes, reinstall the app, and use the new access token.', | ||
|
Comment on lines
+621
to
+622
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the announcement about custom apps we might want to change the language here. |
||
| 'For theme pull, theme list, and theme info, add `read_themes`.', | ||
| 'For theme push and theme dev, add both `read_themes` and `write_themes`.', | ||
| 'If you authenticated with your Shopify account, make sure your staff or collaborator account can access Online Store themes, then run `shopify auth logout` and try again.', | ||
| 'See https://shopify.dev/api/usage/access-scopes.', | ||
| ].join(' ') | ||
|
|
||
| throw recordError( | ||
| new AbortError(`The authenticated account or access token is missing ${requiredAccess}.`, tryMessage), | ||
| ) | ||
| } | ||
|
|
||
| function getRequiredAccessForAccessDeniedError(error: ClientError): string | undefined { | ||
| const graphQLErrors = error.response.errors | ||
| if (!Array.isArray(graphQLErrors)) return undefined | ||
|
|
||
| const accessDeniedError = graphQLErrors.find((graphQLError) => graphQLError.extensions?.code === 'ACCESS_DENIED') | ||
| const requiredAccess = accessDeniedError?.extensions?.requiredAccess | ||
| if (typeof requiredAccess !== 'string') return undefined | ||
|
|
||
| return requiredAccess.replace(/\.$/, '') | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I worry that there's some fragility/assumptions here where we're relying on the shape of the translation from the API but I don't have a good alternative suggestion. |
||
| } | ||
|
|
||
| function themeGid(id: number): string { | ||
| return `gid://shopify/OnlineStoreTheme/${id}` | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are quite a few conditionals in this new logic that aren't covered by the test. We don't need to add all (e.g. don't add a test asserting that the message doesn't appear for other exceptions) but some extra coverage would be good.