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

Clean up messaging around unlinked storefronts when running CLI commands #1937

Merged
merged 1 commit into from
Apr 18, 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
9 changes: 9 additions & 0 deletions .changeset/breezy-jokes-pump.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
'@shopify/cli-hydrogen': patch
---

Clean up messaging around unlinked storefronts when running CLI commands

- When you run `env list`, `env pull`, or `deploy` against a storefront that isn't linked, it will show a warning message instead of an error message.
- If you don't have a storefront to link to on Admin, we will just ask you to create a storefront instead of displaying an option list of size 1.
- If you deleted a storefront on Admin, we will try to relink your storefront when running `env list`, `env pull`, or `deploy`.
105 changes: 105 additions & 0 deletions packages/cli/src/commands/hydrogen/deploy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -708,6 +708,111 @@ describe('deploy', () => {
}
});

describe('user confirmation', () => {
beforeEach(() => {
vi.mocked(getOxygenDeploymentData).mockResolvedValue({
oxygenDeploymentToken: 'some-encoded-token',
environments: [
{
name: 'Production',
handle: 'production',
branch: 'main',
type: 'PRODUCTION',
},
{name: 'Preview', handle: 'preview', branch: null, type: 'PREVIEW'},
{
name: 'Staging',
handle: 'staging',
branch: 'stage-1',
type: 'CUSTOM',
},
],
});
});

describe('user provides a non-preview environment', () => {
it('renders a user confirmation on deploy when production environment is selected', async () => {
vi.mocked(renderSelectPrompt).mockResolvedValue('main');

await runDeploy(deployParams);

expect(renderConfirmationPrompt).toHaveBeenCalledWith({
confirmationMessage: 'Yes, confirm deploy',
cancellationMessage: 'No, cancel deploy',
message: expect.any(String),
});
});

it('renders a user confirmation on deploy when production environment handle is provided', async () => {
await runDeploy({
...deployParams,
env: 'production',
});

expect(renderConfirmationPrompt).toHaveBeenCalledWith({
confirmationMessage: 'Yes, confirm deploy',
cancellationMessage: 'No, cancel deploy',
message: expect.any(String),
});
});

it('renders a user confirmation on deploy when production environment branch is provided', async () => {
await runDeploy({
...deployParams,
envBranch: 'main',
});

expect(renderConfirmationPrompt).toHaveBeenCalledWith({
confirmationMessage: 'Yes, confirm deploy',
cancellationMessage: 'No, cancel deploy',
message: expect.any(String),
});
});
});

describe('user provides a preview environment', () => {
it("doesn't render a user confirmation on deploy when preview environment is selected", async () => {
vi.mocked(renderSelectPrompt).mockResolvedValue(
'shopify-preview-environment.',
);

await runDeploy(deployParams);

expect(renderConfirmationPrompt).not.toHaveBeenCalledWith({
confirmationMessage: 'Yes, confirm deploy',
cancellationMessage: 'No, cancel deploy',
message: expect.any(String),
});
});

it("doesn't render a user confirmation on deploy when preview environment handle is provided", async () => {
await runDeploy({
...deployParams,
env: 'preview',
});

expect(renderConfirmationPrompt).not.toHaveBeenCalledWith({
confirmationMessage: 'Yes, confirm deploy',
cancellationMessage: 'No, cancel deploy',
message: expect.any(String),
});
});

it("doesn't render a user confirmation on deploy when preview environment flag is provided", async () => {
await runDeploy({
...deployParams,
defaultEnvironment: true,
});

expect(renderConfirmationPrompt).not.toHaveBeenCalledWith({
confirmationMessage: 'Yes, confirm deploy',
cancellationMessage: 'No, cancel deploy',
message: expect.any(String),
});
});
});
});

describe('next steps', () => {
it('renders a link to the deployment', async () => {
vi.mocked(createDeploy).mockResolvedValue({
Expand Down
40 changes: 11 additions & 29 deletions packages/cli/src/commands/hydrogen/deploy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -475,37 +475,19 @@ export async function runDeploy(

if (
!isCI &&
(userProvidedEnvironmentTag ||
userChosenEnvironmentTag ||
config.defaultEnvironment)
!config.defaultEnvironment &&
(userProvidedEnvironmentTag || userChosenEnvironmentTag)
) {
let chosenEnvironment: {
name: string;
branch: string | null;
handle: string;
} | null = null;

if (config.defaultEnvironment) {
chosenEnvironment = findEnvironmentOrThrow(
deploymentData!.environments!,
'preview',
);
} else if (config.environmentTag) {
chosenEnvironment = findEnvironmentByBranchOrThrow(
deploymentData!.environments!,
config.environmentTag,
);
}

let confirmationMessage = 'Creating a deployment';
let chosenEnvironment = findEnvironmentByBranchOrThrow(
deploymentData!.environments!,
config.environmentTag!,
);
Comment on lines +481 to +484

Choose a reason for hiding this comment

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

In this case, do we know definitively that these arguments will not be undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, had to scroll up but confirmed deploymentData HAS to be defined or an early return would occur.
config.environmentTag has to be defined if one of the items userProvidedEnvironmentTag or userChosenEnvironmentTag is defined.


if (chosenEnvironment) {
confirmationMessage += ` against ${createEnvironmentCliChoiceLabel(
chosenEnvironment.name,
chosenEnvironment.handle,
chosenEnvironment.branch,
)}`;
}
let confirmationMessage = `Creating a deployment against ${createEnvironmentCliChoiceLabel(
chosenEnvironment.name,
chosenEnvironment.handle,
chosenEnvironment.branch,
)}`;

const confirmPush = await renderConfirmationPrompt({
confirmationMessage: 'Yes, confirm deploy',
Expand Down
46 changes: 13 additions & 33 deletions packages/cli/src/commands/hydrogen/env/list.test.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,13 @@
import {describe, it, expect, vi, beforeEach, afterEach} from 'vitest';
import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output';
import {inTemporaryDirectory} from '@shopify/cli-kit/node/fs';
import {renderConfirmationPrompt} from '@shopify/cli-kit/node/ui';

import {getStorefrontEnvironments} from '../../../lib/graphql/admin/list-environments.js';
import {dummyListEnvironments} from '../../../lib/graphql/admin/test-helper.js';
import {type AdminSession, login} from '../../../lib/auth.js';
import {
renderMissingLink,
renderMissingStorefront,
} from '../../../lib/render-errors.js';
import {linkStorefront} from '../link.js';
import {renderMissingStorefront} from '../../../lib/render-errors.js';
import {runEnvList} from './list.js';
import {verifyLinkedStorefront} from '../../../lib/verify-linked-storefront.js';

const SHOP = 'my-shop';

Expand All @@ -29,6 +25,7 @@ vi.mock('../../../lib/auth.js');
vi.mock('../../../lib/shopify-config.js');
vi.mock('../../../lib/render-errors.js');
vi.mock('../../../lib/graphql/admin/list-environments.js');
vi.mock('../../../lib/verify-linked-storefront.js');
vi.mock('../../../lib/shell.js', () => ({getCliCommand: () => 'h2'}));

describe('listEnvironments', () => {
Expand All @@ -53,6 +50,12 @@ describe('listEnvironments', () => {
config: SHOPIFY_CONFIG,
});

vi.mocked(verifyLinkedStorefront).mockResolvedValue({
id: SHOPIFY_CONFIG.storefront.id,
title: SHOPIFY_CONFIG.storefront.title,
productionUrl: 'https://my-shop.myshopify.com',
});

vi.mocked(getStorefrontEnvironments).mockResolvedValue(
dummyListEnvironments(SHOPIFY_CONFIG.storefront.id),
);
Expand Down Expand Up @@ -98,39 +101,16 @@ describe('listEnvironments', () => {

describe('when there is no linked storefront', () => {
beforeEach(() => {
vi.mocked(login).mockResolvedValue({
session: ADMIN_SESSION,
config: {
...SHOPIFY_CONFIG,
storefront: undefined,
},
});
vi.mocked(verifyLinkedStorefront).mockResolvedValue(undefined);
});

it('calls renderMissingLink', async () => {
it("doesn't list any environments", async () => {
await inTemporaryDirectory(async (tmpDir) => {
await runEnvList({path: tmpDir});

expect(renderMissingLink).toHaveBeenCalledOnce();
});
});

it('prompts the user to create a link', async () => {
vi.mocked(renderConfirmationPrompt).mockResolvedValue(true);
const output = mockAndCaptureOutput();

await inTemporaryDirectory(async (tmpDir) => {
await runEnvList({path: tmpDir});

expect(renderConfirmationPrompt).toHaveBeenCalledWith({
message: expect.arrayContaining([{command: 'h2 link'}]),
});

expect(linkStorefront).toHaveBeenCalledWith(
tmpDir,
ADMIN_SESSION,
{...SHOPIFY_CONFIG, storefront: undefined},
expect.anything(),
);
expect(output.info()).toBe('');
});
});
});
Expand Down
40 changes: 13 additions & 27 deletions packages/cli/src/commands/hydrogen/env/list.ts
Original file line number Diff line number Diff line change
@@ -1,23 +1,18 @@
import {Flags} from '@oclif/core';
import Command from '@shopify/cli-kit/node/base-command';
import {renderConfirmationPrompt} from '@shopify/cli-kit/node/ui';
import {pluralize} from '@shopify/cli-kit/common/string';
import colors from '@shopify/cli-kit/node/colors';
import {
outputContent,
outputInfo,
outputNewline,
} from '@shopify/cli-kit/node/output';
import {linkStorefront} from '../link.js';
import {commonFlags} from '../../../lib/flags.js';
import {getStorefrontEnvironments} from '../../../lib/graphql/admin/list-environments.js';
import {createEnvironmentCliChoiceLabel} from '../../../lib/common.js';
import {
renderMissingLink,
renderMissingStorefront,
} from '../../../lib/render-errors.js';
import {renderMissingStorefront} from '../../../lib/render-errors.js';
import {login} from '../../../lib/auth.js';
import {getCliCommand} from '../../../lib/shell.js';
import {verifyLinkedStorefront} from '../../../lib/verify-linked-storefront.js';

export default class EnvList extends Command {
static descriptionWithMarkdown =
Expand Down Expand Up @@ -46,35 +41,26 @@ export async function runEnvList({path: root = process.cwd()}: Flags) {
getCliCommand(),
]);

let configStorefront = config.storefront;

if (!configStorefront?.id) {
renderMissingLink({session, cliCommand});

const runLink = await renderConfirmationPrompt({
message: ['Run', {command: `${cliCommand} link`}, '?'],
});

if (!runLink) {
return;
}
const linkedStorefront = await verifyLinkedStorefront({
root,
session,
config,
cliCommand,
});

configStorefront = await linkStorefront(root, session, config, {
cliCommand,
});
}
if (!linkedStorefront) return;

if (!configStorefront) return;
config.storefront = linkedStorefront;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused by the use of config.storefront = X in this PR. Should we be using setStorefront from shopify-config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So setStorefront sets your shopify project.json file to have the linked storefront's data.

verifyLinkedStorefront just checks the following:

  • checks to make sure you have any storefront linked in your project.json file
  • talks to Admin and ensures that storefront still exists
    • this prevents the yucky GraphQL error you would get if you deleted your storefront on Admin


const storefront = await getStorefrontEnvironments(
session,
configStorefront.id,
config.storefront.id,
);

if (!storefront) {
renderMissingStorefront({
session,
storefront: configStorefront,
storefront: config.storefront,
cliCommand,
});

Expand All @@ -95,7 +81,7 @@ export async function runEnvList({path: root = process.cwd()}: Flags) {
outputInfo(
pluralizedEnvironments({
environments: storefront.environments,
storefrontTitle: configStorefront.title,
storefrontTitle: config.storefront.title,
}).toString(),
);

Expand Down
Loading
Loading