From ad1d734e548809ab7534545a1b2c7c936ae542eb Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Fri, 27 Aug 2021 12:39:49 +0000 Subject: [PATCH] Bug 16446: Add ability to add patrons to list by borrowernumber AddPatronsToList already took a borrowernumber parameter, however, it did not check if those were valid numbers. This patch expands the search to apply to cardnumber or borrowernumbers in the subroutine. Template and script are adjusted to allow choosing borrowernumbers or cardnumbers To test: 1 - Apply patch 2 - Browse to Tools->Patron lists 3 - Create a list, or choose Actions->Add patrons for an existing list 4 - Click 'Add multiple patrons' 5 - Cardnumbers is preselected 6 - Enter a list of cardnumbers, ensure you test: Cardnumber already in list Cardnumber not in list Non-existent cardnumber 7 - Patrons should be added/errors reported correctly 8 - Repeat with borowernumbers Signed-off-by: Andrew Fuerste-Henry Signed-off-by: Katrin Fischer Signed-off-by: Jonathan Druart --- Koha/List/Patron.pm | 18 +++---- .../prog/en/modules/patron_lists/list.tt | 47 ++++++++++++------- patron_lists/list.pl | 25 ++++++---- t/db_dependent/PatronLists.t | 9 +++- 4 files changed, 63 insertions(+), 36 deletions(-) diff --git a/Koha/List/Patron.pm b/Koha/List/Patron.pm index 7ff95a13e0..cb91275d85 100644 --- a/Koha/List/Patron.pm +++ b/Koha/List/Patron.pm @@ -173,17 +173,19 @@ sub AddPatronsToList { my @borrowernumbers; + my %search_param; if ($cardnumbers) { - @borrowernumbers = - Koha::Database->new()->schema()->resultset('Borrower')->search( - { cardnumber => { 'IN' => $cardnumbers } }, - { columns => [qw/ borrowernumber /] } - )->get_column('borrowernumber')->all(); - } - else { - @borrowernumbers = @$borrowernumbers; + $search_param{cardnumber} = { 'IN' => $cardnumbers }; + } else { + $search_param{borrowernumber} = { 'IN' => $borrowernumbers }; } + @borrowernumbers = + Koha::Database->new()->schema()->resultset('Borrower')->search( + \%search_param, + { columns => [qw/ borrowernumber /] } + )->get_column('borrowernumber')->all(); + my $patron_list_id = $list->patron_list_id(); my $plp_rs = Koha::Database->new()->schema()->resultset('PatronListPatron'); diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/list.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/list.tt index a9f74cf211..d06a9726ff 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/list.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/list.tt @@ -12,10 +12,10 @@ #add_patrons_by_search { display: none; } - #patron_barcodes_line { + #patron_ids_line { display: none; } - #patron_barcodes_submit { + #patron_ids_submit { display: none; } #searchheader { @@ -58,10 +58,10 @@

[% list.name | html %]

[% IF ( not_found.size > 0 ) %] -

Warning, the following cardnumbers were not found:

+

Warning, the following [% id_column | html %] were not found:

- + [% FOREACH nf IN not_found %] @@ -72,10 +72,10 @@ [% END %] [% IF ( existed.size > 0 ) %] -

Warning, the following cardnumbers were already in this list:

+

Warning, the following [% id_column | html %] were already in this list:

Cardnumbers not found
[% id_column | html FILTER ucfirst %] not found
- + [% FOREACH ed IN existed %] @@ -96,16 +96,29 @@ -
  • +
  •   - Enter multiple card numbers
    • -
  • - - + Add multiple patrons
    • +
  • +
    + Choose type of ID to enter + + + + +
    + +
    • -
    +
    @@ -247,14 +260,14 @@ } }); - $("#add_patrons_by_barcode a").on("click", function(){ - $("#add_patrons_by_barcode, #patron_search_line").hide(); - $("#add_patrons_by_search, #patron_barcodes_line, #patron_barcodes_submit").show(); + $("#add_patrons_by_id a").on("click", function(){ + $("#add_patrons_by_id, #patron_search_line").hide(); + $("#add_patrons_by_search, #patron_ids_line, #patron_ids_submit").show(); }); $("#add_patrons_by_search a").on("click", function(){ - $("#add_patrons_by_barcode, #patron_search_line").show(); - $("#add_patrons_by_search, #patron_barcodes_line, #patron_barcodes_submit").hide(); + $("#add_patrons_by_id, #patron_search_line").show(); + $("#add_patrons_by_search, #patron_ids_line, #patron_ids_submit").hide(); }); $('.merge-patrons').on('click', function() { diff --git a/patron_lists/list.pl b/patron_lists/list.pl index 7076b20c9f..45b11a1667 100755 --- a/patron_lists/list.pl +++ b/patron_lists/list.pl @@ -46,22 +46,27 @@ my ($list) = my @existing = $list->patron_list_patrons; -my $cardnumbers = $cgi->param('patrons_by_barcode'); -my @patrons_by_barcode; +my $patrons_by_id = $cgi->param('patrons_by_id'); +my $id_column = $cgi->param('id_column'); -if ( $cardnumbers ){ - push my @patrons_by_barcode, uniq( split(/\s\n/, $cardnumbers) ); - my @results = AddPatronsToList( { list => $list, cardnumbers => \@patrons_by_barcode } ); - my %found = map { $_->borrowernumber->cardnumber => 1 } @results; - my %exist = map { $_->borrowernumber->cardnumber => 1 } @existing; +if ( $patrons_by_id ){ + push my @patrons_list, uniq( split(/\s\n/, $patrons_by_id) ); + my %add_params; + $add_params{list} = $list; + $add_params{ $id_column } = \@patrons_list; + my @results = AddPatronsToList(\%add_params); + my $id = $id_column eq 'borrowernumbers' ? 'borrowernumber' : 'cardnumber'; + my %found = map { $_->borrowernumber->$id => 1 } @results; + my %exist = map { $_->borrowernumber->$id => 1 } @existing; my (@not_found, @existed); - foreach my $barcode ( @patrons_by_barcode ){ - push (@not_found, $barcode) unless defined $found{$barcode}; - push (@existed, $barcode) if defined $exist{$barcode}; + foreach my $patron ( @patrons_list ){ + push (@not_found, $patron) unless defined $found{$patron}; + push (@existed, $patron) if defined $exist{$patron}; } $template->param( not_found => \@not_found, existed => \@existed, + id_column => $id_column, ); } diff --git a/t/db_dependent/PatronLists.t b/t/db_dependent/PatronLists.t index 6e4f5f8efa..82bab67c85 100755 --- a/t/db_dependent/PatronLists.t +++ b/t/db_dependent/PatronLists.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 9; +use Test::More tests => 11; use t::lib::TestBuilder; use t::lib::Mocks; @@ -80,6 +80,13 @@ is( 'AddPatronsToList works for borrowernumbers' ); +my $deleted_patron = $builder->build_object({ class => 'Koha::Patrons' }); +$deleted_patron->delete; +my @result = AddPatronsToList({list => $list2,borrowernumbers => [ $deleted_patron->borrowernumber ]}); +is( scalar @result, 0,'Invalid borrowernumber not added'); +@result = AddPatronsToList({list => $list2,cardnumbers => [ $deleted_patron->cardnumber ]}); +is( scalar @result, 0,'Invalid cardnumber not added'); + my @ids = $list1->patron_list_patrons()->get_column('patron_list_patron_id')->all(); DelPatronsFromList( -- 2.39.5
    Cardnumbers already in list
    [% id_column | html FILTER ucfirst %] already in list