From 9d1382f244fecf88b192938a2b009c4f16ec224c Mon Sep 17 00:00:00 2001
From: Srdjan <srdjan@catalyst.net.nz>
Date: Mon, 10 Sep 2012 13:14:01 +1200
Subject: [PATCH] Bug 7993: Save reports with Group/Subgroup hierarchy

This should make saved reports more manageable.
Group/Subgroup hierarchy is stored in authorised_values,
categories REPORT_GROUP and REPORT_SUBGROUP, connected by
REPORT_SUBGROUP.lib_opac -> REPORT_GROUP.authorised_value

Database changes:
* authorised_values: expanded category to 16 chars
* created default set of REPORT_GROUP authorised values to match
  hardcoded report areas
* reports_dictionary: replaced area int with report_area text, converted
  values
* saved_sql: added report_area, report_group and report_subgroup;
  report_area is not currently used, saved for the record

C4/Reports/Guided.pm:
* Replaced Area numeric values with the mnemonic codes
* get_report_areas(): returns hardcoded areas list
* created get_report_areas(): returns full hierarchy (groups with belonging
  subgroups)
* save_report(): changed iterface, accepts fields hashref as input
* update_sql(): changed iterface, accepts id and fields hashref as input
* get_saved_reports():]
- join to authorised_values to pick group and subgroup name
- accept group and subgroup filter params
* get_saved_report():
- changed iterface, return record hashref
- join to authorised_values to pick group and subgroup name
* build_authorised_value_list(): new sub, moved code from
  reports/guided_reports.pl
* Updated interfaces in:
cronjobs/runreport.pl, svc/report, opac/svc/report: get_saved_report()
reports/dictionary.pl: get_report_areas()
reports/guided_reports.pl

reports/guided_reports_start.tt:
* Reports list:
- added group/subgroup filter
- display area/group/subgroup for the reports
* Create report wizard:
- carry area to the end
- select group and subgroup when saving the report; group defaults to area,
  useful when report groups match areas
* Update report and Create from SQL: added group/subgroup
* Amended reports/guided_reports.pl accordingly

Conflicts:

    C4/Reports/Guided.pm
    admin/authorised_values.pl
    installer/data/mysql/kohastructure.sql
    installer/data/mysql/updatedatabase.pl
    koha-tmpl/intranet-tmpl/prog/en/modules/reports/dictionary.tmpl
    koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tmpl
    misc/cronjobs/runreport.pl
    reports/dictionary.pl
    reports/guided_reports.pl
---
 C4/Reports/Guided.pm                               |  432 +++++++++++-------
 admin/authorised_values.pl                         |   23 +-
 installer/data/mysql/kohastructure.sql             |   11 +-
 installer/data/mysql/updatedatabase.pl             |   32 ++
 .../prog/en/modules/reports/dictionary.tt          |    4 +-
 .../en/modules/reports/guided_reports_start.tt     |  144 +++++-
 misc/cronjobs/runreport.pl                         |   22 +-
 opac/svc/report                                    |   48 +-
 reports/dictionary.pl                              |  292 ++++++------
 reports/guided_reports.pl                          |  482 +++++++++++---------
 svc/report                                         |   55 +--
 11 files changed, 949 insertions(+), 596 deletions(-)

diff --git a/C4/Reports/Guided.pm b/C4/Reports/Guided.pm
index e5c28f3..377a55a 100644
--- a/C4/Reports/Guided.pm
+++ b/C4/Reports/Guided.pm
@@ -26,7 +26,8 @@ use vars qw($VERSION @ISA @EXPORT @EXPORT_OK %EXPORT_TAGS);
 use C4::Context;
 use C4::Dates qw/format_date format_date_in_iso/;
 use C4::Templates qw/themelanguage/;
-use C4::Dates;
+use C4::Koha;
+use C4::Output;
 use XML::Simple;
 use XML::Dumper;
 use C4::Debug;
@@ -34,75 +35,82 @@ use C4::Debug;
 # use Data::Dumper;
 
 BEGIN {
-	# set the version for version checking
+    # set the version for version checking
     $VERSION = 3.07.00.049;
-	require Exporter;
-	@ISA = qw(Exporter);
-	@EXPORT = qw(
-		get_report_types get_report_areas get_columns build_query get_criteria
-	    save_report get_saved_reports execute_query get_saved_report create_compound run_compound
-		get_column_type get_distinct_values save_dictionary get_from_dictionary
-		delete_definition delete_report format_results get_sql
-        nb_rows update_sql
-	);
+    require Exporter;
+    @ISA    = qw(Exporter);
+    @EXPORT = qw(
+      get_report_types get_report_areas get_report_groups get_columns build_query get_criteria
+      save_report get_saved_reports execute_query get_saved_report create_compound run_compound
+      get_column_type get_distinct_values save_dictionary get_from_dictionary
+      delete_definition delete_report format_results get_sql
+      nb_rows update_sql build_authorised_value_list
+    );
 }
 
-our %table_areas;
-$table_areas{'1'} =
-  [ 'borrowers', 'statistics','items', 'biblioitems' ];    # circulation
-$table_areas{'2'} = [ 'items', 'biblioitems', 'biblio' ];   # catalogue
-$table_areas{'3'} = [ 'borrowers' ];        # patrons
-$table_areas{'4'} = ['aqorders', 'biblio', 'items'];        # acquisitions
-$table_areas{'5'} = [ 'borrowers', 'accountlines' ];        # accounts
-our %keys;
-$keys{'1'} = [
-    'statistics.borrowernumber=borrowers.borrowernumber',
-    'items.itemnumber = statistics.itemnumber',
-    'biblioitems.biblioitemnumber = items.biblioitemnumber'
-];
-$keys{'2'} = [
-    'items.biblioitemnumber=biblioitems.biblioitemnumber',
-    'biblioitems.biblionumber=biblio.biblionumber'
-];
-$keys{'3'} = [ ];
-$keys{'4'} = [
-	'aqorders.biblionumber=biblio.biblionumber',
-	'biblio.biblionumber=items.biblionumber'
-];
-$keys{'5'} = ['borrowers.borrowernumber=accountlines.borrowernumber'];
+=item get_report_areas()
 
-# have to do someting here to know if its dropdown, free text, date etc
+This will return a list of all the available report areas
 
-our %criteria;
-# reports on circulation
-$criteria{'1'} = [
-    'statistics.type',   'borrowers.categorycode',
-    'statistics.branch',
-    'biblioitems.publicationyear|date',
-    'items.dateaccessioned|date'
-];
-# reports on catalogue
-$criteria{'2'} =
-  [ 'items.itemnumber|textrange',   'items.biblionumber|textrange',   'items.barcode|textrange', 
-    'biblio.frameworkcode',         'items.holdingbranch',            'items.homebranch', 
-  'biblio.datecreated|daterange',   'biblio.timestamp|daterange',     'items.onloan|daterange', 
-  'items.ccode',                    'items.itemcallnumber|textrange', 'items.itype', 
-  'items.itemlost',                 'items.location' ];
-# reports on borrowers
-$criteria{'3'} = ['borrowers.branchcode', 'borrowers.categorycode'];
-# reports on acquisition
-$criteria{'4'} = ['aqorders.datereceived|date'];
-
-# reports on accounting
-$criteria{'5'} = ['borrowers.branchcode', 'borrowers.categorycode'];
+=cut
+
+my @REPORT_AREA = (
+    [CIRC => "Circulation"],
+    [CAT  => "Catalogue"],
+    [PAT  => "Patrons"],
+    [ACQ  => "Acquisition"],
+    [ACC  => "Accounts"],
+);
+my $AREA_NAME_SQL_SNIPPET
+  = "CASE report_area " .
+    join (" ", map "WHEN '$_->[0]' THEN '$_->[1]'", @REPORT_AREA) .
+    " END AS areaname";
+sub get_report_areas {
+    return \@REPORT_AREA
+}
+
+my %table_areas = (
+    CIRC => [ 'borrowers', 'statistics', 'items', 'biblioitems' ],
+    CAT  => [ 'items', 'biblioitems', 'biblio' ],
+    PAT  => ['borrowers'],
+    ACQ  => [ 'aqorders', 'biblio', 'items' ],
+    ACC  => [ 'borrowers', 'accountlines' ],
+);
+my %keys = (
+    CIRC => [ 'statistics.borrowernumber=borrowers.borrowernumber',
+              'items.itemnumber = statistics.itemnumber',
+              'biblioitems.biblioitemnumber = items.biblioitemnumber' ],
+    CAT  => [ 'items.biblioitemnumber=biblioitems.biblioitemnumber',
+              'biblioitems.biblionumber=biblio.biblionumber' ],
+    PAT  => [],
+    ACQ  => [ 'aqorders.biblionumber=biblio.biblionumber',
+              'biblio.biblionumber=items.biblionumber' ],
+    ACC  => ['borrowers.borrowernumber=accountlines.borrowernumber'],
+);
+
+# have to do someting here to know if its dropdown, free text, date etc
+my %criteria = (
+    CIRC => [ 'statistics.type', 'borrowers.categorycode', 'statistics.branch',
+              'biblioitems.publicationyear|date', 'items.dateaccessioned|date' ],
+    CAT  => [ 'items.itemnumber|textrange', 'items.biblionumber|textrange',
+              'items.barcode|textrange', 'biblio.frameworkcode',
+              'items.holdingbranch', 'items.homebranch',
+              'biblio.datecreated|daterange', 'biblio.timestamp|daterange',
+              'items.onloan|daterange', 'items.ccode',
+              'items.itemcallnumber|textrange', 'items.itype', 'items.itemlost',
+              'items.location' ],
+    PAT  => [ 'borrowers.branchcode', 'borrowers.categorycode' ],
+    ACQ  => ['aqorders.datereceived|date'],
+    ACC  => [ 'borrowers.branchcode', 'borrowers.categorycode' ],
+);
 
 # Adds itemtypes to criteria, according to the syspref
-if (C4::Context->preference('item-level_itypes')) {
-    unshift @{ $criteria{'1'} }, 'items.itype';
-    unshift @{ $criteria{'2'} }, 'items.itype';
+if ( C4::Context->preference('item-level_itypes') ) {
+    unshift @{ $criteria{'CIRC'} }, 'items.itype';
+    unshift @{ $criteria{'CAT'} }, 'items.itype';
 } else {
-    unshift @{ $criteria{'1'} }, 'biblioitems.itemtype';
-    unshift @{ $criteria{'2'} }, 'biblioitems.itemtype';
+    unshift @{ $criteria{'CIRC'} }, 'biblioitems.itemtype';
+    unshift @{ $criteria{'CAT'} }, 'biblioitems.itemtype';
 }
 
 =head1 NAME
@@ -145,26 +153,33 @@ sub get_report_types {
 
 }
 
-=item get_report_areas()
+=item get_report_groups()
 
-This will return a list of all the available report areas
+This will return a list of all the available report areas with groups
 
 =cut
 
-sub get_report_areas {
+sub get_report_groups {
     my $dbh = C4::Context->dbh();
 
-    # FIXME these should be in the database
-    my @reports = ( 'Circulation', 'Catalog', 'Patrons', 'Acquisitions', 'Accounts');
-    my @reports2;
-    for ( my $i = 0 ; $i < 5 ; $i++ ) {
-        my %hashrep;
-        $hashrep{id}   = $i + 1;
-        $hashrep{name} = $reports[$i];
-        push @reports2, \%hashrep;
+    my $groups = GetAuthorisedValues('REPORT_GROUP');
+    my $subgroups = GetAuthorisedValues('REPORT_SUBGROUP');
+
+    my %groups_with_subgroups = map { $_->{authorised_value} => {
+                        name => $_->{lib},
+                        groups => {}
+                    } } @$groups;
+    foreach (@$subgroups) {
+        my $sg = $_->{authorised_value};
+        my $g = $_->{lib_opac}
+          or warn( qq{REPORT_SUBGROUP "$sg" without REPORT_GROUP (lib_opac)} ),
+             next;
+        my $g_sg = $groups_with_subgroups{$g}
+          or warn( qq{REPORT_SUBGROUP "$sg" with invalid REPORT_GROUP "$g"} ),
+             next;
+        $g_sg->{subgroups}{$sg} = $_->{lib};
     }
-    return ( \@reports2 );
-
+    return \%groups_with_subgroups
 }
 
 =item get_all_tables()
@@ -196,8 +211,10 @@ This will return a list of all columns for a report area
 sub get_columns {
 
     # this calls the internal fucntion _get_columns
-    my ($area,$cgi) = @_;
-    my $tables = $table_areas{$area};
+    my ( $area, $cgi ) = @_;
+    my $tables = $table_areas{$area}
+      or die qq{Unsuported report area "$area"};
+
     my @allcolumns;
     my $first = 1;
     foreach my $table (@$tables) {
@@ -383,7 +400,7 @@ sub nb_rows($) {
 
 =item execute_query
 
-  ($results, $total, $error) = execute_query($sql, $offset, $limit)
+  ($results, $error) = execute_query($sql, $offset, $limit)
 
 
 When passed C<$sql>, this function returns an array ref containing a result set
@@ -505,37 +522,47 @@ Returns id of the newly created report
 =cut
 
 sub save_report {
-    my ( $borrowernumber, $sql, $name, $type, $notes, $cache_expiry, $public ) = @_;
-    $cache_expiry ||= 300;
+    my ($fields) = @_;
+    my $borrowernumber = $fields->{borrowernumber};
+    my $sql = $fields->{sql};
+    my $name = $fields->{name};
+    my $type = $fields->{type};
+    my $notes = $fields->{notes};
+    my $area = $fields->{area};
+    my $group = $fields->{group};
+    my $subgroup = $fields->{subgroup};
+    my $cache_expiry = $fields->{cache_expiry} || 300;
+    my $public = $fields->{public};
+
     my $dbh = C4::Context->dbh();
-    $sql =~ s/(\s*\;\s*)$//; # removes trailing whitespace and /;/
-    my $query =
-"INSERT INTO saved_sql (borrowernumber,date_created,last_modified,savedsql,report_name,type,notes,cache_expiry, public)  VALUES (?,now(),now(),?,?,?,?,?,?)";
-    $dbh->do( $query, undef, $borrowernumber, $sql, $name, $type, $notes, $cache_expiry, $public );
+    $sql =~ s/(\s*\;\s*)$//;    # removes trailing whitespace and /;/
+    my $query = "INSERT INTO saved_sql (borrowernumber,date_created,last_modified,savedsql,report_name,report_area,report_group,report_subgroup,type,notes,cache_expiry,public)  VALUES (?,now(),now(),?,?,?,?,?,?,?,?,?)";
+    $dbh->do($query, undef, $borrowernumber, $sql, $name, $area, $group, $subgroup, $type, $notes, $cache_expiry, $public);
+
     my $id = $dbh->selectrow_array("SELECT max(id) FROM saved_sql WHERE borrowernumber=? AND report_name=?", undef,
                                    $borrowernumber, $name);
     return $id;
 }
 
 sub update_sql {
-    my $id = shift || croak "No Id given";
-    my $sql = shift;
-    my $reportname = shift;
-    my $notes = shift;
-    my $cache_expiry = shift;
-    my $public = shift;
-
-    # not entirely a magic number, Cache::Memcached::Set assumed any expiry >= (60*60*24*30) is an absolute unix timestamp (rather than relative seconds)
+    my $id         = shift || croak "No Id given";
+    my $fields     = shift;
+    my $sql = $fields->{sql};
+    my $name = $fields->{name};
+    my $notes = $fields->{notes};
+    my $group = $fields->{group};
+    my $subgroup = $fields->{subgroup};
+    my $cache_expiry = $fields->{cache_expiry};
+    my $public = $fields->{public};
+
     if( $cache_expiry >= 2592000 ){
       die "Please specify a cache expiry less than 30 days\n";
     }
 
-    my $dbh = C4::Context->dbh();
-    $sql =~ s/(\s*\;\s*)$//; # removes trailing whitespace and /;/
-    my $query = "UPDATE saved_sql SET savedsql = ?, last_modified = now(), report_name = ?, notes = ?, cache_expiry = ?, public = ? WHERE id = ? ";
-    my $sth = $dbh->prepare($query);
-    $sth->execute( $sql, $reportname, $notes, $cache_expiry, $public, $id );
-    $sth->finish();
+    my $dbh        = C4::Context->dbh();
+    $sql =~ s/(\s*\;\s*)$//;    # removes trailing whitespace and /;/
+    my $query = "UPDATE saved_sql SET savedsql = ?, last_modified = now(), report_name = ?, report_group = ?, report_subgroup = ?, notes = ?, cache_expiry = ?, public = ? WHERE id = ? ";
+    $dbh->do($query, undef, $sql, $name, $group, $subgroup, $notes, $cache_expiry, $public, $id );
 }
 
 sub store_results {
@@ -582,30 +609,34 @@ sub format_results {
 }	
 
 sub delete_report {
-	my ( $id ) = @_;
-	my $dbh = C4::Context->dbh();
-	my $query = "DELETE FROM saved_sql WHERE id = ?";
-	my $sth = $dbh->prepare($query);
-	$sth->execute($id);
+    my ($id)  = @_;
+    my $dbh   = C4::Context->dbh();
+    my $query = "DELETE FROM saved_sql WHERE id = ?";
+    my $sth   = $dbh->prepare($query);
+    $sth->execute($id);
 }	
 
-# $filter is either { date => $d, author => $a, keyword => $kw }
-# or $keyword. Optional.
+
+my $SAVED_REPORTS_BASE_QRY = <<EOQ;
+SELECT s.*, r.report, r.date_run, $AREA_NAME_SQL_SNIPPET, av_g.lib AS groupname, av_sg.lib AS subgroupname,
+b.firstname AS borrowerfirstname, b.surname AS borrowersurname
+FROM saved_sql s
+LEFT JOIN saved_reports r ON r.report_id = s.id
+LEFT OUTER JOIN authorised_values av_g ON (av_g.category = 'REPORT_GROUP' AND av_g.authorised_value = s.report_group)
+LEFT OUTER JOIN authorised_values av_sg ON (av_sg.category = 'REPORT_SUBGROUP' AND av_sg.lib_opac = s.report_group AND av_sg.authorised_value = s.report_subgroup)
+LEFT OUTER JOIN borrowers b USING (borrowernumber)
+EOQ
 my $DATE_FORMAT = "%d/%m/%Y";
 sub get_saved_reports {
+# $filter is either { date => $d, author => $a, keyword => $kw, }
+# or $keyword. Optional.
     my ($filter) = @_;
     $filter = { keyword => $filter } if $filter && !ref( $filter );
+    my ($group, $subgroup) = @_;
 
     my $dbh   = C4::Context->dbh();
+    my $query = $SAVED_REPORTS_BASE_QRY;
     my (@cond,@args);
-    my $query = "SELECT saved_sql.id, report_id, report,
-                        date_run, date_created, last_modified, savedsql, last_run,
-                        report_name, type, notes,
-                        borrowernumber, surname as borrowersurname, firstname as borrowerfirstname,
-                        cache_expiry, public
-                 FROM saved_sql 
-                 LEFT JOIN saved_reports ON saved_reports.report_id = saved_sql.id
-                 LEFT OUTER JOIN borrowers USING (borrowernumber)";
     if ($filter) {
         if (my $date = $filter->{date}) {
             $date = format_date_in_iso($date);
@@ -629,6 +660,14 @@ sub get_saved_reports {
                          savedsql LIKE ?";
             push @args, $keyword, $keyword, $keyword, $keyword;
         }
+        if ($filter->{group}) {
+            push @cond, "report_group = ?";
+            push @args, $filter->{group};
+        }
+        if ($filter->{subgroup}) {
+            push @cond, "report_subgroup = ?";
+            push @args, $filter->{subgroup};
+        }
     }
     $query .= " WHERE ".join( " AND ", map "($_)", @cond ) if @cond;
     $query .= " ORDER by date_created";
@@ -642,7 +681,6 @@ sub get_saved_reports {
 sub get_saved_report {
     my $dbh   = C4::Context->dbh();
     my $query;
-    my $sth;
     my $report_arg;
     if ($#_ == 0 && ref $_[0] ne 'HASH') {
         ($report_arg) = @_;
@@ -661,10 +699,7 @@ sub get_saved_report {
     } else {
         return;
     }
-    $sth   = $dbh->prepare($query);
-    $sth->execute($report_arg);
-    my $data = $sth->fetchrow_hashref();
-    return ( $data->{'savedsql'}, $data->{'type'}, $data->{'report_name'}, $data->{'notes'}, $data->{'cache_expiry'}, $data->{'public'}, $data->{'id'} );
+    return $dbh->selectrow_hashref($query, undef, $report_arg);
 }
 
 =item create_compound($masterID,$subreportID)
@@ -674,22 +709,27 @@ This will take 2 reports and create a compound report using both of them
 =cut
 
 sub create_compound {
-	my ($masterID,$subreportID) = @_;
-	my $dbh = C4::Context->dbh();
-	# get the reports
-	my ($mastersql,$mastertype) = get_saved_report($masterID);
-	my ($subsql,$subtype) = get_saved_report($subreportID);
-	
-	# now we have to do some checking to see how these two will fit together
-	# or if they will
-	my ($mastertables,$subtables);
-	if ($mastersql =~ / from (.*) where /i){ 
-		$mastertables = $1;
-	}
-	if ($subsql =~ / from (.*) where /i){
-		$subtables = $1;
-	}
-	return ($mastertables,$subtables);
+    my ( $masterID, $subreportID ) = @_;
+    my $dbh = C4::Context->dbh();
+
+    # get the reports
+    my $master = get_saved_report($masterID);
+    my $mastersql = $master->{savedsql};
+    my $mastertype = $master->{type};
+    my $sub = get_saved_report($subreportID);
+    my $subsql = $master->{savedsql};
+    my $subtype = $master->{type};
+
+    # now we have to do some checking to see how these two will fit together
+    # or if they will
+    my ( $mastertables, $subtables );
+    if ( $mastersql =~ / from (.*) where /i ) {
+        $mastertables = $1;
+    }
+    if ( $subsql =~ / from (.*) where /i ) {
+        $subtables = $1;
+    }
+    return ( $mastertables, $subtables );
 }
 
 =item get_column_type($column)
@@ -739,43 +779,41 @@ sub get_distinct_values {
 }	
 
 sub save_dictionary {
-	my ($name,$description,$sql,$area) = @_;
-	my $dbh = C4::Context->dbh();
-	my $query = "INSERT INTO reports_dictionary (name,description,saved_sql,area,date_created,date_modified)
+    my ( $name, $description, $sql, $area ) = @_;
+    my $dbh   = C4::Context->dbh();
+    my $query = "INSERT INTO reports_dictionary (name,description,saved_sql,report_area,date_created,date_modified)
   VALUES (?,?,?,?,now(),now())";
     my $sth = $dbh->prepare($query);
     $sth->execute($name,$description,$sql,$area) || return 0;
     return 1;
 }
 
+my $DICTIONARY_BASE_QRY = <<EOQ;
+SELECT d.*, $AREA_NAME_SQL_SNIPPET
+FROM reports_dictionary d
+EOQ
 sub get_from_dictionary {
-	my ($area,$id) = @_;
-	my $dbh = C4::Context->dbh();
-	my $query = "SELECT * FROM reports_dictionary";
-	if ($area){
-		$query.= " WHERE area = ?";
-	}
-	elsif ($id){
-		$query.= " WHERE id = ?"
-	}
-	my $sth = $dbh->prepare($query);
-	if ($id){
-		$sth->execute($id);
-	}
-	elsif ($area) {
-		$sth->execute($area);
-	}
-	else {
-		$sth->execute();
-	}
-	my @loop;
-	my @reports = ( 'Circulation', 'Catalog', 'Patrons', 'Acquisitions', 'Accounts');
-	while (my $data = $sth->fetchrow_hashref()){
-		$data->{'areaname'}=$reports[$data->{'area'}-1];
-		push @loop,$data;
-		
-	}
-	return (\@loop);
+    my ( $area, $id ) = @_;
+    my $dbh   = C4::Context->dbh();
+    my $query = $DICTIONARY_BASE_QRY;
+    if ($area) {
+        $query .= " WHERE report_area = ?";
+    } elsif ($id) {
+        $query .= " WHERE id = ?";
+    }
+    my $sth = $dbh->prepare($query);
+    if ($id) {
+        $sth->execute($id);
+    } elsif ($area) {
+        $sth->execute($area);
+    } else {
+        $sth->execute();
+    }
+    my @loop;
+    while ( my $data = $sth->fetchrow_hashref() ) {
+        push @loop, $data;
+    }
+    return ( \@loop );
 }
 
 sub delete_definition {
@@ -815,6 +853,72 @@ sub _get_column_defs {
 	close COLUMNS;
 	return \%columns;
 }
+
+=item build_authorised_value_list($authorised_value)
+
+Returns an arrayref - hashref pair. The hashref consists of
+various code => name lists depending on the $authorised_value.
+The arrayref is the hashref keys, in appropriate order
+
+=cut
+
+sub build_authorised_value_list {
+    my ( $authorised_value ) = @_;
+
+    my $dbh = C4::Context->dbh;
+    my @authorised_values;
+    my %authorised_lib;
+
+    # builds list, depending on authorised value...
+    if ( $authorised_value eq "branches" ) {
+        my $branches = GetBranchesLoop();
+        foreach my $thisbranch (@$branches) {
+            push @authorised_values, $thisbranch->{value};
+            $authorised_lib{ $thisbranch->{value} } = $thisbranch->{branchname};
+        }
+    } elsif ( $authorised_value eq "itemtypes" ) {
+        my $sth = $dbh->prepare("SELECT itemtype,description FROM itemtypes ORDER BY description");
+        $sth->execute;
+        while ( my ( $itemtype, $description ) = $sth->fetchrow_array ) {
+            push @authorised_values, $itemtype;
+            $authorised_lib{$itemtype} = $description;
+        }
+    } elsif ( $authorised_value eq "cn_source" ) {
+        my $class_sources  = GetClassSources();
+        my $default_source = C4::Context->preference("DefaultClassificationSource");
+        foreach my $class_source ( sort keys %$class_sources ) {
+            next
+              unless $class_sources->{$class_source}->{'used'}
+                  or ( $class_source eq $default_source );
+            push @authorised_values, $class_source;
+            $authorised_lib{$class_source} = $class_sources->{$class_source}->{'description'};
+        }
+    } elsif ( $authorised_value eq "categorycode" ) {
+        my $sth = $dbh->prepare("SELECT categorycode, description FROM categories ORDER BY description");
+        $sth->execute;
+        while ( my ( $categorycode, $description ) = $sth->fetchrow_array ) {
+            push @authorised_values, $categorycode;
+            $authorised_lib{$categorycode} = $description;
+        }
+
+        #---- "true" authorised value
+    } else {
+        my $authorised_values_sth = $dbh->prepare("SELECT authorised_value,lib FROM authorised_values WHERE category=? ORDER BY lib");
+
+        $authorised_values_sth->execute($authorised_value);
+
+        while ( my ( $value, $lib ) = $authorised_values_sth->fetchrow_array ) {
+            push @authorised_values, $value;
+            $authorised_lib{$value} = $lib;
+
+            # For item location, we show the code and the libelle
+            $authorised_lib{$value} = $lib;
+        }
+    }
+
+    return (\@authorised_values, \%authorised_lib);
+}
+
 1;
 __END__
 
diff --git a/admin/authorised_values.pl b/admin/authorised_values.pl
index 949e488..17917ce 100755
--- a/admin/authorised_values.pl
+++ b/admin/authorised_values.pl
@@ -188,17 +188,18 @@ output_html_with_http_headers $input, $cookie, $template->output;
 exit 0;
 
 sub default_form {
-	# build categories list
-	my $sth = $dbh->prepare("select distinct category from authorised_values");
-	$sth->execute;
-	my @category_list;
-	my %categories;     # a hash, to check that some hardcoded categories exist.
-	while ( my ($category) = $sth->fetchrow_array) {
-		push(@category_list,$category);
-		$categories{$category} = 1;
-	}
-	# push koha system categories
-    foreach (qw(Asort1 Asort2 Bsort1 Bsort2 SUGGEST DAMAGED LOST)) {
+    # build categories list
+    my $sth = $dbh->prepare("select distinct category from authorised_values");
+    $sth->execute;
+    my @category_list;
+    my %categories;    # a hash, to check that some hardcoded categories exist.
+    while ( my ($category) = $sth->fetchrow_array ) {
+        push( @category_list, $category );
+        $categories{$category} = 1;
+    }
+
+    # push koha system categories
+    foreach (qw(Asort1 Asort2 Bsort1 Bsort2 SUGGEST DAMAGED LOST REPORT_GROUP REPORT_SUBGROUP)) {
         push @category_list, $_ unless $categories{$_};
     }
 
diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql
index 91d42a2..ff1c0f1 100644
--- a/installer/data/mysql/kohastructure.sql
+++ b/installer/data/mysql/kohastructure.sql
@@ -97,7 +97,7 @@ CREATE TABLE `auth_types` (
 DROP TABLE IF EXISTS `authorised_values`;
 CREATE TABLE `authorised_values` ( -- stores values for authorized values categories and values
   `id` int(11) NOT NULL auto_increment, -- unique key, used to identify the authorized value
-  `category` varchar(10) NOT NULL default '', -- key used to identify the authorized value category
+  `category` varchar(16) NOT NULL default '', -- key used to identify the authorized value category
   `authorised_value` varchar(80) NOT NULL default '', -- code use to identify the authorized value
   `lib` varchar(80) default NULL, -- authorized value description as printed in the staff client
   `lib_opac` VARCHAR(80) default NULL, -- authorized value description as printed in the OPAC
@@ -1626,8 +1626,9 @@ CREATE TABLE reports_dictionary ( -- definitions (or snippets of SQL) stored for
    `date_created` datetime default NULL, -- date and time this definition was created
    `date_modified` datetime default NULL, -- date and time this definition was last modified
    `saved_sql` text, -- SQL snippet for us in reports
-   `area` int(11) default NULL, -- Koha module this definition is for (1 = Circulation, 2 = Catalog, 3 = Patrons, 4 = Acquistions, 5 = Accounts)
-   PRIMARY KEY  (`id`)
+   report_area varchar(6) DEFAULT NULL, -- Koha module this definition is for Circulation, Catalog, Patrons, Acquistions, Accounts)
+   PRIMARY KEY  (id),
+   KEY dictionary_area_idx (report_area)
 ) ENGINE=InnoDB DEFAULT CHARSET=utf8;
 
 --
@@ -1725,7 +1726,11 @@ CREATE TABLE saved_sql ( -- saved sql reports
    `notes` text, -- the notes or description given to this report
    `cache_expiry` int NOT NULL default 300,
    `public` boolean NOT NULL default FALSE,
+    report_area varchar(6) default NULL,
+    report_group varchar(80) default NULL,
+    report_subgroup varchar(80) default NULL,
    PRIMARY KEY  (`id`),
+   KEY sql_area_group_idx (report_group, report_subgroup),
    KEY boridx (`borrowernumber`)
 ) ENGINE=InnoDB DEFAULT CHARSET=utf8;
 
diff --git a/installer/data/mysql/updatedatabase.pl b/installer/data/mysql/updatedatabase.pl
index 25cd367..3f713eb 100755
--- a/installer/data/mysql/updatedatabase.pl
+++ b/installer/data/mysql/updatedatabase.pl
@@ -5696,6 +5696,38 @@ if (C4::Context->preference("Version") < TransformToNum($DBversion)) {
     SetVersion($DBversion);
 }
 
+
+
+$DBversion = "3.09.00.XXX";
+if (C4::Context->preference("Version") < TransformToNum($DBversion)) {
+    $dbh->do("ALTER TABLE authorised_values MODIFY category varchar(16) NOT NULL DEFAULT '';");
+    $dbh->do("INSERT INTO authorised_values (category, authorised_value, lib) VALUES
+              ('REPORT_GROUP', 'CIRC', 'Circulation'),
+              ('REPORT_GROUP', 'CAT', 'Catalog'),
+              ('REPORT_GROUP', 'PAT', 'Patrons'),
+              ('REPORT_GROUP', 'ACQ', 'Acquisitions'),
+              ('REPORT_GROUP', 'ACC', 'Accounts');");
+
+    $dbh->do("ALTER TABLE reports_dictionary ADD report_area varchar(6) DEFAULT NULL;");
+    $dbh->do("UPDATE reports_dictionary SET report_area = CASE area
+                  WHEN 1 THEN 'CIRC'
+                  WHEN 2 THEN 'CAT'
+                  WHEN 3 THEN 'PAT'
+                  WHEN 4 THEN 'ACQ'
+                  WHEN 5 THEN 'ACC'
+                  END;");
+    $dbh->do("ALTER TABLE reports_dictionary DROP area;");
+    $dbh->do("ALTER TABLE reports_dictionary ADD KEY dictionary_area_idx (report_area);");
+
+    $dbh->do("ALTER TABLE saved_sql ADD report_area varchar(6) DEFAULT NULL;");
+    $dbh->do("ALTER TABLE saved_sql ADD report_group varchar(80) DEFAULT NULL;");
+    $dbh->do("ALTER TABLE saved_sql ADD report_subgroup varchar(80) DEFAULT NULL;");
+    $dbh->do("ALTER TABLE saved_sql ADD KEY sql_area_group_idx (report_group, report_subgroup);");
+
+    print "Upgrade to $DBversion done saved_sql new fields report_group and report_area; authorised_values.category 16 char \n";
+    SetVersion($DBversion);
+}
+
 =head1 FUNCTIONS
 
 =head2 TableExists($table)
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/reports/dictionary.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/reports/dictionary.tt
index 102a6ee..2e15834 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/reports/dictionary.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reports/dictionary.tt
@@ -33,7 +33,7 @@
 		<form action="/cgi-bin/koha/reports/dictionary.pl" method="post">
         <input type="hidden" name="phase" value="View Dictionary" />
 		[% IF ( areas ) %]
-			Filter by area <select name="areas">
+			Filter by area <select name="area">
 			<option value="">All</option>
 			[% FOREACH area IN areas %]
 			    [% IF ( area.selected ) %]
@@ -103,7 +103,7 @@
 <ol><li><input type="hidden" name="phase" value="New Term step 3" />
 <input type="hidden" name="definition_name" value="[% definition_name %]" />
 <input type="hidden" name="definition_description" value="[% definition_description %]" />
-<label for="areas">Select table </label><select name="areas" id="areas">
+<label for="area">Select table </label><select name="area" id="area">
 [% FOREACH area IN areas %]     
 <option value="[% area.id %]">[% area.name %]</option>                  
 [% 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 21d8235..aa6a627 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
@@ -24,6 +24,25 @@
 
 <script type="text/javascript">
 //<![CDATA[
+var group_subgroups = {};
+var no_subgroup_label = _( "(None)" );
+function load_group_subgroups () {
+    var group = $("#group").val();
+    var sg = $("#subgroup");
+    var has_subgroups = false;
+    $(sg).empty().append('<option value="">' + no_subgroup_label + '</option>');
+    if (group) {
+        $.each( group_subgroups[group], function(index, value) {
+                has_subgroups = true;
+            $('<option value="' + value[0] + '">' + value[1] + '</option>').appendTo(sg);
+        } );
+    }
+    if (has_subgroups) {
+        $(sg).show();
+    } else {
+        $(sg).hide();
+    }
+}
 $(document).ready(function(){
 [% IF ( showsql ) %]
     $("#sql").focus(function() {
@@ -138,6 +157,9 @@ canned reports and writing custom SQL reports.</p>
   <th>ID</th>
   <th>Report name</th>
   <th>Type</th>
+  <th>Area</th>
+  <th>Group</th>
+  <th>Subgroup</th>
   <th>Notes</th>
   <th>Author</th>
   <th>Creation date</th>
@@ -155,6 +177,9 @@ canned reports and writing custom SQL reports.</p>
 <td>[% savedreport.id %]</td>
 <td>[% savedreport.report_name %]</td>
 <td>[% savedreport.type %]</td>
+<td>[% savedreport.areaname %]</td>
+<td>[% savedreport.groupname %]</td>
+<td>[% savedreport.subgroupname %]</td>
 <td>[% savedreport.notes %]</td>
 <td>[% savedreport.borrowersurname %][% IF ( savedreport.borrowerfirstname ) %], [% savedreport.borrowerfirstname %][% END %] ([% savedreport.borrowernumber %])</td>
 <td>[% savedreport.date_created %]</td>
@@ -219,7 +244,7 @@ canned reports and writing custom SQL reports.</p>
 <form action="/cgi-bin/koha/reports/guided_reports.pl">
 <fieldset class="rows">
 <legend>Step 1 of 6: Choose a module to report on,[% IF (usecache) %] Set cache expiry, [% END %] and Choose report visibility </legend>
-<ol><li><label for="areas">Choose: </label><select name="areas" id="areas">
+<ol><li><label for="area">Choose: </label><select name="area" id="area">
 [% FOREACH area IN areas %]
 <option value="[% area.id %]">[% area.name %]</option>
 [% END %]
@@ -485,12 +510,38 @@ canned reports and writing custom SQL reports.</p>
 <form action="/cgi-bin/koha/reports/guided_reports.pl" method="post">
 <input type="hidden" name="sql" value="[% sql |html %]" />
 <input type="hidden" name="type" value="[% type %]" />
+<input type="hidden" name="area" value="[% area %]" />
 <input type="hidden" name="public" value="[% public %]" />
 <input type="hidden" name="cache_expiry" value="[% cache_expiry %]" />
 <fieldset class="rows">
 <legend>Save your custom report</legend>
 <ol>
     <li><label for="reportname">Report name: </label><input type="text" id="reportname" name="reportname" /></li>
+    [% IF groups_with_subgroups %]
+    <li><label for="group">Report group: </label><select name="group" id="group" onChange="load_group_subgroups();">
+        [% FOR g IN groups_with_subgroups %]
+            [% IF g.selected %]
+    <option value="[% g.id %]" selected>[% g.name %]</option>
+            [% ELSE %]
+    <option value="[% g.id %]">[% g.name %]</option>
+            [% END %]
+    <script type="text/javascript">
+        var g_sg = new Array();
+            [% FOR sg IN g.subgroups %]
+        g_sg.push(["[% sg.id %]", "[% sg.name %]"]);
+                [% IF sg.selected %]
+        $(document).ready(function() {
+            $("#subgroup").val("[% sg.id %]");
+        });
+                [% END %]
+            [% END %]
+        group_subgroups["[% g.id %]"] = g_sg;
+    </script>
+        [% END %]
+    </select></li>
+    <li><label for="subgroup">Report subgroup: </label><select name="subgroup" id="subgroup">
+    </select></li>
+    [% END %]
     <li><label for="notes">Notes:</label> <textarea name="notes" id="notes"></textarea></li>
 </ol></fieldset>
 <fieldset class="action"><input type="hidden" name="phase" value="Save Report" />
@@ -553,6 +604,11 @@ canned reports and writing custom SQL reports.</p>
 [% END %]
 
 [% IF ( create ) %]
+<script type="text/javascript">
+$(document).ready(function() {
+    load_group_subgroups();
+});
+</script>
 <form action="/cgi-bin/koha/reports/guided_reports.pl" method="post">
 <fieldset class="rows">
 <legend>Create report from SQL</legend>
@@ -561,6 +617,31 @@ canned reports and writing custom SQL reports.</p>
         [% IF ( reportname ) %]<input type="text" id="reportname" name="reportname" value="[% reportname %]" />
         [% ELSE %]<input type="text" id="reportname" name="reportname" />[% END %] 
     </li>
+    [% IF groups_with_subgroups %]
+    <li><label for="group">Report group: </label><select name="group" id="group" onChange="load_group_subgroups();">
+        [% FOR g IN groups_with_subgroups %]
+            [% IF g.selected %]
+    <option value="[% g.id %]" selected>[% g.name %]</option>
+            [% ELSE %]
+    <option value="[% g.id %]">[% g.name %]</option>
+            [% END %]
+    <script type="text/javascript">
+        var g_sg = new Array();
+            [% FOR sg IN g.subgroups %]
+        g_sg.push(["[% sg.id %]", "[% sg.name %]"]);
+                [% IF sg.selected %]
+        $(document).ready(function() {
+            $("#subgroup").val("[% sg.id %]");
+        });
+                [% END %]
+            [% END %]
+        group_subgroups["[% g.id %]"] = g_sg;
+    </script>
+        [% END %]
+    </select></li>
+    <li><label for="subgroup">Report subgroup: </label><select name="subgroup" id="subgroup">
+    </select></li>
+    [% END %]
 [% IF (public) %]
   <li><label for="public">Report is public:</label><select id="public" name="public"> <option value="0">No (default)</option> <option value="1" selected="selected">Yes</public> </select></li>
 [% ELSE %]
@@ -645,6 +726,11 @@ Sub report:<select name="subreport">
 [% END %]
 
 [% IF ( editsql ) %]
+<script type="text/javascript">
+$(document).ready(function() {
+    load_group_subgroups();
+});
+</script>
 <form action="/cgi-bin/koha/reports/guided_reports.pl" method="post">
 <input type="hidden" name="phase" value="Update SQL" />
 <input type="hidden" name="id" value="[% id %]"/>
@@ -652,6 +738,31 @@ Sub report:<select name="subreport">
 <legend>Edit SQL report</legend>
 <ol>
 <li><label for="reportname">Report name:</label><input type="text" id="reportname" name="reportname" value="[% reportname %]" size="50" /></li>
+    [% IF groups_with_subgroups %]
+    <li><label for="group">Report group: </label><select name="group" id="group" onChange="load_group_subgroups();">
+        [% FOR g IN groups_with_subgroups %]
+            [% IF g.selected %]
+    <option value="[% g.id %]" selected>[% g.name %]</option>
+            [% ELSE %]
+    <option value="[% g.id %]">[% g.name %]</option>
+            [% END %]
+    <script type="text/javascript">
+        var g_sg = new Array();
+            [% FOR sg IN g.subgroups %]
+        g_sg.push(["[% sg.id %]", "[% sg.name %]"]);
+                [% IF sg.selected %]
+        $(document).ready(function() {
+            $("#subgroup").val("[% sg.id %]");
+        });
+                [% END %]
+            [% END %]
+        group_subgroups["[% g.id %]"] = g_sg;
+    </script>
+        [% END %]
+    </select></li>
+    <li><label for="subgroup">Report subgroup: </label><select name="subgroup" id="subgroup">
+    </select></li>
+    [% END %]
 [% IF (public) %]
   <li><label for="public">Report is public:</label><select id="public" name="public"> <option value="0">No (default)</option> <option value="1" selected="selected">Yes</public> </select></li>
 [% ELSE %]
@@ -719,12 +830,43 @@ Sub report:<select name="subreport">
 
 [% IF ( saved1 ) %]
 <div id="saved-reports-filter">
+<script type="text/javascript">
+$(document).ready(function() {
+    no_subgroup_label = _( "-- All --" );
+    load_group_subgroups();
+});
+</script>
 <form action="/cgi-bin/koha/reports/guided_reports.pl" method="get">
   <input type="hidden" name="phase" value="Use saved" />
   <input type="hidden" name="filter_set" value="1" />
   <fieldset class="brief">
   <h3>Filter</h3>
   <ol>
+    <li><label for="group">Choose Group and Subgroup: </label>
+    <select name="group" id="group" onChange="load_group_subgroups();">
+        <option value="">-- All --</option>
+    [% FOR g IN groups_with_subgroups %]
+        [% IF g.selected %]
+        <option value="[% g.id %]" selected>[% g.name %]</option>
+        [% ELSE %]
+        <option value="[% g.id %]">[% g.name %]</option>
+        [% END %]
+        <script type="text/javascript">
+            var g_sg = new Array();
+        [% FOR sg IN g.subgroups %]
+            g_sg.push(["[% sg.id %]", "[% sg.name %]"]);
+            [% IF sg.selected %]
+            $(document).ready(function() {
+                $("#subgroup").val("[% sg.id %]");
+            });
+            [% END %]
+        [% END %]
+            group_subgroups["[% g.id %]"] = g_sg;
+        </script>
+    [% END %]
+    </select>
+    <select name="subgroup" id="subgroup"></select>
+    </li>
     <li><label for="filter_date">Date:</label> <input type="text" id="filter_date" name="filter_date" size="10" value="[% filter_date %]" class="datepicker" />
     <div class="hint">[% INCLUDE 'date-format.inc' %]</div>
 
diff --git a/misc/cronjobs/runreport.pl b/misc/cronjobs/runreport.pl
index 6a8cce7..690f216 100755
--- a/misc/cronjobs/runreport.pl
+++ b/misc/cronjobs/runreport.pl
@@ -186,14 +186,26 @@ unless (scalar(@ARGV)) {
 ($verbose) and print scalar(@ARGV), " argument(s) after options: " . join(" ", @ARGV) . "\n";
 
 
-foreach my $report (@ARGV) {
-    my ($sql, $type) = get_saved_report($report);
-    unless ($sql) {
-        carp "ERROR: No saved report $report found";
+foreach my $report_id (@ARGV) {
+    my $report = get_saved_report($report_id);
+    unless ($report) {
+        warn "ERROR: No saved report $report_id found";
         next;
     }
+    my $sql         => $report->{savedsql};
+    my $report_name => $report->{report_name};
+    my $type        => $report->{type};
+
     $verbose and print "SQL: $sql\n\n";
-    # my $results = execute_query($sql, undef, 0, 99999, $format, $report); 
+    if (defined($report_name) and $report_name ne "")
+    {
+        $subject = $report_name ;
+    }
+    else
+    {
+        $subject = 'Koha Saved Report';
+    }
+    # my $results = execute_query($sql, undef, 0, 99999, $format, $report_id);
     my ($sth) = execute_query($sql);
     # execute_query(sql, , 0, 20, , )
     my $count = scalar($sth->rows);
diff --git a/opac/svc/report b/opac/svc/report
index 86b7a00..38cbd42 100755
--- a/opac/svc/report
+++ b/opac/svc/report
@@ -31,38 +31,34 @@ my $query  = CGI->new();
 my $report_id = $query->param('id');
 my $report_name = $query->param('name');
 
-my $cache;
-my $sql;
-my $type;
-my $notes;
-my $cache_expiry;
-my $public;
+my $report_rec = get_saved_report( $report_name ? { 'name' => $report_name } : { 'id' => $report_id } );
+die "Sorry this report is not public\n" unless $report_rec->{public};
 
-( $sql, $type, $report_name, $notes, $cache_expiry, $public, $report_id ) =
-  get_saved_report($report_name ? { 'name' => $report_name } : { 'id' => $report_id } );
-die "Sorry this report is not public\n" unless $public;
 
-if (Koha::Cache->is_cache_active) {
-    $cache = Koha::Cache->new(
-    );
-    my $page = $cache->get_from_cache("opac:report:$report_id");
-    if ($page) {
-        print $query->header;
-        print $page;
-        exit;
-    }
+my $cache_active = Koha::Cache->is_cache_active;
+my ($cache_key, $cache, $json_text);
+if ($cache_active) {
+    $cache_key = "opac:report:".($report_name ? "name:$report_name" : "id:$report_id");
+    $cache = Koha::Cache->new();
+    $json_text = $cache->get_from_cache($cache_key);
 }
 
-print $query->header;
-if ($sql) {
+unless ($json_text) {
     my $offset = 0;
     my $limit  = C4::Context->preference("SvcMaxReportRows") || 10;
-    my ( $sth, $errors ) = execute_query( $sql, $offset, $limit );
-    my $lines     = $sth->fetchall_arrayref;
-    my $json_text = to_json($lines);
-    print $json_text;
+    my ( $sth, $errors ) = execute_query( $report_rec->{savedsql}, $offset, $limit );
+    if ($sth) {
+        my $lines     = $sth->fetchall_arrayref;
+        $json_text = to_json($lines);
 
-    if (Koha::Cache->is_cache_active) {
-        $cache->set_in_cache( "opac:report:$report_id", $json_text, $cache_expiry );
+        if ($cache_active) {
+            $cache->set_in_cache( $cache_key, $json_text, $report_rec->{cache_expiry} );
+        }
+    }
+    else {
+        $json_text = to_json($errors);
     }
 }
+
+print $query->header;
+print $json_text;
diff --git a/reports/dictionary.pl b/reports/dictionary.pl
index 4f94d69..aae96c9 100755
--- a/reports/dictionary.pl
+++ b/reports/dictionary.pl
@@ -37,11 +37,12 @@ my $input = new CGI;
 my $referer = $input->referer();
 
 my $phase = $input->param('phase') || 'View Dictionary';
-my $area = $input->param('areas') || '';
-my $no_html = 0; # this will be set if we dont want to print out an html::template
-my 	( $template, $borrowernumber, $cookie ) = get_template_and_user(
-    {
-        template_name   => "reports/dictionary.tmpl",
+my $definition_name        = $input->param('definition_name');
+my $definition_description = $input->param('definition_description');
+my $area  = $input->param('area') || '';
+my $no_html = 0;    # this will be set if we dont want to print out an html::template
+my ( $template, $borrowernumber, $cookie ) = get_template_and_user(
+    {   template_name   => "reports/dictionary.tmpl",
         query           => $input,
         type            => "intranet",
         authnotrequired => 0,
@@ -51,78 +52,71 @@ my 	( $template, $borrowernumber, $cookie ) = get_template_and_user(
 	);
 
 if ($phase eq 'View Dictionary'){
-	# view the dictionary we use to set up abstract variables such as all borrowers over fifty who live in a certain town
-	my $areas = get_report_areas();
-    foreach (@{ $areas }) {
-        $_->{selected} = 1 if $_->{id} eq $area; # mark active area
-    }
-	my $definitions = get_from_dictionary($area);
-	$template->param( 'areas' => $areas ,
-		'start_dictionary' => 1,
-		'definitions' => $definitions,
-	);
-}
-elsif ($phase eq 'Add New Definition'){
-	# display form allowing them to add a new definition
-	$template->param( 'new_dictionary' => 1,
-		);
+    # view the dictionary we use to set up abstract variables such as all borrowers over fifty who live in a certain town
+    my $definitions = get_from_dictionary($area);
+    $template->param(
+        'areas'=> areas(),
+        'start_dictionary' => 1,
+        'definitions'      => $definitions,
+    );
+} elsif ( $phase eq 'Add New Definition' ) {
+
+    # display form allowing them to add a new definition
+    $template->param( 'new_dictionary' => 1, );
 }
 
-elsif ($phase eq 'New Term step 2'){
-	# Choosing the area
-	my $areas = C4::Reports::Guided::get_report_areas();
-	my $definition_name=$input->param('definition_name');
-	my $definition_description=$input->param('definition_description');		
-	$template->param( 'step_2' => 1,
-		'areas' => $areas,
-		'definition_name' => $definition_name,
-		'definition_description' => $definition_description,
-	);
+elsif ( $phase eq 'New Term step 2' ) {
+
+    # Choosing the area
+    $template->param(
+        'step_2'                 => 1,
+        'areas'                  => areas(),
+        'definition_name'        => $definition_name,
+        'definition_description' => $definition_description,
+    );
 }
 
-elsif ($phase eq 'New Term step 3'){
-	# Choosing the columns
-	my $area = $input->param('areas');
-	my $columns = get_columns($area,$input);
-	my $definition_name=$input->param('definition_name');
-	my $definition_description=$input->param('definition_description');		
-	$template->param( 'step_3' => 1,
-		'area' => $area,
-		'columns' => $columns,
-		'definition_name' => $definition_name,
-		'definition_description' => $definition_description,
-	);
+elsif ( $phase eq 'New Term step 3' ) {
+
+    # Choosing the columns
+    my $columns                = get_columns( $area, $input );
+    $template->param(
+        'step_3'                 => 1,
+        'area'                   => $area,
+        'columns'                => $columns,
+        'definition_name'        => $definition_name,
+        'definition_description' => $definition_description,
+    );
 }
 
-elsif ($phase eq 'New Term step 4'){
-	# Choosing the values
-	my $area=$input->param('area');
-	my $definition_name=$input->param('definition_name');
-	my $definition_description=$input->param('definition_description');		
-    my @columns = $input->param('columns');
-	my $columnstring = join (',',@columns);
-	my @column_loop;
-	foreach my $column (@columns){
-		my %tmp_hash;
-		$tmp_hash{'name'}=$column;
-		my $type =get_column_type($column);
-		if ($type eq 'distinct'){
-			my $values = get_distinct_values($column);
-			$tmp_hash{'values'} = $values;
-			$tmp_hash{'distinct'} = 1;
-			  
-		}
-		if ($type eq 'DATE' || $type eq 'DATETIME'){
-			$tmp_hash{'date'}=1;
-		}
-		if ($type eq 'TEXT' || $type eq 'MEDIUMTEXT'){
-			$tmp_hash{'text'}=1;
-		}
-#		else {
-#			warn $type;#
-#			}
-		push @column_loop,\%tmp_hash;
-		}
+elsif ( $phase eq 'New Term step 4' ) {
+
+    # Choosing the values
+    my @columns                = $input->param('columns');
+    my $columnstring           = join( ',', @columns );
+    my @column_loop;
+    foreach my $column (@columns) {
+        my %tmp_hash;
+        $tmp_hash{'name'} = $column;
+        my $type = get_column_type($column);
+        if ( $type eq 'distinct' ) {
+            my $values = get_distinct_values($column);
+            $tmp_hash{'values'}   = $values;
+            $tmp_hash{'distinct'} = 1;
+
+        }
+        if ( $type eq 'DATE' || $type eq 'DATETIME' ) {
+            $tmp_hash{'date'} = 1;
+        }
+        if ( $type eq 'TEXT' ) {
+            $tmp_hash{'text'} = 1;
+        }
+
+        #		else {
+        #			warn $type;#
+        #			}
+        push @column_loop, \%tmp_hash;
+    }
 
 	$template->param( 'step_4' => 1,
 		'area' => $area,
@@ -134,83 +128,78 @@ elsif ($phase eq 'New Term step 4'){
 	);
 }
 
-elsif ($phase eq 'New Term step 5'){
-	# Confirmation screen
-	my $areas = C4::Reports::Guided::get_report_areas();
-	my $area = $input->param('area');
-    my $areaname = $areas->[$area - 1]->{'name'};
-	my $columnstring = $input->param('columnstring');
-	my $definition_name=$input->param('definition_name');
-	my $definition_description=$input->param('definition_description');	
-	my @criteria = $input->param('criteria_column'); 
-	my $query_criteria;
-	my @criteria_loop;
-	foreach my $crit (@criteria) {
-		my $value = $input->param( $crit . "_value" );
-		if ($value) {
-                    my %tmp_hash;
-                    $tmp_hash{'name'}=$crit;
-                    $tmp_hash{'value'} = $value;
-                    push @criteria_loop,\%tmp_hash;
-                    if ($value =~ C4::Dates->regexp(C4::Context->preference('dateformat'))) {    
-                        my $date = C4::Dates->new($value);
-                        $value = $date->output("iso");
-                    }
-                    $query_criteria .= " AND $crit='$value'";
-		}
-		$value = $input->param( $crit . "_start_value" );
-		if ($value) {
-                    my %tmp_hash;
-                    $tmp_hash{'name'}="$crit Start";
-                    $tmp_hash{'value'} = $value;
-                    push @criteria_loop,\%tmp_hash;
-                    if ($value =~ C4::Dates->regexp(C4::Context->preference('dateformat'))) {    
-                        my $date = C4::Dates->new($value);
-                        $value = $date->output("iso");
-                    }
-                    $query_criteria .= " AND $crit >= '$value'";
-		}
-		$value = $input->param( $crit . "_end_value" );
-		if ($value) {
-                    my %tmp_hash;
-                    $tmp_hash{'name'}="$crit End";
-                    $tmp_hash{'value'} = $value;
-                    push @criteria_loop,\%tmp_hash;
-                    if ($value =~ C4::Dates->regexp(C4::Context->preference('dateformat'))) {    
-                        my $date = C4::Dates->new($value);
-                        $value = $date->output("iso");
-                    }
-                    $query_criteria .= " AND $crit <= '$value'";
-		}		  
-	}
-	$template->param( 'step_5' => 1,
-		'area' => $area,
-		'areaname' => $areaname,
-		'definition_name' => $definition_name,
-		'definition_description' => $definition_description,
-		'query' => $query_criteria,
-		'columnstring' => $columnstring,
-		'criteria_loop' => \@criteria_loop,
-	);
+elsif ( $phase eq 'New Term step 5' ) {
+    # Confirmation screen
+    my $columnstring           = $input->param('columnstring');
+    my @criteria               = $input->param('criteria_column');
+    my $query_criteria;
+    my @criteria_loop;
+
+    foreach my $crit (@criteria) {
+        my $value = $input->param( $crit . "_value" );
+        if ($value) {
+            my %tmp_hash;
+            $tmp_hash{'name'}  = $crit;
+            $tmp_hash{'value'} = $value;
+            push @criteria_loop, \%tmp_hash;
+            if ( $value =~ C4::Dates->regexp( C4::Context->preference('dateformat') ) ) {
+                my $date = C4::Dates->new($value);
+                $value = $date->output("iso");
+            }
+            $query_criteria .= " AND $crit='$value'";
+        }
+        $value = $input->param( $crit . "_start_value" );
+        if ($value) {
+            my %tmp_hash;
+            $tmp_hash{'name'}  = "$crit Start";
+            $tmp_hash{'value'} = $value;
+            push @criteria_loop, \%tmp_hash;
+            if ( $value =~ C4::Dates->regexp( C4::Context->preference('dateformat') ) ) {
+                my $date = C4::Dates->new($value);
+                $value = $date->output("iso");
+            }
+            $query_criteria .= " AND $crit >= '$value'";
+        }
+        $value = $input->param( $crit . "_end_value" );
+        if ($value) {
+            my %tmp_hash;
+            $tmp_hash{'name'}  = "$crit End";
+            $tmp_hash{'value'} = $value;
+            push @criteria_loop, \%tmp_hash;
+            if ( $value =~ C4::Dates->regexp( C4::Context->preference('dateformat') ) ) {
+                my $date = C4::Dates->new($value);
+                $value = $date->output("iso");
+            }
+            $query_criteria .= " AND $crit <= '$value'";
+        }
+    }
+    my %report_areas = map @$_, get_report_areas();
+    $template->param(
+        'step_5'                 => 1,
+        'area'                   => $area,
+        'areaname'               => $report_areas{$area},
+        'definition_name'        => $definition_name,
+        'definition_description' => $definition_description,
+        'query'                  => $query_criteria,
+        'columnstring'           => $columnstring,
+        'criteria_loop'          => \@criteria_loop,
+    );
 }
 
-elsif ($phase eq 'New Term step 6'){
-	# Saving
-	my $area = $input->param('area');
-	my $definition_name=$input->param('definition_name');
-	my $definition_description=$input->param('definition_description');		
-	my $sql=$input->param('sql');
-	save_dictionary($definition_name,$definition_description,$sql,$area);
-	$no_html=1;
-	print $input->redirect("/cgi-bin/koha/reports/dictionary.pl?phase=View%20Dictionary");	
-
+elsif ( $phase eq 'New Term step 6' ) {
+    # Saving
+    my $area                   = $input->param('area');
+    my $sql                    = $input->param('sql');
+    save_dictionary( $definition_name, $definition_description, $sql, $area );
+    $no_html = 1;
+    print $input->redirect("/cgi-bin/koha/reports/dictionary.pl?phase=View%20Dictionary");
+
+} elsif ( $phase eq 'Delete Definition' ) {
+    $no_html = 1;
+    my $id = $input->param('id');
+    delete_definition($id);
+    print $input->redirect("/cgi-bin/koha/reports/dictionary.pl?phase=View%20Dictionary");
 }
-elsif ($phase eq 'Delete Definition'){
-	$no_html=1;
-	my $id = $input->param('id');
-	delete_definition($id);
-	print $input->redirect("/cgi-bin/koha/reports/dictionary.pl?phase=View%20Dictionary");
-	}
 
 $template->param( 'referer' => $referer );
 
@@ -218,3 +207,16 @@ $template->param( 'referer' => $referer );
 if (!$no_html){
 	output_html_with_http_headers $input, $cookie, $template->output;
 }
+
+sub areas {
+    my $areas = get_report_areas();
+    my @a;
+    foreach (@$areas) {
+        push @a, {
+            id => $_->[0],
+            name => $_->[1],
+            selected => ($_->[0] eq $area),
+        };
+    }
+    return \@a;
+}
diff --git a/reports/guided_reports.pl b/reports/guided_reports.pl
index 25a2ffb..85247d3 100755
--- a/reports/guided_reports.pl
+++ b/reports/guided_reports.pl
@@ -18,14 +18,15 @@
 # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
 
 use strict;
-#use warnings; FIXME - Bug 2505
+use warnings;
+
 use CGI;
 use Text::CSV;
 use URI::Escape;
 use C4::Reports::Guided;
 use C4::Auth qw/:DEFAULT get_session/;
 use C4::Output;
-use C4::Dates;
+use C4::Dates qw/format_date/;
 use C4::Debug;
 use C4::Branch; # XXX subfield_is_koha_internal_p
 use Koha::Cache;
@@ -69,7 +70,7 @@ my $session = $cookie ? get_session($cookie->value) : undef;
 my $filter;
 if ( $input->param("filter_set") ) {
     $filter = {};
-    $filter->{$_} = $input->param("filter_$_") foreach qw/date author keyword/;
+    $filter->{$_} = $input->param("filter_$_") foreach qw/date author keyword group subgroup/;
     $session->param('report_filter', $filter) if $session;
     $template->param( 'filter_set' => 1 );
 }
@@ -86,21 +87,27 @@ if ( !$phase ) {
 elsif ( $phase eq 'Build new' ) {
     # build a new report
     $template->param( 'build1' => 1 );
-    $template->param( 'areas' => get_report_areas(), 'usecache' => $usecache, 'cache_expiry' => 300, 'public' => '0' );
-}
-elsif ( $phase eq 'Use saved' ) {
+    my $areas = get_report_areas();
+    $template->param(
+        'areas' => [map { id => $_->[0], name => $_->[1] }, @$areas],
+        'usecache' => $usecache,
+        'cache_expiry' => 300,
+        'public' => '0',
+    );
+} elsif ( $phase eq 'Use saved' ) {
+
     # use a saved report
     # get list of reports and display them
+    my $group = $input->param('group');
+    my $subgroup = $input->param('subgroup');
+    $filter->{group} = $group;
+    $filter->{subgroup} = $subgroup;
     $template->param(
         'saved1' => 1,
         'savedreports' => get_saved_reports($filter),
         'usecache' => $usecache,
+        'groups_with_subgroups'=> groups_with_subgroups($group, $subgroup),
     );
-    if ($filter) {
-        while ( my ($k, $v) = each %$filter ) {
-            $template->param( "filter_$k" => $v ) if $v;
-        }
-    }
 }
 
 elsif ( $phase eq 'Delete Saved') {
@@ -114,30 +121,33 @@ elsif ( $phase eq 'Delete Saved') {
 
 elsif ( $phase eq 'Show SQL'){
 	
-	my $id = $input->param('reports');
-    my ($sql,$type,$reportname,$notes) = get_saved_report($id);
-	$template->param(
+    my $id = $input->param('reports');
+    my $report = get_saved_report($id);
+    $template->param(
         'id'      => $id,
-        'reportname' => $reportname,
-        'notes'      => $notes,
-		'sql'     => $sql,
-		'showsql' => 1,
+        'reportname' => $report->{report_name},
+        'notes'      => $report->{notes},
+	'sql'     => $report->{savedsql},
+	'showsql' => 1,
     );
 }
 
 elsif ( $phase eq 'Edit SQL'){
 	
     my $id = $input->param('reports');
-    my ($sql,$type,$reportname,$notes, $cache_expiry, $public) = get_saved_report($id);
+    my $report = get_saved_report($id);
+    my $group = $report->{report_group};
+    my $subgroup  = $report->{report_subgroup};
     $template->param(
-	    'sql'        => $sql,
-	    'reportname' => $reportname,
-        'notes'      => $notes,
+        'sql'        => $report->{savedsql},
+        'reportname' => $report->{report_name},
+        'groups_with_subgroups' => groups_with_subgroups($group, $subgroup),
+        'notes'      => $report->{notes},
         'id'         => $id,
-        'cache_expiry' => $cache_expiry,
-        'public' => $public,
+        'cache_expiry' => $report->{cache_expiry},
+        'public' => $report->{public},
         'usecache' => $usecache,
-	    'editsql'    => 1,
+        'editsql'    => 1,
     );
 }
 
@@ -145,6 +155,8 @@ elsif ( $phase eq 'Update SQL'){
     my $id         = $input->param('id');
     my $sql        = $input->param('sql');
     my $reportname = $input->param('reportname');
+    my $group      = $input->param('group');
+    my $subgroup   = $input->param('subgroup');
     my $notes      = $input->param('notes');
     my $cache_expiry = $input->param('cache_expiry');
     my $cache_expiry_units = $input->param('cache_expiry_units');
@@ -178,16 +190,22 @@ elsif ( $phase eq 'Update SQL'){
             'errors'    => \@errors,
             'sql'       => $sql,
         );
-    }
-    else {
-        update_sql( $id, $sql, $reportname, $notes, $cache_expiry, $public );
+    } else {
+        update_sql( $id, {
+                sql => $sql,
+                name => $reportname,
+                group => $group,
+                subgroup => $subgroup,
+                notes => $notes,
+                cache_expiry => $cache_expiry,
+                public => $public,
+        } );
         $template->param(
             'save_successful'       => 1,
             'reportname'            => $reportname,
             'id'                    => $id,
         );
     }
-    
 }
 
 elsif ($phase eq 'retrieve results') {
@@ -229,7 +247,7 @@ elsif ( $phase eq 'Report on this Area' ) {
       # they have choosen a new report and the area to report on
       $template->param(
           'build2' => 1,
-          'area'   => $input->param('areas'),
+          'area'   => $input->param('area'),
           'types'  => get_report_types(),
           'cache_expiry' => $cache_expiry,
           'public' => $input->param('public'),
@@ -276,42 +294,42 @@ elsif ( $phase eq 'Choose these criteria' ) {
     my $area     = $input->param('area');
     my $type     = $input->param('type');
     my $column   = $input->param('column');
-	my @definitions = $input->param('definition');
-	my $definition = join (',',@definitions);
+    my @definitions = $input->param('definition');
+    my $definition = join (',',@definitions);
     my @criteria = $input->param('criteria_column');
-	my $query_criteria;
+    my $query_criteria;
     foreach my $crit (@criteria) {
         my $value = $input->param( $crit . "_value" );
-	
-	# If value is not defined, then it may be range values
-	if (!defined $value) {
-
-	    my $fromvalue = $input->param( "from_" . $crit . "_value" );
-	    my $tovalue   = $input->param( "to_"   . $crit . "_value" );
-	    
-	    # If the range values are dates
-	    if ($fromvalue =~ C4::Dates->regexp('syspref') && $tovalue =~ C4::Dates->regexp('syspref')) { 
-		$fromvalue = C4::Dates->new($fromvalue)->output("iso");
-		$tovalue = C4::Dates->new($tovalue)->output("iso");
-	    }
-
-	    if ($fromvalue && $tovalue) {
-		$query_criteria .= " AND $crit >= '$fromvalue' AND $crit <= '$tovalue'";
-	    }
-
-	} else {
-
-	    # If value is a date
-	    if ($value =~ C4::Dates->regexp('syspref')) { 
-		$value = C4::Dates->new($value)->output("iso");
-	    }
-        # don't escape runtime parameters, they'll be at runtime
-        if ($value =~ /<<.*>>/) {
-            $query_criteria .= " AND $crit=$value";
+
+        # If value is not defined, then it may be range values
+        if (!defined $value) {
+
+            my $fromvalue = $input->param( "from_" . $crit . "_value" );
+            my $tovalue   = $input->param( "to_"   . $crit . "_value" );
+
+            # If the range values are dates
+            if ($fromvalue =~ C4::Dates->regexp('syspref') && $tovalue =~ C4::Dates->regexp('syspref')) { 
+                $fromvalue = C4::Dates->new($fromvalue)->output("iso");
+                $tovalue = C4::Dates->new($tovalue)->output("iso");
+            }
+
+            if ($fromvalue && $tovalue) {
+                $query_criteria .= " AND $crit >= '$fromvalue' AND $crit <= '$tovalue'";
+            }
+
         } else {
-            $query_criteria .= " AND $crit='$value'";
+
+            # If value is a date
+            if ($value =~ C4::Dates->regexp('syspref')) { 
+                $value = C4::Dates->new($value)->output("iso");
+            }
+            # don't escape runtime parameters, they'll be at runtime
+            if ($value =~ /<<.*>>/) {
+                $query_criteria .= " AND $crit=$value";
+            } else {
+                $query_criteria .= " AND $crit='$value'";
+            }
         }
-	}
     }
     $template->param(
         'build5'         => 1,
@@ -413,6 +431,7 @@ elsif ( $phase eq 'Build report' ) {
       build_query( \@columns, $query_criteria, $query_orderby, $area, $totals, $definition );
     $template->param(
         'showreport' => 1,
+        'area'       => $area,
         'sql'        => $sql,
         'type'       => $type,
         'cache_expiry' => $input->param('cache_expiry'),
@@ -421,23 +440,29 @@ elsif ( $phase eq 'Build report' ) {
 }
 
 elsif ( $phase eq 'Save' ) {
-	# Save the report that has just been built
+    # Save the report that has just been built
+    my $area           = $input->param('area');
     my $sql  = $input->param('sql');
     my $type = $input->param('type');
     $template->param(
         'save' => 1,
+        'area'  => $area,
         'sql'  => $sql,
         'type' => $type,
         'cache_expiry' => $input->param('cache_expiry'),
         'public' => $input->param('public'),
+        'groups_with_subgroups' => groups_with_subgroups($area), # in case we have a report group that matches area
     );
 }
 
 elsif ( $phase eq 'Save Report' ) {
-    # save the sql pasted in by a user 
-    my $sql  = $input->param('sql');
-    my $name = $input->param('reportname');
-    my $type = $input->param('types');
+    # save the sql pasted in by a user
+    my $area  = $input->param('area');
+    my $group = $input->param('group');
+    my $subgroup = $input->param('subgroup');
+    my $sql   = $input->param('sql');
+    my $name  = $input->param('reportname');
+    my $type  = $input->param('types');
     my $notes = $input->param('notes');
     my $cache_expiry = $input->param('cache_expiry');
     my $cache_expiry_units = $input->param('cache_expiry_units');
@@ -455,7 +480,7 @@ elsif ( $phase eq 'Save Report' ) {
       }
     }
     # check $cache_expiry isnt too large, Memcached::set requires it to be less than 30 days or it will be treated as if it were an absolute time stamp
-    if( $cache_expiry >= 2592000 ){
+    if( $cache_expiry && $cache_expiry >= 2592000 ){
       push @errors, {cache_expiry => $cache_expiry};
     }
     ## FIXME this is AFTER entering a name to save the report under
@@ -463,7 +488,7 @@ elsif ( $phase eq 'Save Report' ) {
         push @errors, {sqlerr => $1};
     }
     elsif ($sql !~ /^(SELECT)/i) {
-        push @errors, {queryerr => 1};
+        push @errors, {queryerr => "No SELECT"};
     }
     if (@errors) {
         $template->param(
@@ -477,161 +502,174 @@ elsif ( $phase eq 'Save Report' ) {
         );
     }
     else {
-        my $id = save_report( $borrowernumber, $sql, $name, $type, $notes, $cache_expiry, $public );
-        $template->param(
-            'save_successful'       => 1,
-            'reportname'            => $name,
-            'id'                    => $id,
-        );
+        save_report( {
+                borrowernumber => $borrowernumber,
+                sql            => $sql,
+                name           => $name,
+                area           => $area,
+                group          => $group,
+                subgroup       => $subgroup,
+                type           => $type,
+                notes          => $notes,
+                cache_expiry   => $cache_expiry,
+                public         => $public,
+            } );
+        $template->param( 'save_successful' => 1, );
     }
 }
 
 elsif ($phase eq 'Run this report'){
     # execute a saved report
-    my $limit  = 20;    # page size. # TODO: move to DB or syspref?
-    my $offset = 0;
-    my $report = $input->param('reports');
+    my $limit      = 20; # page size. # TODO: move to DB or syspref?
+    my $offset     = 0;
+    my $report_id  = $input->param('reports');
     my @sql_params = $input->param('sql_params');
     # offset algorithm
     if ($input->param('page')) {
         $offset = ($input->param('page') - 1) * $limit;
     }
-    my ($sql,$type,$name,$notes) = get_saved_report($report);
-    unless ($sql) {
-        push @errors, {no_sql_for_id=>$report};   
-    } 
-    my @rows = ();
-    # if we have at least 1 parameter, and it's not filled, then don't execute but ask for parameters
-    if ($sql =~ /<</ && !@sql_params) {
-        # split on ??. Each odd (2,4,6,...) entry should be a parameter to fill
-        my @split = split /<<|>>/,$sql;
-        my @tmpl_parameters;
-        for(my $i=0;$i<($#split/2);$i++) {
-            my ($text,$authorised_value) = split /\|/,$split[$i*2+1];
-            my $input;
-            my $labelid;
-            if ($authorised_value eq "date") {
-               $input = 'date';
-            }
-            elsif ($authorised_value) {
-                my $dbh=C4::Context->dbh;
-                my @authorised_values;
-                my %authorised_lib;
-                # builds list, depending on authorised value...
-                if ( $authorised_value eq "branches" ) {
-                    my $branches = GetBranchesLoop();
-                    foreach my $thisbranch (@$branches) {
-                        push @authorised_values, $thisbranch->{value};
-                        $authorised_lib{$thisbranch->{value}} = $thisbranch->{branchname};
-                    }
+
+    my ( $sql, $type, $name, $notes );
+    if (my $report = get_saved_report($report_id)) {
+        $sql   = $report->{savedsql};
+        $name  = $report->{report_name};
+        $notes = $report->{notes};
+
+        my @rows = ();
+        # if we have at least 1 parameter, and it's not filled, then don't execute but ask for parameters
+        if ($sql =~ /<</ && !@sql_params) {
+            # split on ??. Each odd (2,4,6,...) entry should be a parameter to fill
+            my @split = split /<<|>>/,$sql;
+            my @tmpl_parameters;
+            for(my $i=0;$i<($#split/2);$i++) {
+                my ($text,$authorised_value) = split /\|/,$split[$i*2+1];
+                my $input;
+                my $labelid;
+                if ($authorised_value eq "date") {
+                   $input = 'date';
                 }
-                elsif ( $authorised_value eq "itemtypes" ) {
-                    my $sth = $dbh->prepare("SELECT itemtype,description FROM itemtypes ORDER BY description");
-                    $sth->execute;
-                    while ( my ( $itemtype, $description ) = $sth->fetchrow_array ) {
-                        push @authorised_values, $itemtype;
-                        $authorised_lib{$itemtype} = $description;
+                elsif ($authorised_value) {
+                    my $dbh=C4::Context->dbh;
+                    my @authorised_values;
+                    my %authorised_lib;
+                    # builds list, depending on authorised value...
+                    if ( $authorised_value eq "branches" ) {
+                        my $branches = GetBranchesLoop();
+                        foreach my $thisbranch (@$branches) {
+                            push @authorised_values, $thisbranch->{value};
+                            $authorised_lib{$thisbranch->{value}} = $thisbranch->{branchname};
+                        }
                     }
-                }
-                elsif ( $authorised_value eq "cn_source" ) {
-                    my $class_sources = GetClassSources();
-                    my $default_source = C4::Context->preference("DefaultClassificationSource");
-                    foreach my $class_source (sort keys %$class_sources) {
-                        next unless $class_sources->{$class_source}->{'used'} or
-                                    ($class_source eq $default_source);
-                        push @authorised_values, $class_source;
-                        $authorised_lib{$class_source} = $class_sources->{$class_source}->{'description'};
+                    elsif ( $authorised_value eq "itemtypes" ) {
+                        my $sth = $dbh->prepare("SELECT itemtype,description FROM itemtypes ORDER BY description");
+                        $sth->execute;
+                        while ( my ( $itemtype, $description ) = $sth->fetchrow_array ) {
+                            push @authorised_values, $itemtype;
+                            $authorised_lib{$itemtype} = $description;
+                        }
                     }
-                }
-                elsif ( $authorised_value eq "categorycode" ) {
-                    my $sth = $dbh->prepare("SELECT categorycode, description FROM categories ORDER BY description");
-                    $sth->execute;
-                    while ( my ( $categorycode, $description ) = $sth->fetchrow_array ) {
-                        push @authorised_values, $categorycode;
-                        $authorised_lib{$categorycode} = $description;
+                    elsif ( $authorised_value eq "cn_source" ) {
+                        my $class_sources = GetClassSources();
+                        my $default_source = C4::Context->preference("DefaultClassificationSource");
+                        foreach my $class_source (sort keys %$class_sources) {
+                            next unless $class_sources->{$class_source}->{'used'} or
+                                        ($class_source eq $default_source);
+                            push @authorised_values, $class_source;
+                            $authorised_lib{$class_source} = $class_sources->{$class_source}->{'description'};
+                        }
                     }
+                    elsif ( $authorised_value eq "categorycode" ) {
+                        my $sth = $dbh->prepare("SELECT categorycode, description FROM categories ORDER BY description");
+                        $sth->execute;
+                        while ( my ( $categorycode, $description ) = $sth->fetchrow_array ) {
+                            push @authorised_values, $categorycode;
+                            $authorised_lib{$categorycode} = $description;
+                        }
+
+                        #---- "true" authorised value
+                    }
+                    else {
+                        my $authorised_values_sth = $dbh->prepare("SELECT authorised_value,lib FROM authorised_values WHERE category=? ORDER BY lib");
 
-                    #---- "true" authorised value
-                }
-                else {
-                    my $authorised_values_sth = $dbh->prepare("SELECT authorised_value,lib FROM authorised_values WHERE category=? ORDER BY lib");
-
-                    $authorised_values_sth->execute( $authorised_value);
+                        $authorised_values_sth->execute( $authorised_value);
 
-                    while ( my ( $value, $lib ) = $authorised_values_sth->fetchrow_array ) {
-                        push @authorised_values, $value;
-                        $authorised_lib{$value} = $lib;
-                        # For item location, we show the code and the libelle
-                        $authorised_lib{$value} = $lib;
+                        while ( my ( $value, $lib ) = $authorised_values_sth->fetchrow_array ) {
+                            push @authorised_values, $value;
+                            $authorised_lib{$value} = $lib;
+                            # For item location, we show the code and the libelle
+                            $authorised_lib{$value} = $lib;
+                        }
                     }
-                }
-                $labelid = $text;
-                $labelid =~ s/\W//g;
-                $input =CGI::scrolling_list(      # FIXME: factor out scrolling_list
-                    -name     => "sql_params",
-                    -id       => "sql_params_".$labelid,
-                    -values   => \@authorised_values,
+                    $labelid = $text;
+                    $labelid =~ s/\W//g;
+                    $input =CGI::scrolling_list(      # FIXME: factor out scrolling_list
+                        -name     => "sql_params",
+                        -id       => "sql_params_".$labelid,
+                        -values   => \@authorised_values,
 #                     -default  => $value,
-                    -labels   => \%authorised_lib,
-                    -override => 1,
-                    -size     => 1,
-                    -multiple => 0,
-                    -tabindex => 1,
-                );
-
-            } else {
-                $input = "text";
+                        -labels   => \%authorised_lib,
+                        -override => 1,
+                        -size     => 1,
+                        -multiple => 0,
+                        -tabindex => 1,
+                    );
+                } else {
+                    $input = "text";
+                }
+                push @tmpl_parameters, {'entry' => $text, 'input' => $input, 'labelid' => $labelid };
             }
-            push @tmpl_parameters, {'entry' => $text, 'input' => $input, 'labelid' => $labelid };
-        }
-        $template->param('sql'         => $sql,
-                        'name'         => $name,
-                        'sql_params'   => \@tmpl_parameters,
-                        'enter_params' => 1,
-                        'reports'      => $report,
-                        );
-    } else {
-        # OK, we have parameters, or there are none, we run the report
-        # if there were parameters, replace before running
-        # split on ??. Each odd (2,4,6,...) entry should be a parameter to fill
-        my @split = split /<<|>>/,$sql;
-        my @tmpl_parameters;
-        for(my $i=0;$i<$#split/2;$i++) {
-            my $quoted = C4::Context->dbh->quote($sql_params[$i]);
-            # if there are special regexp chars, we must \ them
-            $split[$i*2+1] =~ s/(\||\?|\.|\*|\(|\)|\%)/\\$1/g;
-            $sql =~ s/<<$split[$i*2+1]>>/$quoted/;
-        }
-        my ($sth, $errors) = execute_query($sql, $offset, $limit);
-        my $total = nb_rows($sql) || 0;
-        unless ($sth) {
-            die "execute_query failed to return sth for report $report: $sql";
+            $template->param('sql'         => $sql,
+                            'name'         => $name,
+                            'sql_params'   => \@tmpl_parameters,
+                            'enter_params' => 1,
+                            'reports'      => $report_id,
+                            );
         } else {
-            my $headref = $sth->{NAME} || [];
-            my @headers = map { +{ cell => $_ } } @$headref;
-            $template->param(header_row => \@headers);
-            while (my $row = $sth->fetchrow_arrayref()) {
-                my @cells = map { +{ cell => $_ } } @$row;
-                push @rows, { cells => \@cells };
+            # OK, we have parameters, or there are none, we run the report
+            # if there were parameters, replace before running
+            # split on ??. Each odd (2,4,6,...) entry should be a parameter to fill
+            my @split = split /<<|>>/,$sql;
+            my @tmpl_parameters;
+            for(my $i=0;$i<$#split/2;$i++) {
+                my $quoted = C4::Context->dbh->quote($sql_params[$i]);
+                # if there are special regexp chars, we must \ them
+                $split[$i*2+1] =~ s/(\||\?|\.|\*|\(|\)|\%)/\\$1/g;
+                $sql =~ s/<<$split[$i*2+1]>>/$quoted/;
+            }
+            my ($sth, $errors) = execute_query($sql, $offset, $limit);
+            my $total = nb_rows($sql) || 0;
+            unless ($sth) {
+                die "execute_query failed to return sth for report $report_id: $sql";
+            } else {
+                my $headref = $sth->{NAME} || [];
+                my @headers = map { +{ cell => $_ } } @$headref;
+                $template->param(header_row => \@headers);
+                while (my $row = $sth->fetchrow_arrayref()) {
+                    my @cells = map { +{ cell => $_ } } @$row;
+                    push @rows, { cells => \@cells };
+                }
             }
-        }
 
-        my $totpages = int($total/$limit) + (($total % $limit) > 0 ? 1 : 0);
-        my $url = "/cgi-bin/koha/reports/guided_reports.pl?reports=$report&amp;phase=Run%20this%20report";
-        if (@sql_params) {
-            $url = join('&amp;sql_params=', $url, map { URI::Escape::uri_escape($_) } @sql_params);
+            my $totpages = int($total/$limit) + (($total % $limit) > 0 ? 1 : 0);
+            my $url = "/cgi-bin/koha/reports/guided_reports.pl?reports=$report_id&amp;phase=Run%20this%20report";
+            if (@sql_params) {
+                $url = join('&amp;sql_params=', $url, map { URI::Escape::uri_escape($_) } @sql_params);
+            }
+            $template->param(
+                'results' => \@rows,
+                'sql'     => $sql,
+                'id'      => $report_id,
+                'execute' => 1,
+                'name'    => $name,
+                'notes'   => $notes,
+                'errors'  => $errors,
+                'pagination_bar'  => pagination_bar($url, $totpages, $input->param('page')),
+                'unlimited_total' => $total,
+            );
         }
-        $template->param(
-            'results' => \@rows,
-            'sql'     => $sql,
-            'id'      => $report,
-            'execute' => 1,
-            'name'    => $name,
-            'notes'   => $notes,
-            'errors'  => $errors,
-            'pagination_bar'  => pagination_bar($url, $totpages, $input->param('page')),
-            'unlimited_total' => $total,
-        );
+    }
+    else {
+        push @errors, { no_sql_for_id => $report_id };
     }
 }
 
@@ -681,16 +719,26 @@ elsif ($phase eq 'Export'){
     );
 }
 
-elsif ($phase eq 'Create report from SQL') {
-	# allow the user to paste in sql
-    if ($input->param('sql')) {
+elsif ( $phase eq 'Create report from SQL' ) {
+
+    my ($group, $subgroup);
+    # allow the user to paste in sql
+    if ( $input->param('sql') ) {
+        $group = $input->param('report_group');
+        $subgroup  = $input->param('report_subgroup');
         $template->param(
             'sql'           => $input->param('sql'),
             'reportname'    => $input->param('reportname'),
             'notes'         => $input->param('notes'),
         );
     }
-        $template->param('create' => 1, 'public' => '0', 'cache_expiry' => 300, 'usecache' => $usecache);
+    $template->param(
+        'create' => 1,
+        'groups_with_subgroups' => groups_with_subgroups($group, $subgroup),
+        'public' => '0',
+        'cache_expiry' => 300,
+        'usecache' => $usecache,
+    );
 }
 
 elsif ($phase eq 'Create Compound Report'){
@@ -729,3 +777,29 @@ $template->param(   'referer' => $input->referer(),
                 );
 
 output_html_with_http_headers $input, $cookie, $template->output;
+
+sub groups_with_subgroups {
+    my ($group, $subgroup) = @_;
+
+    my $groups_with_subgroups = get_report_groups();
+    my @g_sg;
+    while (my ($g_id, $v) = each %$groups_with_subgroups) {
+        my @subgroups;
+        if (my $sg = $v->{subgroups}) {
+            while (my ($sg_id, $n) = each %$sg) {
+                push @subgroups, {
+                    id => $sg_id,
+                    name => $n,
+                    selected => ($group && $g_id eq $group && $subgroup && $sg_id eq $subgroup ),
+                };
+            }
+        }
+        push @g_sg, {
+            id => $g_id,
+            name => $v->{name},
+            selected => ($group && $g_id eq $group),
+            subgroups => \@subgroups,
+        };
+    }
+    return \@g_sg;
+}
diff --git a/svc/report b/svc/report
index b0507bd..654ef15 100755
--- a/svc/report
+++ b/svc/report
@@ -33,13 +33,6 @@ my $query  = CGI->new();
 my $report_id = $query->param('id');
 my $report_name = $query->param('name');
 
-my $cache;
-my $sql;
-my $type;
-my $notes;
-my $cache_expiry;
-my $public;
-
 my ( $template, $loggedinuser, $cookie ) = get_template_and_user(
     {
         template_name   => "intranet-main.tmpl",
@@ -50,39 +43,31 @@ my ( $template, $loggedinuser, $cookie ) = get_template_and_user(
     }
 );
 
-if (Koha::Cache->is_cache_active) {
-    if ($report_name) { # When retrieving by name, we have to hit the
-                        # database to get the ID before we can check
-                        # the cache. Yuck.
-        ( $sql, $type, $report_name, $notes, $cache_expiry, $public, $report_id ) =
-            get_saved_report( { 'name' => $report_name } );
-    }
-
+my $cache_active = Koha::Cache->is_cache_active;
+my ($cache_key, $cache, $json_text);
+if ($cache_active) {
+    $cache_key = "intranet:report:".($report_name ? "name:$report_name" : "id:$report_id");
     $cache = Koha::Cache->new();
-    my $page = $cache->get_from_cache("intranet:report:$report_id");
-    if ($page) {
-        print $query->header;
-        print $page;
-        exit;
-    }
+    $json_text = $cache->get_from_cache($cache_key);
 }
 
-print $query->header;
-
-# $public isnt used for intranet
-unless ($sql) {
-    ( $sql, $type, $report_name, $notes, $cache_expiry, $public, $report_id ) =
-        get_saved_report($report_name ? { 'name' => $report_name } : { 'id' => $report_id } );
-}
-if ($sql) {
+unless ($json_text) {
+    my $report_rec = get_saved_report($report_name ? { 'name' => $report_name } : { 'id' => $report_id });
     my $offset = 0;
     my $limit  = C4::Context->preference("SvcMaxReportRows") || 10;
-    my ( $sth, $errors ) = execute_query( $sql, $offset, $limit );
-    my $lines     = $sth->fetchall_arrayref;
-    my $json_text = to_json($lines);
-    print $json_text;
+    my ( $sth, $errors ) = execute_query( $report_rec->{savedsql}, $offset, $limit );
+    if ($sth) {
+        my $lines     = $sth->fetchall_arrayref;
+        $json_text = to_json($lines);
 
-    if (Koha::Cache->is_cache_active) {
-        $cache->set_in_cache( "intranet:report:$report_id", $json_text, $cache_expiry );
+        if ($cache_active) {
+            $cache->set_in_cache( $cache_key, $json_text, $report_rec->{cache_expiry} );
+        }
+    }
+    else {
+        $json_text = to_json($errors);
     }
 }
+
+print $query->header;
+print $json_text;
-- 
1.7.9.5