Bug 22357 - Every run of runreport.pl with --store-results creates a new row in saved reports
Summary: Every run of runreport.pl with --store-results creates a new row in saved rep...
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Target Milestone: ---
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 17047 17535
Blocks:
  Show dependency treegraph
 
Reported: 2019-02-17 11:26 UTC by Katrin Fischer
Modified: 2019-05-24 17:39 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.05.00, 18.11.06


Attachments
Bug 22357: Fix saved reports table when runreport.pl with --store-results is used (1.71 KB, patch)
2019-04-09 11:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22357: Do not duplicate report rows if several reports exist (1.71 KB, patch)
2019-04-29 01:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22357: Do not duplicate report rows if several reports exist (1.75 KB, patch)
2019-04-29 16:00 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22357: Do not duplicate report rows if several reports exist (1.81 KB, patch)
2019-04-29 20:06 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2019-02-17 11:26:50 UTC
While testing bug 22323 I noticed an issue with the reports table in combination with stored results. Stealing a part of the test plan:

1) Create a new item with itemnotes 'accentué'
2) Create a report and note the report number
...
5) Run from command line (replace X by report number) :
    misc/cronjobs/runreport.pl X --format=csv --csv-header --store-results
...
7) Go to saved reports table
8) Look at the table, each run of the cronjob will create a new row in the table instead of just updating the saved results column.
Comment 1 Katrin Fischer 2019-03-08 07:37:06 UTC
I suspect a problem with the group by somewhere.
Comment 2 Katrin Fischer 2019-04-08 21:40:56 UTC
This gets out of hand pretty fast.
Comment 3 Katrin Fischer 2019-04-09 11:17:50 UTC
Created attachment 87598 [details] [review]
Bug 22357: Fix saved reports table when runreport.pl with --store-results is used

At the moment every time you run runreport.pl with the --store-results
option another line will appear for your report in the saved
reports table. This is not a data, but a display problem as the
report is still only stored once.

1) Create a report and note the report number
2) Run from command line (replace X by report number) :
    misc/cronjobs/runreport.pl X --format=csv --csv-header --store-results
3) Go to saved reports table
4) Look at the table, each run of the cronjob will create a new row
   in the table instead of just updating the saved results column.
5) Apply patch
6) Veriy the table displays correctly again and there are no regressions
Comment 4 Katrin Fischer 2019-04-09 11:18:47 UTC
I think I found a fix and couldn't find any regressions. But I have to admit I am not sure why the line removed from the SQL was there in the first place... please test thoroughly.
Comment 5 Katrin Fischer 2019-04-09 11:27:09 UTC
This patch breaks keyword searching again - as Josef pointed out the line I removed was added by bug 17535.

It looks like we want to search on the stored results with the keyword search, but in that case I am not sure how to fix the display issue.
Comment 6 Liz Rea 2019-04-09 14:59:17 UTC
So am I to understand that we can have many saved results for one report, and that the interface isn't currently supporting that well?
Comment 7 Katrin Fischer 2019-04-09 17:14:46 UTC
Each 'line' in the saved reports table shows all dates in the saved results column. So the entries for one report are identical and complete... there are just too many.

But when we want to search in the saved results, we can't change the SQL used for search... but still don't want to show them duplicated. I am stuck on how to do this.
Comment 8 Jonathan Druart 2019-04-29 01:41:46 UTC
Created attachment 88992 [details] [review]
Bug 22357: Do not duplicate report rows if several reports exist

At the moment every time you run runreport.pl with the --store-results
option another line will appear for your report in the saved
reports table. This is not a data, but a display problem as the
report is still only stored once.

1) Create a report and note the report number
2) Run from command line (replace X by report number) :
    misc/cronjobs/runreport.pl X --format=csv --csv-header --store-results
3) Go to saved reports table
4) Look at the table, each run of the cronjob will create a new row
   in the table instead of just updating the saved results column.
5) Apply patch
6) Veriy the table displays correctly again and there are no regressions

QA: Run t/db_dependent/Reports/Guided.t
Comment 9 Jonathan Druart 2019-04-29 01:46:49 UTC
This could be a fix (yes, ugly). We could also remove the s.* from the fields selection, but this will need to know which one is used (how, without making sure we will not introduce regressions?)

Or we could have 1 query to fetch the reports, then 1 query per report to fetch the results (O(n), bad)
Comment 10 Liz Rea 2019-04-29 16:00:58 UTC
Created attachment 89036 [details] [review]
Bug 22357: Do not duplicate report rows if several reports exist

At the moment every time you run runreport.pl with the --store-results
option another line will appear for your report in the saved
reports table. This is not a data, but a display problem as the
report is still only stored once.

1) Create a report and note the report number
2) Run from command line (replace X by report number) :
    misc/cronjobs/runreport.pl X --format=csv --csv-header --store-results
3) Go to saved reports table
4) Look at the table, each run of the cronjob will create a new row
   in the table instead of just updating the saved results column.
5) Apply patch
6) Veriy the table displays correctly again and there are no regressions

QA: Run t/db_dependent/Reports/Guided.t

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
I didn't assess this for "code ugliness" but the functionality does seem good, and fixes the issue.
Comment 11 Katrin Fischer 2019-04-29 20:06:27 UTC
Created attachment 89055 [details] [review]
Bug 22357: Do not duplicate report rows if several reports exist

At the moment every time you run runreport.pl with the --store-results
option another line will appear for your report in the saved
reports table. This is not a data, but a display problem as the
report is still only stored once.

1) Create a report and note the report number
2) Run from command line (replace X by report number) :
    misc/cronjobs/runreport.pl X --format=csv --csv-header --store-results
3) Go to saved reports table
4) Look at the table, each run of the cronjob will create a new row
   in the table instead of just updating the saved results column.
5) Apply patch
6) Veriy the table displays correctly again and there are no regressions

QA: Run t/db_dependent/Reports/Guided.t

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Katrin Fischer 2019-04-29 20:07:48 UTC
Works for me, no regressions found (keyword search still works too!). Thx for the fix!
Comment 13 Nick Clemens 2019-04-30 13:42:16 UTC
Awesome work all!

Pushed to master for 19.05
Comment 14 Nick Clemens 2019-04-30 13:43:30 UTC
(In reply to Jonathan Druart from comment #9)
> Or we could have 1 query to fetch the reports, then 1 query per report to
> fetch the results (O(n), bad)

I think this is not such a bad idea, but we would move the listing of saved reports to the specific report page (instead of listing them for all the reports on the main page) If you are saving a report regularly it will be taking a lot of screen eventually
Comment 15 Martin Renvoize 2019-05-08 14:10:31 UTC
Pushed to 18.11.x for 18.11.06
Comment 16 Martin Renvoize 2019-05-08 18:58:04 UTC
Depends on bug 17047 only in so much as it adds the mana_id field to the group by clause.  RMaint note: I removed that field to get my tests to pass again as mana is not in the 18.11.x series.
Comment 17 Lucas Gass 2019-05-24 17:39:46 UTC
missing dependencies, wont backport to 18.05.x