[Console] Fix load from remote #52814

jloleysens · 2019-12-11T20:34:36Z

Summary

Addresses this issue #52747.

Some functionality was erroneously removed from Console here #52270, but in that PR the functionality was already not being used anywhere - looks like this PR broke it #45627.

Test on IE11
Test on FF
Test on Safari
Test on Chrome

This will have to be backported to 7.x and 7.5 as a fix release.

Release note

We fixed "View in Console" behaviour used when viewing a Console snippet from Elasticsearch docs.

elasticmachine · 2019-12-11T20:34:39Z

Pinging @elastic/es-ui (Team:Elasticsearch UI)

cjcenizal

I tested locally and these changes fix the bug, but as I noted in one of my comments it looks like the loaded data overwrites everything in the request panel. This seems like a poor UX. Was the original behavior also destructive? Maybe we should prepend the loaded request to the saved requests?

The code LGTM, I just had a couple questions/suggestions but nothing worth blocking on.

...ugins/console/public/np_ready/application/containers/editor/legacy/console_editor/editor.tsx

cjcenizal · 2019-12-11T21:35:55Z

...ugins/console/public/np_ready/application/containers/editor/legacy/console_editor/editor.tsx

+        };
+
+        if (/https?:\/\/api\.github\.com/.test(sourceLocation)) {
+          loadFrom.headers = { Accept: 'application/vnd.github.v3.raw' };


What use case does this support?

This was copied from the previous behaviour, perhaps there is specific case for loading from github that required an additional header? I thought I would keep the legacy behaviour unchanged until we decide to officially deprecate "load from" behaviour or revisit it in future.

Sounds good, thanks for the explanation!

cjcenizal · 2019-12-11T21:38:40Z

...ugins/console/public/np_ready/application/containers/editor/legacy/console_editor/editor.tsx

+        }
+
+        $.ajax(loadFrom).done(async data => {
+          await editorInstanceRef.current!.update(data);


The behavior I'm seeing is that the loaded data overwrites everything in the request panel. Is this intended? Perhaps we should prepend it to the saved requests instead?

It looks like this is consistent with legacy behaviour:

Starting here:

kibana/src/legacy/core_plugins/console/public/src/app.js

Line 82 in 44a4cee

$.ajax(loadFrom).done((data) => {

Which would ultimately call:

kibana/src/legacy/core_plugins/console/public/src/sense_editor/editor.js

Line 170 in 44a4cee

editor.update = function (data, callback) {

Which sets the value for the entire current buffer. Also checked to on 7.4 branch to make doubly sure 😂

I agree that changing the current behaviour to rather prepend would be better, but I think once we have multiple buffers we can deal with this in a more elegant manner, perhaps with a dedicated "view in console" buffer - I wouldn't want to change the legacy behaviour in this fix. Wdyt?

I wouldn't want to change the legacy behaviour in this fix.

I agree!

cjcenizal · 2019-12-11T21:39:23Z

...ugins/console/public/np_ready/application/containers/editor/legacy/console_editor/editor.tsx

+    window.addEventListener('hashchange', onHashChange);
+
+    const firstParamsCheck = getQueryParams();
+    if (firstParamsCheck && firstParamsCheck.load_from) {


This check looks like a duplicate of the check on line 96. Maybe we only need one or the other?

So the two checks are for different points in time:

Startup - when we want to figure out should we load from remote or should we load from localStorage

Later, when hash changes occur, should we load from remote - we don't want to load from localStorage if not from remote in this case.

I guess the alternative could be to set a Ref and check if we have done the "initial load". But this way can be restructured to only do the check once

… method

cjcenizal

Tested locally, code LGTM. Thanks for fixing this and for the refactor!

* Fix load remote state * Clean up variable usage, add comment, move forceRetokenize to private method * Optimize sequence of checking hash on initial load

elasticmachine · 2019-12-12T17:12:29Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: fddaad4

History

💚 Build #15055 succeeded fddaad4
💚 Build #15047 succeeded c23b3e8
💚 Build #14947 succeeded 8cb1cfc

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* Fix load remote state * Clean up variable usage, add comment, move forceRetokenize to private method * Optimize sequence of checking hash on initial load

* [Console] Fix load from remote (#52814) * Fix load remote state * Clean up variable usage, add comment, move forceRetokenize to private method * Optimize sequence of checking hash on initial load * Fix backport

…aved-objects * upstream/master: (134 commits) [Dashboard] Add visualization from dasbhoard empty screen (elastic#52670) Print out agent debugging links during CI (elastic#52812) Add babel-plugin-styled-components to webpack config (elastic#52862) [Console] Fix load from remote (elastic#52814) Ensure APM agent config file path respects CWD (elastic#52880) [Watcher] Removed overwritten property (elastic#49998) [Data Plugin]: Remove `export *` for common code from public/server index files (elastic#52821) Hide stderr git output during APM agent configuration (elastic#52878) Polish migration.md (elastic#52764) Change ajax_stream to use new-line delimited JSON (elastic#52797) Stabilize dashboard save modal functional test (elastic#52761) [Discover] Place tooltip at bottom of filter button (elastic#52720) Disable/enable filter with click+shift on a filter badge (elastic#52751) [APM] Make client-side routes static (elastic#52574) [Maps] Get basic structure of NP client shim in place (elastic#52551) update chromedriver to 79 (elastic#52784) [DOCS] Adds example of assigning roles in Reporting (elastic#52757) Add instructions for setting up remote clusters needed for CCS and CCR (elastic#52796) [docs] max-old-space-size (elastic#52310) [Monitoring] Fix 7.5 cloud test issues (elastic#51781) ...

Fix load remote state

8cb1cfc

jloleysens added Feature:Console Dev Tools Console Feature release_note:fix v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.6.0 v7.5.1 labels Dec 11, 2019

jloleysens requested a review from cjcenizal December 11, 2019 20:34

jloleysens changed the title ~~[Console] Fix load remote from remote~~ [Console] Fix load from remote Dec 11, 2019

cjcenizal reviewed Dec 11, 2019

View reviewed changes

jloleysens added 2 commits December 12, 2019 10:45

Clean up variable usage, add comment, move forceRetokenize to private…

c23b3e8

… method

Optimize sequence of checking hash on initial load

fddaad4

cjcenizal approved these changes Dec 12, 2019

View reviewed changes

jloleysens merged commit 21334e7 into elastic:master Dec 12, 2019

jloleysens deleted the fix/console-load-remote branch December 12, 2019 15:15

This was referenced Dec 12, 2019

[7.x] [Console] Fix load from remote (#52814) #52895

Merged

[7.5] [Console] Fix load from remote (#52814) #52897

Merged

jloleysens added a commit that referenced this pull request Dec 13, 2019

[Console] Fix load from remote (#52814) (#52895)

ad5b013

* Fix load remote state * Clean up variable usage, add comment, move forceRetokenize to private method * Optimize sequence of checking hash on initial load

jloleysens mentioned this pull request Jan 8, 2020

View in Console Not Working #52747

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Console] Fix load from remote #52814

[Console] Fix load from remote #52814

jloleysens commented Dec 11, 2019 •

edited

Loading

elasticmachine commented Dec 11, 2019

cjcenizal left a comment

cjcenizal Dec 11, 2019

jloleysens Dec 12, 2019 •

edited

Loading

cjcenizal Dec 12, 2019

cjcenizal Dec 11, 2019

jloleysens Dec 12, 2019 •

edited

Loading

cjcenizal Dec 12, 2019

cjcenizal Dec 11, 2019

jloleysens Dec 12, 2019

cjcenizal left a comment

elasticmachine commented Dec 12, 2019

[Console] Fix load from remote #52814

[Console] Fix load from remote #52814

Conversation

jloleysens commented Dec 11, 2019 • edited Loading

Summary

Release note

elasticmachine commented Dec 11, 2019

cjcenizal left a comment

Choose a reason for hiding this comment

cjcenizal Dec 11, 2019

Choose a reason for hiding this comment

jloleysens Dec 12, 2019 • edited Loading

Choose a reason for hiding this comment

cjcenizal Dec 12, 2019

Choose a reason for hiding this comment

cjcenizal Dec 11, 2019

Choose a reason for hiding this comment

jloleysens Dec 12, 2019 • edited Loading

Choose a reason for hiding this comment

cjcenizal Dec 12, 2019

Choose a reason for hiding this comment

cjcenizal Dec 11, 2019

Choose a reason for hiding this comment

jloleysens Dec 12, 2019

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment

elasticmachine commented Dec 12, 2019

💚 Build Succeeded

History

jloleysens commented Dec 11, 2019 •

edited

Loading

jloleysens Dec 12, 2019 •

edited

Loading

jloleysens Dec 12, 2019 •

edited

Loading