-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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: Error when searching for a dashboard in the chart list #24546
fix: Error when searching for a dashboard in the chart list #24546
Conversation
col: 'dashboards', | ||
opr: FilterOperator.relationManyMany, | ||
col: 'dashboard_title', | ||
opr: FilterOperator.startsWith, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's using startsWith
which is the same operator used by the Datasets filter.
endpoint: !filterValue | ||
? `/api/v1/dashboard/?q=${queryParams}` | ||
: `/api/v1/chart/?q=${queryParams}`, | ||
endpoint: `/api/v1/dashboard/?q=${queryParams}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michael-s-molina I briefly looked into this logic previously. Could you confirm that your logic also works when you include a filterValue
. i.e., text in the SEARCH
box?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use the search input (not the dashboard select) to look for a dashboard name, it won't work. That would require creating a search filter but that's out of the scope of this PR.
Codecov Report
@@ Coverage Diff @@
## master #24546 +/- ##
===========================================
- Coverage 69.07% 58.24% -10.84%
===========================================
Files 1904 1906 +2
Lines 74027 74127 +100
Branches 8118 8154 +36
===========================================
- Hits 51136 43177 -7959
- Misses 20780 28831 +8051
- Partials 2111 2119 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 292 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
SUMMARY
#21760 added the cross-referenced dashboards feature which is really valuable to our users. As part of the changes, the function to fetch the dashboards was operating differently depending on the existence of a search text. When a search was present, it was invoking
/api/v1/chart
which brings only the dashboards associated to the charts and when no search was present it was invoking/api/v1/dashboard
which ignores the charts relationship.This was causing some problems:
/api/v1/chart
related to the constructed query.This PR changes the logic to always query the
/api/v1/dashboard
endpoint and ignore the relationships with charts.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Screen.Recording.2023-06-28.at.16.26.09.mov
Screen.Recording.2023-06-28.at.16.23.26.mov
TESTING INSTRUCTIONS
Check the videos for instructions.
ADDITIONAL INFORMATION