AdHocFilters: Show reason for non-applicable filters #1181
Conversation
…th reason for nonapplicability
| @@ -172,7 +172,7 @@ export function AdHocFilterPill({ filter, model, readOnly, focusOnWipInputRef }: | |||
| </Tooltip> | |||
| )} | |||
|
|
|||
| {filter.origin && !filter.restorable && !filter.readOnly && ( | |||
| {filter.origin && !filter.restorable && !filter.readOnly && !filter.nonApplicable && ( | |||
There was a problem hiding this comment.
Can we extract this condition evaluation to either a function or a variable with a comment that describes what this condition is actually doing?
| update.originFilters?.forEach((f) => { | ||
| const isApplicable = response.includes(f.key); | ||
| const filter = responseMap.get(`${f.key}-${f.origin}`); |
There was a problem hiding this comment.
Does this code depend on #1185?
There was a problem hiding this comment.
no, this is totally independent although the key is similar
| if (!ds || !ds.getFiltersApplicability()) { | ||
| if (setStateOnCheckFail) { | ||
| this.setState(update); | ||
| } |
There was a problem hiding this comment.
I don't think I understand the setStateOnCheckFail flag. Isn't this an equivalent of !ds || !ds.getFiltersApplicability really? IIUC what you want to do here is to allow immediate setState in case ds does not support the getFiltersApplicability.
There was a problem hiding this comment.
Yeah this is a bit confusing, I refactored it a bit, hoping it makes more sense. Basically that extra flag is for not setting state in the activation handler. So in most cases I replace a setState with this new call, but in the activation handler I am not replacing any setState: if there is no filters applicability call then I simply want to return because there is for sure no state update. That is what this flag does
| const ds = await this._dataSourceSrv.get(this.state.datasource, this._scopedVars); | ||
| // @ts-expect-error (temporary till we update grafana/data) | ||
| if (!ds || !ds.getApplicableFilters) { | ||
| return []; | ||
| if (!ds || !ds.getFiltersApplicability()) { |
There was a problem hiding this comment.
Why are you calling getFiltersApplicability method here? This should be just a check whether or not the data source has this API implemented. The actual execution is happening after that check.
There was a problem hiding this comment.
yess, this is a typo 😞 thank you for catching this!
| @@ -837,6 +837,8 @@ describe.each(['11.1.2', '11.1.1'])('AdHocFiltersVariable', (v) => { | |||
|
|
|||
| scopesVariable.update(); | |||
|
|
|||
| await new Promise((r) => setTimeout(r, 1)); | |||
There was a problem hiding this comment.
need to add this to all scopes updating tests since now it goes through _setStateWithFiltersApplicabilityCheck which awaits for the ds query
| @@ -325,7 +336,10 @@ export class AdHocFiltersVariable | |||
| ]; | |||
|
|
|||
| // maintain other originFilters in the array, only update scopes ones | |||
| this.setState({ originFilters: [...finalFilters, ...remainingFilters] }); | |||
| this._setStateWithFiltersApplicabilityCheck({ | |||
| filters: this.state.filters, | |||
There was a problem hiding this comment.
I reckon we do not need to clone this as scenes deals nicely with it and keeps things cleaner imo in the adhocs setState call when checking update state changes.
|
|
||
| public async _setStateWithFiltersApplicabilityCheck( | ||
| update: Partial<AdHocFiltersVariableState>, | ||
| returnWithoutStateUpdate?: boolean |
There was a problem hiding this comment.
@mdvictor please add a verbose explanation of this flage.
There was a problem hiding this comment.
removed flag alltogether, the code management overhead is quite large for this small of a performance optimization imo
|
This sounds like a nice improvement 👏 Small request from me - might you be able to put a screenshot on the PR description, so we can easily see what this will look like? 🙏 |
mdvictor
added
minor
|
🚀 PR was released in |
Builds on top of #1152 and refactors things a bit, mainly calls a different DS method from
getApplicableFilterstogetFiltersApplicabilityNon-applicable filters can now also show a reason why the filter pill is invalid. It will also re-evaluate applicability on filter removal.