Validate on `shopify theme share/pull/push/serve` if users are running the command in a theme/empty directory by Poitrin · Pull Request #2680 · Shopify/shopify-cli

Poitrin · 2022-11-08T14:06:11Z

WHY are these changes introduced?

Fixes Validate on shopify theme share/pull/push/serve/dev if users are running the command in a theme/empty directory #2669
Fixes Prevent shopify theme push/serve from deleting a theme when developers run it in an invalid directory #2623
CLI 3.x related PR: Add --force flag for validation on shopify theme dev/pull/push/share if users are running the command in a theme directory cli#751

We want to…

ensure that partners execute shopify theme share/pull/push/serve/dev in the correct directory
enforce the usage of the theme architecture directory structure

WHAT is this pull request doing?

Requests a confirmation from the user to the following question …

It doesn’t seem like you’re running this command in a theme directory. Are you sure you want to proceed?

… if users …

try running shopify theme share in a directory that doesn't seem [1] like a theme directory
try running shopify theme serve/dev in a directory that doesn't seem [1] like a theme directory
try running shopify theme push in a directory that doesn't seem [1] like a theme directory
try running shopify theme pull in a directory that doesn't seem [1] like a theme directory or it's not empty

[1] https://shopify.dev/themes/architecture#directory-structure-and-component-types

How to test your changes?

No confirmation needed

Part 1:

cd into a directory that seems like a theme directory and contains some theme files.
Run shopify theme share/serve/push
Are the commands running without any confirmation?

Part 2:

cd into an empty directory, or a directory with at least the folders %w(config layout sections templates)
Run shopify theme pull
Is the command running without any confirmation?

Confirmation needed

Part 1:

cd into an empty directory, or a directory that does not contain all the folders %w(config layout sections templates)
Run shopify theme share/serve/push
Does the desired confirmation dialogue appear?
Does the CLI exit when you answer No?
Does the CLI continue when you answer Yes?

Part 2:

cd into a non-empty directory that does not contain all the folders %w(config layout sections templates)
Run shopify theme pull
Does the desired confirmation dialogue appear?
Does the CLI exit when you answer No?
Does the CLI continue when you answer Yes?

Post-release steps

~~Update shopify.dev docs, as --force flag has been added.~~ – flag is hidden, so no need to update docs f.t.m.

Update checklist

I've added a CHANGELOG entry for this PR (if the change is public-facing)
I've considered possible cross-platform impacts (Mac, Linux, Windows).
I've left the version number as is (we'll handle incrementing this when releasing).
I've included any post-release steps in the section above (if needed).

Poitrin · 2022-11-09T09:53:21Z

@@ -0,0 +1,31 @@
+# frozen_string_literal: true


Got influenced by lib/project_types/extension/models/specification_handlers/theme_app_extension.rb,
especially the concept of SpecificationHandlers

Poitrin · 2022-11-09T10:01:10Z

+            @ctx,
+            [],
+            title: @ctx.message("theme.confirm_current_directory"),
+            force: options.flags[:force],


The two existing ConfirmStore.asks support the --force flag.
As it also makes sense for this PR's confirmation dialogue, I added it to all commands.

we should definitely get input from UX before making an overarching change like this 😌

functionally i think this makes sense, we're giving developers the previous behavior where you can run theme commands in any directory and allowing them to do that with --force

also after poking around in the repo a bit, i don't see any classes that try to directly look at this flag. so unexpected downstream effects seem unlikely 😄

alternatively maybe a new flag make sense? --ignore-theme-validation or something of that kind? --force comes off as pretty destructive of a flag (cc: git push --force lol)

we should definitely get input from UX before making an overarching change like this 😌

Agreed. I will wait for Guilherme's input, before asking Meredith then :)

also after poking around in the repo a bit, i don't see any classes that try to directly look at this flag. so unexpected downstream effects seem unlikely 😄

Thanks for checking too :)

maybe a new flag make sense? --ignore-theme-validation or something of that kind? --force comes off as pretty destructive of a flag (cc: git push --force lol)

Contouring the validation with --force is kinda destructive: It can overwrite a potentially valid theme with some irrelevant/invalid files 😄.

Guilherme suggests to hide the --force flag (hide it from help message in 2.x / hidden: true in 3.x) and to create a an issue (enhancement) at shopify/cli suggesting the introduction of the -f/--force as a pattern in the whole CLI. Then Meredith can give her input.

I agree with this suggestion.

+1 sounds good!

Issue created: Shopify/cli#760

Poitrin · 2022-11-09T10:12:34Z


      def setup
        super
+        project_context("theme")


By default, cwd is test/fixtures/project.
In order to work in a valid theme directory, I have used project_context, which includes a cd into test/fixtures/theme.
I initially tried with setting the root parameter (at multiple places), but it kept staying in test/fixtures/project.
Even though I don't like changing dirs during tests, I think it's a cleaner solution than explicitly setting root everywhere (provided that this would work).

very cool find! TIL about project_context

Even though I don't like changing dirs during tests, I think it's a cleaner solution than explicitly setting root everywhere (provided that this would work).

i don't know if i agree? it's cleaner in the sense it has less lines of code but project_context("theme") doesn't have a clear function without exploring the meaning of that method. "theme" means the theme fixture but will that be super clear when we come back to this code in 6 months?

regardless of what we pick, i see we're setting root in some tests still while also using project_context. setting root seems to be a convention in theme tests and using project_context is a convention for app tests. i don't think conventions are sacred. they should be should be challenged but we can still be intentional and consistent when deciding to make a change 😄

project_context("theme") doesn't have a clear function without exploring the meaning of that method

That's true. I trusted/got inspired by the existing code where project_context was used at least 7 times 😄.

will that be super clear when we come back to this code in 6 months?

Probably not, but I accepted this, as I thought we would migrate the existing theme code to 3.x sooner 😄. But I will try again with setting root in the tests then :)

Poitrin · 2022-11-09T10:20:16Z

      end

      def test_push_with_root
+        specified_root = ShopifyCLI::ROOT + "/test/fixtures/theme"


I had to use a different root, because theme serve push in a non-existing folder like dist does not make sense.

Poitrin · 2022-11-09T12:42:03Z


      def call(_args, name)
        root = root_value(options, name)
+        return unless exist_and_empty?(root) ||


When the user answers with No, the exit code is 0.
I think that's okay, as the program finished successfully.

mgmanzella

thank you @Poitrin! this look great 🔥 dropped some questions 🙏

mgmanzella · 2022-11-09T19:43:50Z


      def setup
        super
+        project_context("theme")


very cool find! TIL about project_context

Even though I don't like changing dirs during tests, I think it's a cleaner solution than explicitly setting root everywhere (provided that this would work).

i don't know if i agree? it's cleaner in the sense it has less lines of code but project_context("theme") doesn't have a clear function without exploring the meaning of that method. "theme" means the theme fixture but will that be super clear when we come back to this code in 6 months?

regardless of what we pick, i see we're setting root in some tests still while also using project_context. setting root seems to be a convention in theme tests and using project_context is a convention for app tests. i don't think conventions are sacred. they should be should be challenged but we can still be intentional and consistent when deciding to make a change 😄

mgmanzella · 2022-11-09T20:08:24Z

+            @ctx,
+            [],
+            title: @ctx.message("theme.confirm_current_directory"),
+            force: options.flags[:force],


we should definitely get input from UX before making an overarching change like this 😌

functionally i think this makes sense, we're giving developers the previous behavior where you can run theme commands in any directory and allowing them to do that with --force

also after poking around in the repo a bit, i don't see any classes that try to directly look at this flag. so unexpected downstream effects seem unlikely 😄

mgmanzella · 2022-11-09T20:13:00Z

+        end
+
+        def current_directory_confirmed?
+          return false if @ctx.testing?


why do we need this override when testing?

I don't remember if it was with dev test or rake test, but after adding the current_directory_confirmed? method, running the tests and the current working directory initially was no valid theme directory, the confirmation dialogue kicked in and the tests instantly finished with Interrupted. So suddenly only 800+ tests run, instead of 1500+.

In order to avoid confirmation dialogues leading to less tests being executed, I prefer failing tests – therefore the return false.

I'm open to better ideas here 😄
I could also raise an error saying something like "You can't open a confirmation dialogue during tests".

I prefer failing tests – therefore the return false.

ah yeh i see! and agreed 😄

I could also raise an error saying something like "You can't open a confirmation dialogue during tests".

+1 for this approach

+1 for this approach

Great, CLI will raise an error now for this case :)

mgmanzella · 2022-11-09T20:14:53Z


      def call(_args, name)
        root = root_value(options, name)
+        return unless theme_directory?(root) || current_directory_confirmed?


/nit doing the check in your head of unless plus an || statement feels is a little hard to read

I agree, I will change it :)

mgmanzella · 2022-11-09T20:27:33Z

one more thing! i noticed when testing i select no to not proceed and from the CLI 3, the CLI hangs and doesn't exit. I have to ctrl-C to exit the CLI. have you seen this happen? (it could also be my setup so im skeptical there's actually a bug 😅 but thought i'd mention anyway)

pepicrft · 2022-11-10T08:33:05Z

+        REQUIRED_FOLDERS = %w(config layout sections templates).map { |folder| "#{folder}/" }
+
+        def initialize(root)
+          Dir.chdir(root) do


Nitpick: Would it work if instead of choosing a directory through Dir.chdir, we do:

self.folders = Dir[File.join(root, "*/")] + Dir[File.join(root, "templates/*/")]

The code you posted does not work out-of-the-box, because Dir[…] would return the complete absolute paths and I couldn't subtract these from REQUIRED_FOLDERS so easily.

However, the more I understood the issue's actual requirements, the more I could simplify code to…

def valid? REQUIRED_FOLDERS.all? { |required_folder| Dir.exist?(File.join(root, required_folder)) } end

… so thanks for leading me to a better solution ✨

pepicrft · 2022-11-10T08:38:18Z

+        attr_accessor :folders
+        attr_accessor :missing_folders
+
+        def validate


How many directories do templates have? If it's not too many, I'd refrain from having an internal state, missing_folders, that's internally mutated as a side effect of doing a validation. What about?

def missing_folders REQUIRED_FOLDERS - folders end

I was inspired by Rails (calling .valid? to get the correct results for .errors) for this solution, but I agree, so I changed it :)

pepicrft · 2022-11-10T08:39:50Z

+
      def call(_args, name)
        root = root_value(options, name)
+        return unless theme_directory?(root) || current_directory_confirmed?


I see this logic duplicated across theme commands. Have you considered extracting it into a base class or a module that's included in all the theme classes? We mitigate thus the risk of potential inconsistencies in the future.

I have introduced another method so that the lines look now like…

return unless valid_theme_directory?(root)

pepicrft · 2022-11-10T08:40:25Z

        @command.options.flags[:theme] = 1234
        @command.call([], "pull")
+
+        FileUtils.rmdir(specified_root)


What happens if the test fails before reaching this line? Who will remove the specified_root directory?

Good catch! 👍🏻
I have added an ensure before this line, which should hopefully fix the issue :)

pepicrft

👏🏼 Great job!

Poitrin · 2022-11-10T09:43:29Z

i noticed when testing i select no to not proceed and from the CLI 3, the CLI hangs and doesn't exit. I have to ctrl-C to exit the CLI. have you seen this happen? (it could also be my setup so im skeptical there's actually a bug 😅 but thought i'd mention anyway)

Thanks for highlighting this!
I will take a closer look again and then potentially add changes to Shopify/cli#743 :-)

…t_and_not_empty?

Poitrin · 2022-11-10T15:16:22Z

-        @command.call([], "push")
-      end
-
-      def test_push_with_root


As I had to set root in every test to point to a valid theme directory, the test_push_with_root made no sense anymore, so I deleted it.

Poitrin · 2022-11-10T15:17:36Z

        end
      end

-      def test_can_specify_root


See comment above about why this kind of tests could be deleted.

New changes, requesting a new review

mgmanzella

amazing 🚀 this was a change that affected all theme commands, required almost every theme command test to be modified, and you were still able to keep it at ~300 lines. well done!!

mgmanzella · 2022-11-10T18:49:19Z

+        end
+
+        def current_directory_confirmed?
+          return false if @ctx.testing?


I prefer failing tests – therefore the return false.

ah yeh i see! and agreed 😄

I could also raise an error saying something like "You can't open a confirmation dialogue during tests".

+1 for this approach

…ed? is called during tests

karreiro

Thank you, @Poitrin! Awesome stuff 🚀

I've left just two minor comments, please let me know what you think about them :)

…re` if users are running the command in a theme directory (#751) ### WHY are these changes introduced? - Fixes Shopify/shopify-cli#2669 - Addition to Shopify/shopify-cli#2680 We want to… - ensure that partners execute `shopify theme serve/dev` in the _correct_ directory - enforce the usage of the theme [architecture](https://shopify.dev/themes/architecture#directory-structure-and-component-types) directory structure ### WHAT is this pull request doing? Add `--force` flag for validation on `shopify theme dev` if users are running the command in a theme directory. ### How to test your changes? See Shopify/shopify-cli#2680, but test with `shopify theme dev`, instead of `shopify theme serve`. Make sure that your CLI 2.x version has the correct changes.

…n dialogue respect `SHOPIFY_CLI_TTY` (#1369) ### WHY are these changes introduced? Fixes #1136 #### Issue 1: `@ctx.testing?` is `true` even outside a testing environment CLI was hanging when `theme pull` was executed in a directory with already existing files. CLI should have shown question > It doesn’t seem like you’re running this command in a theme directory. Are you sure you want to proceed? but was never shown, due to ```ruby raise "Current theme directory can't be confirmed during tests" if @ctx.testing? ``` Discussion why `raise … if @ctx.testing?` was introduced can be found [here](Shopify/shopify-cli#2680 (comment)). Because `@ctx.testing?` returns `true` based on `ENV["CI"]`, we can’t differentiate between CLI running in partner’s pipeline or in CLI tests. #### Issue 2: Confirmation dialogue ignores environment variable `SHOPIFY_CLI_TTY` Moreover, even after having removed `raise … if @ctx.testing?` and `SHOPIFY_CLI_TTY` being set to `0`, CLI would still hang at the “It doesn’t seem like …” question. ### WHAT is this pull request doing? 1. Removes `raise … if @ctx.testing?`. 2. Shows a warning `It doesn’t seem like you’re running this command in a theme directory.` when `SHOPIFY_CLI_TTY` is set to `0`, but continues to `pull`. * `--force` flag as a way to confirm the command will not be included in the error message yet. * → Shopify/shopify-cli#2680 (comment) * → #760

Julien Poitrin added 15 commits November 8, 2022 14:08

Introduce SpecificationHandlers::Theme

1200ea8

Get rid of root and templates folders differentiation

312daac

Ignore superfluous folders

5017859

Ask user to confirm cwd

66e8918

Move code into RootHelper, rename theme_specification_handler variable

79460f5

Fix Rubocop offenses

956a899

Use real files in unit tests

8b4625f

Split theme_directory?, avoid interruption during unit tests

056bdd3

Add theme directory check to serve command

a10cdcb

Add theme directory check to push command

ccbe295

Add theme/empty directory check to pull command

652ebde

Add --force flag to all commands

74bad58

Use project_context instead of Dir.chdir

a6b99a9

return false if @ctx.testing?

34e3834

Rename test, use FileUtils instead of Dir

81b4083

Poitrin commented Nov 9, 2022

View reviewed changes

Julien Poitrin added 2 commits November 9, 2022 11:43

Update CHANGELOG.md

8c6c771

Add --force flag to help messages

a490f24

Poitrin changed the title ~~Validate on shopify theme share/pull/push/serve/dev if users are running the command in a theme/empty directory~~ Validate on shopify theme share/pull/push/serve if users are running the command in a theme/empty directory Nov 9, 2022

Poitrin mentioned this pull request Nov 9, 2022

Add --force flag for validation on shopify theme dev/pull/push/share if users are running the command in a theme directory Shopify/cli#751

Merged

5 tasks

Poitrin commented Nov 9, 2022

View reviewed changes

Poitrin marked this pull request as ready for review November 9, 2022 12:42

Poitrin requested review from a team November 9, 2022 12:42

mgmanzella previously requested changes Nov 9, 2022

View reviewed changes

pepicrft reviewed Nov 10, 2022

View reviewed changes

Comment thread lib/project_types/theme/models/specification_handlers/theme.rb

pepicrft reviewed Nov 10, 2022

View reviewed changes

pepicrft approved these changes Nov 10, 2022

View reviewed changes

Julien Poitrin added 9 commits November 10, 2022 11:21

Simplify logic by getting rid of missing_folders

0ed7366

Make logic more readable, introduce invalid_theme_directory? and exis…

eaaae91

…t_and_not_empty?

invalid_theme_directory? –> valid_theme_directory?

7c81632

ensure that specified_root is rmdir'ed

9f703db

Get rid of project_context("theme") in pull_test

8ada024

Get rid of project_context("theme") in push_test

b9eb985

Get rid of project_context("theme") in serve_test

1d72a40

Get rid of project_context("theme") in share_test

e1afbbd

Hide --force flag in docs

01859f4

Poitrin mentioned this pull request Nov 10, 2022

[Enhancement] Introduce the -f/--force flag as a pattern for confirmation dialogues in the whole CLI Shopify/cli#760

Closed

Poitrin commented Nov 10, 2022

View reviewed changes

Poitrin requested a review from mgmanzella November 10, 2022 15:19

mgmanzella approved these changes Nov 10, 2022

View reviewed changes

Raise error, instead of returning false, if current_directory_confirm…

c3feed5

…ed? is called during tests

karreiro reviewed Nov 11, 2022

View reviewed changes

Comment thread lib/project_types/theme/models/specification_handlers/theme.rb Outdated

Comment thread lib/project_types/theme/commands/pull.rb

karreiro approved these changes Nov 11, 2022

View reviewed changes

Use @ root, instead of attr_accessor

c5a6c21

Poitrin merged commit a8c5922 into main Nov 11, 2022

Poitrin deleted the fix-2669 branch November 11, 2022 10:19

isaacroldan mentioned this pull request Nov 14, 2022

Packaging for release v2.32.0 #2689

Merged

illarionvk mentioned this pull request Nov 17, 2022

[Bug]: Help message for shopify theme pull doesn't include the --force flag #2690

Closed

3 tasks

This was referenced Feb 14, 2023

Remove theme directory confirmation during tests and make confirmation dialogue respect SHOPIFY_CLI_TTY Shopify/cli#1369

Merged

Remove theme directory confirmation during tests and make confirmation dialogue respect SHOPIFY_CLI_TTY #2730

Closed

Conversation

Poitrin commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

WHAT is this pull request doing?

How to test your changes?

No confirmation needed

Confirmation needed

Post-release steps

Update checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgmanzella left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgmanzella commented Nov 9, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pepicrft Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pepicrft left a comment

Poitrin commented Nov 8, 2022 •

edited

Loading

pepicrft Nov 10, 2022 •

edited

Loading