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

UI-0 - (UKS-5248) handle unexpected values in filter objects #47

Closed

Conversation

ibolari42
Copy link
Collaborator

@ibolari42 ibolari42 commented Nov 1, 2022

This PR adds a function that checks the filters provided in each dashboard for non-string elements and removes then as the only expected elements in the filter are mdx strings.

ibolari42 and others added 2 commits October 17, 2022 13:47
The else condition was causing the widget to be removed to not be included in the migrated content. so when the content got passed later on to the `removewidget` function it was incomplete leading to an erroneous result
Copy link
Collaborator

@antoinetissier antoinetissier left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @ibolari42.

Besides the comments below, it seems that the two commits are orthogonal. Could you do separate PRs for each of them to keep changes atomic ?

Comment on lines +111 to +114
errorReport.pages[sectionKey].widgets[`All widgets in ${pageName}`] = {
widgetName: "All widgets",
error,
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a bit of a workaround.
I would prefer to introduce an error at the root of DashboardErrorReport and at the root of each page if necessary, in a preliminary PR.

Comment on lines +79 to +80
pageKey: string;
pageName: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Following the same idea, I would make those optional, as it is not needed for dashboard filters.

src/migrateDashboard.ts Outdated Show resolved Hide resolved
improvement to comments

Co-authored-by: Antoine Tissier <[email protected]>
@ibolari42
Copy link
Collaborator Author

Thanks for the PR @ibolari42.

Besides the comments below, it seems that the two commits are orthogonal. Could you do separate PRs for each of them to keep changes atomic ?

I'll get these seperated, Initiallly bound them together to make it easy for the people who needed the changes to download the branch before it was officially merged

@ibolari42
Copy link
Collaborator Author

i'll close this request and creeate a new branch to keep changes atomic

@ibolari42 ibolari42 closed this Nov 9, 2022
@ibolari42 ibolari42 deleted the ibo/UKS-5248-handle-unexpected-values-in-filter-objects branch November 9, 2022 13:25
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.

Bug: --remove-widgets flag causes removal of wrong widgets/deletion of dashboards
2 participants