Bugzilla – Attachment 177146 Details for
Bug 38838
optgroup construct needs cleaning in the reports module
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 38838: Clean C4::Reports::Guided->get_columns
Bug-38838-Clean-C4ReportsGuided-getcolumns.patch (text/plain), 11.27 KB, created by
David Nind
on 2025-01-25 21:00:09 UTC
(
hide
)
Description:
Bug 38838: Clean C4::Reports::Guided->get_columns
Filename:
MIME Type:
Creator:
David Nind
Created:
2025-01-25 21:00:09 UTC
Size:
11.27 KB
patch
obsolete
>From 16c523369556b245c551cb0e496352d583804c96 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >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 <david@davidnind.com> >--- > 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 @@ > <input type="hidden" name="definition_name" value="[% definition_name | html %]" /> > <input type="hidden" name="definition_description" value="[% definition_description | html %]" /> > >-<select id="availableColumns" name="columns" size="25" style="width:200px;height:300px;margin:1em;"> >-[% FOREACH column IN columns %] >-[% IF ( column.table ) %] >-[% IF ( loop.first ) %] >-[% ELSE %] >-</optgroup> >-[% END %] >- >-<optgroup label="[% column.table | html %]"> >-[% ELSE %] >-<option value="[% column.name | html %]"> >-[% IF ( column.description ) %][% column.description | html %] >-[% ELSE %] >-[% column.name | html %] >-[% END %] >-</option> >-[% END %] >-[% END %] >-</optgroup> >+<select id="availableColumns" name="columns" size="25" style="min-width:200px; height:300px; margin:1em;"> >+ [% FOREACH table IN columns.keys.sort %] >+ <optgroup label="[% table | html %]"> >+ [% FOREACH column IN columns.item(table) %] >+ <option value="[% column.name | html %]"> >+ [% IF ( column.description ) %] >+ [% column.description | html %] / [% column.name | html %] >+ [% ELSE %] >+ [% column.name | html %] >+ [% END %] >+ </option> >+ [% END %] >+ </optgroup> >+ [% END %] > </select> > > <input type="hidden" name="op" value="cud-add_form_4" /> >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 @@ > <div class="row"> > <div class="col-sm-6"> > <div style="float: left;"> >- <select id="availableColumns" name="oldcolumns2" multiple="multiple" size="25" style="min-width: 200px;height:300px;"> >- [% FOREACH column IN columns %] >- [% IF ( column.table ) %] >- [% IF ( loop.first ) %] >- [% ELSE %] >- </optgroup> >+ <select id="availableColumns" name="oldcolumns2" multiple="multiple" size="25" style="min-width:200px; height:300px;"> >+ [% FOREACH table IN columns.keys.sort %] >+ <optgroup label="[% table | html %]"> >+ [% FOREACH column IN columns.item(table) %] >+ <option value="[% column.name | html %]"> >+ [% IF ( column.description ) %] >+ [% column.description | html %] / [% column.name | html %] >+ [% ELSE %] >+ [% column.name | html %] >+ [% END %] >+ </option> > [% END %] >- <optgroup label="[% column.table | html %]"> >- [% ELSE %] >- <option value="[% column.name | html %]"> >- [% IF ( column.description ) %] >- [% column.description | html %] / [% column.name | html %] >- [% ELSE %] >- [% column.name | html %] >- [% END %] >- </option> >- [% END %] >+ </optgroup> > [% END %] >- </optgroup> > </select> > </div> > <div style="width: 6.3em; float: right; margin-top: 100px"> >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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 38838
:
176170
| 177146