Bug 23626 - Add a system preference to limit the number of rows of data used when charting or exporting report results
Summary: Add a system preference to limit the number of rows of data used when chartin...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: master
Hardware: All All
: P2 major (vote)
Assignee: Paul Hoffman
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 23624
Blocks: 28470
  Show dependency treegraph
 
Reported: 2019-09-16 19:51 UTC by Paul Hoffman
Modified: 2022-06-06 20:25 UTC (History)
10 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.05


Attachments
New syspref ReportsMaxRowsInChart (3.27 KB, patch)
2019-09-16 19:51 UTC, Paul Hoffman
Details | Diff | Splinter Review
Bug 23626 - Add a system preference to limit the number of rows of data used in a chart when viewing report results (4.22 KB, patch)
2019-09-17 17:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23626 - Add a system preference to limit the number of rows of data used in a chart when viewing report results (4.22 KB, patch)
2019-09-17 17:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23626 - Add a system preference to limit the number of rows of data used in a chart when viewing report results (4.22 KB, patch)
2019-09-17 18:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23626: Add database update and system preference to web ui (2.08 KB, patch)
2019-09-17 18:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23626: Don't enable by default (2.50 KB, patch)
2019-09-17 18:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23626 - Add a system preference to limit the number of rows of data used in a chart when viewing report results (4.28 KB, patch)
2019-09-17 19:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23626: Add database update and system preference to web ui (2.13 KB, patch)
2019-09-17 19:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23626: Don't enable by default (2.56 KB, patch)
2019-09-17 19:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23626: Move message to relavent chart control (3.14 KB, patch)
2019-09-20 10:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23626: Move message to relevant chart control (3.14 KB, patch)
2019-09-20 10:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23626: [alternate] Use svc/reports to fetch chart data (4.83 KB, patch)
2019-09-23 15:42 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23626: [alternate] Use svc/reports to fetch chart data (4.89 KB, patch)
2019-09-24 11:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Broaden the scope of the system preference to include exported report results (8.32 KB, patch)
2019-09-27 12:28 UTC, Paul Hoffman
Details | Diff | Splinter Review
Bug 23626: [alternate] Only fetch full chart data if requested (8.40 KB, patch)
2019-10-02 10:07 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23262: Use correct parameter in URL (1.66 KB, patch)
2019-10-03 13:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23626: [alternate] Only fetch full chart data if requested (8.39 KB, patch)
2019-10-04 09:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23626: [alternate] Only fetch full chart data if requested (8.57 KB, patch)
2019-10-04 09:27 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 23626: [alternate] Only fetch full chart data if requested (8.69 KB, patch)
2019-10-04 10:01 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 23626: [alternate] Only fetch full chart data if requested (8.75 KB, patch)
2019-10-09 12:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23626: [alternate] Default to including all rows (1.33 KB, patch)
2019-10-09 12:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23626: Add missing TT failure (1.26 KB, patch)
2019-10-09 12:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23626: (follow-up) Fix JS when not viewing results (1.22 KB, patch)
2019-10-10 16:41 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Hoffman 2019-09-16 19:51:19 UTC
Created attachment 92845 [details] [review]
New syspref ReportsMaxRowsInChart

The number of rows generated by a report is potentially unlimited; this has the potential to exhaust all available memory, bringing a Koha server down to its knees.

The attached patch introduces a system preference to limit the number of rows used when charting report results; at this time, and with bug #23624 fixed, that is the only aspect of reporting that loads all results into memory at once.
Comment 1 Paul Hoffman 2019-09-17 15:00:28 UTC
I've changed the priority and severity on this bug report, along with #23624, to P2/major since the problem they solve can make Koha unusable.
Comment 2 Kyle M Hall 2019-09-17 17:11:47 UTC
Created attachment 92904 [details] [review]
Bug 23626 - Add a system preference to limit the number of rows of data used in a chart when viewing report results

The number of rows generated by a report is potentially unlimited; this has the potential to exhaust all available memory, bringing a Koha server down to its knees.

The attached patch introduces a system preference to limit the number of rows used when charting report results; at this time, and with bug #23624 fixed, that is the only aspect of reporting that loads all results into memory at once.
Comment 3 Kyle M Hall 2019-09-17 17:40:05 UTC
Created attachment 92905 [details] [review]
Bug 23626 - Add a system preference to limit the number of rows of data used in a chart when viewing report results

The number of rows generated by a report is potentially unlimited; this has the potential to exhaust all available memory, bringing a Koha server down to its knees.

The attached patch introduces a system preference to limit the number of rows used when charting report results; at this time, and with bug #23624 fixed, that is the only aspect of reporting that loads all results into memory at once.
Comment 4 Kyle M Hall 2019-09-17 18:20:31 UTC
Created attachment 92908 [details] [review]
Bug 23626 - Add a system preference to limit the number of rows of data used in a chart when viewing report results

The number of rows generated by a report is potentially unlimited; this has the potential to exhaust all available memory, bringing a Koha server down to its knees.

The attached patch introduces a system preference to limit the number of rows used when charting report results; at this time, and with bug #23624 fixed, that is the only aspect of reporting that loads all results into memory at once.
Comment 5 Kyle M Hall 2019-09-17 18:20:38 UTC
Created attachment 92909 [details] [review]
Bug 23626: Add database update and system preference to web ui
Comment 6 Kyle M Hall 2019-09-17 18:20:41 UTC
Created attachment 92910 [details] [review]
Bug 23626: Don't enable by default
Comment 7 Kyle M Hall 2019-09-17 18:47:40 UTC
I see the message about data limiting, but the charge doesn't seem to change.
I don't see anything in the patch that actually changes the chart generating logic.

Also, just so you know, your patches were formatted for git. I went ahead and made them into git patches for you!
Comment 8 Kyle M Hall 2019-09-17 18:48:04 UTC
> Also, just so you know, your patches were formatted for git. I went ahead
> and made them into git patches for you!

*not* that is, your patches were not formatted for applying with git.
Comment 9 Kyle M Hall 2019-09-17 19:30:55 UTC
Created attachment 92919 [details] [review]
Bug 23626 - Add a system preference to limit the number of rows of data used in a chart when viewing report results

The number of rows generated by a report is potentially unlimited; this has the potential to exhaust all available memory, bringing a Koha server down to its knees.

The attached patch introduces a system preference to limit the number of rows used when charting report results; at this time, and with bug #23624 fixed, that is the only aspect of reporting that loads all results into memory at once.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Kyle M Hall 2019-09-17 19:31:04 UTC
Created attachment 92920 [details] [review]
Bug 23626: Add database update and system preference to web ui

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Kyle M Hall 2019-09-17 19:31:06 UTC
Created attachment 92921 [details] [review]
Bug 23626: Don't enable by default

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 12 Tomás Cohen Arazi 2019-09-19 14:58:08 UTC
Hi, I would like an example query to be used on charts, to understand the problem better.
My understanding is that charts should be used in statistical reports, i.e. those that count things, calculate averages, etc. Those kind of reports aren't usually returning thousands of rows. I'm sure I'm missing something, so please share with me some real-life report to understand the problem better
Comment 13 Paul Hoffman 2019-09-19 16:35:13 UTC
Here's an example from our Koha instance:

SELECT monthname(timestamp) AS month, year(timestamp) AS year, count(itemnumber) AS count
FROM items
WHERE timestamp BETWEEN <<Between (yyyy-mm-dd)|date>> AND <<and (yyyy-mm-dd)|date>>
GROUP BY year(timestamp), month(timestamp)

I have screenshots showing the steps in charting this -- I can attach them if they're useful.

However, I don't want this to distract from the problem that this bug report and patch are trying to mitigate (not solve!) which is that no matter what the report is and what the user might end up doing with it -- charting, downloading as *.ods or *.csv, or just viewing page by page -- the full report results (n rows of data with n unbounded) are loaded into memory on the Koha server, converted to JSON, and then returned to the user embedded in the generated HTML.

For example, I ran a query that returned a single row; it yielded HTML containing the following (some whitespace, along with the bulk of the function body, removed for clarity):

--------------------------------------------------------------------------------
$('#draw-chart').click(function() {
    var x_elements = $('select[name="x"]').val();
    var y_elements = [];
    var groups = [];
    var lines = [];
    var options = {};
    headers = [{"cell":"month"},{"cell":"year"},{"cell":"count"}];
    var results;
    if ($('input[name="chart-include-all"]').prop('checked')) {
        results = [{"cells":[{"cell":"September"},{"cell":2019},{"cell":39}]}]
    }
    else {
        results = [{"cells":[{"cell":"September"},{"cell":"2019"},{"cell":"39"}]}]
    }
    [...]
}
--------------------------------------------------------------------------------

The first line that sets the _results_ variable is where the full report results appear; if the report had yielded 10,000 rows, there would be 10,000 elements in the array.

And I haven't even touched on the possibility of a query like this that fails to properly JOIN tables:

SELECT foo.bar, baz.qux
FROM   foo, baz

If foo and baz each contain 10,000 rows, that's 100 million rows returned by the DB, loaded into memory, converted to JSON, and -- well, it won't get that far, because nobody is running a Koha instance with enough RAM and swap to survive this.  But I would bet that a lot of Koha instances have (staff) users with the requisite permissions to write reports but not the expertise in SQL and RDBMSes that would steer them away from writing a query like this.

Stepping outside the bounds of this bug report for a moment, I'll just mention some options for truly *fixing* the underlying problem that may be worth considering; these include (a) Removing the option to chart the full report results; (b) Implementing full-data charting using callbacks (AJAX or whatever) to fetch data from Koha only as it's needed; and (c) Ripping out the charting feature altogether on the principle of "do one thing well", which entails taking the position that charting is best done in a tool (Libreoffice Calc, Excel, whatever) dedicated to that task.  This last one is my favorite, but I'm sure not everyone agrees!  :-)
Comment 14 Tomás Cohen Arazi 2019-09-19 18:25:55 UTC
Paul, I understand the problem now. I'm not sure what's the best solution, even though I have a tendency to think we could send the data stream into a temporary file if the resultset size exceeds some fixed value, then send the file.

That said, I think this issue and a proper solution deserves being discussed on our regular dev meetings on IRC. The next one is due to take place on sept 25th. [1]

I'd suggest you add this problem and approach to the agenda so the dev team as a whole can discuss it. Also, adding it to the agenda in advance gives everyone time to think about it.

As of the patchset itself, the only thing I wouldn't accept is that there's no fallback to a default value when reading the syspref. i.e. it could be undef and break things.

[1] https://wiki.koha-community.org/wiki/Next_IRC_meetings
Comment 15 Paul Hoffman 2019-09-19 20:37:40 UTC
Thanks, Tomás.  You're right; the patch should be changed so that it defaults to zero:

            my $max_rows = C4::Context->preference('ReportsMaxRowsInChart') || 0;

Should I submit a modified patch and mark the previous one (attachment 92919 [details] [review]) obsolete?  The only problem is -- I'm embarrased to admit this -- I don't know how to make git format-patch generate the right format.
Comment 16 Kyle M Hall 2019-09-20 10:54:16 UTC
Created attachment 92992 [details] [review]
Bug 23626: Move message to relavent chart control

As it stands, this limit message makes it sounds like the chart data is limited where it clearly is not.
Instead, we should show the limit on the 'use all data' checkbox.

As it stands, you could actual have more results data in the chart than the system preference allows
by only using the displayed results! I think this is a benefit and not a problem, since we are not
refetching those data to generate the chart.

Instead, we need just move the 'warning' to make it clear when the syspref limit will be used and relevant.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall 2019-09-20 10:59:33 UTC
Created attachment 92994 [details] [review]
Bug 23626: Move message to relevant chart control

As it stands, this limit message makes it sounds like the chart data is limited where it clearly is not.
Instead, we should show the limit on the 'use all data' checkbox.

As it stands, you could actual have more results data in the chart than the system preference allows
by only using the displayed results! I think this is a benefit and not a problem, since we are not
refetching those data to generate the chart.

Instead, we need just move the 'warning' to make it clear when the syspref limit will be used and relevant.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Kyle M Hall 2019-09-20 11:03:05 UTC
Video Demonstration: https://monosnap.com/file/eCK8NlXEG5hN0Z0KwHkxsuR0Hf7nq6
Comment 19 Nick Clemens 2019-09-23 15:42:14 UTC Comment hidden (obsolete)
Comment 20 Nick Clemens 2019-09-23 15:47:39 UTC Comment hidden (obsolete)
Comment 21 Katrin Fischer 2019-09-23 16:06:09 UTC Comment hidden (obsolete)
Comment 22 Nick Clemens 2019-09-23 16:35:23 UTC Comment hidden (obsolete)
Comment 23 Kyle M Hall 2019-09-24 11:12:25 UTC
Created attachment 93137 [details] [review]
Bug 23626: [alternate] Use svc/reports to fetch chart data

We have a method for getting json report data via svc, and this has a built in
limiter in the sysprefs. We can utilise this for our charts to avoid
passing all data in memory

To test:
1 - Write a report that can be charted, for example:
    SELECT RIGHT(barcode,2), COUNT(*)
    FROM items
    WHERE RIGHT(barcode,1) != <<Number>>
    GROUP BY RIGHT(barcode,2)
2 - Set the 'Cache expiry' to 1 for this report
3 - Run it and click 'Create chart' on results page
    Chart type: Pie
    x column RIGHT(barcode,2)
    Include all rows: UNCHECKED
4 - Note you get a nice pie chart
5 - Click 'Create chart', same except check 'Include all rows'
6 - Note you get a different pie
7 - Apply patch
8 - Repeat above - note that 'all rows' is now 'Fetch additional rows' and notes the sys pref control
9 - Increase SvcMaxReportRows and note chart changes

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Katrin Fischer 2019-09-24 11:15:58 UTC
I feel if we need this limit, we should not tie it to the pref and have another (yes...) and also display a tooltip or something. Otherwise this will be very hard to figure out for users. And the preset limit of 20 or something for the pref is very low.
Comment 25 Paul Hoffman 2019-09-24 18:43:56 UTC
I'm worried that a rewrite of the report charting capability is going to take some time, and meanwhile our Koha server is liable to run out of memory at any time when someone runs a report that generates a lot of results.

How would you all feel about sticking to the original bug fix plan for now and reworking charting later?
Comment 26 Katrin Fischer 2019-09-24 21:06:39 UTC
(In reply to Paul Hoffman from comment #25)
> I'm worried that a rewrite of the report charting capability is going to
> take some time, and meanwhile our Koha server is liable to run out of memory
> at any time when someone runs a report that generates a lot of results.
> 
> How would you all feel about sticking to the original bug fix plan for now
> and reworking charting later?

Hi Paul, the maintenance releases just happened, so it will take some time to get a fix into Koha proper (until next month's bugfix release). Using reports is permission based, maybe inform your staff of the problem? Also implementing a temporary fix might be an option if you are running your own installation.

Btw - I like the idea of a separate preference as I don't want to mix things with the web service preference/caching that could cause too much confusion.
Comment 27 Nick Clemens 2019-09-24 23:40:26 UTC
(In reply to Katrin Fischer from comment #26)
> (In reply to Paul Hoffman from comment #25)
> > I'm worried that a rewrite of the report charting capability is going to
> > take some time, and meanwhile our Koha server is liable to run out of memory
> > at any time when someone runs a report that generates a lot of results.
> > 
> > How would you all feel about sticking to the original bug fix plan for now
> > and reworking charting later?
> 
> Hi Paul, the maintenance releases just happened, so it will take some time
> to get a fix into Koha proper (until next month's bugfix release). Using
> reports is permission based, maybe inform your staff of the problem? Also
> implementing a temporary fix might be an option if you are running your own
> installation.
> 
> Btw - I like the idea of a separate preference as I don't want to mix things
> with the web service preference/caching that could cause too much confusion.

Obsoleted my patches, this should be ready for QA
Comment 28 Marcel de Rooy 2019-09-27 09:10:24 UTC
QA Comment:

We are ignoring execute_query's parameters: $offset, $limit
In Guided.pm you can find code around stripping limits etc. too.
And now we are looping thru all results in order to apply the limit.
Does not look good to me.
Note that the interface should not offer more results than we allow. And should we offer the offset parameter on Reporting ?

Minor detail:
ReportsMaxRowsInChart: I think that it should go under Tools, not under Webservices.
Comment 29 Marcel de Rooy 2019-09-27 09:11:53 UTC
Detail on submitting patches:

Processing additional checks
        * Commit title does not start with 'Bug XXXXX: ' - 2cd0b04b9d
Comment 30 Martin Renvoize 2019-09-27 09:20:16 UTC
We discussed this bug during the Development Meeting and concluded that a simpler fix for now would be to add a system preference to enable/disable charting functionality entirely.

In this way we would be able to skip the 'all results' call entirely whilst we work on another implementation to allow fetching of results in a more memory friendly way.
Comment 31 Paul Hoffman 2019-09-27 12:00:36 UTC
(In reply to Paul Hoffman from comment #0)
> The attached patch introduces a system preference to limit the number of
> rows used when charting report results; at this time, and with bug #23624
> fixed, that is the only aspect of reporting that loads all results into
> memory at once.

Well, I was totally wrong.  Exporting the results (tab-delimited, CSV, or *.ods) also loads all results into memory at once.

I am working on patches that broaden the sys pref and use it to limit the rows when exporting.
Comment 32 Paul Hoffman 2019-09-27 12:28:29 UTC
Created attachment 93204 [details] [review]
Broaden the scope of the system preference to include exported report results

This patch brings the previous set of patches together, renames the system preference, and applies the limit (if set) to exported results.

It will require some additional tweaking to insert a prominent note in the UI when the limit has been set.
Comment 33 Paul Hoffman 2019-09-27 12:50:33 UTC
(In reply to Martin Renvoize from comment #30)
> We discussed this bug during the Development Meeting and concluded that a
> simpler fix for now would be to add a system preference to enable/disable
> charting functionality entirely.
> 
> In this way we would be able to skip the 'all results' call entirely whilst
> we work on another implementation to allow fetching of results in a more
> memory friendly way.

Since the problem now turns out to affect exporting as well as charting, I don't think we should add a system preference to enable/disable charting.  Or, if we do, it should be a lower priority than this bug.
Comment 34 Nick Clemens 2019-10-02 10:07:07 UTC
Created attachment 93406 [details] [review]
Bug 23626: [alternate] Only fetch full chart data if requested

This patchset prevents the full return of report data unless explicitly requested by the user for charting purposes
Additionally the user is warned if requesting more than 1000 rows of data

To test:
 1 - Create a report that returns over 1000 rows of data
 2 - Run the report
 3 - Note you have two buttons now 'Chart data' and 'Fetch all data for chart'
 4 - Click chart data
 5 - Note the note that you are only charting visible data
 6 - Create the chart and confirm it works
 7 - Close the chart
 8 - Click 'Fetch all data'
 9 - Note the confirm window
10 - Click 'cancel', note there is no change
11 - Repeat and click ok
12 - Fetch all data button is gone
13 - Page to next data, note fetch all does not return
14 - Click 'Chart data'
15 - Note you now have checkbox option to use all data in report
16 - Click it
17 - Create chart
18 - Confirm it works as expected
Comment 35 Nick Clemens 2019-10-02 10:10:40 UTC
Hi Paul,

We talked at the hackfest, people prefer retaining the ability to fetch full data when charting.

I have provided an alternate patch that prevents loading the full data on initial report load and adds a button to load the data if requested. There is a warning when fetching over 1000 rows. The button could also be hidden if you never want to use additional rows.

Can you test this and confirm it works as needed?

The issues with exporting should probably be filed on a new separate bug to assess.

Thanks,
Nick
Comment 36 Séverine Queune 2019-10-02 15:48:48 UTC
The sandbox you've requested is not ready.
Some problems occurred applying patches from bug 23626:
<h1>Something went wrong !</h1>Applying: Limit memory consumption when exporting, too
fatal: sha1 information is lacking or useless (installer/data/mysql/atomicupdate/bug_23626.perl).
error: could not build fake ancestor
Patch failed at 0001 Limit memory consumption when exporting, too
Comment 37 Nick Clemens 2019-10-03 11:56:23 UTC
(In reply to Séverine Queune from comment #36)
> The sandbox you've requested is not ready.
> Some problems occurred applying patches from bug 23626:
> <h1>Something went wrong !</h1>Applying: Limit memory consumption when
> exporting, too
> fatal: sha1 information is lacking or useless
> (installer/data/mysql/atomicupdate/bug_23626.perl).
> error: could not build fake ancestor
> Patch failed at 0001 Limit memory consumption when exporting, too

I obsoleted the original to allow this to apply on a sandbox
Comment 38 Nick Clemens 2019-10-03 13:00:44 UTC
Created attachment 93589 [details] [review]
Bug 23262: Use correct parameter in URL
Comment 39 Nick Clemens 2019-10-04 09:15:07 UTC
Created attachment 93720 [details] [review]
Bug 23626: [alternate] Only fetch full chart data if requested

This patchset prevents the full return of report data unless explicitly requested by the user for charting purposes
Additionally the user is warned if requesting more than 1000 rows of data

To test:
 1 - Create a report that returns over 1000 rows of data
 2 - Run the report
 3 - Note you have two buttons now 'Chart data' and 'Fetch all data for chart'
 4 - Click chart data
 5 - Note the note that you are only charting visible data
 6 - Create the chart and confirm it works
 7 - Close the chart
 8 - Click 'Fetch all data'
 9 - Note the confirm window
10 - Click 'cancel', note there is no change
11 - Repeat and click ok
12 - Fetch all data button is gone
13 - Page to next data, note fetch all does not return
14 - Click 'Chart data'
15 - Note you now have checkbox option to use all data in report
16 - Click it
17 - Create chart
18 - Confirm it works as expected
Comment 40 Biblibre Sandboxes 2019-10-04 09:27:11 UTC
Patch tested with a sandbox, by Séverine QUEUNE <severine.queune@bulac.fr>
Comment 41 Biblibre Sandboxes 2019-10-04 09:27:34 UTC
Created attachment 93723 [details] [review]
Bug 23626: [alternate] Only fetch full chart data if requested

This patchset prevents the full return of report data unless explicitly requested by the user for charting purposes
Additionally the user is warned if requesting more than 1000 rows of data

To test:
 1 - Create a report that returns over 1000 rows of data
 2 - Run the report
 3 - Note you have two buttons now 'Chart data' and 'Fetch all data for chart'
 4 - Click chart data
 5 - Note the note that you are only charting visible data
 6 - Create the chart and confirm it works
 7 - Close the chart
 8 - Click 'Fetch all data'
 9 - Note the confirm window
10 - Click 'cancel', note there is no change
11 - Repeat and click ok
12 - Fetch all data button is gone
13 - Page to next data, note fetch all does not return
14 - Click 'Chart data'
15 - Note you now have checkbox option to use all data in report
16 - Click it
17 - Create chart
18 - Confirm it works as expected

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 42 Biblibre Sandboxes 2019-10-04 10:01:11 UTC
Patch tested with a sandbox, by Séverine QUEUNE <severine.queune@bulac.fr>
Comment 43 Biblibre Sandboxes 2019-10-04 10:01:36 UTC
Created attachment 93729 [details] [review]
Bug 23626: [alternate] Only fetch full chart data if requested

This patchset prevents the full return of report data unless explicitly requested by the user for charting purposes
Additionally the user is warned if requesting more than 1000 rows of data

To test:
 1 - Create a report that returns over 1000 rows of data
 2 - Run the report
 3 - Note you have two buttons now 'Chart data' and 'Fetch all data for chart'
 4 - Click chart data
 5 - Note the note that you are only charting visible data
 6 - Create the chart and confirm it works
 7 - Close the chart
 8 - Click 'Fetch all data'
 9 - Note the confirm window
10 - Click 'cancel', note there is no change
11 - Repeat and click ok
12 - Fetch all data button is gone
13 - Page to next data, note fetch all does not return
14 - Click 'Chart data'
15 - Note you now have checkbox option to use all data in report
16 - Click it
17 - Create chart
18 - Confirm it works as expected

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 44 Paul Hoffman 2019-10-04 14:06:01 UTC
(In reply to Nick Clemens from comment #35)

> The issues with exporting should probably be filed on a new separate bug to
> assess.

I've reopened bug 23685, which covers the issue with exporting.
Comment 45 Kyle M Hall 2019-10-09 12:29:39 UTC
Created attachment 93914 [details] [review]
Bug 23626: [alternate] Only fetch full chart data if requested

This patchset prevents the full return of report data unless explicitly requested by the user for charting purposes
Additionally the user is warned if requesting more than 1000 rows of data

To test:
 1 - Create a report that returns over 1000 rows of data
 2 - Run the report
 3 - Note you have two buttons now 'Chart data' and 'Fetch all data for chart'
 4 - Click chart data
 5 - Note the note that you are only charting visible data
 6 - Create the chart and confirm it works
 7 - Close the chart
 8 - Click 'Fetch all data'
 9 - Note the confirm window
10 - Click 'cancel', note there is no change
11 - Repeat and click ok
12 - Fetch all data button is gone
13 - Page to next data, note fetch all does not return
14 - Click 'Chart data'
15 - Note you now have checkbox option to use all data in report
16 - Click it
17 - Create chart
18 - Confirm it works as expected

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 46 Kyle M Hall 2019-10-09 12:29:47 UTC
Created attachment 93915 [details] [review]
Bug 23626: [alternate] Default to including all rows

If a person is fetching all data, it seems most likely that that person wants
to see all the data in the chart. We should default to that behavior.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 47 Kyle M Hall 2019-10-09 12:29:50 UTC
Created attachment 93916 [details] [review]
Bug 23626: Add missing TT failure

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 48 Martin Renvoize 2019-10-09 13:32:50 UTC
Nice work!

Pushed to master for 19.11.00
Comment 49 Nick Clemens 2019-10-10 16:41:16 UTC
Created attachment 93981 [details] [review]
Bug 23626: (follow-up) Fix JS when not viewing results
Comment 50 Martin Renvoize 2019-10-17 16:46:02 UTC
Followup pushed.. I just forgot to note it here :)
Comment 51 Fridolin Somers 2019-11-08 15:50:12 UTC
Pushed to 19.05.x for 19.05.05
Comment 52 Martin Renvoize 2019-11-26 11:37:16 UTC
Moved sponsorship detail to a git follow-up for the release notes (and about page)