Variables: Fix assetId field variable replacement #172
Conversation
idastambuk
requested review from
fridgepoet and
sarahzinger
and removed request for
a team
|
Backend code coverage report for PR #172 |
|
Frontend code coverage report for PR #172
|
|
Levitate is-compatible report: 🔍 Resolving @grafana/data@latest... 🔬 Checking compatibility between ./src/module.ts and @grafana/data@9.4.7... 🔬 Checking compatibility between ./src/module.ts and @grafana/ui@9.4.7... 🔬 Checking compatibility between ./src/module.ts and @grafana/runtime@9.4.7... 🔬 Checking compatibility between ./src/module.ts and @grafana/e2e-selectors@9.4.7... ✔️ ./src/module.ts appears to be compatible with @grafana/data,@grafana/ui,@grafana/runtime,@grafana/e2e-selectors |
| }) | ||
| : null; | ||
| // Sitewise doesn't support adhoc vars so this should be fine | ||
| if (variableValue && variableValue.type !== 'adhoc') { |
There was a problem hiding this comment.
Ad hoc vars are key-value pairs that can be added on the go, directly from the panel view. Im not entirely sure how one gets values from them, but Sitewise doesn't support them (at least that's what it was when I try to add one), so we don't need to account for them anyway here.
This specific if block is mostly a type guard, as getVariables can return adHoc variable type too, and those objects don't have a value.current that we can get the value from.
| @@ -143,7 +149,7 @@ export class DataSource extends DataSourceWithBackend<SitewiseQuery, SitewiseOpt | |||
| propertyAlias: templateSrv.replace(query.propertyAlias, scopedVars), | |||
| region: templateSrv.replace(query.region || '', scopedVars), | |||
| propertyId: templateSrv.replace(query.propertyId || '', scopedVars), | |||
| assetIds, | |||
| assetIds: [...new Set(assetIds)], | |||
There was a problem hiding this comment.
Is it necessary to convert the array into a set and then back to an array?
There was a problem hiding this comment.
Initially I didn't do this, but when I was testing I realized if I had two variables that contained the same id, they would get repeated in the assetId array, so doing this removes the duplicate values. Duplicates are probably fine and it won't break anything, but just in case, maybe?
src/DataSource.test.ts
Outdated
| getTemplateSrv: () => { | ||
| return { | ||
| getVariableName(variableId: string) { | ||
| return variableId.slice(1); |
There was a problem hiding this comment.
I think I don't follow this slice, is it just generating a string? Would it work the same as if we returned the variableId?
There was a problem hiding this comment.
Youre totally right, this was initially gonna be a more complicated test so this can definitely be simplified now! Just pushed
idastambuk
deleted the
158-cannot-select-defined-hierarchies-when-using-list-associated-assets-query-type-with-another-variable
branch
What this PR does / why we need it:
When selecting variables for assetIds field, if a variable is a string, the JSON.parse() fails, so the variable is added with the value of its ID instead of being correctly interpolated. This fixes that and adds some tests for assetIds.
Which issue(s) this PR fixes:
Fixes #158
Special notes for your reviewer:
This can be reproduced by following the steps from the original PR and with Sitewise from datasource provisioning. The assetId field in the query object in the network tab should contain all the values defined for the variables.
This also fixes the child not appearing when a parent is selected in the List associated assets.
Note: There's another variable interpolation issue which comes from core grafana, the fix for which will be released in 9.5