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

Manage new_updated_removed dashboard extensions in confirmation prompt #3226

Merged
merged 1 commit into from
Jan 15, 2024

Conversation

alvaro-shopify
Copy link
Contributor

@alvaro-shopify alvaro-shopify commented Jan 5, 2024

WHY are these changes introduced?

Follow-up: #3205

UX: figma

Right now the source extension breakdown info data is different for the deploy and the release command. The former uses the local information and the second the API operation appVersionDiff
The appVersionDiff API return properly classified the dashboard extensions as new, update or removed (but it doesn't distinguish between dashboard or extension), however the local logic in the deploy command manages a list of affected dashboard extension regardless the modification made to them.
This drives us to a situation like this.

  • release command prompt
  • deploy command prompt

the extension my cool admin link is the same dashboard extension in both cases

WHAT is this pull request doing?

  • Extensions breakdown info now includes the experience of the extension
  • Extensions breakdown info doesn't return the dashboard extensions in a specific list but included in the list toUpdate, toCreate, toRemove
  • The method that builds the prompt TableInfo content from the extensions breakdown info, now manage the dashboard extensions:
    • In each of the three list the extensions goes always first and then the dashboard
    • dashboard extensions includes a custom suffix which value depends on the list type (new, from Partners dashboard, from Partners Dashboard, `removed, from Partners Dashboard)

How to test your changes?

  • NOTE: Prompts will always display ┃ + webhooks (new) and it's not possible to display the message No changes. The problem is that api_version inside webhooks is not versioned yet in the server side thus it's not deployed. Each time you run deploy command you will see the change. This behaviour will be fixed once this Add support for the versioned field webhooks->api_version PR is merged
  • Create a new cli app and run the p shopify app config link --path /PATH/TO/YOUR/APP command to link it to a new remote app.

Scenario 1

  • Generate a new extension p shopify app generate extension --path /PATH/TO/YOUR/APP
  • Create a new dashboard extension from partners in the app created in the first step (ie Admin link)
  • Run the SHOPIFY_CLI_VERSIONED_APP_CONFIG=1 npm run shopify app deploy --path /PATH/TO/YOUR/APP and confirm the deployment

Scenario 2

  • Generate a new extension p shopify app generate extension --path /PATH/TO/YOUR/APP
  • Create a new dashboard extension from partners in the app created in the first step (ie Admin link)
  • Run the SHOPIFY_CLI_VERSIONED_APP_CONFIG=1 npm run shopify app deploy --path /PATH/TO/YOUR/APP

Scenario 3

  • Delete one of your local previous extension
  • Delete one of the dashboard extension
  • Generate a new extension p shopify app generate extension --path /PATH/TO/YOUR/APP
  • Create a new dashboard extension from partners in the app created in the first step (ie Admin link)
  • Run the SHOPIFY_CLI_VERSIONED_APP_CONFIG=1 npm run shopify app deploy --path /PATH/TO/YOUR/APP

Repeat the same three scenarios but in this case using the release command so instead of using deploy with release you should run:

  • SHOPIFY_CLI_VERSIONED_APP_CONFIG=1 npm run shopify app deploy --path /PATH/TO/YOUR/APP --no-release
  • SHOPIFY_CLI_VERSIONED_APP_CONFIG=1 npm run shopify app release --path /PATH/TO/YOUR/APP --version PREVIOUS_VERSION
    The prompt output should be the same

Post-release steps

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes
  • I've made sure that any changes to dev or deploy have been reflected in the internal flowchart.

Copy link
Contributor

github-actions bot commented Jan 5, 2024

We detected some changes at either packages/*/src or packages/cli-kit/assets/cli-ruby/** and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

Copy link
Contributor

github-actions bot commented Jan 5, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
73.86% (+0.11% 🔼)
6555/8875
🟡 Branches
70.99% (+0.08% 🔼)
3196/4502
🟡 Functions
73.31% (+0.27% 🔼)
1706/2327
🟡 Lines
74.98% (+0.09% 🔼)
6202/8272
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / schema.ts
100% 100% 100% 100%
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / release.ts
100%
75% (-5% 🔻)
100% 100%
🟢
... / ConcurrentOutput.tsx
97.62% (-2.38% 🔻)
75% (-8.33% 🔻)
100%
97.44% (-2.56% 🔻)

Test suite run success

1537 tests passing in 705 suites.

Report generated by 🧪jest coverage report action from 2e09d37

@pt2pham pt2pham added the #gsd:36684 Versioned app configurations project (https://vault.shopify.io/gsd/projects/36684) label Jan 5, 2024
@alvaro-shopify alvaro-shopify force-pushed the add-modifications-to-deploy-release-prompt branch from e913011 to de885db Compare January 11, 2024 09:44
@alvaro-shopify alvaro-shopify force-pushed the manage-new-removed-dashboard-extensions branch from 13e3af9 to 76955fa Compare January 11, 2024 11:18
@alvaro-shopify alvaro-shopify force-pushed the add-modifications-to-deploy-release-prompt branch from 36c6e76 to 422b37d Compare January 12, 2024 12:17
@alvaro-shopify alvaro-shopify force-pushed the manage-new-removed-dashboard-extensions branch from 76955fa to 2e09d37 Compare January 12, 2024 12:17
Copy link
Contributor

Differences in type declarations

We detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:

  • Some seemingly private modules might be re-exported through public modules.
  • If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

We found no new type declarations in this PR

Existing type declarations

packages/cli-kit/dist/public/common/object.d.ts
@@ -62,4 +62,11 @@ export declare function getPathValue<T = object>(object: object, path: string):
  * @param value - The value to set.
  * @returns - Returns object.
  */
-export declare function setPathValue(object: object, path: string, value?: unknown): object;
\ No newline at end of file
+export declare function setPathValue(object: object, path: string, value?: unknown): object;
+/**
+ * Checks if value is an empty object, collection, map, or set.
+ *
+ * @param object - The value to check.
+ * @returns - Returns true if value is empty, else false.
+ */
+export declare function isEmpty(object: object): boolean;
\ No newline at end of file
packages/cli-kit/dist/public/node/schema.d.ts
@@ -1 +1,9 @@
-export { z as zod } from 'zod';
\ No newline at end of file
+import { ZodTypeAny } from 'zod';
+export { z as zod } from 'zod';
+/**
+ * Returns a new schema that is the same as the input schema, but with all nested schemas set to strict.
+ *
+ * @param schema - The schema to make strict.
+ * @returns The result strict schema.
+ */
+export declare function deepStrict(schema: ZodTypeAny): ZodTypeAny;
\ No newline at end of file
packages/cli-kit/dist/private/node/ui/components/List.d.ts
@@ -1,9 +1,16 @@
 import { InlineToken, TokenItem } from './TokenizedText.js';
 import { TextProps } from 'ink';
 import { FunctionComponent } from 'react';
+export interface CustomListItem {
+    type?: string;
+    item: TokenItem<InlineToken>;
+    bullet?: string;
+    color?: TextProps['color'];
+}
+type ListItem = TokenItem<InlineToken> | CustomListItem;
 interface ListProps {
     title?: TokenItem<InlineToken>;
-    items: TokenItem<InlineToken>[];
+    items: ListItem[];
     ordered?: boolean;
     margin?: boolean;
     color?: TextProps['color'];
packages/cli-kit/dist/private/node/ui/components/Prompts/InfoTable.d.ts
@@ -1,13 +1,15 @@
+import { CustomListItem } from '../List.js';
 import { InlineToken, TokenItem } from '../TokenizedText.js';
 import { TextProps } from 'ink';
 import { FunctionComponent } from 'react';
-type Items = TokenItem<InlineToken>[];
+type Items = (TokenItem<InlineToken> | CustomListItem)[];
 export interface InfoTableSection {
     color?: TextProps['color'];
     header: string;
     bullet?: string;
     helperText?: string;
     items: Items;
+    emptyItemsText?: string;
 }
 export interface InfoTableProps {
     table: {

Copy link
Contributor

@gonzaloriestra gonzaloriestra left a comment

Choose a reason for hiding this comment

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

It works as expected and the code LGTM! 👌

Base automatically changed from add-modifications-to-deploy-release-prompt to main January 12, 2024 14:40
Copy link
Contributor

@pt2pham pt2pham left a comment

Choose a reason for hiding this comment

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

LGTM!

@alvaro-shopify alvaro-shopify added this pull request to the merge queue Jan 15, 2024
Merged via the queue into main with commit 489ccac Jan 15, 2024
@alvaro-shopify alvaro-shopify deleted the manage-new-removed-dashboard-extensions branch January 15, 2024 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
#gsd:36684 Versioned app configurations project (https://vault.shopify.io/gsd/projects/36684)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants