[Fix] VisType switching persistence and selectively show warning #3715

ashwin-pc · 2023-03-28T02:06:21Z

Description

The previous implementation of persistence when switching vis types did not check the schema's agg filter to check if the new agg is compatible with it. This change is based on the previous implementation but simplifies the logic and is a simple function instead of a hook. It also only selectively shows the warning when switching the vis type causes aggregations to be dropped.

Screen.Recording.2023-03-27.at.6.56.39.PM.mov

Issues Resolved

#3647

Check List

All tests pass
- yarn test:jest
- yarn test:jest_integration
- yarn test:ftr
New functionality includes testing.
New functionality has been documented.
Update CHANGELOG.md // Skipping changelog since this is a fix within the same release
Commits are signed per the DCO using --signoff

Signed-off-by: Ashwin Pc <[email protected]>

Signed-off-by: Ashwin P Chandran <[email protected]>

ashwin-pc · 2023-03-28T03:48:13Z

test/functional/apps/vis_builder/_base.ts

-
-    it('should show warning before changing visualization type', async () => {
-      await PageObjects.visBuilder.selectVisType('metric', false);
-      const confirmModalExists = await testSubjects.exists('confirmVisChangeModal');
-      expect(confirmModalExists).to.be(true);
-
-      await testSubjects.click('confirmModalCancelButton');
-    });
-
-    it('should change visualization type', async () => {
-      const pickerValue = await PageObjects.visBuilder.selectVisType('metric');
-
-      expect(pickerValue).to.eql('Metric');
-    });


Deleted these tests because we now have cypress tests for these. Will remove all the others after 2.7

ashwin-pc · 2023-03-28T04:05:12Z

~~The cypress test failures are expected since the vis type picker no longer has a popup which is expected. PR to fix the tests here: opensearch-project/opensearch-dashboards-functional-test#600~~

Update: PR merged, tests should not fail

codecov-commenter · 2023-03-28T04:30:18Z

Codecov Report

Merging #3715 (6c7997e) into main (fd61f7a) will decrease coverage by 0.02%.
The diff coverage is 59.57%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #3715      +/-   ##
==========================================
- Coverage   66.45%   66.43%   -0.02%     
==========================================
  Files        3210     3210              
  Lines       61670    61677       +7     
  Branches     9514     9522       +8     
==========================================
- Hits        40981    40978       -3     
- Misses      18410    18419       +9     
- Partials     2279     2280       +1

Flag	Coverage Δ
Linux	`66.38% <59.57%> (-0.02%)`	⬇️
Windows	`66.38% <59.57%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...uilder/public/application/components/right_nav.tsx	`8.69% <0.00%> (-2.42%)`	⬇️
...plication/utils/state_management/shared_actions.ts	`100.00% <ø> (ø)`
...blic/application/utils/get_persisted_agg_params.ts	`84.84% <84.84%> (ø)`

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

joshuarrrr

A couple minor questions/suggestions. Is the updated warning message signed off by UX/product?

src/plugins/vis_builder/public/application/components/right_nav.tsx

Signed-off-by: Ashwin P Chandran <[email protected]>

joshuarrrr · 2023-03-31T15:58:51Z

@ashwin-pc The visbuilder cypress tests still seem to be failing. Shouldn't they pass on this PR?

abbyhu2000 · 2023-03-31T17:58:39Z

From the PR description, it seems like if no aggregations get dropped, then it should not show the warning sign. I pulled down the branch and tried, it is still showing the warning sign even though in this case no aggregations are dropped. @ashwin-pc

Screen.Recording.2023-03-31.at.10.56.17.AM.mov

abbyhu2000

It seems like the persistence rule is changed. Should we document this new persisting rule somewhere? Maybe in the code? @ashwin-pc

abbyhu2000 · 2023-03-31T18:53:09Z

src/plugins/vis_builder/public/application/utils/get_persisted_agg_params.ts

+  // Check if schema1 and schema2 exist
+  if (!schema1 || !schema2) return false;
+
+  if (schema1.name !== schema2.name) return false;


Is that line needed? Seemed like if schema1 and schema2 already exists, they should already have same schema.name since they are both found by using schema.name === aggConfigParam.schema

Yeah its a little redundant, but are you okay with it going through though? All the tests have passed and making this change would require all the tests to pass again.

abbyhu2000 · 2023-03-31T18:54:19Z

src/plugins/vis_builder/public/application/utils/get_persisted_agg_params.ts

+  return updatedAggConfigParams;
+};
+
+function isSchemaEqual(schema1?: Schema, schema2?: Schema) {


nit: I think the naming isSchemaEqual is a little confusing here since it is also checking if the schemas exist or not.

No this is common. Having an undefined check in the beginning of a function does not change the purpose of the function. The decision to return false instead of throwing an error however is subjective since the main purpose of the function is to check if two schemas are equal. If either or both is undefined, for the purposes of this check, that's correct.

abbyhu2000 · 2023-03-31T18:55:41Z

src/plugins/vis_builder/public/application/utils/get_persisted_agg_params.ts

+      return schema.name === aggConfigParam.schema;
+    });
+
+    // see if a matching schma exists in the new visualization type


nit: typo schma --> schema

abbyhu2000 · 2023-03-31T18:56:20Z

src/plugins/vis_builder/public/application/utils/get_persisted_agg_params.ts

+      // Check if the schema group is the same
+      if (schema.group !== currentSchema!.group) continue;
+
+      const compatibleSchema = filterByType([aggConfigParam], schema.aggFilter).length !== 0;


Could you explain more what this line does?

This was the main reason for the change. So each schema today can define a list of agg types it supports. Its not a simple lift and it can sometimes be something like !filter, In this case we want to make sure that when we select a new schema type to map the agg to, the agg type for the agg config is allowed by the new schema.

Thanks, make sense

ashwin-pc · 2023-03-31T20:51:35Z

@ashwin-pc The visbuilder cypress tests still seem to be failing. Shouldn't they pass on this PR?

yeah made a blooper in my revision, fixed now :)

ashwin-pc · 2023-03-31T20:54:13Z

It seems like the persistence rule is changed. Should we document this new persisting rule somewhere? Maybe in the code? @ashwin-pc

The only thing thats changed is that we also filter for agg filters now. Did we document the original persistence logic in code somewhere?

abbyhu2000 · 2023-03-31T21:19:25Z

It seems like the persistence rule is changed. Should we document this new persisting rule somewhere? Maybe in the code? @ashwin-pc

The only thing thats changed is that we also filter for agg filters now. Did we document the original persistence logic in code somewhere?

@ashwin-pc I did not document the rules in the code, i just documented the rules in the proposal issue here: #3482

The reason i said the logics changed is because i see the mapping rule here add logics that will map the identical agg schema first, and then it maps the rest of the aggs to similar schemas. The original mapping rule only does metric to metric, bucket to bucket without mapping the identical schema first.

abbyhu2000

LGTM
Just some nits and typos, not a blocker

* Refactor persistnece for simpler logic Signed-off-by: Ashwin Pc <[email protected]> * renamed files Signed-off-by: Ashwin Pc <[email protected]> * only show warning when dropping fields Signed-off-by: Ashwin P Chandran <[email protected]> * delete unnecessary tests Signed-off-by: Ashwin P Chandran <[email protected]> * addresses PR feedback Signed-off-by: Ashwin P Chandran <[email protected]> * Fixes comparison --------- Signed-off-by: Ashwin Pc <[email protected]> Signed-off-by: Ashwin P Chandran <[email protected]> (cherry picked from commit 2db2c43) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…nsearch-project#3715) * Refactor persistnece for simpler logic Signed-off-by: Ashwin Pc <[email protected]> * renamed files Signed-off-by: Ashwin Pc <[email protected]> * only show warning when dropping fields Signed-off-by: Ashwin P Chandran <[email protected]> * delete unnecessary tests Signed-off-by: Ashwin P Chandran <[email protected]> * addresses PR feedback Signed-off-by: Ashwin P Chandran <[email protected]> * Fixes comparison --------- Signed-off-by: Ashwin Pc <[email protected]> Signed-off-by: Ashwin P Chandran <[email protected]> Signed-off-by: vagimeli <[email protected]>

…) (#3760) * Refactor persistnece for simpler logic * renamed files * only show warning when dropping fields * delete unnecessary tests * addresses PR feedback * Fixes comparison --------- (cherry picked from commit 2db2c43) Signed-off-by: Ashwin Pc <[email protected]> Signed-off-by: Ashwin P Chandran <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…nsearch-project#3715) * Refactor persistnece for simpler logic Signed-off-by: Ashwin Pc <[email protected]> * renamed files Signed-off-by: Ashwin Pc <[email protected]> * only show warning when dropping fields Signed-off-by: Ashwin P Chandran <[email protected]> * delete unnecessary tests Signed-off-by: Ashwin P Chandran <[email protected]> * addresses PR feedback Signed-off-by: Ashwin P Chandran <[email protected]> * Fixes comparison --------- Signed-off-by: Ashwin Pc <[email protected]> Signed-off-by: Ashwin P Chandran <[email protected]> Signed-off-by: David Sinclair <[email protected]>

ashwin-pc and others added 3 commits March 24, 2023 11:42

Refactor persistnece for simpler logic

226536f

Signed-off-by: Ashwin Pc <[email protected]>

renamed files

0782d0e

Signed-off-by: Ashwin Pc <[email protected]>

only show warning when dropping fields

cf6bcb5

Signed-off-by: Ashwin P Chandran <[email protected]>

ashwin-pc added the vis builder label Mar 28, 2023

ashwin-pc requested a review from a team as a code owner March 28, 2023 02:06

ashwin-pc assigned abbyhu2000 Mar 28, 2023

ashwin-pc added Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry backport 2.x v2.7.0 labels Mar 28, 2023

ashwin-pc self-assigned this Mar 28, 2023

ashwin-pc linked an issue Mar 28, 2023 that may be closed by this pull request

[VisBuilder] Remove warning popup for vis type change #3647

Closed

delete unnecessary tests

2005395

Signed-off-by: Ashwin P Chandran <[email protected]>

ashwin-pc commented Mar 28, 2023

View reviewed changes

ashwin-pc mentioned this pull request Mar 28, 2023

[VisBuilder] Fixes tests for vis type persistence change opensearch-project/opensearch-dashboards-functional-test#600

Merged

1 task

Merge branch 'main' into fix-persistence

2cd5597

joshuarrrr reviewed Mar 30, 2023

View reviewed changes

src/plugins/vis_builder/public/application/components/right_nav.tsx Outdated Show resolved Hide resolved

src/plugins/vis_builder/public/application/components/right_nav.tsx Outdated Show resolved Hide resolved

joshuarrrr previously approved these changes Mar 30, 2023

View reviewed changes

addresses PR feedback

be6cbc4

Signed-off-by: Ashwin P Chandran <[email protected]>

ashwin-pc dismissed joshuarrrr’s stale review via be6cbc4 March 31, 2023 02:02

Merge branch 'main' into fix-persistence

752a866

joshuarrrr previously approved these changes Mar 31, 2023

View reviewed changes

Fixes comparison

6c7997e

ashwin-pc dismissed joshuarrrr’s stale review via 6c7997e March 31, 2023 18:08

abbyhu2000 reviewed Mar 31, 2023

View reviewed changes

joshuarrrr approved these changes Mar 31, 2023

View reviewed changes

abbyhu2000 approved these changes Mar 31, 2023

View reviewed changes

ashwin-pc merged commit 2db2c43 into opensearch-project:main Mar 31, 2023

opensearch-trigger-bot bot mentioned this pull request Mar 31, 2023

[Backport 2.x] [Fix] VisType switching persistence and selectively show warning #3760

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix] VisType switching persistence and selectively show warning #3715

[Fix] VisType switching persistence and selectively show warning #3715

ashwin-pc commented Mar 28, 2023 •

edited

Loading

ashwin-pc Mar 28, 2023

ashwin-pc commented Mar 28, 2023 •

edited

Loading

codecov-commenter commented Mar 28, 2023 •

edited

Loading

joshuarrrr left a comment

joshuarrrr commented Mar 31, 2023

abbyhu2000 commented Mar 31, 2023

abbyhu2000 left a comment

abbyhu2000 Mar 31, 2023

ashwin-pc Mar 31, 2023

abbyhu2000 Mar 31, 2023

ashwin-pc Mar 31, 2023

abbyhu2000 Mar 31, 2023

abbyhu2000 Mar 31, 2023

ashwin-pc Mar 31, 2023

abbyhu2000 Mar 31, 2023

ashwin-pc commented Mar 31, 2023 •

edited

Loading

ashwin-pc commented Mar 31, 2023 •

edited

Loading

abbyhu2000 commented Mar 31, 2023

abbyhu2000 left a comment

[Fix] VisType switching persistence and selectively show warning #3715

[Fix] VisType switching persistence and selectively show warning #3715

Conversation

ashwin-pc commented Mar 28, 2023 • edited Loading

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

ashwin-pc commented Mar 28, 2023 • edited Loading

codecov-commenter commented Mar 28, 2023 • edited Loading

Codecov Report

joshuarrrr left a comment

Choose a reason for hiding this comment

joshuarrrr commented Mar 31, 2023

abbyhu2000 commented Mar 31, 2023

abbyhu2000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashwin-pc commented Mar 31, 2023 • edited Loading

ashwin-pc commented Mar 31, 2023 • edited Loading

abbyhu2000 commented Mar 31, 2023

abbyhu2000 left a comment

Choose a reason for hiding this comment

ashwin-pc commented Mar 28, 2023 •

edited

Loading

ashwin-pc commented Mar 28, 2023 •

edited

Loading

codecov-commenter commented Mar 28, 2023 •

edited

Loading

ashwin-pc commented Mar 31, 2023 •

edited

Loading

ashwin-pc commented Mar 31, 2023 •

edited

Loading