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

Fix individual dashboard upload when filter datasets are not the same as in charts #269

Conversation

grvoicu
Copy link

@grvoicu grvoicu commented Mar 14, 2024

Preset supports dashboards with filters on different datasets than the datasets used by the charts. currently, individually uploading such a dashboard with Preset CLI fails, because the filters datasets are not included in the list of dependencies for the dashboard asset.

Note: I consider having dashboard filters on different datasets than the datasets used by the charts is not a bug, but a feature. An example where this is useful: a dataset with all the countries in the world and multiple datasets, each one with data for a subset of countries. A filter on the world countries dataset can then be applied to any dashboard, without having to take care to know which dataset is used in the dashboard charts. As long as the country column is named the same and the values are consistent across all datasets it will work.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@Vitor-Avila
Copy link
Contributor

Hi @grvoicu,

Thank you so much for contributing this fix to the CLI. Since the CLI is under an all-rights-reserved project, we can only accept external contributions in case users sign a CLA (we have an automated flow through the PR comments).

We also have a 100% test coverage requirement in the repository, so we might need to add a few test cases (and review any test that might break with changes) to be able to merge it.

Would you like to proceed with this contribution? If so please sign the CLA, and let me know if you would need assistance with the tests.

Thank you!

@Vitor-Avila
Copy link
Contributor

Hey @grvoicu,

Wondering if you had a chance to check my message. Thank you!

Extract native filters dataset UUID from a dashboard config.
"""
uuids = set()
for child in config["metadata"]["native_filter_configuration"]:

Choose a reason for hiding this comment

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

This might actually fail for dashboard without either metadata nor native_filter_configuration attributes. Better to add

if config.get('metadata') and config['metadata'].get('native_filter_configuration'):

@Vitor-Avila
Copy link
Contributor

Closing in favor of #292

@Vitor-Avila Vitor-Avila closed this May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants