Bugzilla – Attachment 158346 Details for
Bug 12133
Guarantor requirements when registering a patron
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 12133: (QA follow-up) Tidy for qa script
Bug-12133-QA-follow-up-Tidy-for-qa-script.patch (text/plain), 19.09 KB, created by
Kyle M Hall (khall)
on 2023-11-03 15:45:23 UTC
(
hide
)
Description:
Bug 12133: (QA follow-up) Tidy for qa script
Filename:
MIME Type:
Creator:
Kyle M Hall (khall)
Created:
2023-11-03 15:45:23 UTC
Size:
19.09 KB
patch
obsolete
>From 397606d53828d8405ff37ec98e2311d5e106d751 Mon Sep 17 00:00:00 2001 >From: Kyle M Hall <kyle@bywatersolutions.com> >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 <kyle@bywatersolutions.com> >--- > 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 6a0cc5dd769..d3ca50c4f94 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 16b3a5b4d48..94d841d3ffb 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 e426f787eba..f3ef424d3a4 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 a5d0d6c3fc7..57e780cfecd 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 9d1e0ea45f9..e07e90eaf82 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.30.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 12133
:
27517
|
27584
|
33459
|
91482
|
94371
|
94538
|
94544
|
99629
|
113099
|
113100
|
134013
|
134014
|
134015
|
146827
|
147440
|
148058
|
148070
|
152706
|
153082
|
153154
|
154300
|
154316
|
156387
|
156446
|
156447
|
158344
|
158345
| 158346 |
158919
|
158979