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

Warn less often about scope mismatch #4574

Merged
merged 1 commit into from
Oct 4, 2024
Merged
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
66 changes: 64 additions & 2 deletions packages/app/src/cli/services/dev.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import {developerPreviewController} from './dev.js'
import {developerPreviewController, warnIfScopesDifferBeforeDev} from './dev.js'
import {fetchAppPreviewMode} from './dev/fetch.js'
import {testDeveloperPlatformClient} from '../models/app/app.test-data.js'
import {testApp, testDeveloperPlatformClient, testOrganizationApp} from '../models/app/app.test-data.js'
import {describe, expect, test, vi} from 'vitest'
import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output'

vi.mock('./dev/fetch.js')

Expand Down Expand Up @@ -35,3 +36,64 @@ describe('developerPreviewController', () => {
expect(fetchAppPreviewMode).toHaveBeenCalledTimes(2)
})
})

describe('warnIfScopesDifferBeforeDev', () => {
const appsWithScopes = (local: string, remote: string) => {
const localApp = testApp({}, 'current')
const remoteApp = testOrganizationApp()
localApp.configuration = {
...localApp.configuration,
access_scopes: {scopes: local, use_legacy_install_flow: false},
} as any
remoteApp.configuration = {
...remoteApp.configuration,
access_scopes: {scopes: remote, use_legacy_install_flow: false},
} as any
return {
localApp,
remoteApp,
}
}

test('does not warn if the scopes are the same', async () => {
// Given
const developerPlatformClient = testDeveloperPlatformClient({supportsDevSessions: false})
const apps = appsWithScopes('scopes1,scopes2', 'scopes1,scopes2')

// When
const mockOutput = mockAndCaptureOutput()
mockOutput.clear()
await warnIfScopesDifferBeforeDev({...apps, developerPlatformClient})

// Then
expect(mockOutput.warn()).toBe('')
})

test('warns if the scopes differ', async () => {
// Given
const apps = appsWithScopes('scopes1,scopes2', 'scopes3,scopes4')
const developerPlatformClient = testDeveloperPlatformClient({supportsDevSessions: false})

// When
const mockOutput = mockAndCaptureOutput()
mockOutput.clear()
await warnIfScopesDifferBeforeDev({...apps, developerPlatformClient})

// Then
expect(mockOutput.warn()).toContain("The scopes in your TOML don't match")
})

test('silent if scopes differ cosmetically', async () => {
// Given
const apps = appsWithScopes('scopes1, scopes2 ', ' scopes2, scopes1')
const developerPlatformClient = testDeveloperPlatformClient({supportsDevSessions: false})

// When
const mockOutput = mockAndCaptureOutput()
mockOutput.clear()
await warnIfScopesDifferBeforeDev({...apps, developerPlatformClient})

// Then
expect(mockOutput.warn()).toBe('')
})
})
76 changes: 51 additions & 25 deletions packages/app/src/cli/services/dev.ts
Original file line number Diff line number Diff line change
Expand Up @@ -164,32 +164,58 @@ async function prepareForDev(commandOptions: DevOptions): Promise<DevConfig> {
}
}

async function actionsBeforeSettingUpDevProcesses({localApp, remoteApp, developerPlatformClient}: DevConfig) {
if (developerPlatformClient.supportsDevSessions) return
if (
isCurrentAppSchema(localApp.configuration) &&
!localApp.configuration.access_scopes?.use_legacy_install_flow &&
localApp.configuration.access_scopes?.scopes !== remoteApp.configuration?.access_scopes?.scopes
) {
const nextSteps = [
[
'Run',
{command: formatPackageManagerCommand(localApp.packageManager, 'shopify app deploy')},
'to push your scopes to the Partner Dashboard',
],
]
async function actionsBeforeSettingUpDevProcesses(devConfig: DevConfig) {
await warnIfScopesDifferBeforeDev(devConfig)
}

renderWarning({
headline: [`The scopes in your TOML don't match the scopes in your Partner Dashboard`],
body: [
`Scopes in ${basename(localApp.configuration.path)}:`,
scopesMessage(getAppScopesArray(localApp.configuration)),
'\n',
'Scopes in Partner Dashboard:',
scopesMessage(remoteApp.configuration?.access_scopes?.scopes?.split(',') || []),
],
nextSteps,
})
/**
* Show a warning if the scopes in the local app configuration do not match the scopes in the remote app configuration.
*
* This is to flag that the developer may wish to run `shopify app deploy` to push the latest scopes.
*
*/
export async function warnIfScopesDifferBeforeDev({
localApp,
remoteApp,
developerPlatformClient,
}: Pick<DevConfig, 'localApp' | 'remoteApp' | 'developerPlatformClient'>) {
if (developerPlatformClient.supportsDevSessions) return
if (isCurrentAppSchema(localApp.configuration)) {
const localAccess = localApp.configuration.access_scopes
const remoteAccess = remoteApp.configuration?.access_scopes

const rationaliseScopes = (scopeString: string | undefined) => {
if (!scopeString) return scopeString
return scopeString
.split(',')
.map((scope) => scope.trim())
.sort()
.join(',')
}
const localScopes = rationaliseScopes(localAccess?.scopes)
const remoteScopes = rationaliseScopes(remoteAccess?.scopes)

if (!localAccess?.use_legacy_install_flow && localScopes !== remoteScopes) {
const nextSteps = [
[
'Run',
{command: formatPackageManagerCommand(localApp.packageManager, 'shopify app deploy')},
'to push your scopes to the Partner Dashboard',
],
]

renderWarning({
headline: [`The scopes in your TOML don't match the scopes in your Partner Dashboard`],
body: [
`Scopes in ${basename(localApp.configuration.path)}:`,
scopesMessage(getAppScopesArray(localApp.configuration)),
'\n',
'Scopes in Partner Dashboard:',
scopesMessage(remoteAccess?.scopes?.split(',') || []),
],
nextSteps,
})
}
}
}

Expand Down
Loading