Themekit: theme_id as string defaults to live theme

[macdonaldr93]

This one was a bit of a scary one haha. We wrote a CLI tool to help us work faster. While converting JSON to YML, the theme_id wasn't parsed as an integer which caused ThemeKit to default to the live theme.

development:
  theme_id: "1111111111"
  password: "apipassword"
  store: "test.myshopify.com"
  ignore_files:
    - config/settings_data.json
    - config/settings.html
    - "*.png"
    - "*.jpg"
    - "*.jpeg"    
    - "*.gif"
    - "*.pdf" 
    - "*.mp4" 

[jtgrenz]

for safety sake, it might be a good idea to not default to the published live theme if the id is left blank or is a non-integer. If you don't know what you are doing, or you are working on integrating another tool like above, you can replace a published theme unintentionally with no way to restore it. ThemeKit should probably parse a string for theme_id and it should probably take an explicit value for the published theme ie "published" or "live" or 0; something that is very much intentional.

development:
  theme_id: "published"
  password: "apipassword"
  store: "test.myshopify.com"
...

[mcpatten]

@chrisbutcher @ilikeorangutans @Thibaut @davidcornu

I think this is a pretty critical issue. Is ThemeKit the right place to resolve this, or does this need to be handled on the OSE side?

[jtgrenz]

@m-ux I'm thinking this is definitely a themeKit issue. I can't think of a way to alter this on the on the OSE without locking the published theme from API modifications which I don't think is the right action. Working on the live theme is great in a dev shop. Although, I would LOVE the _option_ to lock a theme from any sort of updates, or updates from a single source, thats a whole different issue.

With this, I think a blank or non-integer theme_id should trip an error and shouldn't be caught and defaulted to the live theme. I can't think of an instance where an implied default is beneficial here where a special string ("live" or "published") wouldn't accomplish the same thing with greater safety.

[jtgrenz]

scratch that, it might not be a bad idea to globally lock the live theme if the store front password is disabled. That would force all shops to either be in development mode (password up), or to use an unpublished theme for API alterations, a very good practice to encourage, if not enforce.

ThemeKit would still benefit from throwing an error though all the same, even if Shopify could prevent the published theme from being edited on a live site.

[mcpatten]

Okay, so I think we're going to wrap this with a warning on the slate end for now, and in the near future build in a slate task to create a new theme on the selected store using the theme create endpoint.

[chryton]

@jtgrenz by lock do you mean locking only write access or read and write? I can see the reasoning to block write access but not reasoning to block read.

[jtgrenz]

@chryton just write. There's never any harm in anyone seeing code thats live or being worked. We already have the ability to bar apps and staff from the themes section entirely, I don't think it needs to be more granular than that.

[chryton]

@jtgrenz Just checking; we sync down to our locals from the live theme often so that would have been a big problem for keeping everything in sync. Thanks for the clarification!

[chrisbutcher]

Hey all. I'm just back from vacation this week, but I'll do a patch along the lines of @jtgrenz's suggestion (error out if the theme_id is non-integer).

Thanks Ryan for the bug report and everyone for their input!

[chrisbutcher]

@macdonaldr93 @m-ux

I have a branch that fixes this to ensure that theme_id, when specified as a string (i.e. an integer wrapped in quotes), is parsed correctly. Two other small points:
See http://themekit.cat/docs/#config-variables

Note that defaulting to the live, production theme is currently a default behaviour:
theme_id: The ID of the theme to upload changes to. Beware that if this is blank all changes will be uploaded to your user visible theme.

Do we want to keep this default behavior? Two alternatives are: abort with an error if the theme_id is not specified, and make it a required field to theme configure / required in config.yml. Edit: Perhaps we could fetch all theme names and ids during the configure step and ask the merchant to pick one, so long as they specify their domain and api password.

Or, we can prompt the user with a [Y]es or [N]o prompt to proceed, despite a warning. I'm actually in favour of the first solution, since prompting them will become annoying and most will (I imagine) just decide to specify the theme id in config.yml. Also, adding that prompt makes themekit a bit weird to interact with when calling from other tools like Slate (Slate can't just call it and wait for a 0 return code, since the program might be blocking on user input).

What do you think?

[macdonaldr93]

@chrisbutcher that's awesome about the parsing. As for the default behaviour, I like your first solution. Without reading the docs, I expected that theme_id was a required field and that it would throw an error if I didn't specify one. During theme configure, it would be amazing if it prompted for domain, api key, and then fetched the possible theme ids with the theme title for the user to select.

Edit: ^^ I suppose that will give the same problem you mentioned at the end about blocking user input... Maybe it would be best to just ask for the id.

One last scenario that I'm unsure of is if the dev passes, for example, "live" as a string. Would you parsing just return a type error NaN?

[chrisbutcher]

@macdonaldr93 re: the last question. If the value cannot be parsed straightforwardly as a 64 bit integer, Theme Kit will assume you haven't entered a theme_id. How it behaves if one isn't specified or is invalid will be the same behaviour: this behaviour, I think should be to use the provided required fields domain and password/access_token (they are interchangeable) to fetch and list the theme ids for selection.

[stevebosworth]

@chrisbutcher this sounds great! I like the new default behaviour. When we grab the theme ids from the store, can we get any other identifying information? Theme ids are pretty hard to grok if your new to the shopify platform. IIRC you can only find the theme id in the admin by looking at the URL in the Theme Editor?

[macdonaldr93]

/admin/themes.json returns name, so we should be able to pull that?

[chrisbutcher]

Yeah I was thinking of something like this:

Choose a theme
[1] "Launchpad" (theme id 123)
[2] "Launchpad-Star-dev" (theme id 321)

[macdonaldr93]

Coolio, and just to confirm that would only be on theme configure? The other commands that interact with config.yml (replace, upload, etc.) would throw an error?

[chrisbutcher]

Yeah the prompt should just be on theme configure and running other commands should otherwise just fail with a message