From f22d2e7200ee8b35aff66b26acc3e2daa49f9f0d Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Fri, 13 Jan 2017 15:48:57 +0100 Subject: [PATCH] Bug 17898: Automagically convert SQL reports Bug 17196 move the marcxml out of the biblioitems table. That will break SQL reports using it. It would be handy to propose an automagically way to convert the SQL reports. We do not want to update the reports automatically without user inputs, it will be too hasardous. However we can lead the user to convert them. In this patchset I suggest to warn the user if a report is subject to be updated. TODO: Add a way to mark this job done (using a pref?) to remove the check and not to display false positives. Test plan: - Create some SQL reports (see https://wiki.koha-community.org/wiki/SQL_Reports_Library) - Go on the report list page (/reports/guided_reports.pl?phase=Use saved) - For the reports using biblioitems.marcxml you will see a new column warning you that it is obsolete - Click on update link => that will open a modal with the converted SQL query - Click on the update button => you will be informed that the query has been updated If all the reports are updated, the new column "Update" will no longer be displayed. Signed-off-by: Tomas Cohen Arazi Signed-off-by: Kyle M Hall --- C4/Reports/Guided.pm | 21 ++++++- .../prog/en/modules/reports/convert_report.tt | 19 +++++++ .../modules/reports/guided_reports_start.tt | 44 ++++++++++++++ reports/guided_reports.pl | 30 ++++++++++ svc/convert_report | 53 +++++++++++++++++ t/db_dependent/Reports/Guided.t | 57 ++++++++++++++++++- 6 files changed, 222 insertions(+), 2 deletions(-) create mode 100644 koha-tmpl/intranet-tmpl/prog/en/modules/reports/convert_report.tt create mode 100755 svc/convert_report diff --git a/C4/Reports/Guided.pm b/C4/Reports/Guided.pm index ac5633a121..717d44f1fd 100644 --- a/C4/Reports/Guided.pm +++ b/C4/Reports/Guided.pm @@ -854,7 +854,6 @@ sub get_sql { sub get_results { my ( $report_id ) = @_; my $dbh = C4::Context->dbh; - warn $report_id; return $dbh->selectall_arrayref(q| SELECT id, report, date_run FROM saved_reports @@ -988,6 +987,26 @@ sub _get_display_value { return $original_value; } + +=head3 convert_sql + +my $updated_sql = C4::Reports::Guided::convert_sql( $sql ); + +Convert a sql query using biblioitems.marcxml to use the new +biblio_metadata.metadata field instead + +=cut + +sub convert_sql { + my ( $sql ) = @_; + my $updated_sql = $sql; + if ( $sql =~ m|biblioitems| and $sql =~ m|marcxml| ) { + $updated_sql =~ s|biblioitems|biblio_metadata|; + $updated_sql =~ s|marcxml|metadata|; + } + return $updated_sql; +} + 1; __END__ diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/reports/convert_report.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/reports/convert_report.tt new file mode 100644 index 0000000000..de77876850 --- /dev/null +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reports/convert_report.tt @@ -0,0 +1,19 @@ +[% INCLUDE 'doc-head-open.inc' %] +Koha › Reports › Convert report + + + +
+ [% IF msg == 'no_report' %] + There is no valid report for this id. + [% ELSIF msg == 'can_be_updated' %] +

Existing sql

+
[% current_sql %]
+

Updated sql

+
[% updated_sql %]
+ [% ELSE %] + Something went wrong. + [% END %] +
+ + diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt index 8d86efd9ba..6d854dd085 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tt @@ -98,6 +98,7 @@ $("#delColumn").on("click",function(){ ], "aoColumns": [ null,null,null,null,null,null,null,null,{ "sType": "title-string" },null,null,null,[% IF (usecache) %]null,[% END %]null,null + null,null,null,null,null,null,null,null,{ "sType": "title-string" },null,null,null,[% IF (usecache) %]null,[% END %]null,null[% IF has_obsolete_reports %],null[% END %] ], 'oLanguage': { 'sZeroRecords': _("No matching reports found") @@ -166,6 +167,20 @@ $("#delColumn").on("click",function(){ return false; } }); + + $("body").on("click", ".update_sql", function(e){ + e.preventDefault(); + var ltitle = $(this).text(); + var report_id = $(this).data("report_id"); + var page = $(this).attr("href"); + $("#update_sql .modal-body").load(page + " div"); + $('#update_sql').modal({show:true}); + $("#update_sql_button").attr("href", "/cgi-bin/koha/reports/guided_reports.pl?phase=Use saved&op=convert&report_id=" + report_id); + }); + $("#update_sql").on("hidden", function(){ + $("#update_sql_label").html(""); + $("#update_sql .modal-body").html("
\"\" "+_("Loading")+"
"); + }); [% END %] [% IF ( showsql ) %] @@ -267,6 +282,20 @@ $("#delColumn").on("click",function(){ [% END %] + +
@@ -306,6 +335,12 @@ canned reports and writing custom SQL reports.

[% END %] +[% IF report_converted %] +
+ The report "[% report_converted %]" has been converted. +
+[% END %] + [% IF ( saved1 ) %] [% IF ( savedreports ) %]

Saved reports

@@ -342,6 +377,7 @@ canned reports and writing custom SQL reports.

Public [% IF (usecache) %] Cache expiry (seconds) [% END %] Saved results + [% IF has_obsolete_reports %]Update[% END %]   @@ -381,6 +417,14 @@ canned reports and writing custom SQL reports.


[% END %] + [% IF has_obsolete_reports %] + + [% IF savedreport.seems_obsolete %] + This report seems obsolete, it uses biblioitems.marcxml field. + Update SQL + [% END %] + + [% END %]
diff --git a/reports/guided_reports.pl b/reports/guided_reports.pl index 3e69e16f3b..edff7d03bd 100755 --- a/reports/guided_reports.pl +++ b/reports/guided_reports.pl @@ -89,6 +89,7 @@ elsif ($session and not $input->param('clear_filters')) { $filter = $session->param('report_filter'); } +my $op = $input->param('op') || q||; my @errors = (); if ( !$phase ) { @@ -106,6 +107,29 @@ elsif ( $phase eq 'Build new' ) { ); } elsif ( $phase eq 'Use saved' ) { + if ( $op eq 'convert' ) { + my $report_id = $input->param('report_id'); + my $report = C4::Reports::Guided::get_saved_report($report_id); + warn $report_id; + use Data::Printer colored => 1; warn p $report; + if ($report) { + my $updated_sql = C4::Reports::Guided::convert_sql( $report->{savedsql} ); + C4::Reports::Guided::update_sql( + $report_id, + { + sql => $updated_sql, + name => $report->{report_name}, + group => $report->{report_group}, + subgroup => $report->{report_subgroup}, + notes => $report->{notes}, + public => $report->{public}, + cache_expiry => $report->{cache_expiry}, + } + ); + $template->param( report_converted => $report->{report_name} ); + } + } + # use a saved report # get list of reports and display them my $group = $input->param('group'); @@ -113,8 +137,13 @@ elsif ( $phase eq 'Build new' ) { $filter->{group} = $group; $filter->{subgroup} = $subgroup; my $reports = get_saved_reports($filter); + my $has_obsolete_reports; for my $report ( @$reports ) { $report->{results} = C4::Reports::Guided::get_results( $report->{id} ); + if ( $report->{savedsql} =~ m|marcxml| ) { + $report->{seems_obsolete} = 1; + $has_obsolete_reports++; + } } $template->param( 'saved1' => 1, @@ -122,6 +151,7 @@ elsif ( $phase eq 'Build new' ) { 'usecache' => $usecache, 'groups_with_subgroups'=> groups_with_subgroups($group, $subgroup), filters => $filter, + has_obsolete_reports => $has_obsolete_reports, ); } diff --git a/svc/convert_report b/svc/convert_report new file mode 100755 index 0000000000..659de68547 --- /dev/null +++ b/svc/convert_report @@ -0,0 +1,53 @@ +#!/usr/bin/perl + +# This file is part of Koha. +# +# Copyright 2017 Koha Development Team +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; + +use C4::Auth; +use C4::Reports::Guided; +use C4::Output; +use CGI qw ( -utf8 ); + +my $query = CGI->new(); +my $report_id = $query->param('report_id'); + +my ( $template, $loggedinuser, $cookie ) = get_template_and_user( + { + template_name => "reports/convert_report.tt", + query => $query, + type => "intranet", + authnotrequired => 0, + flagsrequired => { reports => 'execute_reports' }, + } +); + +my $report = get_saved_report( $report_id ); + +my $params; +if ( $report ) { + my $sql = $report->{savedsql}; + my $updated_sql = C4::Reports::Guided::convert_sql( $sql ); + $params = { msg => 'can_be_updated', updated_sql => $updated_sql, current_sql => $sql }; +} else { + $params = { msg => 'no_report' }; +} + +$template->param( %$params ); + +output_html_with_http_headers $query, $cookie, $template->output; diff --git a/t/db_dependent/Reports/Guided.t b/t/db_dependent/Reports/Guided.t index 78058db2c8..0c410dcf49 100644 --- a/t/db_dependent/Reports/Guided.t +++ b/t/db_dependent/Reports/Guided.t @@ -18,7 +18,7 @@ use Modern::Perl; -use Test::More tests => 8; +use Test::More tests => 9; use Test::Warn; use t::lib::TestBuilder; @@ -288,6 +288,61 @@ subtest 'Ensure last_run is populated' => sub { isnt( $report->last_run, $previous_last_run, 'Second run of report updates last_run' ); }; +subtest 'convert_sql' => sub { + plan tests => 3; + + my $sql = q| + SELECT biblionumber, ExtractValue(marcxml, +'count(//datafield[@tag="505"])') AS count505 + FROM biblioitems + HAVING count505 > 1|; + my $expected_converted_sql = q| + SELECT biblionumber, ExtractValue(metadata, +'count(//datafield[@tag="505"])') AS count505 + FROM biblio_metadata + HAVING count505 > 1|; + + is( C4::Reports::Guided::convert_sql( $sql ), $expected_converted_sql, "Simple query should have been correctly converted"); + + $sql = q| + SELECT biblionumber, substring( +ExtractValue(marcxml,'//controlfield[@tag="008"]'), 8,4 ) AS 'PUB DATE', +title + FROM biblioitems + INNER JOIN biblio USING (biblionumber) + WHERE biblionumber = 14|; + + $expected_converted_sql = q| + SELECT biblionumber, substring( +ExtractValue(metadata,'//controlfield[@tag="008"]'), 8,4 ) AS 'PUB DATE', +title + FROM biblio_metadata + INNER JOIN biblio USING (biblionumber) + WHERE biblionumber = 14|; + is( C4::Reports::Guided::convert_sql( $sql ), $expected_converted_sql, "Query with biblio info should have been correctly converted"); + + $sql = q| + SELECT concat(b.title, ' ', ExtractValue(m.marcxml, +'//datafield[@tag="245"]/subfield[@code="b"]')) AS title, b.author, +count(h.reservedate) AS 'holds' + FROM biblio b + LEFT JOIN biblioitems m USING (biblionumber) + LEFT JOIN reserves h ON (b.biblionumber=h.biblionumber) + GROUP BY b.biblionumber + HAVING count(h.reservedate) >= 42|; + + $expected_converted_sql = q| + SELECT concat(b.title, ' ', ExtractValue(m.metadata, +'//datafield[@tag="245"]/subfield[@code="b"]')) AS title, b.author, +count(h.reservedate) AS 'holds' + FROM biblio b + LEFT JOIN biblio_metadata m USING (biblionumber) + LEFT JOIN reserves h ON (b.biblionumber=h.biblionumber) + GROUP BY b.biblionumber + HAVING count(h.reservedate) >= 42|; + is( C4::Reports::Guided::convert_sql( $sql ), $expected_converted_sql, "Query with 2 joins should have been correctly converted"); +}; + $schema->storage->txn_rollback; sub trim { -- 2.39.5