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

scripted fields preview and validate script before saving #20746

Merged
merged 19 commits into from
Jul 18, 2018

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Jul 12, 2018

fixes #19504, #11804

This PR adds the ability to run scripted fields in the script edit view that allows the user to view the results and see if the script works as intended. Users can select additional fields to view in the preview to provide document value context.

screen shot 2018-07-13 at 7 22 27 pm

And the request generated to preview the results

{
  "query": {
    "match_all": {}
  },
  "script_fields": {
    "kb": {
      "script": {
        "lang": "painless",
        "source": "doc['bytes'].value * 0.001"
      }
    }
  },
  "size": 10,
  "_source": [
    "bytes"
  ]
}

@elasticmachine
Copy link
Contributor

💔 Build Failed

cjcenizal and others added 2 commits July 13, 2018 14:41
* Move previewed results into flyout. Execute script as soon as the tab is selected.

* Format error in a danger callout.
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@cjcenizal
Copy link
Contributor

This also fixes #5088

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@cjcenizal
Copy link
Contributor

This also partially fixes #14546

@nreese nreese changed the title scripted fields preview scripted fields preview and validate script before saving Jul 16, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@nreese nreese removed the WIP Work in progress label Jul 17, 2018
@nreese nreese requested review from jen-huang and cjcenizal July 17, 2018 02:26
@nreese nreese added the review label Jul 17, 2018
Copy link
Contributor

@jen-huang jen-huang left a comment

Choose a reason for hiding this comment

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

Very nice!! Tested locally and it functions well. Just have a small suggestion and noticed a few console errors:

screen shot 2018-07-17 at 3 02 43 pm

Otherwise LGTM!

<h3>Preview results</h3>
<p>
Run your script to preview the first 10 results. You can also select some
additional fields to include in your results to gain more context.
Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice to clarify this a little bit. initially, I thought that adding additional fields somehow affected my actual script, but they are only shown as part of the preview.

not sure if there's value, but maybe we can also add a "Show query" link that displays the request, which will include the additional _source fields

Copy link
Contributor

Choose a reason for hiding this comment

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

How about this:

Run your script to preview the first 10 results. You can also select some
additional fields to include in each result to help tell your results apart.

@nreese
Copy link
Contributor Author

nreese commented Jul 17, 2018

How did you produce those console warnings? I am unable to re-create them

@jen-huang
Copy link
Contributor

jen-huang commented Jul 17, 2018

@nreese Click 'Edit' on an existing field (regular field, not scripted field). You should see the console errors right away. If not, refresh the edit page. I should have tried to replicate with my original review, since it took me a little while to find it again 😄

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

isVisible={field.scripted && showScriptingHelp}
onClose={this.hideScriptingHelp}
/>
{scriptDisabledCallout}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd love to see this moved out to a separate renderScriptingPanels() method or similar, which can then check field.scripted and return these components, or null. then this render method can avoid the let vars and use {this.renderScriptingPanels()}

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Looks great! Just had one suggestion. Reviewed code and UI.

const { field, hasScriptError } = this.state;
const isInvalid = !field.script || !field.script.trim() || hasScriptError;
const errorMsg = hasScriptError
? 'Script is invalid. View script preview for details'
Copy link
Contributor

Choose a reason for hiding this comment

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

You can wrap this in a span with a test subject selector for use in tests:

<span data-test-subj="invalidScriptError">Script is invalid. View script preview for details.</span>

await PageObjects.settings.setScriptedFieldScript(`doc['iHaveNoClosingTick].value`);
await PageObjects.settings.clickSaveScriptedField();
await retry.try(async () => {
const formErrorExists = await find.existsByDisplayedByCssSelector('.euiFormErrorText');
Copy link
Contributor

Choose a reason for hiding this comment

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

Then we can find the error with the test subject selector instead of the className.

@nreese
Copy link
Contributor Author

nreese commented Jul 18, 2018

The last commit just moves the delete button to the right to be like other delete buttons

screen shot 2018-07-18 at 12 19 10 pm


return field.scripted ? (
<EuiFormRow
label="Script"
helpText={(<EuiLink onClick={this.showScriptingHelp}>Scripting help</EuiLink>)}
helpText={(
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove this and instead have renderScript return the help as a paragraph, to give it a bit more visual prominence:

    return field.scripted ? (
      <Fragment>
        <EuiFormRow
          label="Script"
          isInvalid={isInvalid}
          error={isInvalid ? errorMsg : null}
        >
          <EuiTextArea
            value={field.script}
            data-test-subj="editorFieldScript"
            onChange={this.onScriptChange}
            isInvalid={isInvalid}
          />
        </EuiFormRow>

        <EuiFormRow>
          <EuiText>
            Access fields with <code>doc['some_field'].value</code>. You can also{' '}
            <a onClick={this.showScriptingHelp} data-test-subj="scriptedFieldsHelpLink">
              get help with the syntax and preview the results of your script
            </a>.
          </EuiText>
        </EuiFormRow>
      </Fragment>
    ) : null;

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately EuiText makes that link keyboard-inaccessible. Here's new code which will make the link keyboard-accessible.

    return field.scripted ? (
      <Fragment>
        <EuiFormRow
          label="Script"
          isInvalid={isInvalid}
          error={isInvalid ? errorMsg : null}
        >
          <EuiTextArea
            value={field.script}
            data-test-subj="editorFieldScript"
            onChange={this.onScriptChange}
            isInvalid={isInvalid}
          />
        </EuiFormRow>

        <EuiFormRow>
          <Fragment>
            <EuiText>Access fields with <code>doc['some_field'].value</code>.</EuiText>
            <br />
            <EuiLink onClick={this.showScriptingHelp} data-test-subj="scriptedFieldsHelpLink">
              Get help with the syntax and preview the results of your script.
            </EuiLink>
          </Fragment>
        </EuiFormRow>
      </Fragment>
    ) : null;

It does affect the formatting though.

image

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@nreese
Copy link
Contributor Author

nreese commented Jul 18, 2018

jenkins, test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@malcolmm83
Copy link

I would love to take advantage of this feature, but the index I'm using includes several log files, and running on the first 10 entries doesn't get to the log file I need. It would be great if I could add a filter so I can test on the right log entries.

@nreese
Copy link
Contributor Author

nreese commented Dec 26, 2019

@malcolmm83 There is a community PR open that adds filtering to scripted fields preview, #44220.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Easy debug / test of scripted fields
5 participants