Bug 12133: (QA follow-up) Tidy for qa script

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

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
+            );
         }
     }
 

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},

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";
     },

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 {

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.';
 };