...and can be removed.
Created attachment 40488 [details] [review] Bug 14435: Drop table saved_reports The table saved_reports is never populated and is not used since 14be4400d84b28369d095b3b0bfa79c3396f44d4 (2009). The 2 subroutines store_results and format_results are never reachable and will also be removed. Note that an empty column "Saved results" were always displayed on the saved report list. Test plan: 0/ Confirm that there is nothing in the saved_reports table and that you have never seen something in it before. 1/ After applying the patch, confirm that you have still acess to the saved report list (reports/guided_reports.pl?phase=Use saved)
Created attachment 40489 [details] [review] Bug 14435: Drop table saved_reports - DB Changes
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 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.
Created attachment 40753 [details] [review] Bug 14435: Drop table saved_reports The table saved_reports is never populated and is not used since 14be4400d84b28369d095b3b0bfa79c3396f44d4 (2009). The 2 subroutines store_results and format_results are never reachable and will also be removed. Note that an empty column "Saved results" were always displayed on the saved report list. Test plan: 0/ Confirm that there is nothing in the saved_reports table and that you have never seen something in it before. 1/ After applying the patch, confirm that you have still acess to the saved report list (reports/guided_reports.pl?phase=Use saved)
(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 :)
Created attachment 40796 [details] [review] Bug 14435: Drop table saved_reports - DB Changes Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com>
Created attachment 40797 [details] [review] Bug 14435: Drop table saved_reports The table saved_reports is never populated and is not used since 14be4400d84b28369d095b3b0bfa79c3396f44d4 (2009). The 2 subroutines store_results and format_results are never reachable and will also be removed. Note that an empty column "Saved results" were always displayed on the saved report list. Test plan: 0/ Confirm that there is nothing in the saved_reports table and that you have never seen something in it before. 1/ After applying the patch, confirm that you have still acess to the saved report list (reports/guided_reports.pl?phase=Use saved) Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com>
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?
Created attachment 40929 [details] [review] Bug 14435: Make the tests pass
Created attachment 40930 [details] [review] Bug 14435: Remove the SavedReport DBIx::Class schema file
(In reply to Kyle M Hall from comment #9) Good catch, thanks!
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
Created attachment 42196 [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.
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.
It looks good. Minor merge conflict on master.
Created attachment 44767 [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.
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(....)".
Created attachment 44795 [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.
(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.
Hi Josef, did you intend to mark this patch 'signed off'?
Sorry, it was unintented, bad bug number :(
Can't count how often I mistype bug numbers... :)
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>
Commit message still says: This patch is a proof of concept and is not very polished (understood: it cannot be pushed like that).
(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...
(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?
(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?
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!
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.
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>
I've filed bug 17350 for dealing with the deletion of data.
Pushed to master for 16.11, thanks Jonathan!
Enhancement, skipping for 16.05.x