Dashboard: when updating parameters, run only relevant queries

[rauchy]

When dashboard-level parameters are updated, all widgets in that dashboard are updated, even if they are unaffected by the new parameter values.

For example - in this dashboard, the bottom widget is completely unrelated to param1, yet updating its value refreshes the widget.

This PR changes this behavior by only refreshing the widgets that have at least parameter which maps to one of the dashboard-level parameter which were updated.

[gabrieldutra]

Now we can use parametersWithPendingValues here too :)

[rauchy]

Not sure I understand. Are you referring to using parametersWithPendingValues in the following line? As in

forEach(parametersWithPendingValues, p => p.applyPendingValue());

If that's what you mean, I deliberately avoided that in order to future-proof someone removing the hasPendingValue check in applyPendingValue without knowing of this external concern.

[gabrieldutra]

I wouldn't be that concerned about it (doesn't seem a likely thing or something that would cause a big issue here). But not a big deal in any case :)

[ranbena]

If I understand this correctly, it doesn't work as it should.

@rauchy your PR description is lacking - plz add a detailed problem->solution, a demo dashboard with STR so we can compare master to.

[ranbena]

I'm uncertain Array.includes is supported in Edge 11. Better use lodash.

[ranbena]

Filtering by name won't do as param names are unique only within a widget scope.
The same widget can show up twice in a dashboard - one with a dashboard-level param, the other with a widget-level param, and with this implementation, both will get refreshed instead of only one. (Same goes for different widgets that have the same param names).

Here's a demo https://deploy-preview-3804--redash-preview.netlify.com/dashboard/param-update-tester. Change the dashboard parameter, and all widgets refresh even though the middle one shouldn't, since it only has a widget-level param.

[rauchy]

Good catch. I switched it to use the proper parameter mapping.

[ranbena]

Works perfectly 👍

I know you must have worked on making the filter logic readable but I think it's still a bit complex to comprehend. Here's my suggestion:

let affectedWidgets = dashboard.widgets;
if (updatedParameters.length) {
  const updateParamNames = updatedParameters.map(p => p.name); // do just once
  affectedWidgets = dashboard.widgets.filter(
    widget => _.chain(widget.getParameterMappings()) // I like chaining :P
      .filter({ type: 'dashboard-level' })
      .some(({ mapTo }) => _.includes(updateParamNames, mapTo))
      .value(),
  );
}

[arikfr]

Chaining prevents webpack from including only the functions we use from lodash 😬

[rauchy]

@ranbena thanks for introducing me to lodash chain sequences!

I don't think it would be significant to lose tree shaking on lodash, but I'm still not entirely convinced that the change from

filter({ type }) => type === 'dashboard-level')

to

filter({ type: 'dashboard-level' })

makes things simpler, especially given that some simpletons such as myself would possibly have to scratch their heads around _.chain and value(). The main disadvantage I see here is transforming this from an expression (const affectedWidgets = stuff) to a declarative block.

I personally find expressions a primary goal for achieving simplicity.

[gabrieldutra]

Chaining prevents webpack from including only the functions we use from lodash

It does makes things a lot more readable, I wonder if it's really that costly when considering Lodash 😬

[ranbena]

Forget it! Let's land this already!

[rauchy]

your PR description is lacking - plz add a detailed problem->solution, a demo dashboard with STR so we can compare master to.

@ranbena hehe, I used hub pull-request so I guess the original issue was lacking that info. I can go ahead and update it, but it looks like you already created a demo dashboard :)

[ranbena]

One code suggestion. Not a must.

[ranbena]

Works perfectly 👍

I know you must have worked on making the filter logic readable but I think it's still a bit complex to comprehend. Here's my suggestion:

let affectedWidgets = dashboard.widgets;
if (updatedParameters.length) {
  const updateParamNames = updatedParameters.map(p => p.name); // do just once
  affectedWidgets = dashboard.widgets.filter(
    widget => _.chain(widget.getParameterMappings()) // I like chaining :P
      .filter({ type: 'dashboard-level' })
      .some(({ mapTo }) => _.includes(updateParamNames, mapTo))
      .value(),
  );
}