Apply sub url tracking utils to visualize and discover by maryia-lapata · Pull Request #57307 · elastic/kibana

maryia-lapata · 2020-02-11T12:58:40Z

This PR applies the new sub url tracking utils from #55818 to the Visualize and Discover plugins.

elasticmachine · 2020-02-11T12:58:43Z

Pinging @elastic/kibana-app (Team:KibanaApp)

flash1293

In src/legacy/core_plugins/kibana/index.js disableSubUrlTracking: true has to be added to the respective nav links to disable the angular tracker. Otherwise it's also running and updating.
In src/legacy/core_plugins/kibana/public/visualize/np_ready/editor/editor.js legacy chrome is called to update the nav link: legacyChrome.trackSubUrlForApp('kibana:visualize', savedVisualizationParsedUrl); Please check whether that's still necessary and remove if not
In src/legacy/core_plugins/kibana/public/visualize/np_ready/listing/visualize_listing.js legacy chrome is called to filter out navlinks referencing deleted objects: legacyChrome.untrackNavLinksForDeletedSavedObjects(selectedItems.map(item => item.id)); Please check whether that's still necessary and delete if not. If we still need this we should think about how to do this without legacy chrome

maryia-lapata · 2020-02-12T10:24:11Z

In src/legacy/core_plugins/kibana/public/visualize/np_ready/editor/editor.js legacy chrome is called to update the nav link: legacyChrome.trackSubUrlForApp('kibana:visualize', savedVisualizationParsedUrl); Please check whether that's still necessary and remove if not

In src/legacy/core_plugins/kibana/public/visualize/np_ready/listing/visualize_listing.js legacy chrome is called to filter out navlinks referencing deleted objects: legacyChrome.untrackNavLinksForDeletedSavedObjects(selectedItems.map(item => item.id)); Please check whether that's still necessary and delete if not. If we still need this we should think about how to do this without legacy chrome

I checked 1 and 2 in master and it seems that they don't work.

a) Firstly, linkToLastSubUrl should be truly in app link for both call (trackSubUrlForApp and untrackNavLinksForDeletedSavedObjects)

kibana/src/legacy/ui/public/chrome/api/nav.ts

Lines 109 to 110 in 7e753e9

    
           if (link.linkToLastSubUrl === false) { 
        
             return;

b) Secondly, even if I define linkToLastSubUrl as true for visualize (here), it still doesn't work because of a typo (I guess) in

kibana/src/core/server/legacy/plugins/find_legacy_plugin_specs.ts

Line 91 in 7e753e9

linkToLastSub: 'linkToLastSubUrl' in spec ? spec.linkToLastSubUrl : false,

linkToLastSub -> linkToLastSubUrl.

If I apply a and b, trackSubUrlForApp works (if we create a vis from dashboard, that vis will be opened by clicking on Visualize from nav bar). As for untrackNavLinksForDeletedSavedObjects I don't quite understand the case. We can remove a vis from the listing page, and the function checks if the current url (the listing page) contains a deleted url, that 's never going to happen

kibana/src/legacy/ui/public/chrome/api/nav.ts

Line 51 in 2abc3f5

if (link.linkToLastSubUrl && urlContainsDeletedId(link.url!)) {

Probably I miss something, but it doesn't make sense for me.

Anyway since the visualize app doesn't have truly linkToLastSubUrl, I think we can remove trackSubUrlForApp and untrackNavLinksForDeletedSavedObjects.

# Conflicts: # src/legacy/ui/public/new_platform/new_platform.karma_mock.js

maryia-lapata · 2020-02-12T10:34:22Z

src/legacy/core_plugins/kibana/public/visualize/np_ready/editor/editor.js

@@ -588,10 +587,6 @@ function VisualizeAppController(
              });
              // Manually insert a new url so the back button will open the saved visualization.
              $window.history.pushState({}, '', savedVisualizationParsedUrl.getRootRelativePath());


@flash1293 also I noticed that this new history state doesn't trigger history listener:

kibana/src/plugins/kibana_utils/public/state_management/url/kbn_url_tracker.ts

Line 137 in c1a40f3

const urlWithHashes = baseUrl + '#' + location.pathname + location.search;

So in this PR just #/visualize/create link is saved and opens by clicking on Visualize in nav bar.

This is strange, because open a visualization after creating it in a dashboard working fine in my PR, but there is the case with the back button works wrong, so you could also check it.

@sulemanof I checked again, you're right (probably when I tested before, something was cached). But a saved visualization currently opens just because of the typo (there is a check link.linkToLastSubUrl === false, but linkToLastSubUrl is undefined now due to the typo).

As for the history, I can navigate to a saved visualization by clicking twice on the back button. @flash1293 do you think it's a valid case?

So I tested the current state and everything is working fine for me. When creating a vis within dashboard, it will store the id in the navlink (which is correct) and it will go back to the saved visualization (which is also correct IMHO).

flash1293 · 2020-02-12T10:58:54Z

@maryia-lapata

For 2 I agree that it doesn't make sense, let's remove that one.

For 1

So in this PR just #/visualize/create link is saved and opens by clicking on Visualize in nav bar.

That should not happen, it should pick up the saved id. It seems like we havbe to extend the kbn url tracker to expose a function to imperatively set the active url (and call it in this case to update).

Something like this (quick and dirty just to illustrate the approach):

function createKbnUrlTracker() {
// ...
  return {
    // ...
    setActiveUrl(newUrl) {
      const urlWithHashes = baseUrl + '#' + newUrl;
      let urlWithStates = '';
      try {
        urlWithStates = unhashUrl(urlWithHashes);
      } catch (e) {
        toastNotifications.addDanger(e.message);
      }

      activeUrl = getActiveSubUrl(urlWithStates || urlWithHashes);
      storageInstance.setItem(storageKey, activeUrl);
    }
  };
// ...
// editor.js

const newUrl = savedVisualizationParsedUrl.getRootRelativePath();
$window.history.pushState({}, '', newUrl);
urlTracker.setActiveUrl(newUrl);

Maybe @Dosant has a better idea.

pgayvallet

LGTM for platform changes

pgayvallet · 2020-02-12T14:47:05Z

src/legacy/core_plugins/kibana/public/visualize/legacy.ts

    ...npSetup.plugins,
-    __LEGACY: {
-      legacyChrome,
-    },
+    npData: npSetup.plugins.data,


Why the redundant npData: npSetup.plugins.data ? why not just instance.setup(npSetup.core, npSetup.plugins) and access the data plugin with it's proper data name?

@pgayvallet I will clean this up with #57331

src/legacy/core_plugins/kibana/public/discover/plugin.ts

flash1293

Tested and works fine for me, left some small comments but no need to review again from my side

src/legacy/core_plugins/kibana/public/visualize/plugin.ts

src/plugins/kibana_utils/public/state_management/url/kbn_url_tracker.ts

flash1293 · 2020-02-13T10:39:38Z

src/legacy/core_plugins/kibana/public/visualize/legacy.ts

-    __LEGACY: {
-      legacyChrome,
-    },
+    npData: npSetup.plugins.data,


I missunderstood @pgayvallet in my first comment, I thought the import was just moved, but it's in fact a new one. Please just call it data within the plugin, then

instance.setup(npSetup.core, npSetup.plugins);

Is enough. I have a draft PR up to clean up the other shims in the same way

# Conflicts: # src/core/server/legacy/plugins/find_legacy_plugin_specs.ts # src/legacy/core_plugins/kibana/public/discover/plugin.ts # src/legacy/core_plugins/kibana/public/visualize/legacy.ts # src/legacy/core_plugins/kibana/public/visualize/np_ready/application.ts # src/legacy/core_plugins/kibana/public/visualize/plugin.ts

alexwizp · 2020-02-17T11:19:17Z

src/legacy/core_plugins/kibana/public/visualize/plugin.ts

+      updater$: this.appStateUpdater.asObservable(),
+      navLinkId: 'kibana:visualize',
      mount: async (params: AppMountParameters) => {
        const [coreStart] = await core.getStartServices();


@lizozom @flash1293
If I'm not wrong we decided to use approach with creating a services.ts file and pass start dependencies using createGetterSetter method.

I wasn't aware this is a definitive rule, just an option how to do things - passing down services directly can IMHO be preferable, depending on the situation (e.g. because you don't have to mock out the getters in tests)

Looks like my comment is not related to this PR but here we pass some dependencies using core.getStartServices(); but some comes from this.startDependencies object.

Is it possible to refactor it in future and do it in a common with other plugins way?

flash1293 · 2020-02-17T15:20:17Z

@Dosant can you check this one out for the app arch team review?

Dosant · 2020-02-17T15:24:45Z

ACK. Reviewing today

Dosant

LGTM!
Smoke checked in chrome. Tried both expanded url and hashed urls.

Dosant · 2020-02-17T16:34:22Z

src/legacy/core_plugins/kibana/public/discover/plugin.ts

  public initializeServices?: () => Promise<{ core: CoreStart; plugins: DiscoverStartPlugins }>;

  setup(core: CoreSetup, plugins: DiscoverSetupPlugins): DiscoverSetup {
+    const { querySyncStateContainer, stop: stopQuerySyncStateContainer } = getQueryStateContainer(


just fwd: working on improving this #56128

maryia-lapata · 2020-02-18T08:09:52Z

@elasticmachine merge upstream

kibanamachine · 2020-02-18T09:48:28Z

💚 Build Succeeded

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

History

💚 Build #27106 succeeded 37ecbd0
💚 Build #26778 succeeded 480cceb
💔 Build #26742 failed e224aad
💛 Build #26526 was flaky 0081923
💛 Build #26486 was flaky 51f906b

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

* Apply sub url tracking utils to visualize and discover * Update query karma mock * Remove unnecessary chrome legacy calls * Fix typo * Add setActiveUrl * Add unit test for setActiveUrl * Refactoring Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Apply sub url tracking utils to visualize and discover

12cb082

maryia-lapata added Feature:Discover Discover Application Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:NP Migration v7.7.0 labels Feb 11, 2020

Update query karma mock

9404e34

maryia-lapata requested a review from flash1293 February 11, 2020 15:07

maryia-lapata marked this pull request as ready for review February 11, 2020 15:08

maryia-lapata requested a review from a team February 11, 2020 15:08

flash1293 requested changes Feb 11, 2020

View reviewed changes

maryia-lapata added 2 commits February 12, 2020 12:22

Merge branch 'master' into url-tracking

a558bba

Remove unnecessary chrome legacy calls

7c7e852

Fix typo

2e6e70a

maryia-lapata requested a review from a team as a code owner February 12, 2020 10:24

Merge remote-tracking branch 'kibana/master' into url-tracking

6769ed7

# Conflicts: # src/legacy/ui/public/new_platform/new_platform.karma_mock.js

maryia-lapata commented Feb 12, 2020

View reviewed changes

Add setActiveUrl

e44a7ea

maryia-lapata requested a review from a team as a code owner February 12, 2020 13:02

pgayvallet approved these changes Feb 12, 2020

View reviewed changes

Merge branch 'master' into url-tracking

198824c

flash1293 reviewed Feb 13, 2020

View reviewed changes

src/legacy/core_plugins/kibana/public/discover/plugin.ts Outdated Show resolved Hide resolved

flash1293 approved these changes Feb 13, 2020

View reviewed changes

maryia-lapata and others added 3 commits February 13, 2020 15:46

Add unit test for setActiveUrl

3d819b1

Refactoring

51f906b

Merge branch 'master' into url-tracking

0081923

elasticmachine and others added 3 commits February 14, 2020 00:53

Merge branch 'master' into url-tracking

e224aad

Merge branch 'master' into url-tracking

480cceb

alexwizp reviewed Feb 17, 2020

View reviewed changes

alexwizp approved these changes Feb 17, 2020

View reviewed changes

Dosant self-requested a review February 17, 2020 15:24

Dosant approved these changes Feb 17, 2020

View reviewed changes

Merge branch 'master' into url-tracking

7045052

maryia-lapata merged commit 5b7734c into elastic:master Feb 18, 2020

maryia-lapata mentioned this pull request Feb 18, 2020

[7.x] Apply sub url tracking utils to visualize and discover (#57307) #57830

Merged

maryia-lapata deleted the url-tracking branch February 18, 2020 10:55

Dosant mentioned this pull request Feb 18, 2020

[State Management] State syncing helpers for query service I #56128

Merged

7 tasks

maryia-lapata added the backported label Feb 19, 2020

Conversation

maryia-lapata commented Feb 11, 2020

Uh oh!

elasticmachine commented Feb 11, 2020

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

maryia-lapata commented Feb 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flash1293 commented Feb 12, 2020

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexwizp Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flash1293 commented Feb 17, 2020

Uh oh!

Dosant commented Feb 17, 2020

Uh oh!

Dosant left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maryia-lapata commented Feb 18, 2020

Uh oh!

kibanamachine commented Feb 18, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

alexwizp Feb 17, 2020 •

edited

Loading