Bugzilla – Attachment 170931 Details for
Bug 23426
Empty AV field returned in 'patron info' in addition to those requested
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 23426: (QA follow-up) Tidy
Bug-23426-follow-up-Tidy.patch (text/plain), 22.43 KB, created by
Jan Kissig
on 2024-09-02 07:43:37 UTC
(
hide
)
Description:
Bug 23426: (QA follow-up) Tidy
Filename:
MIME Type:
Creator:
Jan Kissig
Created:
2024-09-02 07:43:37 UTC
Size:
22.43 KB
patch
obsolete
>From 40c3f183d5ddb9ab49ed9b593a7989ae8c4b621b Mon Sep 17 00:00:00 2001 >From: Jan Kissig <jkissig@th-wildau.de> >Date: Mon, 2 Sep 2024 09:12:10 +0200 >Subject: [PATCH] Bug 23426: (follow-up) Tidy > >--- > C4/SIP/ILS/Patron.pm | 325 ++++++++++++++++++++++++------------------ > C4/SIP/Sip/MsgType.pm | 29 +++- > 2 files changed, 209 insertions(+), 145 deletions(-) > >diff --git a/C4/SIP/ILS/Patron.pm b/C4/SIP/ILS/Patron.pm >index 1483f0ebf9..6c7651d824 100644 >--- a/C4/SIP/ILS/Patron.pm >+++ b/C4/SIP/ILS/Patron.pm >@@ -39,7 +39,7 @@ our $kp; # koha patron > =cut > > sub new { >- my ($class, $patron_id) = @_; >+ my ( $class, $patron_id ) = @_; > my $type = ref($class) || $class; > my $self; > >@@ -47,31 +47,41 @@ sub new { > if ( ref $patron_id eq "HASH" ) { > if ( $patron_id->{borrowernumber} ) { > $patron = Koha::Patrons->find( $patron_id->{borrowernumber} ); >- } elsif ( $patron_id->{cardnumber} ) { >- $patron = Koha::Patrons->find( { cardnumber => $patron_id->{cardnumber} } ); >- } elsif ( $patron_id->{userid} ) { >+ } >+ elsif ( $patron_id->{cardnumber} ) { >+ $patron = >+ Koha::Patrons->find( { cardnumber => $patron_id->{cardnumber} } ); >+ } >+ elsif ( $patron_id->{userid} ) { > $patron = Koha::Patrons->find( { userid => $patron_id->{userid} } ); > } >- } else { >+ } >+ else { > $patron = Koha::Patrons->find( { cardnumber => $patron_id } ) >- || Koha::Patrons->find( { userid => $patron_id } ); >+ || Koha::Patrons->find( { userid => $patron_id } ); > } > > unless ($patron) { >- siplog("LOG_DEBUG", "new ILS::Patron(%s): no such patron", $patron_id); >+ siplog( "LOG_DEBUG", "new ILS::Patron(%s): no such patron", >+ $patron_id ); > return; > } > $kp = $patron->unblessed; >- my $pw = $kp->{password}; >- my $flags = C4::Members::patronflags( $kp ); >- my $debarred = $patron->is_debarred; >- siplog( "LOG_DEBUG", "Debarred = %s : ", ( $debarred || 'undef' ) ); # Do we need more debug info here? >+ my $pw = $kp->{password}; >+ my $flags = C4::Members::patronflags($kp); >+ my $debarred = $patron->is_debarred; >+ siplog( "LOG_DEBUG", "Debarred = %s : ", ( $debarred || 'undef' ) ) >+ ; # Do we need more debug info here? > my $expired = 0; >- if ( $kp->{'dateexpiry'} && C4::Context->preference('NotifyBorrowerDeparture') ) { >+ if ( $kp->{'dateexpiry'} >+ && C4::Context->preference('NotifyBorrowerDeparture') ) >+ { > my ( $today_year, $today_month, $today_day ) = Today(); >- my ( $warning_year, $warning_month, $warning_day ) = split /-/, $kp->{'dateexpiry'}; >- my $days_to_expiry = Date_to_Days( $warning_year, $warning_month, $warning_day ) - >- Date_to_Days( $today_year, $today_month, $today_day ); >+ my ( $warning_year, $warning_month, $warning_day ) = split /-/, >+ $kp->{'dateexpiry'}; >+ my $days_to_expiry = >+ Date_to_Days( $warning_year, $warning_month, $warning_day ) - >+ Date_to_Days( $today_year, $today_month, $today_day ); > my $dt = dt_from_string( $kp->{'dateexpiry'}, 'iso' ); > my $dateexpiry = output_pref( { dt => $dt, dateonly => 1 } ); > if ( $days_to_expiry < 0 ) { >@@ -82,7 +92,10 @@ sub new { > } > $kp->{opacnote} .= "Your account has expired as of $dateexpiry"; > $expired = 1; >- } elsif ( $days_to_expiry < C4::Context->preference('NotifyBorrowerDeparture') ) { >+ } >+ elsif ( $days_to_expiry < >+ C4::Context->preference('NotifyBorrowerDeparture') ) >+ { > > # borrower card soon to expire, warn the borrower > if ( $kp->{opacnote} ) { >@@ -92,104 +105,127 @@ sub new { > } > } > my %ilspatron; >- my $adr = _get_address($kp); >- my $dob = $kp->{dateofbirth}; >- $dob and $dob =~ s/-//g; # YYYYMMDD >- my $dexpiry = $kp->{dateexpiry}; >+ my $adr = _get_address($kp); >+ my $dob = $kp->{dateofbirth}; >+ $dob and $dob =~ s/-//g; # YYYYMMDD >+ my $dexpiry = $kp->{dateexpiry}; > $dexpiry and $dexpiry =~ s/-//g; # YYYYMMDD > >- # Get fines and add fines for guarantees (depends on preference NoIssuesChargeGuarantees) >- my $patron_charge_limits = $patron->is_patron_inside_charge_limits(); >- my $fines_amount = $patron_charge_limits->{noissuescharge}->{charge}; >+# Get fines and add fines for guarantees (depends on preference NoIssuesChargeGuarantees) >+ my $patron_charge_limits = $patron->is_patron_inside_charge_limits(); >+ my $fines_amount = $patron_charge_limits->{noissuescharge}->{charge}; > my $personal_fines_amount = $fines_amount; >- my $fee_limit = $patron_charge_limits->{noissuescharge}->{limit} || 5; >+ my $fee_limit = $patron_charge_limits->{noissuescharge}->{limit} || 5; > my $noissueschargeguarantorswithguarantees = >- $patron_charge_limits->{NoIssuesChargeGuarantorsWithGuarantees}->{limit}; >- my $noissueschargeguarantees = $patron_charge_limits->{NoIssuesChargeGuarantees}->{limit}; >+ $patron_charge_limits->{NoIssuesChargeGuarantorsWithGuarantees}->{limit}; >+ my $noissueschargeguarantees = >+ $patron_charge_limits->{NoIssuesChargeGuarantees}->{limit}; > >- my $fines_msg = ""; >+ my $fines_msg = ""; > my $fine_blocked = 0; > if ( $patron_charge_limits->{noissuescharge}->{overlimit} ) { > $fine_blocked = 1; > $fines_msg .= " -- " . "Patron blocked by fines" if $fine_blocked; >- } elsif ($noissueschargeguarantorswithguarantees) { >- $fines_amount = $patron_charge_limits->{NoIssuesChargeGuarantorsWithGuarantees}->{charge}; >- $fine_blocked = $patron_charge_limits->{NoIssuesChargeGuarantorsWithGuarantees}->{overlimit}; >- $fines_msg .= " -- " . "Patron blocked by fines ($fines_amount) on related accounts" if $fine_blocked; >- } elsif ($noissueschargeguarantees) { >+ } >+ elsif ($noissueschargeguarantorswithguarantees) { >+ $fines_amount = >+ $patron_charge_limits->{NoIssuesChargeGuarantorsWithGuarantees} >+ ->{charge}; >+ $fine_blocked = >+ $patron_charge_limits->{NoIssuesChargeGuarantorsWithGuarantees} >+ ->{overlimit}; >+ $fines_msg .= " -- " >+ . "Patron blocked by fines ($fines_amount) on related accounts" >+ if $fine_blocked; >+ } >+ elsif ($noissueschargeguarantees) { > if ( $patron->guarantee_relationships->count ) { >- $fines_amount += $patron_charge_limits->{NoIssuesChargeGuarantees}->{charge}; >- $fine_blocked = $patron_charge_limits->{NoIssuesChargeGuarantees}->{overlimit}; >- $fines_msg .= " -- " . "Patron blocked by fines ($fines_amount) on guaranteed accounts" if $fine_blocked; >+ $fines_amount += >+ $patron_charge_limits->{NoIssuesChargeGuarantees}->{charge}; >+ $fine_blocked = >+ $patron_charge_limits->{NoIssuesChargeGuarantees}->{overlimit}; >+ $fines_msg .= " -- " >+ . "Patron blocked by fines ($fines_amount) on guaranteed accounts" >+ if $fine_blocked; > } > } > > # Get currency 3 chars max >- my $currency = substr Koha::Acquisition::Currencies->get_active->currency, 0, 3; >+ my $currency = substr Koha::Acquisition::Currencies->get_active->currency, >+ 0, 3; > >- my $circ_blocked =( C4::Context->preference('OverduesBlockCirc') ne "noblock" && defined $flags->{ODUES}->{itemlist} ) ? 1 : 0; >+ my $circ_blocked = >+ ( C4::Context->preference('OverduesBlockCirc') ne "noblock" >+ && defined $flags->{ODUES}->{itemlist} ) ? 1 : 0; > { >- no warnings; # any of these $kp->{fields} being concat'd could be undef >- %ilspatron = ( >- name => $kp->{firstname} . " " . $kp->{surname}, >- id => $kp->{cardnumber}, # to SIP, the id is the BARCODE, not userid >- password => $pw, >- ptype => $kp->{categorycode}, # 'A'dult. Whatever. >- dateexpiry => $dexpiry, >- dateexpiry_iso => $kp->{dateexpiry}, >- birthdate => $dob, >- birthdate_iso => $kp->{dateofbirth}, >- branchcode => $kp->{branchcode}, >- library_name => "", # only populated if needed, cached here >- borrowernumber => $kp->{borrowernumber}, >- address => $adr, >- home_phone => $kp->{phone}, >- email_addr => $kp->{email}, >- charge_ok => ( !$debarred && !$expired && !$fine_blocked && !$circ_blocked), >- renew_ok => ( !$debarred && !$expired && !$fine_blocked), >- recall_ok => ( !$debarred && !$expired && !$fine_blocked), >- hold_ok => ( !$debarred && !$expired && !$fine_blocked), >- card_lost => ( $kp->{lost} || $kp->{gonenoaddress} || $flags->{LOST} ), >- claims_returned => 0, >- fines => $personal_fines_amount, >- fees => 0, # currently not distinct from fines >- recall_overdue => 0, >- items_billed => 0, >- screen_msg => 'Greetings from Koha. ' . $kp->{opacnote} . $fines_msg, >- print_line => '', >- items => [], >- hold_items => $flags->{WAITING}->{itemlist}, >- overdue_items => $flags->{ODUES}->{itemlist}, >- too_many_overdue => $circ_blocked, >- fine_items => [], >- recall_items => [], >- unavail_holds => [], >- inet => ( !$debarred && !$expired ), >- debarred => $debarred, >- expired => $expired, >- fine_blocked => $fine_blocked, >- fee_limit => $fee_limit, >- userid => $kp->{userid}, >- currency => $currency, >- ); >+ no warnings; # any of these $kp->{fields} being concat'd could be undef >+ %ilspatron = ( >+ name => $kp->{firstname} . " " . $kp->{surname}, >+ id => $kp->{cardnumber}, # to SIP, the id is the BARCODE, not userid >+ password => $pw, >+ ptype => $kp->{categorycode}, # 'A'dult. Whatever. >+ dateexpiry => $dexpiry, >+ dateexpiry_iso => $kp->{dateexpiry}, >+ birthdate => $dob, >+ birthdate_iso => $kp->{dateofbirth}, >+ branchcode => $kp->{branchcode}, >+ library_name => "", # only populated if needed, cached here >+ borrowernumber => $kp->{borrowernumber}, >+ address => $adr, >+ home_phone => $kp->{phone}, >+ email_addr => $kp->{email}, >+ charge_ok => ( >+ !$debarred && !$expired && !$fine_blocked && !$circ_blocked >+ ), >+ renew_ok => ( !$debarred && !$expired && !$fine_blocked ), >+ recall_ok => ( !$debarred && !$expired && !$fine_blocked ), >+ hold_ok => ( !$debarred && !$expired && !$fine_blocked ), >+ card_lost => >+ ( $kp->{lost} || $kp->{gonenoaddress} || $flags->{LOST} ), >+ claims_returned => 0, >+ fines => $personal_fines_amount, >+ fees => 0, # currently not distinct from fines >+ recall_overdue => 0, >+ items_billed => 0, >+ screen_msg => 'Greetings from Koha. ' >+ . $kp->{opacnote} >+ . $fines_msg, >+ print_line => '', >+ items => [], >+ hold_items => $flags->{WAITING}->{itemlist}, >+ overdue_items => $flags->{ODUES}->{itemlist}, >+ too_many_overdue => $circ_blocked, >+ fine_items => [], >+ recall_items => [], >+ unavail_holds => [], >+ inet => ( !$debarred && !$expired ), >+ debarred => $debarred, >+ expired => $expired, >+ fine_blocked => $fine_blocked, >+ fee_limit => $fee_limit, >+ userid => $kp->{userid}, >+ currency => $currency, >+ ); > } > > if ( $patron->is_debarred and $patron->debarredcomment ) { > $ilspatron{screen_msg} .= " -- " . $patron->debarredcomment; > } > >- if ( $circ_blocked ) { >+ if ($circ_blocked) { > $ilspatron{screen_msg} .= " -- " . "Patron has overdues"; > } > > for (qw(EXPIRED CHARGES CREDITS GNA LOST NOTES)) { >- ($flags->{$_}) or next; >- if ($_ ne 'NOTES' and $flags->{$_}->{message}) { >- $ilspatron{screen_msg} .= " -- " . $flags->{$_}->{message}; # show all but internal NOTES >+ ( $flags->{$_} ) or next; >+ if ( $_ ne 'NOTES' and $flags->{$_}->{message} ) { >+ $ilspatron{screen_msg} .= >+ " -- " . $flags->{$_}->{message}; # show all but internal NOTES > } >- if ($flags->{$_}->{noissues}) { >+ if ( $flags->{$_}->{noissues} ) { > foreach my $toggle (qw(charge_ok renew_ok recall_ok hold_ok inet)) { >- $ilspatron{$toggle} = 0; # if we get noissues, disable everything >+ $ilspatron{$toggle} = >+ 0; # if we get noissues, disable everything > } > } > } >@@ -203,25 +239,31 @@ sub new { > ); > my @messages_array; > while ( my $message = $patron_messages->next ) { >- my $messagedt = dt_from_string( $message->message_date, 'iso' ); >- my $formatted_date = output_pref( { dt => $messagedt, dateonly => 1 } ); >+ my $messagedt = dt_from_string( $message->message_date, 'iso' ); >+ my $formatted_date = >+ output_pref( { dt => $messagedt, dateonly => 1 } ); > push @messages_array, $formatted_date . ": " . $message->message; > } > if (@messages_array) { >- $ilspatron{screen_msg} .= " Messages for you: " . join( ' / ', @messages_array ); >+ $ilspatron{screen_msg} .= >+ " Messages for you: " . join( ' / ', @messages_array ); > } > } > > # FIXME: populate recall_items >- $ilspatron{unavail_holds} = _get_outstanding_holds($kp->{borrowernumber}); >+ $ilspatron{unavail_holds} = _get_outstanding_holds( $kp->{borrowernumber} ); > > # fine items > my $debits = $patron->account->outstanding_debits; > my @fine_items; > while ( my $debit = $debits->next ) { >- push @fine_items, { account_line => sprintf '%s %.02f', $debit->description, $debit->amountoutstanding }; >+ push @fine_items, >+ { >+ account_line => sprintf '%s %.02f', >+ $debit->description, $debit->amountoutstanding >+ }; > } >- $ilspatron{fine_items} =\@fine_items; >+ $ilspatron{fine_items} = \@fine_items; > > my $pending_checkouts = $patron->pending_checkouts; > my @barcodes; >@@ -231,48 +273,49 @@ sub new { > $ilspatron{items} = \@barcodes; > > $self = \%ilspatron; >- siplog("LOG_DEBUG", "new ILS::Patron(%s): found patron '%s'", $patron_id,$self->{id}); >+ siplog( "LOG_DEBUG", "new ILS::Patron(%s): found patron '%s'", >+ $patron_id, $self->{id} ); > bless $self, $type; > return $self; > } > >- > # 0 means read-only > # 1 means read/write > > my %fields = ( >- id => 0, >- borrowernumber => 0, >- name => 0, >- address => 0, >- email_addr => 0, >- home_phone => 0, >- birthdate => 0, >- birthdate_iso => 0, >- dateexpiry => 0, >- dateexpiry_iso => 0, >- debarred => 0, >- fine_blocked => 0, >- ptype => 0, >- charge_ok => 0, # for patron_status[0] (inverted) >- renew_ok => 0, # for patron_status[1] (inverted) >- recall_ok => 0, # for patron_status[2] (inverted) >- hold_ok => 0, # for patron_status[3] (inverted) >- card_lost => 0, # for patron_status[4] >- recall_overdue => 0, >- currency => 1, >- fee_limit => 0, >- screen_msg => 1, >- print_line => 1, >- too_many_charged => 0, # for patron_status[5] >- too_many_overdue => 0, # for patron_status[6] >- too_many_renewal => 0, # for patron_status[7] >- too_many_claim_return => 0, # for patron_status[8] >-# excessive_fines => 0, # for patron_status[10] >-# excessive_fees => 0, # for patron_status[11] >- recall_overdue => 0, # for patron_status[12] >- too_many_billed => 0, # for patron_status[13] >- inet => 0, # EnvisionWare extension >+ id => 0, >+ borrowernumber => 0, >+ name => 0, >+ address => 0, >+ email_addr => 0, >+ home_phone => 0, >+ birthdate => 0, >+ birthdate_iso => 0, >+ dateexpiry => 0, >+ dateexpiry_iso => 0, >+ debarred => 0, >+ fine_blocked => 0, >+ ptype => 0, >+ charge_ok => 0, # for patron_status[0] (inverted) >+ renew_ok => 0, # for patron_status[1] (inverted) >+ recall_ok => 0, # for patron_status[2] (inverted) >+ hold_ok => 0, # for patron_status[3] (inverted) >+ card_lost => 0, # for patron_status[4] >+ recall_overdue => 0, >+ currency => 1, >+ fee_limit => 0, >+ screen_msg => 1, >+ print_line => 1, >+ too_many_charged => 0, # for patron_status[5] >+ too_many_overdue => 0, # for patron_status[6] >+ too_many_renewal => 0, # for patron_status[7] >+ too_many_claim_return => 0, # for patron_status[8] >+ >+ # excessive_fines => 0, # for patron_status[10] >+ # excessive_fees => 0, # for patron_status[11] >+ recall_overdue => 0, # for patron_status[12] >+ too_many_billed => 0, # for patron_status[13] >+ inet => 0, # EnvisionWare extension > ); > > our $AUTOLOAD; >@@ -401,30 +444,31 @@ sub drop_hold { > sub x_items { > my $self = shift; > my $array_var = shift or return; >- my ($start, $end) = @_; >+ my ( $start, $end ) = @_; > > my $item_list = []; >- if ($self->{$array_var}) { >+ if ( $self->{$array_var} ) { > >- if ($start && $start > 1) { >+ if ( $start && $start > 1 ) { > --$start; > } > else { > $start = 0; > } >- if ( $end && $end < @{$self->{$array_var}} ) { >- --$end; >+ if ( $end && $end < @{ $self->{$array_var} } ) { >+ --$end; > } > else { >- $end = @{$self->{$array_var}}; >+ $end = @{ $self->{$array_var} }; > --$end; > } >- @{$item_list} = @{$self->{$array_var}}[ $start .. $end ]; >+ @{$item_list} = @{ $self->{$array_var} }[ $start .. $end ]; > > } > return $item_list; > } > >+ > # > # List of outstanding holds placed > # >@@ -440,25 +484,30 @@ sub hold_items { > > sub overdue_items { > my $self = shift; >- return $self->x_items('overdue_items', @_); >+ return $self->x_items( 'overdue_items', @_ ); > } >+ > sub charged_items { > my $self = shift; >- return $self->x_items('items', @_); >+ return $self->x_items( 'items', @_ ); > } >+ > sub fine_items { > my $self = shift; >- return $self->x_items('fine_items', @_); >+ return $self->x_items( 'fine_items', @_ ); > } >+ > sub recall_items { > my $self = shift; >- return $self->x_items('recall_items', @_); >+ return $self->x_items( 'recall_items', @_ ); > } >+ > sub unavail_holds { > my $self = shift; >- return $self->x_items('unavail_holds', @_); >+ return $self->x_items( 'unavail_holds', @_ ); > } > >+ > sub block { > my ($self, $card_retained, $blocked_card_msg) = @_; > foreach my $field ('charge_ok', 'renew_ok', 'recall_ok', 'hold_ok', 'inet') { >diff --git a/C4/SIP/Sip/MsgType.pm b/C4/SIP/Sip/MsgType.pm >index 18dc6cb3f8..038a91fe73 100644 >--- a/C4/SIP/Sip/MsgType.pm >+++ b/C4/SIP/Sip/MsgType.pm >@@ -957,16 +957,23 @@ sub summary_info { > { func => $patron->can("charged_items"), fid => FID_CHARGED_ITEMS }, > { func => $patron->can("fine_items"), fid => FID_FINE_ITEMS }, > { func => $patron->can("recall_items"), fid => FID_RECALL_ITEMS }, >- { func => $patron->can("unavail_holds"), fid => FID_UNAVAILABLE_HOLD_ITEMS }, >- { func => $patron->can("fine_items"), fid => FID_FINE_ITEMS }, >+ { >+ func => $patron->can("unavail_holds"), >+ fid => FID_UNAVAILABLE_HOLD_ITEMS >+ }, >+ { func => $patron->can("fine_items"), fid => FID_FINE_ITEMS }, > ); > > my $summary_type = index( $summary, 'Y' ); > return q{} if $summary_type == -1; # No detailed information required. >- return q{} if $summary_type > 6; # Positions 7-9 are not defined in the sip spec, >- # and we have no extensions to handle them. >+ return q{} >+ if $summary_type > 6; # Positions 7-9 are not defined in the sip spec, >+ # and we have no extensions to handle them. > >- siplog( "LOG_DEBUG", "Summary_info: index == '%d', field '%s'", $summary_type, $summary_map[$summary_type]->{fid} ); >+ siplog( >+ "LOG_DEBUG", "Summary_info: index == '%d', field '%s'", >+ $summary_type, $summary_map[$summary_type]->{fid} >+ ); > > my $func = $summary_map[$summary_type]->{func}; > my $fid = $summary_map[$summary_type]->{fid}; >@@ -974,13 +981,21 @@ sub summary_info { > > # fine items use account_line as key > if ( $summary_type == 6 ) { >- siplog( "LOG_DEBUG", "summary_info: list = (%s)", join( ", ", map { $_->{account_line} } @{$itemlist} ) ); >+ siplog( >+ "LOG_DEBUG", >+ "summary_info: list = (%s)", >+ join( ", ", map { $_->{account_line} } @{$itemlist} ) >+ ); > foreach my $i ( @{$itemlist} ) { > $resp .= add_field( $fid, $i->{account_line}, $server ); > } > } > else { >- siplog( "LOG_DEBUG", "summary_info: list = (%s)", join( ", ", map{ $_->{barcode} } @{$itemlist} ) ); >+ siplog( >+ "LOG_DEBUG", >+ "summary_info: list = (%s)", >+ join( ", ", map { $_->{barcode} } @{$itemlist} ) >+ ); > foreach my $i ( @{$itemlist} ) { > $resp .= add_field( $fid, $i->{barcode}, $server ); > } >-- >2.39.2
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 23426
:
91992
|
165507
|
165508
|
165509
|
165654
|
166240
|
169272
|
170931
|
172383
|
172384
|
174232
|
174233
|
174234
|
174464
|
174465
|
174479