Attachment 58955 Details for Bug 17898 – Bug 17898: Automagically convert SQL reports

[patch] Bug 17898: Automagically convert SQL reports

Bug-17898-Automagically-convert-SQL-reports.patch (text/plain), 14.50 KB, created by Jonathan Druart on 2017-01-13 14:58:56 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jonathan Druart

Created: 2017-01-13 14:58:56 UTC

Size: 14.50 KB

patch

obsolete

>From 8701f1a794289ec9c405aae1c9a48b4db4094a09 Mon Sep 17 00:00:00 2001
>From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
>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.
>---
> C4/Reports/Guided.pm                               | 21 +++++++-
> .../prog/en/modules/reports/convert_report.tt      | 19 +++++++
> .../en/modules/reports/guided_reports_start.tt     | 45 ++++++++++++++++-
> reports/guided_reports.pl                          | 30 +++++++++++
> svc/convert_report                                 | 53 ++++++++++++++++++++
> t/db_dependent/Reports/Guided.t                    | 58 +++++++++++++++++++++-
> 6 files changed, 223 insertions(+), 3 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 901edc4..a2b3a8d 100644
>--- a/C4/Reports/Guided.pm
>+++ b/C4/Reports/Guided.pm
>@@ -849,7 +849,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
>@@ -983,6 +982,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 0000000..de77876
>--- /dev/null
>+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reports/convert_report.tt
>@@ -0,0 +1,19 @@
>+[% INCLUDE 'doc-head-open.inc' %]
>+<title>Koha &rsaquo; Reports &rsaquo; Convert report</title>
>+    <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
>+    </head>
>+    <body id="report_convert" class="catalog">
>+        <div id="main" style="direction: ltr;">
>+            [% IF msg == 'no_report' %]
>+                There is no valid report for this id.
>+            [% ELSIF msg == 'can_be_updated' %]
>+                <h3>Existing sql</h3>
>+                <pre>[% current_sql %]</pre>
>+                <h3>Updated sql</h3>
>+                <pre>[% updated_sql %]</pre>
>+            [% ELSE %]
>+                Something went wrong.
>+            [% END %]
>+        </div>
>+    </body>
>+</html>
>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 71fed7f..47d9809 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
>@@ -95,7 +95,7 @@ $("#delColumn").on("click",function(){
>             { "aTargets": [ 1, 2 ], "sType": "natural"  }
>         ],
>         "aoColumns": [
>-            null,null,null,null,null,null,null,null,{ "sType": "title-string" },null,[% IF (usecache) %]null,[% END %]null,null
>+            null,null,null,null,null,null,null,null,{ "sType": "title-string" },null,[% IF (usecache) %]null,[% END %]null,null[% IF has_obsolete_reports %],null[% END %]
>         ],
>         'oLanguage': {
>             'sZeroRecords': _("No matching reports found")
>@@ -164,6 +164,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("<div id=\"loading\"><img src=\"[% interface %]/[% theme %]/img/spinner-small.gif\" alt=\"\" /> "+_("Loading")+"</div>");
>+    });
> [% END %]
> 
> [% IF ( showsql ) %]
>@@ -259,6 +273,20 @@ $("#delColumn").on("click",function(){
> [% END %]
> </div>
> 
>+<div id="update_sql" class="modal hide" tabindex="-1" role="dialog" aria-labelledby="update_sql_label" aria-hidden="true">
>+    <div class="modal-header">
>+        <button type="button" class="closebtn" data-dismiss="modal" aria-hidden="true">Ã</button>
>+        <h3 id="update_sql_label">Update SQL</h3>
>+    </div>
>+    <div class="modal-body">
>+        <div id="loading"> <img src="[% interface %]/[% theme %]/img/spinner-small.gif" alt="" /> Loading </div>
>+    </div>
>+    <div class="modal-footer">
>+        <a href="#" class="btn" id="update_sql_button" role="button" data-toggle="modal">Update</a>
>+        <button class="btn" data-dismiss="modal" aria-hidden="true">Close</button>
>+    </div>
>+</div>
>+
> <div id="doc3" class="yui-t1">
> <div id="bd">
> <div id="yui-main">
>@@ -298,6 +326,12 @@ canned reports and writing custom SQL reports.</p>
> </form>
> [% END %]
> 
>+[% IF report_converted %]
>+    <div class="dialog message">
>+        The report "[% report_converted %]" has been converted.
>+    </div>
>+[% END %]
>+
> [% IF ( saved1 ) %]
> [% IF ( savedreports ) %]<h1>Saved reports</h1>
> 
>@@ -332,6 +366,7 @@ canned reports and writing custom SQL reports.</p>
>                     <th>Public</th>
>                     [% IF (usecache) %] <th>Cache expiry (seconds)</th> [% END %]
>                     <th>Saved results</th>
>+                    [% IF has_obsolete_reports %]<th>Update</th>[% END %]
>                     <th>&nbsp;</th>
>                 </tr>
>             </thead>
>@@ -369,6 +404,14 @@ canned reports and writing custom SQL reports.</p>
>                                 <br/>
>                             [% END %]
>                         </td>
>+                        [% IF has_obsolete_reports %]
>+                        <td>
>+                            [% IF savedreport.seems_obsolete %]
>+                                This report seems obsolete, it uses biblioitems.marcxml field.
>+                                <a href="/cgi-bin/koha/svc/convert_report?report_id=[% savedreport.id %]" data-report_id="[% savedreport.id %]" class="update_sql btn btn-mini" title="Update SQL"><i class="fa fa-eye"></i> Update SQL</a>
>+                            [% END %]
>+                        </td>
>+                        [% END %]
>                         <td>
>                             <div class="dropup">
>                                 <div class="btn-group">
>diff --git a/reports/guided_reports.pl b/reports/guided_reports.pl
>index a5e15be..2ac49e3 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 0000000..659de68
>--- /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 <http://www.gnu.org/licenses>.
>+
>+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 c11d6c9..7c72af8 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 => 7;
>+use Test::More tests => 8;
> use Test::Warn;
> 
> use t::lib::TestBuilder;
>@@ -260,6 +260,62 @@ subtest 'get_saved_reports' => sub {
>         "get_report_areas returns the correct array of report areas");
> };
> 
>+
>+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.1.4

Actions: View | Diff | Splinter Review

Attachments on bug 17898: 58955 | 58961 | 63582 | 63583 | 63613 | 63614