[Scripts] Handle Configuration Definition Error

[adampetro]

WHY are these changes introduced?

Closes https://github.com/Shopify/script-service/issues/4086

We want to handle the configuration_definition_error error tag on the appScriptSet mutation with script-service.

WHAT is this pull request doing?

The first commit of this PR adds handling of the case when the error tag on the appScriptSet mutation is configuration_definition_error so that we can display a nice error message that is actually helpful.

This is what it looks like. Note that Error at myString: must be an integer. comes entirely from the server so if we don't like the structure of that sentence it is not a change that can be made in this PR. @kwringe WDYT?

┏━━ Checking dependencies with npm ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
┃ ✓ None required
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ (0.0s) ━━
┏━━ Building ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
┃ ✓ Built
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ (3.48s) ━━
✗ Pushing
✗ Error
Couldn't push script to app. There was a problem with the configuration defined in script.config.yml. Error at `myString`: must be an integer. Fix the error and try again.

The second commit of this PR refactors a bunch of configuration-related errors to have a generic filename so that they actually display the correct filename (this was an oversight of mine in #1826). I realize that we will soon be getting rid of much of the V1 configuration and corresponding errors, as well as script.json, but nonetheless I believe it is good to have this be generic so that in the future it is easier to change the name of the configuration file, should we need that.

If you think I should do this in two separate PRs, just let me know and I will break it up.

How to test your changes?

There are many ways to see the V2 configuration error on push. One such way would be with the following script.config.yml:

---
version: '2'
title: configuration-definition-error
description: Shipping methods default script
configuration:
  type: object
  fields:
    myString:
      type: number_integer
      name: My String
      options:
        - type: min
          value: "asdf"

To 🎩 the refactor of the existing configuration errors, there are a number of things you can do:

• no config file
• invalid JSON in script.json
• invalid YAML in script.config.yml
• V1 configuration with the type invalid (not list or single)
• V1 configuration missing type
• V1 configuration with schema as an object instead of array
• V1 configuration with field that has invalid type (not single_line_text_field)
• V1 configuration with field that is missing type

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.

[kwringe]

This is what it looks like. Note that Error at myString: must be an integer. comes entirely from the server so if we don't like the structure of that sentence it is not a change that can be made in this PR. @kwringe WDYT?

It's a shame that we can't do much with the format of that string. Even just being able to add the word value or field I think would help. myString field: value must be an integer.

When you say that this error comes from the server -- is this server under our or metafield's control at all?

[adampetro]

When you say that this error comes from the server -- is this server under our or metafield's control at all?

The part before the colon is something we do in script-service here, while the part after the colon is either from metafields or us, depending on the error. In this case, it comes from metafields (here).

[pepicrft]

@MeredithCastile there have been some content changes here. It'd be great to get your thoughts on those.

[adampetro]

With the exception of the added messages (configuration_error_cause and configuration_error_help), all of the messages should be the same, just with placeholders for the file name during this grace period when we support both script.config.yml and script.json, and to make it easier to change the filename in the future (not hard-coded in a bunch of strings).

[kwringe]

When you say that this error comes from the server -- is this server under our or metafield's control at all?

The part before the colon is something we do in script-service here, while the part after the colon is either from metafields or us, depending on the error. In this case, it comes from metafields (here).

Thanks for this context Adam.
Could we say this then?
Couldn't push script to app. In the script.config.yml file, the `myString` field has an error. It's value must be an integer.
Or
Couldn't push script to app. In the script.config.yml file, the `myString` value must be an integer.

[adampetro]

Thanks for this context Adam.
Could we say this then?
Couldn't push script to app. In the script.config.yml file, the `myString` field has an error. It's value must be an integer.
Or
Couldn't push script to app. In the script.config.yml file, the `myString` value must be an integer.

Ideally:

1. script-service doesn't know or care about the fact that the configuration file is called script.config.yml.
2. script-service returns complete sentences.

Would something like this work? Couldn't push script to app. In the script.config.yml file, there was a problem with the configuration. ...

Then for the ..., that would be a change in script-service, which I am happy to do. I am a bit wary about the concatenation proposed because I'm not sure that the sentence will always make sense given the different error messages, hence why we currently have a colon. For example, this error message wouldn't really work with the proposed concatenation.

[kwringe]

Thanks for this context Adam.
Could we say this then?
Couldn't push script to app. In the script.config.yml file, the `myString` field has an error. It's value must be an integer.
Or
Couldn't push script to app. In the script.config.yml file, the `myString` value must be an integer.

Ideally:

1. script-service doesn't know or care about the fact that the configuration file is called script.config.yml.
2. script-service returns complete sentences.

Would something like this work? Couldn't push script to app. In the script.config.yml file, there was a problem with the configuration. ...

Then for the ..., that would be a change in script-service, which I am happy to do. I am a bit wary about the concatenation proposed because I'm not sure that the sentence will always make sense given the different error messages, hence why we currently have a colon. For example, this error message wouldn't really work with the proposed concatenation.

This sounds reasonable!