Skip to content

scripted fields preview and validate script before saving - #20746

Merged
nreese merged 19 commits into
elastic:masterfrom
nreese:scripted_fields_preview
Jul 18, 2018
Merged

nreese merged 19 commits into
elastic:masterfrom
nreese:scripted_fields_preview

Conversation

@nreese

@nreese nreese commented Jul 12, 2018 •

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@cjcenizal

Copy link
Copy Markdown
Contributor

This also fixes #5088

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@cjcenizal

Copy link
Copy Markdown
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
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine

Copy link
Copy Markdown
Contributor

💚 Build Succeeded

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

@jen-huang jen-huang left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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

nreese commented Jul 17, 2018

Copy link
Copy Markdown
Contributor Author

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

@jen-huang

jen-huang commented Jul 17, 2018 •

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
Contributor

💚 Build Succeeded

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

Copy link
Copy Markdown
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
Copy Markdown
Contributor

💔 Build Failed

@cjcenizal cjcenizal left a comment •

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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

nreese commented Jul 18, 2018

Copy link
Copy Markdown
Contributor Author

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

<EuiFormRow
label="Script"
helpText={(<EuiLink onClick={this.showScriptingHelp}>Scripting help</EuiLink>)}
helpText={(

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

💚 Build Succeeded

@nreese

nreese commented Jul 18, 2018

Copy link
Copy Markdown
Contributor Author

jenkins, test this

@elasticmachine

Copy link
Copy Markdown
Contributor

💚 Build Succeeded

@malcolmm83

Copy link
Copy Markdown

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

nreese commented Dec 26, 2019

Copy link
Copy Markdown
Contributor Author

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

patrykkopycinski pushed a commit to patrykkopycinski/kibana that referenced this pull request May 6, 2026
)

* add preview for creating scripted fields

* Move previewed results into flyout.  (elastic#25)

* Move previewed results into flyout. Execute script as soon as the tab is selected.

* Format error in a danger callout.

* add functional test for preview

* move executeScript functionallity into lib so it can be used to validate on save

* validate script on save

* add functional test for script validation during save

* fix jest tests

* fix jest tests

* only validate script when field is scripted

* only add script callouts and help flyout to DOM for scripted fields

* move scripting stuff to its own render method

* right justify delete button to have same style as delete button for user management

* use data-test-subj key to find invalid script error msg in functional tests

* move help flyout link out of formRow help text and into its own row
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