Legacy SO import: Fix bug causing multiple overrides to only show the last confirm modal #76482

pgayvallet · 2020-09-02T09:17:42Z

Summary

Fix a bug causing legacy saved object imports to only show one confirmation modal when multiple objects needed a override confirmation in Request action when conflict occurs mode

Checklist

Unit or functional tests were updated or added to match the most common scenarios

… last confirm modal

pgayvallet · 2020-09-02T09:26:09Z

src/plugins/saved_objects_management/public/lib/resolve_saved_objects.ts

-  await awaitEachItemInParallel(docTypes.indexPatterns, async (indexPatternDoc) => {
+  // As the confirmation opens a modal, and as we only allow one modal at a time
+  // (opening a new one close the previous with a rejection)
+  // we can't do that in parallel
+  for (const indexPatternDoc of docTypes.indexPatterns) {


So, this was present for a very long time (since core's ModalService presence), as we are dismissing any previous modal when opening a new one

kibana/src/core/public/overlays/modal/modal_service.tsx

Lines 123 to 128 in 395d621

open: (mount: MountPoint, options: OverlayModalOpenOptions = {}): OverlayRef => {

// If there is an active modal, close it before opening a new one.

if (this.activeModal) {

this.activeModal.close();

this.cleanupDom();

}

The awaitEachItemInParallel was causing multiple modals to eventually open at the same time, dismissing the previous one when the next one kicked in.

Using a sort of confirmation queue here to pool the modal promises and only open the next one when the previous one is answered by the user is a nightmare, as some confirmations modal are opened as down as savedobject loaders underlying implementations in some cases. As this is only impacting the legacy imports, removing the parallel work and doing one object at a time was the only sane option.

From a quick manual benchmark, it doesn't even impact the process time that much.

So the modern approach won't be affected by this fix?

Yea. the modern approach is not impacted by this bug, and is not relying on this mechanism to prompt user confirmations, so the PR has no impact on it

kibanamachine · 2020-09-02T12:22:50Z

💚 Build Succeeded

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

Build metrics

page load bundle size

id	value	diff	baseline
savedObjectsManagement	225.9KB	+1.4KB	224.5KB

History

💔 Build #71815 failed fd7f71b

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

elasticmachine · 2020-09-02T12:29:46Z

Pinging @elastic/kibana-platform (Team:Platform)

mshustov · 2020-09-02T14:42:38Z

test/functional/apps/management/_import_objects.js

+        // with is why we call clickConfirmOnModal with ensureHidden: false in previous statement
+        // but as the initial popin can take a few ms before fading, we need to wait a little
+        // to avoid clicking twice on the same modal.
+        await delay(1000);


can be a source of flakiness.... ok, we will see

Yea, there is a risk here. But I just couldn't find any better solution for these blinking confirmation modals

mshustov · 2020-09-02T14:44:41Z

test/functional/apps/management/_import_objects.js

+        await delay(1000);
+        await PageObjects.common.clickConfirmOnModal(false);
+
+        const isSuccessful = await testSubjects.exists('importSavedObjectsSuccess');


optional: can be combined with an assertion as await testSubjects.exists('importSavedObjectsSuccess');

mshustov · 2020-09-02T14:47:19Z

src/plugins/saved_objects_management/public/lib/resolve_saved_objects.ts

-  await awaitEachItemInParallel(docTypes.indexPatterns, async (indexPatternDoc) => {
+  // As the confirmation opens a modal, and as we only allow one modal at a time
+  // (opening a new one close the previous with a rejection)
+  // we can't do that in parallel
+  for (const indexPatternDoc of docTypes.indexPatterns) {


So the modern approach won't be affected by this fix?

… last confirm modal (elastic#76482) * Legacy SO import: Fix bug causing multiple overrides to only show the last confirm modal * eslint * fix for loops

… last confirm modal (#76482) (#76598) * Legacy SO import: Fix bug causing multiple overrides to only show the last confirm modal * eslint * fix for loops

… last confirm modal (#76482) (#76597) * Legacy SO import: Fix bug causing multiple overrides to only show the last confirm modal * eslint * fix for loops

…nes/processors-forms-k-s * 'master' of github.com:elastic/kibana: (216 commits) [Ingest Manager] Split Registry errors into Connection & Response (elastic#76558) [Security Solution] add an excess validation instead of the exact match (elastic#76472) Introduce TS incremental builds & move src/test_utils to TS project (elastic#76082) fix bad merge (elastic#76629) [Newsfeed] Ensure the version format when calling the API (elastic#76381) remove server_extensions mixin (elastic#76606) Remove legacy applications and legacy mode (elastic#75987) [Discover] Fix sidebar element focus behavior when adding / removing columns (elastic#75749) Replace FetchOptions with ISearchOptions (elastic#76538) Move streams utils to the core (elastic#76397) [Ingest Manager] Wording & linking improvements (elastic#76284) remove legacy kibana plugin (elastic#76064) [Form lib] Fix regression on field not being validated after reset to its default value. (elastic#76379) Legacy SO import: Fix bug causing multiple overrides to only show the last confirm modal (elastic#76482) [APM] Service maps layout enhancements (elastic#76481) [Security Solution][Detection Engine] Remove RuleTypeSchema in favor of Type for TypeScript (elastic#76586) [Security Solution][Exceptions] - Updates enum schema and tests (elastic#76544) Index Pattern class - remove toJSON and toString (elastic#76246) skip failing suite (elastic#76581) Split up and clarify Enterprise Search codeowners (elastic#76580) ... # Conflicts: # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processor_settings_fields.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/foreach.tsx

…oleysens/kibana into ingest-pipelines/processors-forms-k-s * 'ingest-pipelines/processors-forms-k-s' of github.com:jloleysens/kibana: (216 commits) [Ingest Manager] Split Registry errors into Connection & Response (elastic#76558) [Security Solution] add an excess validation instead of the exact match (elastic#76472) Introduce TS incremental builds & move src/test_utils to TS project (elastic#76082) fix bad merge (elastic#76629) [Newsfeed] Ensure the version format when calling the API (elastic#76381) remove server_extensions mixin (elastic#76606) Remove legacy applications and legacy mode (elastic#75987) [Discover] Fix sidebar element focus behavior when adding / removing columns (elastic#75749) Replace FetchOptions with ISearchOptions (elastic#76538) Move streams utils to the core (elastic#76397) [Ingest Manager] Wording & linking improvements (elastic#76284) remove legacy kibana plugin (elastic#76064) [Form lib] Fix regression on field not being validated after reset to its default value. (elastic#76379) Legacy SO import: Fix bug causing multiple overrides to only show the last confirm modal (elastic#76482) [APM] Service maps layout enhancements (elastic#76481) [Security Solution][Detection Engine] Remove RuleTypeSchema in favor of Type for TypeScript (elastic#76586) [Security Solution][Exceptions] - Updates enum schema and tests (elastic#76544) Index Pattern class - remove toJSON and toString (elastic#76246) skip failing suite (elastic#76581) Split up and clarify Enterprise Search codeowners (elastic#76580) ...

Legacy SO import: Fix bug causing multiple overrides to only show the…

f991e39

… last confirm modal

pgayvallet added Feature:Saved Objects release_note:fix v7.10.0 v7.9.2 Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Sep 2, 2020

eslint

fd7f71b

pgayvallet commented Sep 2, 2020

View reviewed changes

fix for loops

d80ad52

pgayvallet marked this pull request as ready for review September 2, 2020 12:29

pgayvallet requested a review from a team as a code owner September 2, 2020 12:29

mshustov approved these changes Sep 2, 2020

View reviewed changes

pgayvallet merged commit 210d6f2 into elastic:master Sep 3, 2020

pgayvallet mentioned this pull request Sep 3, 2020

[7.x] Legacy SO import: Fix bug causing multiple overrides to only show the last confirm modal (#76482) #76597

Merged

pgayvallet mentioned this pull request Sep 3, 2020

[7.9] Legacy SO import: Fix bug causing multiple overrides to only show the last confirm modal (#76482) #76598

Merged

pgayvallet mentioned this pull request Apr 20, 2021

Fix flaky SO import tests #97586

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Legacy SO import: Fix bug causing multiple overrides to only show the last confirm modal #76482

Legacy SO import: Fix bug causing multiple overrides to only show the last confirm modal #76482

pgayvallet commented Sep 2, 2020 •

edited

Loading

pgayvallet Sep 2, 2020

mshustov Sep 2, 2020

pgayvallet Sep 2, 2020

kibanamachine commented Sep 2, 2020

elasticmachine commented Sep 2, 2020

mshustov Sep 2, 2020

pgayvallet Sep 2, 2020

mshustov Sep 2, 2020

mshustov Sep 2, 2020

	open: (mount: MountPoint, options: OverlayModalOpenOptions = {}): OverlayRef => {
	// If there is an active modal, close it before opening a new one.
	if (this.activeModal) {
	this.activeModal.close();
	this.cleanupDom();
	}

Legacy SO import: Fix bug causing multiple overrides to only show the last confirm modal #76482

Legacy SO import: Fix bug causing multiple overrides to only show the last confirm modal #76482

Conversation

pgayvallet commented Sep 2, 2020 • edited Loading

Summary

Checklist

pgayvallet Sep 2, 2020

Choose a reason for hiding this comment

mshustov Sep 2, 2020

Choose a reason for hiding this comment

pgayvallet Sep 2, 2020

Choose a reason for hiding this comment

kibanamachine commented Sep 2, 2020

💚 Build Succeeded

Build metrics

page load bundle size

History

elasticmachine commented Sep 2, 2020

mshustov Sep 2, 2020

Choose a reason for hiding this comment

pgayvallet Sep 2, 2020

Choose a reason for hiding this comment

mshustov Sep 2, 2020

Choose a reason for hiding this comment

mshustov Sep 2, 2020

Choose a reason for hiding this comment

pgayvallet commented Sep 2, 2020 •

edited

Loading