Explore / Query Library: Enable run button #87882
Conversation
gelicia
force-pushed
the
kristina/run-query-library
branch
from
53c55cd
to
1645856
Compare
| }); | ||
| }; | ||
|
|
||
| const validateDatasources = async (datasourceUids: Array<string | undefined>): Promise<Promise<boolean>> => { |
There was a problem hiding this comment.
@ifrost - this is new logic I'm adding for this component. Basically, I discovered one of the tests for the RichHistoryCard isn't testing the right thing, and passes for an unrelated reason to what the test is for. (This has a button thats disabled because "mixed" isnt in the datasource filter, not because the query datasource doesnt exist. It's an edge case bug, so I'm not too concerned about it).
TL;DR: We aren't doing query-level datasource validation yet and we probably should to be sure people arent trying to run queries on invalid datasources.
I want to fix that and make sure that we're sure when we go to run a query that the datasources for it exist. However, this does mean a datasource lookup per query (filtering for unique values) since we do this once per button. Since it's likely we'd have several of these buttons rendered for the same datasource, maybe it's inefficient and we should be testing that outside of the button - but it does make sense for the button to know if it should be disabled or not. I don't think this should slow anything down but it might be inefficient.
There was a problem hiding this comment.
So it looks like a bug - in mixed we always shows the button enabled. To me it feels like it should be tackled in a separate PR to make it easier to review/track. I would prefer to have features and bugs committed separately (e.g. in case feature needs to be reverted/rolled back - we don't want to roll back the fix :D)
Not sure if we need to pull the API of the data source. Maybe getting settings dataSourceSrv.getInstanceSettings is enough (it's synchronous so it's simpler). And as you mentioned we already have some sort of validation in DatasourceInfo component so might be better to do it outside run button. The solution may depend on UX feedback as well, and we may try out different things so might be better to isolate it to a seprate isse. wdyt?
…na/run-query-library
| @@ -304,117 +298,6 @@ describe('RichHistoryCard', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('run queries', () => { | |||
| it('should be disabled if at least one query datasource is missing when using mixed', async () => { | |||
There was a problem hiding this comment.
This test was not accurate and was removed. A test that actually works will be developed for #88001
gelicia
added
no-backport
gelicia
requested review from
tskarhed and
JoaoSilvaGrafana
and removed request for
a team
There was a problem hiding this comment.
This is fantastic! Great stuff! 🎉
@diegoadams qq - should we close the drawer when a query is run? 🤔 (not a blocker for this PR, we can add it as a follow-up)
|
Hey @ifrost ,Yes, the drawer should close after the user hits “Run Query.” While the query library allows for editing, deleting etc., its core user flow is still to open the drawer, find the query, and run it. Therefore, closing the drawer after running the query makes sense in this context |
What is this feature?
This adds a component that will be either a simple button to run a query (if not in split mode), or be a menu dropdown to allow the user to choose what pane to run a query in (if in split mode). This takes the code written for Query History / RichHistoryCard and makes it generic, then uses it for Query Library.
Why do we need this feature?
Both query library and query history will use this button the same way, and it's possible other features going forward may as well.
Which issue(s) does this PR fix?:
Fixes #83097
Special notes for your reviewer:
While moving the tests over from RichHistoryCard, I discovered one of the tests does pass but not for the thing we were testing. Simply put, while Query History does root-level datasource validation, it does not validate query datasources, meaning if a mixed datasource is saved but one of the query datasources is deleted, it will still be allowed to run. After discussing it with @ifrost , we decided to move that problem to its own ticket and deal with it separately from this work. That ticket is #88001
Its also worth noting that this solution deviates a bit from the attached ticket. We use the existing paradigm to indicate what pane should run the query, but we do not prompt that a query will be overwritten. I think it is fine - it's explicit enough of an action that the overwriting is known, and we can revisit adding a modal to warn users if they run into issues. Its just important to note that the behavior will be the same between this and query history.
Please check that: