Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[VisBuilder ] Fix filter and query bugs in the saved objects #6460

Merged
merged 4 commits into from
Apr 30, 2024

Conversation

ananzh
Copy link
Member

@ananzh ananzh commented Apr 15, 2024

Description

  • Save filter and query in vb
  • Clean filterManager when start vb
  • Add opensearch_dashboards_context to embeddable. This can add filter and query in the expression.

Issues Resolved

The fix is this part in the src/plugins/vis_builder/public/application/utils/get_top_nav_config.tsx

    const searchSourceInstance = savedVisBuilderVis.searchSourceFields;
    searchSourceInstance.query = data.query.queryString.getQuery() || null;
    searchSourceInstance.filter = data.query.filterManager.getFilters() || null;
    savedVisBuilderVis.searchSourceFields = searchSourceInstance;

VisBuilder is using searchSourceFields to save states. This is different from Discover which uses searchSource. This fix will ensure the searchSourceInstance get updated filer and query and allow them to be saved in the saved object.

The fix is in src/plugins/vis_builder/public/plugin.ts to ensure filter state is cleaned if not pin filter.

// make sure the filterManager is refreshed
const filters = pluginsStart.data.query.filterManager.getFilters();
const pinFilters = filters.filter(opensearchFilters.isFilterPinned);
pluginsStart.data.query.filterManager.setFilters(pinFilters ? pinFilters : []);

The fix is in src/plugins/vis_builder/public/application/utils/use/use_saved_vis_builder_vis.ts

const filters = savedVisBuilderVis.searchSourceFields.filter;
 const query = savedVisBuilderVis.searchSourceFields.query || data.query.queryString.getDefaultQuery();
const actualFilters = [];
if (filters !== undefined) {
       const result = typeof filters === 'function' ? filters() : filters;
       if (result !== undefined) {
            actualFilters.push(...(Array.isArray(result) ? result : [result]));
      }
}
data.query.filterManager.setAppFilters(actualFilters);
data.query.queryString.setQuery(query);

Basically it loads filter and query from saved object and do some type check. Then it updates data service.

The fix is in src/plugins/vis_builder/public/embeddable/vis_builder_embeddable.tsx and src/plugins/vis_builder/public/embeddable/vis_builder_embeddable_factory.tsx and src/plugins/visualizations/public/index.ts (just export prepareJson method). The bug is due to we don't have opensearch_dashboards_context in vis builder which is normally used for filter and quey info.

Screenshot

fiter-not-global.mov
save-filter-query.mov
  • When a dashboard loads a saved vis builder, any saved filters and queries will automatically be applied to the embedded vis builder. If new filters or queries are added within the dashboard, these will also apply to the embedded visualization builder. However, these newly added filters and queries will not impact the original settings of the saved vis builder. This is consistent with other applications, like discover and table vis. This resolves [BUG] VisBuilder Embeddable don't have filter and query  #6512.
filter_query_embeddable.mov
  • Also checks and ensures the pinned filter work.
pin-filter.mov

Changelog

  • skip

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Copy link
Contributor

❌ Invalid Changelog Heading

The '## Changelog' heading in your PR description is either missing or malformed. Please make sure that your PR description includes a '## Changelog' heading with proper spelling, capitalization, spacing, and Markdown syntax.

Copy link
Contributor

❌ Invalid Changelog Heading

The '## Changelog' heading in your PR description is either missing or malformed. Please make sure that your PR description includes a '## Changelog' heading with proper spelling, capitalization, spacing, and Markdown syntax.

Copy link
Contributor

❌ Invalid Prefix For Manual Changeset Creation

Invalid description prefix. Found "fix". Only "skip" entry option is permitted for manual commit of changeset files.

If you were trying to skip the changelog entry, please use the "skip" entry option in the ##Changelog section of your PR description.

Copy link
Contributor

❌ Invalid Prefix For Manual Changeset Creation

Invalid description prefix. Found "fix". Only "skip" entry option is permitted for manual commit of changeset files.

If you were trying to skip the changelog entry, please use the "skip" entry option in the ##Changelog section of your PR description.

Copy link

codecov bot commented Apr 25, 2024

Codecov Report

Attention: Patch coverage is 17.85714% with 23 lines in your changes are missing coverage. Please review.

Project coverage is 67.73%. Comparing base (876224b) to head (2baae30).

Files Patch % Lines
...ilder/public/embeddable/vis_builder_embeddable.tsx 0.00% 12 Missing ⚠️
...application/utils/use/use_saved_vis_builder_vis.ts 0.00% 8 Missing ⚠️
src/plugins/vis_builder/public/plugin.ts 0.00% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6460   +/-   ##
=======================================
  Coverage   67.73%   67.73%           
=======================================
  Files        3414     3414           
  Lines       66838    66860   +22     
  Branches    10872    10879    +7     
=======================================
+ Hits        45270    45285   +15     
- Misses      18923    18930    +7     
  Partials     2645     2645           
Flag Coverage Δ
Linux_1 33.20% <17.85%> (-0.01%) ⬇️
Linux_2 55.56% <ø> (ø)
Linux_3 45.25% <ø> (ø)
Linux_4 34.87% <ø> (ø)
Windows_1 33.25% <17.85%> (+0.01%) ⬆️
Windows_2 55.53% <ø> (ø)
Windows_3 45.25% <ø> (ø)
Windows_4 34.87% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ananzh
Copy link
Member Author

ananzh commented Apr 25, 2024

@ashwin-pc I moved the save button logic from this PR to #6636

For tests, I think these behavior should better be tested in ftr. Create a meta issue to add cypress tests for VisBuilder #6635

ashwin-pc
ashwin-pc previously approved these changes Apr 29, 2024
Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested one change, but approving since i dont think its a release blocker. However it would be nice if you can make it.

@@ -79,6 +81,7 @@ export class VisBuilderEmbeddable extends Embeddable<VisBuilderInput, VisBuilder
private subscriptions: Subscription[] = [];
private node?: HTMLElement;
private savedVis?: VisBuilderSavedVis;
private savedObject?: VisBuilderSavedObject;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of passing the saved object down, cant you modify the getStateFromSavedObject function to return the searchSourceFields as a part of savedVis too.

export interface VisBuilderSavedVis
  extends Pick<
    VisBuilderSavedObjectAttributes,
    'id' | 'title' | 'description' | 'searchSourceFields'
  > {
  state: RenderState;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we could. let me polish the code.

if (this.handler) {
this.updateHandler();
}
if (this.handler && dirty) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Why do we need this change? Feels like the original logic was intentional.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't remember why but it is a request change. otherwise will see
Screenshot 2024-04-29 at 1 15 05 PM

ashwin-pc
ashwin-pc previously approved these changes Apr 29, 2024
Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

ashwin-pc
ashwin-pc previously approved these changes Apr 29, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
@@ -125,6 +125,10 @@ export class VisBuilderPlugin

// make sure the index pattern list is up to date
pluginsStart.data.indexPatterns.clearCache();
// make sure the filterManager is refreshed
const filters = pluginsStart.data.query.filterManager.getFilters();
const pinFilters = filters.filter(opensearchFilters.isFilterPinned);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: pinnedFilters

In this PR:
add filter and query in vb
clean filterManager when start vb
add opensearch_dashboards_context to embeddable. This can add filter and query in the expression.

Issue Resolve
opensearch-project#5643
opensearch-project#5644
opensearch-project#5646
opensearch-project#6512

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
CHANGELOG.md Outdated Show resolved Hide resolved
ananzh and others added 3 commits April 29, 2024 17:22
Co-authored-by: Miki <amoo_miki@yahoo.com>
Signed-off-by: Anan Zhuang <ananzh@amazon.com>
…_vis_builder_vis.ts

Co-authored-by: Miki <amoo_miki@yahoo.com>
Signed-off-by: Anan Zhuang <ananzh@amazon.com>
Signed-off-by: Anan Zhuang <ananzh@amazon.com>
@ananzh ananzh merged commit 0ac9db3 into opensearch-project:main Apr 30, 2024
65 of 67 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 30, 2024
* [VisBuilder] Fix filter and query bugs

In this PR:
add filter and query in vb
clean filterManager when start vb
add opensearch_dashboards_context to embeddable. This can add filter and query in the expression.

Issue Resolve
#5643
#5644
#5646
#6512

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

---------

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
Co-authored-by: Miki <amoo_miki@yahoo.com>
(cherry picked from commit 0ac9db3)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
ananzh pushed a commit that referenced this pull request Apr 30, 2024
…6686)

* [VisBuilder] Fix filter and query bugs

In this PR:
add filter and query in vb
clean filterManager when start vb
add opensearch_dashboards_context to embeddable. This can add filter and query in the expression.

Issue Resolve
#5643
#5644
#5646
#6512

Signed-off-by: Anan Zhuang <ananzh@amazon.com>

---------

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
Co-authored-by: Miki <amoo_miki@yahoo.com>
(cherry picked from commit 0ac9db3)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
LDrago27 pushed a commit to LDrago27/OpenSearch-Dashboards that referenced this pull request Jun 3, 2024
…rch-project#6460)

* [VisBuilder] Fix filter and query bugs

In this PR:
add filter and query in vb
clean filterManager when start vb
add opensearch_dashboards_context to embeddable. This can add filter and query in the expression.

Issue Resolve
opensearch-project#5643
opensearch-project#5644
opensearch-project#5646
opensearch-project#6512

Signed-off-by: Anan Zhuang <ananzh@amazon.com>


---------

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
Co-authored-by: Miki <amoo_miki@yahoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x bug Something isn't working distinguished-contributor Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry v2.14.0 vis builder
Projects
None yet
4 participants