From 16c523369556b245c551cb0e496352d583804c96 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Tue, 7 Jan 2025 10:23:22 +0100 Subject: [PATCH] Bug 38838: Clean C4::Reports::Guided->get_columns reports/dictionary.tt and reports/guided_reports_start.tt display a list of the columns available using optgroup to group the columns per table. The template code is quite ugly because of the perl structure choose in C4::Reports::Guided->get_columns This patch removes $cgi and the 'first' variable that were not used. Now we have an usual structure with the table names as keys and column info in an arrayref. Test plan: Create a report and a new dictionary definition. At step 3 you should see the list of the columns. Note that they are now identical. The list when you create a dictionary now show the description of the columns. Signed-off-by: David Nind --- C4/Reports/Guided.pm | 57 +++++++++---------- .../prog/en/modules/reports/dictionary.tt | 33 +++++------ .../modules/reports/guided_reports_start.tt | 29 ++++------ reports/dictionary.pl | 3 +- reports/guided_reports.pl | 2 +- t/db_dependent/Reports/Guided.t | 20 ++++++- 6 files changed, 73 insertions(+), 71 deletions(-) diff --git a/C4/Reports/Guided.pm b/C4/Reports/Guided.pm index 1f1dd80ca2..938eacd65b 100644 --- a/C4/Reports/Guided.pm +++ b/C4/Reports/Guided.pm @@ -191,42 +191,37 @@ This will return a list of all columns for a report area =cut sub get_columns { - - # this calls the internal function _get_columns - my ( $area, $cgi ) = @_; + my ($area) = @_; my %table_areas = get_table_areas; - my $tables = $table_areas{$area} - or die qq{Unsuported report area "$area"}; - - my @allcolumns; - my $first = 1; - foreach my $table (@$tables) { - my @columns = _get_columns($table,$cgi, $first); - $first = 0; - push @allcolumns, @columns; + my $tables = $table_areas{$area} + or die qq{Unsupported report area "$area"}; + + my $columns; + for my $table (@$tables) { + $columns->{$table} = _get_columns($table); } - return ( \@allcolumns ); + return $columns; } sub _get_columns { - my ($tablename,$cgi, $first) = @_; - my $dbh = C4::Context->dbh(); - my $sth = $dbh->prepare("show columns from $tablename"); - $sth->execute(); - my @columns; - my $columns = Koha::Database::Columns->columns; - my %tablehash; - $tablehash{'table'}=$tablename; - $tablehash{'__first__'} = $first; - push @columns, \%tablehash; - while ( my $data = $sth->fetchrow_arrayref() ) { - my %temphash; - $temphash{'name'} = "$tablename.$data->[0]"; - $temphash{'description'} = $columns->{$tablename}->{$data->[0]}; - push @columns, \%temphash; - } - $sth->finish(); - return (@columns); + my ($table_name) = @_; + my $dbh = C4::Context->dbh(); + my $all_columns = Koha::Database::Columns->columns; + + # Sanitize input, just in case + die sprintf( 'Invalid table name %s', $table_name ) unless exists $all_columns->{$table_name}; + + my $columns = $dbh->selectall_arrayref( sprintf( q{SHOW COLUMNS FROM %s}, $table_name ), { Slice => {} } ); + return [ + map { + my $column_name = $_->{Field}; + { + name => sprintf( "%s.%s", $table_name, $column_name ), + description => $all_columns->{$table_name}->{$column_name}, + }; + + } @$columns + ]; } =head2 build_query($columns,$criteria,$orderby,$area) 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 660ec26ad9..aff052b5cc 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/reports/dictionary.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reports/dictionary.tt @@ -191,25 +191,20 @@ - + [% FOREACH table IN columns.keys.sort %] + + [% FOREACH column IN columns.item(table) %] + + [% END %] + + [% 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 cdba5aad88..fe6834791c 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 @@ -546,25 +546,20 @@
- + [% FOREACH table IN columns.keys.sort %] + + [% FOREACH column IN columns.item(table) %] + [% END %] - - [% ELSE %] - - [% END %] + [% END %] -
diff --git a/reports/dictionary.pl b/reports/dictionary.pl index 123dab5340..c187282088 100755 --- a/reports/dictionary.pl +++ b/reports/dictionary.pl @@ -67,11 +67,10 @@ elsif ( $op eq 'cud-add_form_2' ) { elsif ( $op eq 'cud-add_form_3' ) { # Choosing the columns - my $columns = get_columns( $area, $input ); $template->param( 'step_3' => 1, 'area' => $area, - 'columns' => $columns, + 'columns' => get_columns($area), 'definition_name' => $definition_name, 'definition_description' => $definition_description, ); diff --git a/reports/guided_reports.pl b/reports/guided_reports.pl index 27f82e9b7a..2c9c176c65 100755 --- a/reports/guided_reports.pl +++ b/reports/guided_reports.pl @@ -313,7 +313,7 @@ elsif ( $op eq 'cud-choose_type' ) { 'build3' => 1, 'area' => $area, 'type' => $type, - columns => get_columns($area,$input), + columns => get_columns($area), 'cache_expiry' => scalar $input->param('cache_expiry'), 'public' => scalar $input->param('public'), ); diff --git a/t/db_dependent/Reports/Guided.t b/t/db_dependent/Reports/Guided.t index e84ccc2e00..d139940588 100755 --- a/t/db_dependent/Reports/Guided.t +++ b/t/db_dependent/Reports/Guided.t @@ -20,7 +20,7 @@ use Modern::Perl; -use Test::More tests => 12; +use Test::More tests => 13; use Test::Warn; use t::lib::TestBuilder; @@ -535,6 +535,24 @@ subtest 'Returning passwords tests' => sub { $schema->storage->txn_rollback; }; +subtest 'get_columns' => sub { + plan tests => 7; + $schema->storage->txn_begin; + my $area = 'foo'; + my $success = eval { C4::Reports::Guided::get_columns($area) }; + ok !$success && $@ =~ m{^Unsupported report area "$area"}, 'Call with wrong report area explodes'; + + my $columns = C4::Reports::Guided::get_columns('CAT'); + ok exists $columns->{biblio}; + ok exists $columns->{biblioitems}; + ok exists $columns->{items}; + ok scalar grep { $_->{name} eq 'biblio.biblionumber' } @{ $columns->{biblio} }; + ok scalar grep { $_->{description} =~ m{Biblio number} } @{ $columns->{biblio} }; + ok !scalar grep { $_->{name} eq 'not_a_column' } @{ $columns->{biblio} }; + + $schema->storage->txn_rollback; +}; + sub trim { my ($s) = @_; $s =~ s/^\s*(.*?)\s*$/$1/s; -- 2.39.5