From b0c4bc36238a5afec267a3e34a62c2af6c0c0599 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Fri, 3 Nov 2023 15:35:35 +0000 Subject: [PATCH] Bug 12133: (QA follow-up) Tidy for qa script Signed-off-by: Kyle M Hall Signed-off-by: Tomas Cohen Arazi --- Koha/Exceptions/Patron/Relationship.pm | 23 ++-- Koha/Patron.pm | 103 +++++++++--------- .../data/mysql/atomicupdate/bug_12133.pl | 12 +- members/memberentry.pl | 15 +-- t/db_dependent/Koha/Patron.t | 29 ++--- 5 files changed, 89 insertions(+), 93 deletions(-) diff --git a/Koha/Exceptions/Patron/Relationship.pm b/Koha/Exceptions/Patron/Relationship.pm index 6a0cc5dd76..d3ca50c4f9 100644 --- a/Koha/Exceptions/Patron/Relationship.pm +++ b/Koha/Exceptions/Patron/Relationship.pm @@ -32,7 +32,7 @@ use Exception::Class ( 'Koha::Exceptions::Patron::Relationship::InvalidRelationship' => { isa => 'Koha::Exceptions::Patron::Relationship', description => 'The specified relationship is invalid', - fields => [ 'relationship', 'no_relationship', 'invalid_guarantor' ] + fields => [ 'relationship', 'no_relationship', 'invalid_guarantor' ] }, 'Koha::Exceptions::Patron::Relationship::NoGuarantor' => { isa => 'Koha::Exceptions::Patron::Relationship', @@ -45,23 +45,20 @@ sub full_message { my $msg = $self->message; - unless ( $msg) { + unless ($msg) { if ( $self->isa('Koha::Exceptions::Patron::Relationship::InvalidRelationship') ) { if ( $self->no_relationship ) { - $msg = sprintf( "No relationship passed." ); - } - elsif ( $self->invalid_guarantor ) { + $msg = sprintf("No relationship passed."); + } elsif ( $self->invalid_guarantor ) { $msg = sprintf("Child patron cannot be a guarantor."); + } else { + $msg = sprintf( "Invalid relationship passed, '%s' is not defined.", $self->relationship ); } - else { - $msg = sprintf("Invalid relationship passed, '%s' is not defined.", $self->relationship ); - } - } - elsif ( $self->isa('Koha::Exceptions::Patron::Relationship::DuplicateRelationship') ) { - $msg - = sprintf( + } elsif ( $self->isa('Koha::Exceptions::Patron::Relationship::DuplicateRelationship') ) { + $msg = sprintf( "There already exists a relationship for the same guarantor (%s) and guarantee (%s) combination", - $self->guarantor_id, $self->guarantee_id ); + $self->guarantor_id, $self->guarantee_id + ); } } diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 7c7f069be6..1a4a920580 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -191,7 +191,7 @@ to db =cut sub store { - my $self = shift; + my $self = shift; my $params = @_ ? shift : {}; my $guarantors = $params->{guarantors} // []; @@ -202,7 +202,7 @@ sub store { C4::Context->preference("autoMemberNum") and ( not defined $self->cardnumber or $self->cardnumber eq '' ) - ) + ) { # Warning: The caller is responsible for locking the members table in write # mode, to avoid database corruption. @@ -210,7 +210,7 @@ sub store { $self->fixup_cardnumber; } - unless( $self->category->in_storage ) { + unless ( $self->category->in_storage ) { Koha::Exceptions::Object::FKConstraint->throw( broken_fk => 'categorycode', value => $self->categorycode, @@ -221,15 +221,15 @@ sub store { my $new_cardnumber = $self->cardnumber; Koha::Plugins->call( 'patron_barcode_transform', \$new_cardnumber ); - $self->cardnumber( $new_cardnumber ); + $self->cardnumber($new_cardnumber); # Set surname to uppercase if uppercasesurname is true - $self->surname( uc($self->surname) ) + $self->surname( uc( $self->surname ) ) if C4::Context->preference("uppercasesurnames"); - $self->relationship(undef) # We do not want to store an empty string in this field - if defined $self->relationship - and $self->relationship eq ""; + $self->relationship(undef) # We do not want to store an empty string in this field + if defined $self->relationship + and $self->relationship eq ""; unless ( $self->in_storage ) { #AddMember @@ -251,18 +251,21 @@ sub store { # Set the privacy depending on the patron's category my $default_privacy = $self->category->default_privacy || q{}; $default_privacy = - $default_privacy eq 'default' ? 1 - : $default_privacy eq 'never' ? 2 - : $default_privacy eq 'forever' ? 0 - : undef; + $default_privacy eq 'default' ? 1 + : $default_privacy eq 'never' ? 2 + : $default_privacy eq 'forever' ? 0 + : undef; $self->privacy($default_privacy); # Call any check_password plugins if password is passed if ( C4::Context->config("enable_plugins") && $self->password ) { - my @plugins = Koha::Plugins->new()->GetPlugins({ - method => 'check_password', - }); - foreach my $plugin ( @plugins ) { + my @plugins = Koha::Plugins->new()->GetPlugins( + { + method => 'check_password', + } + ); + foreach my $plugin (@plugins) { + # This plugin hook will also be used by a plugin for the Norwegian national # patron database. This is why we need to pass both the password and the # borrowernumber to the plugin. @@ -281,17 +284,22 @@ sub store { # Make a copy of the plain text password for later use $self->plain_text_password( $self->password ); - $self->password_expiration_date( $self->password + $self->password_expiration_date( + $self->password ? $self->category->get_password_expiry_date || undef - : undef ); + : undef + ); + # Create a disabled account if no password provided - $self->password( $self->password + $self->password( + $self->password ? Koha::AuthUtils::hash_password( $self->password ) - : '!' ); + : '!' + ); $self->borrowernumber(undef); - if ( C4::Context->preference('ChildNeedsGuarantor') + if ( C4::Context->preference('ChildNeedsGuarantor') and ( $self->is_child or $self->category->can_be_guarantee ) and $self->contactname eq "" and !@$guarantors ) @@ -300,9 +308,8 @@ sub store { } foreach my $guarantor (@$guarantors) { - if ( $guarantor->is_child or $guarantor->category->can_be_guarantee) { - Koha::Exceptions::Patron::Relationship::InvalidRelationship - ->throw( invalid_guarantor => 1 ); + if ( $guarantor->is_child or $guarantor->category->can_be_guarantee ) { + Koha::Exceptions::Patron::Relationship::InvalidRelationship->throw( invalid_guarantor => 1 ); } } @@ -311,50 +318,45 @@ sub store { $self->add_enrolment_fee_if_needed(0); logaction( "MEMBERS", "CREATE", $self->borrowernumber, "" ) - if C4::Context->preference("BorrowersLog"); - } - else { #ModMember + if C4::Context->preference("BorrowersLog"); + } else { #ModMember my $self_from_storage = $self->get_from_storage; # Do not accept invalid userid here $self->generate_userid unless $self->userid; Koha::Exceptions::Patron::InvalidUserid->throw( userid => $self->userid ) - unless $self->has_valid_userid; + unless $self->has_valid_userid; # If a borrower has set their privacy to never we should immediately anonymize # their checkouts - if( $self->privacy() == 2 && $self_from_storage->privacy() != 2 ){ - try{ + if ( $self->privacy() == 2 && $self_from_storage->privacy() != 2 ) { + try { $self->old_checkouts->anonymize; - } - catch { - Koha::Exceptions::Patron::FailedAnonymizing->throw( - error => @_ - ); + } catch { + Koha::Exceptions::Patron::FailedAnonymizing->throw( error => @_ ); }; } # Password must be updated using $self->set_password - $self->password($self_from_storage->password); + $self->password( $self_from_storage->password ); + + if ( $self->category->categorycode ne $self_from_storage->category->categorycode ) { - if ( $self->category->categorycode ne - $self_from_storage->category->categorycode ) - { # Add enrolement fee on category change if required $self->add_enrolment_fee_if_needed(1) - if C4::Context->preference('FeeOnChangePatronCategory'); + if C4::Context->preference('FeeOnChangePatronCategory'); # Clean up guarantors on category change if required $self->guarantor_relationships->delete - unless ( $self->category->can_be_guarantee ); + unless ( $self->category->can_be_guarantee ); } my @existing_guarantors = $self->guarantor_relationships()->guarantors->as_list; push @$guarantors, @existing_guarantors; - if ( C4::Context->preference('ChildNeedsGuarantor') + if ( C4::Context->preference('ChildNeedsGuarantor') and ( $self->is_child or $self->category->can_be_guarantee ) and $self->contactname eq "" and !@$guarantors ) @@ -363,9 +365,8 @@ sub store { } foreach my $guarantor (@$guarantors) { - if ( $guarantor->is_child or $guarantor->category->can_be_guarantee) { - Koha::Exceptions::Patron::Relationship::InvalidRelationship - ->throw( invalid_guarantor => 1 ); + if ( $guarantor->is_child or $guarantor->category->can_be_guarantee ) { + Koha::Exceptions::Patron::Relationship::InvalidRelationship->throw( invalid_guarantor => 1 ); } } @@ -378,19 +379,13 @@ sub store { for my $key ( keys %{$from_storage} ) { next if any { /$key/ } @skip_fields; if ( - ( - !defined( $from_storage->{$key} ) - && defined( $from_object->{$key} ) - ) + ( !defined( $from_storage->{$key} ) && defined( $from_object->{$key} ) ) || ( defined( $from_storage->{$key} ) && !defined( $from_object->{$key} ) ) - || ( - defined( $from_storage->{$key} ) + || ( defined( $from_storage->{$key} ) && defined( $from_object->{$key} ) - && ( $from_storage->{$key} ne - $from_object->{$key} ) + && ( $from_storage->{$key} ne $from_object->{$key} ) ) ) - ) { $info->{$key} = { before => $from_storage->{$key}, diff --git a/installer/data/mysql/atomicupdate/bug_12133.pl b/installer/data/mysql/atomicupdate/bug_12133.pl index e426f787eb..f3ef424d3a 100755 --- a/installer/data/mysql/atomicupdate/bug_12133.pl +++ b/installer/data/mysql/atomicupdate/bug_12133.pl @@ -1,16 +1,18 @@ use Modern::Perl; return { - bug_number => "12133", + bug_number => "12133", description => "Add system preference ChildNeedsGuarantor", - up => sub { + up => sub { my ($args) = @_; - my ($dbh, $out) = @$args{qw(dbh out)}; + my ( $dbh, $out ) = @$args{qw(dbh out)}; - $dbh->do( q{ + $dbh->do( + q{ INSERT IGNORE INTO systempreferences (variable,value,explanation,options,type) VALUES('ChildNeedsGuarantor', 0, 'If ON, a child patron must have a guarantor when adding the patron.', '', 'YesNo'); - } ); + } + ); say $out "Added new system preference 'ChildNeedsGuarantor')\n"; }, diff --git a/members/memberentry.pl b/members/memberentry.pl index a5d0d6c3fc..57e780cfec 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -261,13 +261,14 @@ if ( ( $op eq 'insert' ) and !$nodouble ) { #Attempt to delete guarantors my @delete_guarantor = $input->multi_param('delete_guarantor'); -if(@delete_guarantor){ +if (@delete_guarantor) { if ( C4::Context->preference('ChildNeedsGuarantor') - && scalar @guarantors - scalar @delete_guarantor == 0 ) { + && scalar @guarantors - scalar @delete_guarantor == 0 ) + { push @errors, 'ERROR_cannot_delete_guarantor'; } else { - foreach my $id ( @delete_guarantor ) { - my $r = Koha::Patron::Relationships->find( $id ); + foreach my $id (@delete_guarantor) { + my $r = Koha::Patron::Relationships->find($id); $r->delete() if $r; } } @@ -275,7 +276,7 @@ if(@delete_guarantor){ #Check if guarantor requirements are met my $valid_guarantor = @guarantors ? @guarantors : $newdata{'contactname'}; -if ( ( $op eq 'save' || $op eq 'insert' ) +if ( ( $op eq 'save' || $op eq 'insert' ) && C4::Context->preference('ChildNeedsGuarantor') && ( $category->category_type eq 'C' || $category->can_be_guarantee ) && !$valid_guarantor ) @@ -427,7 +428,7 @@ if ((!$nok) and $nodouble and ($op eq 'insert' or $op eq 'save')){ delete $newdata{password2}; $success = 1; $patron = try { - Koha::Patron->new(\%newdata)->store({ guarantors => \@guarantors }); + Koha::Patron->new( \%newdata )->store( { guarantors => \@guarantors } ); } catch { $success = 0; $nok = 1; @@ -521,7 +522,7 @@ if ((!$nok) and $nodouble and ($op eq 'insert' or $op eq 'save')){ delete $newdata{password2}; try { - $patron->set(\%newdata)->store({ guarantors => \@guarantors }) if scalar(keys %newdata) > 1; + $patron->set( \%newdata )->store( { guarantors => \@guarantors } ) if scalar( keys %newdata ) > 1; # bug 4508 - avoid crash if we're not updating any columns in the borrowers table (editing patron attrs or msg prefs) $success = 1; } catch { diff --git a/t/db_dependent/Koha/Patron.t b/t/db_dependent/Koha/Patron.t index 9d1e0ea45f..e07e90eaf8 100755 --- a/t/db_dependent/Koha/Patron.t +++ b/t/db_dependent/Koha/Patron.t @@ -2153,12 +2153,12 @@ subtest 'guarantor requirements tests' => sub { $schema->storage->txn_begin; my $branchcode = $builder->build( { source => 'Branch' } )->{branchcode}; - my $child_category = $builder->build( - { source => 'Category', value => { category_type => 'C', can_be_guarantee => 1 } } ) - ->{categorycode}; - my $patron_category = $builder->build( - { source => 'Category', value => { category_type => 'A', can_be_guarantee => 0 } } ) - ->{categorycode}; + my $child_category = + $builder->build( { source => 'Category', value => { category_type => 'C', can_be_guarantee => 1 } } ) + ->{categorycode}; + my $patron_category = + $builder->build( { source => 'Category', value => { category_type => 'A', can_be_guarantee => 0 } } ) + ->{categorycode}; t::lib::Mocks::mock_preference( 'ChildNeedsGuarantor', 0 ); @@ -2200,36 +2200,37 @@ subtest 'guarantor requirements tests' => sub { throws_ok { $child2->store(); } 'Koha::Exceptions::Patron::Relationship::NoGuarantor', - 'Exception thrown when guarantor is required but not provided.'; + 'Exception thrown when guarantor is required but not provided.'; my @guarantors = ( $patron, $child3 ); throws_ok { $child2->store( { guarantors => \@guarantors } ); } 'Koha::Exceptions::Patron::Relationship::InvalidRelationship', - 'Exception thrown when child patron is added as guarantor.'; + 'Exception thrown when child patron is added as guarantor.'; #test ModMember - @guarantors = ( $patron ); + @guarantors = ($patron); $child2->store( { guarantors => \@guarantors } )->discard_changes(); t::lib::Mocks::mock_preference( 'borrowerRelationship', '' ); my $relationship = Koha::Patron::Relationship->new( - { guarantor_id => $patron->borrowernumber, + { + guarantor_id => $patron->borrowernumber, guarantee_id => $child2->borrowernumber, relationship => '' } ); $relationship->store(); - ok( $child2->store(), 'Child patron can be modified and stored when guarantor is stored'); + ok( $child2->store(), 'Child patron can be modified and stored when guarantor is stored' ); - @guarantors = ( $child3 ); + @guarantors = ($child3); throws_ok { $child2->store( { guarantors => \@guarantors } ); } 'Koha::Exceptions::Patron::Relationship::InvalidRelationship', - 'Exception thrown when child patron is modified and child patron is added as guarantor.'; + 'Exception thrown when child patron is modified and child patron is added as guarantor.'; $relationship->delete; throws_ok { $child2->store(); } 'Koha::Exceptions::Patron::Relationship::NoGuarantor', - 'Exception thrown when guarantor is deleted.'; + 'Exception thrown when guarantor is deleted.'; }; -- 2.39.5