Merge pull request #739 from Shopify/make_tight_theme_id_constraint

Making theme ID required to make it harder to edit a live theme

cmd/get.go

@@ -28,6 +28,10 @@ var getCmd = &cobra.Command{
  For more documentation please see http://shopify.github.io/themekit/commands/#get
  `,
 	RunE: func(cmd *cobra.Command, args []string) error {
+		// This is a hack to get around theme ID validation for the list operation which doesnt need it
+		if flags.List {
+			flags.ThemeID = "1337"
+		}
 		return cmdutil.ForDefaultClient(flags, args, getTheme)
 	},
 }

cmd/theme.go

@@ -88,6 +88,7 @@ func init() {
 	ThemeCmd.PersistentFlags().StringArrayVar(&flags.IgnoredFiles, "ignored-file", []string{}, "A single file to ignore, use the flag multiple times to add multiple.")
 	ThemeCmd.PersistentFlags().StringArrayVar(&flags.Ignores, "ignores", []string{}, "A path to a file that contains ignore patterns.")
 	ThemeCmd.PersistentFlags().BoolVar(&flags.DisableIgnore, "no-ignore", false, "Will disable config ignores so that all files can be changed")
+	ThemeCmd.PersistentFlags().BoolVar(&flags.AllowLive, "allow-live", false, "Will allow themekit to make changes to the live theme on the store.")
 
 	watchCmd.Flags().StringVarP(&flags.NotifyFile, "notify", "n", "", "file to touch when workers have gone idle")
 	watchCmd.Flags().BoolVarP(&flags.AllEnvs, "allenvs", "a", false, "run command with all environments")

docs/commands/index.md

@@ -104,8 +104,7 @@ development:
 |**Required Flags**||
 |`-p`|`--password`| Password for access to your Shopify account.
 |`-s`|`--store   `| Your store's domain for changes to take effect
-|**Optional Flags**||
-|`-t`|`--themeid `| The ID of the theme that you want changes to take effect, if no theme id is passed, your live theme will be fetched
+|`-t`|`--themeid `| The ID of the theme that you want changes to take effect.
 
 ## New
 

docs/configuration/index.md

@@ -15,7 +15,7 @@ There are general values that you will be able to config for all of Theme Kit ac
 | config       | A custom path to your config file. This defaults to the current path that the command is run in.
 | store        | Your store's Shopify domain with the `.myshopify.com` postfix. Please see the [setup docs]({{ '/#get-api-access' | prepend: site.baseurl }}) on how to get this value.
 | password     | Your API password. Please see the [setup docs]({{ '/#get-api-access' | prepend: site.baseurl }}) on how to get this value.
-| theme_id     | The theme that you want the command to take effect on. If you want to make changes to the current live theme you may set this value to `'live'`. Please see the [setup docs]({{ '/#get-api-access' | prepend: site.baseurl }}) on how to get this value.
+| theme_id     | The theme that you want the command to take effect on. Please see the [setup docs]({{ '/#get-api-access' | prepend: site.baseurl }}) on how to get this value.
 | directory    | The project root directory. This allows you to run the command from another directory.
 | ignore_files | A list of patterns to ignore when executing commands. Please see the [Ignore Patterns]({{ '/ignores' | prepend: site.baseurl }})  documentation.
 | ignores      | A list of file paths to files that contain ignore patterns. Please see the [Ignore Patterns]({{ '/ignores' | prepend: site.baseurl }})  documentation.

src/cmdutil/_testdata/config.yml

@@ -1,11 +1,13 @@
 development:
+  theme_id: 123
   password: abracadabra
   store: store.myshopify.com
   ignore_files:
   - charmander
   - bulbasaur
   - squirtle
 production:
+  theme_id: 123
   password: abracadabra
   store: store.myshopify.com
   ignore_files:

src/cmdutil/util.go

@@ -21,7 +21,10 @@ import (
 )
 
 // ErrReload is an error to return from a command if you want to reload and run again
-var ErrReload = errors.New("reloading config")
+var (
+	ErrReload    = errors.New("reloading config")
+	ErrLiveTheme = errors.New("cannot make changes to a live theme without an override")
+)
 
 // Flags encapsulates all the possible flags that can be set in the themekit
 // command line. Some of the values are used across different commands
@@ -50,6 +53,7 @@ type Flags struct {
 	With                  string
 	List                  bool
 	NoDelete              bool
+	AllowLive             bool
 }
 
 // Ctx is a specific context that a command will run in
@@ -109,13 +113,19 @@ func createCtx(newClient clientFact, conf env.Conf, e *env.Env, flags Flags, arg
 	if setTheme {
 		for _, theme := range themes {
 			if theme.Role == "main" {
-				if fmt.Sprintf("%v", theme.ID) == e.ThemeID || e.ThemeID == "" {
-					e.ThemeID = fmt.Sprintf("%v", theme.ID) // record the theme id for the live id
+				if fmt.Sprintf("%v", theme.ID) == e.ThemeID && flags.AllowLive {
 					colors.ColorStdOut.Printf(
 						"[%s] Warning, this is the live theme on %s.",
 						colors.Yellow(e.Name),
 						colors.Yellow(shop.Name),
 					)
+				} else if fmt.Sprintf("%v", theme.ID) == e.ThemeID && !flags.AllowLive {
+					colors.ColorStdOut.Printf(
+						"[%s] This is the live theme on %s. If you wish to make changes to it, then you will have to pass the --allow-live flag",
+						colors.Red(e.Name),
+						colors.Yellow(shop.Name),
+					)
+					return &Ctx{}, ErrLiveTheme
 				}
 				break
 			}

src/cmdutil/util_test.go

@@ -50,12 +50,12 @@ func TestCreateCtx(t *testing.T) {
 		assert.Contains(t, err.Error(), "[API] Invalid API key or access token (unrecognized login or wrong password)")
 	}
 
-	e = &env.Env{Proxy: "http://localhost:3000"}
+	e = &env.Env{ThemeID: "1234", Proxy: "http://localhost:3000"}
 	client = new(mocks.ShopifyClient)
 	client.On("GetShop").Return(shopify.Shop{}, nil)
 	client.On("Themes").Return([]shopify.Theme{{ID: 65443, Role: "unpublished"}, {ID: 1234, Role: "main"}}, nil)
 	_, err = createCtx(factory, env.Conf{}, e, Flags{DisableIgnore: true}, []string{}, nil, true)
-	assert.Nil(t, err)
+	assert.Equal(t, ErrLiveTheme, err)
 	assert.Equal(t, e.ThemeID, "1234")
 }
 
@@ -96,7 +96,7 @@ func TestCtx_DoneTask(t *testing.T) {
 func TestGenerateContexts(t *testing.T) {
 	factory := func(*env.Env) (shopifyClient, error) { return nil, nil }
 	_, err := generateContexts(factory, nil, Flags{Environments: []string{"development"}}, []string{})
-	assert.EqualError(t, err, "invalid environment [development]: (missing store domain,missing password)")
+	assert.EqualError(t, err, "invalid environment [development]: (missing theme_id,missing store domain,missing password)")
 
 	client := new(mocks.ShopifyClient)
 	factory = func(*env.Env) (shopifyClient, error) { return client, nil }
@@ -109,7 +109,7 @@ func TestGenerateContexts(t *testing.T) {
 	client = new(mocks.ShopifyClient)
 	factory = func(*env.Env) (shopifyClient, error) { return client, nil }
 	_, err = generateContexts(factory, nil, Flags{ConfigPath: "_testdata/config.yml", Environments: []string{"nope"}}, []string{})
-	assert.EqualError(t, err, "invalid environment [nope]: (missing store domain,missing password)")
+	assert.EqualError(t, err, "invalid environment [nope]: (missing theme_id,missing store domain,missing password)")
 
 	client = new(mocks.ShopifyClient)
 	factory = func(*env.Env) (shopifyClient, error) { return client, fmt.Errorf("not today") }
@@ -302,7 +302,7 @@ func TestForDefaultClient(t *testing.T) {
 
 	factory := func(*env.Env) (shopifyClient, error) { return nil, nil }
 	err := forDefaultClient(factory, Flags{}, []string{}, safeHandler)
-	assert.EqualError(t, err, "invalid environment [development]: (missing store domain,missing password)")
+	assert.EqualError(t, err, "invalid environment [development]: (missing theme_id,missing store domain,missing password)")
 
 	client := new(mocks.ShopifyClient)
 	factory = func(*env.Env) (shopifyClient, error) { return client, nil }
@@ -315,19 +315,19 @@ func TestForDefaultClient(t *testing.T) {
 	factory = func(*env.Env) (shopifyClient, error) { return client, nil }
 	client.On("GetShop").Return(shopify.Shop{}, nil)
 	client.On("Themes").Return([]shopify.Theme{}, nil)
-	err = forDefaultClient(factory, Flags{Domain: "shop.myshopify.com", Password: "123"}, []string{}, safeHandler)
+	err = forDefaultClient(factory, Flags{Domain: "shop.myshopify.com", Password: "123", ThemeID: "123"}, []string{}, safeHandler)
 	assert.Nil(t, err)
 
 	client = new(mocks.ShopifyClient)
 	factory = func(*env.Env) (shopifyClient, error) { return client, fmt.Errorf("server err") }
-	err = forDefaultClient(factory, Flags{Domain: "shop.myshopify.com", Password: "123"}, []string{}, safeHandler)
+	err = forDefaultClient(factory, Flags{Domain: "shop.myshopify.com", Password: "123", ThemeID: "123"}, []string{}, safeHandler)
 	assert.EqualError(t, err, "server err")
 
 	client = new(mocks.ShopifyClient)
 	factory = func(*env.Env) (shopifyClient, error) { return client, nil }
 	client.On("GetShop").Return(shopify.Shop{}, nil)
 	client.On("Themes").Return([]shopify.Theme{}, nil)
-	err = forDefaultClient(factory, Flags{Domain: "shop.myshopify.com", Password: "123"}, []string{}, errHandler)
+	err = forDefaultClient(factory, Flags{Domain: "shop.myshopify.com", Password: "123", ThemeID: "123"}, []string{}, errHandler)
 	assert.EqualError(t, err, gandalfErr.Error())
 
 	stdErr := bytes.NewBufferString("")

src/env/_testdata/projectdir/valid_config.yml

@@ -1,4 +1,5 @@
 development:
+  theme_id: 123
   password: ${STOREPASS}
   store: $STOREDOMAIN
   ignore_files:

src/env/conf_test.go

@@ -78,8 +78,8 @@ func TestConf_Set(t *testing.T) {
 	}{
 		{name: "", initial: Env{}, err: ErrInvalidEnvironmentName.Error()},
 		{name: "development", initial: Env{}, err: "invalid environment"},
-		{name: "development", initial: Env{Domain: "yes.myshopify.com", Password: "abc123"}, expected: Env{Name: "development", Domain: "yes.myshopify.com", Password: "abc123", Directory: Default.Directory, Timeout: Default.Timeout}},
-		{name: "development", initial: Env{Domain: "yes.myshopify.com", Password: "abc123", Directory: filepath.Join("..", "file")}, expected: Env{Name: "development", Domain: "yes.myshopify.com", Password: "abc123", Directory: dir, Timeout: Default.Timeout}},
+		{name: "development", initial: Env{ThemeID: "123", Domain: "yes.myshopify.com", Password: "abc123"}, expected: Env{ThemeID: "123", Name: "development", Domain: "yes.myshopify.com", Password: "abc123", Directory: Default.Directory, Timeout: Default.Timeout}},
+		{name: "development", initial: Env{ThemeID: "123", Domain: "yes.myshopify.com", Password: "abc123", Directory: filepath.Join("..", "file")}, expected: Env{ThemeID: "123", Name: "development", Domain: "yes.myshopify.com", Password: "abc123", Directory: dir, Timeout: Default.Timeout}},
 		{name: "development", initial: Env{Domain: "yes.myshopify.com", Password: "abc123"}, overrides: []Env{{ThemeID: "12345"}}, expected: Env{Name: "development", Domain: "yes.myshopify.com", Password: "abc123", ThemeID: "12345", Directory: Default.Directory, Timeout: Default.Timeout}},
 	}
 
@@ -108,7 +108,7 @@ func TestConf_Get(t *testing.T) {
 		err                  error
 		overrides            []Env
 	}{
-		{path: "_testdata/projectdir/valid_config.yml", toGet: "development", err: nil},
+		{path: "_testdata/projectdir/valid_config.yml", toGet: "development", err: nil, themeid: "123"},
 		{path: "_testdata/projectdir/valid_config.yml", toGet: "development", err: nil, themeid: "12345", overrides: []Env{{ThemeID: "12345"}}},
 		{path: "_testdata/projectdir/valid_config.yml", toGet: "nope", err: ErrEnvDoesNotExist},
 		{path: "_testdata/projectdir/bad_format.yml", toGet: "other", err: ErrEnvNotDefined},

src/env/env.go

@@ -50,12 +50,11 @@ func (env *Env) validate() error {
 	errors := []string{}
 
 	env.ThemeID = strings.ToLower(strings.TrimSpace(env.ThemeID))
-	if env.ThemeID != "" {
-		if env.ThemeID == "live" {
-			env.ThemeID = ""
-		} else if _, err := strconv.ParseInt(env.ThemeID, 10, 64); err != nil {
-			errors = append(errors, "invalid theme_id")
-		}
+
+	if env.ThemeID == "" {
+		errors = append(errors, "missing theme_id")
+	} else if _, err := strconv.ParseInt(env.ThemeID, 10, 64); err != nil {
+		errors = append(errors, "invalid theme_id")
 	}
 
 	if len(env.Domain) == 0 {

src/env/env_test.go

@@ -45,13 +45,13 @@ func TestEnv_Validate(t *testing.T) {
 		notwindows bool
 	}{
 		{env: Env{Password: "file", ThemeID: "123", Domain: "test.myshopify.com"}},
-		{env: Env{Password: "file", ThemeID: "live", Domain: "test.myshopify.com"}},
+		{env: Env{Password: "file", ThemeID: "live", Domain: "test.myshopify.com"}, err: "invalid theme_id"},
 		{env: Env{ThemeID: "123", Domain: "test.myshopify.com"}, err: "missing password"},
 		{env: Env{Password: "test", ThemeID: "123", Domain: "test.nope.com"}, err: "invalid store domain"},
 		{env: Env{Password: "test", ThemeID: "123"}, err: "missing store domain"},
-		{env: Env{Password: "test", Domain: "test.myshopify.com"}},
+		{env: Env{Password: "test", Domain: "test.myshopify.com"}, err: "missing theme_id"},
 		{env: Env{Password: "file", ThemeID: "abc", Domain: "test.myshopify.com"}, err: "invalid theme_id"},
-		{notwindows: true, env: Env{Password: "abc123", Domain: "test.myshopify.com", Directory: filepath.Join("_testdata", "symlink_projectdir")}},
+		{notwindows: true, env: Env{Password: "abc123", Domain: "test.myshopify.com", ThemeID: "123", Directory: filepath.Join("_testdata", "symlink_projectdir")}},
 		{notwindows: true, env: Env{Password: "abc123", Domain: "test.myshopify.com", Directory: filepath.Join("_testdata", "bad_symlink")}, err: "invalid project symlink"},
 		{notwindows: true, env: Env{Password: "abc123", Domain: "test.myshopify.com", Directory: filepath.Join("_testdata", "symlink_file")}, err: "is not a directory"},
 		{env: Env{Password: "abc123", Domain: "test.myshopify.com", Directory: "not_a_dir"}, err: "invalid project directory"},

src/shopify/theme_client.go

@@ -26,9 +26,9 @@ var (
 	ErrMalformedResponse = errors.New("received a malformed response from shopify, this usually indicates a problem with your connection")
 	// ErrZipPathRequired is returned if a source path was not provided to create a new theme
 	ErrZipPathRequired = errors.New("theme zip path is required")
-	// ErrInfoWithoutThemeID will be returned if GetInfo is called on a live theme
+	// ErrInfoWithoutThemeID will be returned if GetInfo is called without a theme ID
 	ErrInfoWithoutThemeID = errors.New("cannot get info without a theme id")
-	// ErrPublishWithoutThemeID will be returned if PublishTheme is called on a live theme
+	// ErrPublishWithoutThemeID will be returned if PublishTheme is called without a theme ID
 	ErrPublishWithoutThemeID = errors.New("cannot publish a theme without a theme id set")
 	// ErrThemeNotFound will be returned if trying to get a theme that does not exist
 	ErrThemeNotFound = errors.New("requested theme was not found")