QueryHistory: Improve handling of mixed datasource entries #62214
Conversation
Elfo404
added
pr/early-feedback
Elfo404
requested review from
axelavargas,
ivanortegaalba,
joshhunt and
ashharrison90
and removed request for
a team
|
For the cases where a datasource was removed, I would recommend graying out the query in the UI, and greying out the button if all queries in the history are greyed out. I think we should still keep the text in the db as usual and display it in case the user has a query or something they want to copy/paste out of there. |
|
Backend code coverage report for PR #62214 |
|
Frontend code coverage report for PR #62214
|
|
after discussing briefly this last week we decided to disable running queries if at least one of the queries in an entry has a missing datasource. |
| { query: 'query3', refId: 'C' }, | ||
| ], | ||
| }, | ||
| }); | ||
| const datasourceName = await screen.findByLabelText('Data source name'); | ||
| expect(datasourceName).toHaveTextContent('Data source does not exist anymore'); |
There was a problem hiding this comment.
Would we want to test the button is disabled here too?
| setQueries(exploreId, queriesToRun); | ||
| } else { | ||
| setQueries(exploreId, queriesToRun); | ||
| await changeDatasource(exploreId, query.datasourceUid); |
There was a problem hiding this comment.
👏 importing queries doesn't make any sense here - thank you!
| @@ -161,42 +152,68 @@ export function RichHistoryCard(props: Props) { | |||
| } = props; | |||
| const [activeUpdateComment, setActiveUpdateComment] = useState(false); | |||
| const [comment, setComment] = useState<string | undefined>(query.comment); | |||
| const [queryDsInstance, setQueryDsInstance] = useState<DataSourceApi | undefined>(undefined); | |||
| const { value, loading } = useAsync(async () => { | |||
There was a problem hiding this comment.
This is the nittyest of nits, but what do you think about something besides value for this variable name? I know it's literally the value of the rich history card so your variable name makes sense, but we have areas reading queries values from props and some from this, it might be helpful to either define this variable name or leave a comment. Maybe like dsPopulatedData dsEnhancedRecord for possible variable names?
There was a problem hiding this comment.
tbh i think with a few rafactorings that happened variable names in this component went all bonkers (like queryDsInstance, dsInstance, query (which is instead historyEntry) ), i actually settled for value because it felt the lcearer to me :D
i wanted to rename ~all the things but didn't want to pollute the PR with a bunch of renaming that would have made reviewing harder. if it's ok tho i'll just push an extra commit to do all the renaming. wdyt?
There was a problem hiding this comment.
Nah, I understand where you're coming from. Feel free to leave it, it's easy enough to understand what's going on
There was a problem hiding this comment.
i'll create another issue when merging this and a follow up pr to address all the ambiguous names in this file
What is this feature?
Improves QueryHistory display and handling of mixed datasource queries in Explore.
A mixed entry where all datasources implement
getQueryDisplayText:note that explore's root datasource was set to Mixed. when an entry datasource does not match explore's root datasource, this would be shown instead:
A regular entry wit no mixed queries:
A mixed entry with one of the datasources that does not implement
getQueryDisplayText:open points:
A mixed entry that has queries from an unavailable datasources still shows an active run queries button. upon clicking it Explore will show an error i the query row stating that the datasource was not found (for each query that has a missing datasource).
Not sure what the best approach is here: removing the queries without a datasource seem sensible when a mixed entry also contains queries for which the datasource is still available. and in case no query has a datasource available maybe the card should behave like a regular entry; disabling the run query button (or probably removing it in the first place)
opinions?
Why do we need this feature?
We are gradually introducing support for Mixed datasource queries in Explore. Query History was not initially meant to handle Mixed datasource entries.
Which issue(s) does this PR fix?:
Fixes #61089
Special notes for your reviewer: