Shouldn't request_parameters_to_modify be applied to the parameters so they are modified in the JS (ie, in the dataTableInstance.params var)? Or maybe that's custom_parameters...

Good point, so we could basically automatically add request_parameters_to_modify to custom_parameters?

One problem might be to know which of those custom params should be used for the export link. Eg sometimes Visualizations etc assign custom parameters like viewDataTable or columns and I'm not sure how we would know which params from custom_parameters to ignore when generating the export link.

I'm not exactly sure how this relates to this issue, but my understanding is that request_parameters_to_modify is for the internal request where custom_parameters is for forwarding params to js. Maybe the visualization is doing something strange?

Not really, as we need to make sure the same request parameters that were used to fetch the datatable in the visualization internally are also used when exporting the datatable. Otherwise the result would not be the same.

Eg the Insights API needs additional parameters apart from date, period and idSite to work. This way we make sure the results will be the same and that it just works.

Perhaps in that case, the parameters should be added to both request_parameters_to_modify and custom_parameters? Assuming that actually works, it seems simpler than changing the DataTable export link code.

The export link code does not use custom_parameters currently as we not want to have all custom_parameters in the export link. Some custom parameters are only meant for the visualization but not for the export/API eg viewDataTable or columns. Using them also for the export link could result in errors eg re columns etc.

I mean custom_parameters in Visualization is used to set the parameters passed to JavaScript (see Visualization::getClientSideParametersToSet()). In the JavaScript the parameters (from self.param) are used to build the export URL (see handleExportBox in dataTable.js). So if setting a custom_parameter changes the JavaScript self.param value, setting a custom parameter for the Insights use case might fix the export link problem. It might also cause some side effects, and I haven't done any testing.

Anyway, I'm not invested in how this issue is solved.

It wouldn't fix the export link problem since the handleExportBox code doesn't simply use all custom_parameters and neither all self.param parameters. It only uses specific ones eg self.param.date or self.param.filter_pattern which is good since not all self.param parameters are supposed to be used for this export link. If I'd assign them to custom parameters I would have to add a hack with a list of 5 more parameters to dataTable.js just for Insights but it should not have any knowledge about Insights. Insights is not a special use case. Other visualizations/reports already have the same problem.

The only thing I could think of would be to set request_parameters_to_modify to custom_parameters eg we would have custom_parameters.request_parameters_to_modify = {...} but then it is no longer only custom parameters as we would have to do this always.

It would be maybe worth refactoring dataTable.js to have self.params and self.requestParams similar to our two Visualizations configs (Vis\Config and Vis\RequestConfig) but then we will rewrite it to use AngularJS at some point anyway...

FYI, in case it's not clear, I have no issues w/ this pull request being merged.

I got that. Just tried to understand in case I misunderstood something... would have been nice if there was a better / easier way

As you said, angular will provide one ;)