Bug 14435

Summary: Recover feature to store and access results of a report
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: ReportsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: f.demians, gwilliams, indradg, josef.moravec, katrin.fischer, kyle, m.de.rooy, mtj
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13618
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10627
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 17350, 17535    
Attachments: Bug 14435: Drop table saved_reports
Bug 14435: Drop table saved_reports - DB Changes
Bug 14435: Drop table saved_reports
Bug 14435: Drop table saved_reports - DB Changes
Bug 14435: Drop table saved_reports
Bug 14435: Make the tests pass
Bug 14435: Remove the SavedReport DBIx::Class schema file
Bug 14435: Add the ability to store result's report
Bug 14435: Add the ability to store result's report
Bug 14435: Add the ability to store result's report
Bug 14435: Add the ability to store result's report
[PASSED QA] Bug 14435: Add the ability to store result's report

Description Jonathan Druart 2015-06-22 15:58:05 UTC
...and can be removed.
Comment 1 Jonathan Druart 2015-06-22 16:05:16 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2015-06-22 16:05:33 UTC Comment hidden (obsolete)
Comment 3 Mark Tompsett 2015-06-25 21:27:20 UTC
Comment on attachment 40488 [details] [review]
Bug 14435: Drop table saved_reports

Review of attachment 40488 [details] [review]:
-----------------------------------------------------------------

My quick eyeball only noticed this. Not tested yet.

::: koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt
@@ +345,1 @@
>                          </td>

Unmatched </td>, I believe.
Comment 4 Mark Tompsett 2015-06-25 21:53:35 UTC
Comment on attachment 40489 [details] [review]
Bug 14435: Drop table saved_reports - DB Changes

Review of attachment 40489 [details] [review]:
-----------------------------------------------------------------

The drop table saved reports atomic update is empty.
Comment 5 Jonathan Druart 2015-07-02 08:42:50 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2015-07-02 08:44:20 UTC
(In reply to M. Tompsett from comment #3)
> Comment on attachment 40488 [details] [review] [review]
> Bug 14435: Drop table saved_reports
> 
> Review of attachment 40488 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> My quick eyeball only noticed this. Not tested yet.
> 
> ::: koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt
> @@ +345,1 @@
> >                          </td>
> 
> Unmatched </td>, I believe.

Good catch! Fixed in the last patch.

(In reply to M. Tompsett from comment #4)
> Comment on attachment 40489 [details] [review] [review]
> Bug 14435: Drop table saved_reports - DB Changes
> 
> Review of attachment 40489 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> The drop table saved reports atomic update is empty.

What do you mean? The file is empty?

--- /dev/null
+++ b/installer/data/mysql/atomicupdate/bug_14435_drop_table_saved_reports.sql
@@ -0,0 +1 @@
+DROP TABLE IF EXISTS saved_reports;

It only contains 1 line but it's not empty :)
Comment 7 Indranil Das Gupta 2015-07-05 21:41:44 UTC Comment hidden (obsolete)
Comment 8 Indranil Das Gupta 2015-07-05 21:42:54 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2015-07-10 15:00:14 UTC
Unit test fails:

prove t/db_dependent/Reports_Guided.t
t/db_dependent/Reports_Guided.t .. 1/18 DBD::mysql::db selectall_arrayref failed: Unknown column 'report' in 'where clause' at C4/Reports/Guided.pm line 664.
DBD::mysql::db selectall_arrayref failed: Unknown column 'report' in 'where clause' at C4/Reports/Guided.pm line 664.
# Looks like you planned 18 tests but ran 7.
# Looks like your test exited with 255 just after 7.
t/db_dependent/Reports_Guided.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 11/18 subtests

Test Summary Report
-------------------
t/db_dependent/Reports_Guided.t (Wstat: 65280 Tests: 7 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 18 tests but ran 7.
Files=1, Tests=7,  0 wallclock secs ( 0.02 usr  0.00 sys +  0.26 cusr  0.00 csys =  0.28 CPU)
Result: FAIL

Also, can you delete Koha/Schema/Result/SavedReport.pm?
Comment 10 Jonathan Druart 2015-07-10 15:19:24 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2015-07-10 15:21:01 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2015-07-10 15:21:23 UTC
(In reply to Kyle M Hall from comment #9)

Good catch, thanks!
Comment 13 Katrin Fischer 2015-09-01 23:58:22 UTC
What I am missing here a bit is an idea about what this code was actually used for in the past. 

According to my research it seems like you used to be able to store the results of a report in that table - which got broken at some point. We have a newer bug report asking for such a feature:
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10627
Comment 14 Jonathan Druart 2015-09-02 10:57:26 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2015-09-02 10:58:21 UTC
I propose to reintroduce the broken feature.
Have a look and test this last patch and tell me what you think about it, to know if it's useful to continue in this direction.
Comment 16 Frédéric Demians 2015-11-11 14:28:21 UTC
It looks good. Minor merge conflict on master.
Comment 17 Jonathan Druart 2015-11-11 14:51:15 UTC Comment hidden (obsolete)
Comment 18 Frédéric Demians 2015-11-11 15:47:06 UTC
Thnaks for the rebase. It works as described. 

A new column in the 'Saved report' may be one too many. We can imagine
scenario where librarian will run a report once a day. The 'Saved result' will
be overcrowded. A simple tick in a column may be enough with detailed
information reported to another page, or the 'run' page.

There is a bug is the result page after you run any report. After the table
containing the result, there is a section called 'Saved report results'
displaying a table populated with "HASH(....)".
Comment 19 Jonathan Druart 2015-11-12 10:25:51 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2015-11-12 10:28:37 UTC
(In reply to Frédéric Demians from comment #18)
> Thnaks for the rebase. It works as described. 
> 
> A new column in the 'Saved report' may be one too many. We can imagine
> scenario where librarian will run a report once a day. The 'Saved result'
> will
> be overcrowded. A simple tick in a column may be enough with detailed
> information reported to another page, or the 'run' page.

Yes agreed, I have just tried to rescue this feature as quickly as possible.

> There is a bug is the result page after you run any report. After the table
> containing the result, there is a section called 'Saved report results'
> displaying a table populated with "HASH(....)".

The same variable name was used, I have changed one for 'saved_results'. I should be fixed now.
Comment 21 Katrin Fischer 2016-02-29 22:55:52 UTC
Hi Josef, did you intend to mark this patch 'signed off'?
Comment 22 Josef Moravec 2016-03-01 06:34:16 UTC
Sorry, it was unintented, bad bug number :(
Comment 23 Katrin Fischer 2016-03-01 06:37:54 UTC
Can't count how often I mistype bug numbers... :)
Comment 24 Chris Cormack 2016-06-03 11:03:11 UTC
Created attachment 51987 [details] [review]
Bug 14435: Add the ability to store result's report

At one time it was possible to store the results of a report into the
saved_reports table.
This allowed the librarians to compare different results, from the Koha
interface.

This patch is a proof of concept and is not very polished (understood:
it cannot be pushed like that).

Test plan:
Execute the runreport.pl cronjob script with the new --store-results
option.
This will serialize into json the results and put it into the
saved_reports table.

On the "Saved report" list, the "Saved results" column is now populated
with a date (note that you can have several date for a given report).
If you click on this link, the data will be displayed in a simple table.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 25 Marcel de Rooy 2016-06-24 11:30:07 UTC
Commit message still says:

This patch is a proof of concept and is not very polished (understood:
it cannot be pushed like that).
Comment 26 Jonathan Druart 2016-06-24 13:46:37 UTC
(In reply to Marcel de Rooy from comment #25)
> Commit message still says:
> 
> This patch is a proof of concept and is not very polished (understood:
> it cannot be pushed like that).

I do not plan to work more on this one, unless several people are interested.
But I think it's a good start for someone who wants to continue...
Comment 27 Marcel de Rooy 2016-06-27 06:40:25 UTC
(In reply to Jonathan Druart from comment #26)
> (In reply to Marcel de Rooy from comment #25)
> > Commit message still says:
> > 
> > This patch is a proof of concept and is not very polished (understood:
> > it cannot be pushed like that).
> 
> I do not plan to work more on this one, unless several people are interested.
> But I think it's a good start for someone who wants to continue...

If this cannot be pushed in this form, could you adjust the status accordingly?
Comment 28 Jonathan Druart 2016-06-27 12:12:27 UTC
(In reply to Marcel de Rooy from comment #27)
> If this cannot be pushed in this form, could you adjust the status
> accordingly?

Since it does (should) not introduce regression but reintroduced a broken feature, it may be good to push it and see if someone is interested in it.
What do you think?
Comment 29 Katrin Fischer 2016-08-28 22:45:38 UTC
I got no problem with this not being perfect for the time being, but could we add one little thing? 

I am missing an option to delete the saved results - either from the GUI or maybe when the runreport.pl is run an option to clean the last x saved results... I am a little worried that this table will fill up quickly with no way to get rid of stored results.

Please read "failed qa" as asking for feedback!
Comment 30 Jonathan Druart 2016-08-29 07:50:04 UTC
Hi Katrin,
This patch restore a broken behavior, instead of removing it completely.
If the feature is used and some guys want to improve it, let's them do it :)
I do not plan to improve it by myself, especially because IMO it's out of the scope of this bug report.
Comment 31 Katrin Fischer 2016-09-26 05:20:04 UTC
Created attachment 55814 [details] [review]
[PASSED QA] Bug 14435: Add the ability to store result's report

At one time it was possible to store the results of a report into the
saved_reports table.
This allowed the librarians to compare different results, from the Koha
interface.

This patch is a proof of concept and is not very polished (understood:
it cannot be pushed like that).

Test plan:
Execute the runreport.pl cronjob script with the new --store-results
option.
This will serialize into json the results and put it into the
saved_reports table.

On the "Saved report" list, the "Saved results" column is now populated
with a date (note that you can have several date for a given report).
If you click on this link, the data will be displayed in a simple table.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Katrin Fischer  <katrin.fischer@bsz-bw.de>
Comment 32 Katrin Fischer 2016-09-26 05:20:10 UTC
I've filed bug 17350 for dealing with the deletion of data.
Comment 33 Kyle M Hall 2016-09-27 12:48:46 UTC
Pushed to master for 16.11, thanks Jonathan!
Comment 34 Mason James 2016-12-14 10:08:36 UTC
Enhancement, skipping for 16.05.x