|
Description
Lari Strand
2025-02-03 07:32:16 UTC
The problem comes from Koha::Patron->store called in update_lastseen. Since we validate guarantor in Koha::Patron->store but don't store them in there, new call to store fails because it doesn't receive guarantor info as parameter. To test: 1. Select option "Creating a patron" in "TrackLastPatronActivityTriggers" syspref. 2. Enable syspref "ChildNeedsGuarantor" and make sure you have options in "borrowerRelationship" syspref. 3. Create a guarantee patron, fill all required fields and add guarantor for patron. 4. Attempt to save patron. => Error box is displayed with text "The following fields are wrong. Please fix them.". => Logs show error "Patron creation failed! - [Child patron needs a guarantor]". I think we need a new method validate_guarantor instead of checking guarantor validity in Koha::Patron->store. This way we can ensure that saving patron doesn't fail if store is called again in another function and we can create tests for this new method. Created attachment 177563 [details] [review] Bug 39014: Add new method validate_guarantor Adding a new guarantee and storing it fails with "Creating a patron" selected in TrackLastPatronActivityTriggers when guarantors are set to be mandatory. The problem comes from Koha::Patron->store called in update_lastseen. Since we validate guarantor in Koha::Patron->store but don't store them in there, new call to store fails because it doesn't receive guarantor info as parameter. As a solution, we should validate guarantors in separated method. Validation can't be moved e.g. to Koha::Patron::Relationship->store since this could lead to situations where guarantee is stored without guarantor. This patch adds new method validate_guarantor and adjusts old guarantor requirements tests to match this new method. To test: 1. Select option "Creating a patron" in "TrackLastPatronActivityTriggers" syspref. 2. Enable syspref "ChildNeedsGuarantor" and make sure you have options in "borrowerRelationship" syspref. 3. Create a guarantee patron, fill all required fields and add guarantor for patron (either add Patron guarantor or fill Non-patron guarantor inputs). 4. Attempt to save patron. => Error box is displayed with text "The following fields are wrong. Please fix them.". => Logs show error "Patron creation failed! - [Child patron needs a guarantor]". 5. Apply this patch. 6. Create a guarantee patron again and save. => Patron should be saved correctly. 7. Create a new guarantee patron but this time don't add patron guarantor or fill non-patron guarantor inputs. 8. Attempt to save patron. => Save should fail with error "A child patron needs a guarantor." 9. Add patron guarantor who can also be a guarantee and save. => Save should fail with error "A guarantor cannot be a guarantee." Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy Created attachment 177564 [details] [review] Bug 39014: Add new method validate_guarantor Adding a new guarantee and storing it fails with "Creating a patron" selected in TrackLastPatronActivityTriggers when guarantors are set to be mandatory. The problem comes from Koha::Patron->store called in update_lastseen. Since we validate guarantor in Koha::Patron->store but don't store them in there, new call to store fails because it doesn't receive guarantor info as parameter. As a solution, we should validate guarantors in separated method. Validation can't be moved e.g. to Koha::Patron::Relationship->store since this could lead to situations where guarantee is stored without guarantor. This patch adds new method validate_guarantor and adjusts old guarantor requirements tests to match this new method. To test: 1. Select option "Creating a patron" in "TrackLastPatronActivityTriggers" syspref. 2. Enable syspref "ChildNeedsGuarantor" and make sure you have options in "borrowerRelationship" syspref. 3. Create a guarantee patron, fill all required fields and add guarantor for patron (either add Patron guarantor or fill Non-patron guarantor inputs). 4. Attempt to save patron. => Error box is displayed with text "The following fields are wrong. Please fix them.". => Logs show error "Patron creation failed! - [Child patron needs a guarantor]". 5. Apply this patch. 6. Create a guarantee patron again and save. => Patron should be saved correctly. 7. Create a new guarantee patron but this time don't add patron guarantor or fill non-patron guarantor inputs. 8. Attempt to save patron. => Save should fail with error "A child patron needs a guarantor." 9. Add patron guarantor who can also be a guarantee and save. => Save should fail with error "A guarantor cannot be a guarantee." Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy Created attachment 177619 [details] [review] Bug 39014: Add new method validate_guarantor Adding a new guarantee and storing it fails with "Creating a patron" selected in TrackLastPatronActivityTriggers when guarantors are set to be mandatory. The problem comes from Koha::Patron->store called in update_lastseen. Since we validate guarantor in Koha::Patron->store but don't store them in there, new call to store fails because it doesn't receive guarantor info as parameter. As a solution, we should validate guarantors in separated method. Validation can't be moved e.g. to Koha::Patron::Relationship->store since this could lead to situations where guarantee is stored without guarantor. This patch adds new method validate_guarantor and adjusts old guarantor requirements tests to match this new method. To test: 1. Select option "Creating a patron" in "TrackLastPatronActivityTriggers" syspref. 2. Enable syspref "ChildNeedsGuarantor" and make sure you have options in "borrowerRelationship" syspref. 3. Create a guarantee patron, fill all required fields and add guarantor for patron (either add Patron guarantor or fill Non-patron guarantor inputs). 4. Attempt to save patron. => Error box is displayed with text "The following fields are wrong. Please fix them.". => Logs show error "Patron creation failed! - [Child patron needs a guarantor]". 5. Apply this patch. 6. Create a guarantee patron again and save. => Patron should be saved correctly. 7. Create a new guarantee patron but this time don't add patron guarantor or fill non-patron guarantor inputs. 8. Attempt to save patron. => Save should fail with error "A child patron needs a guarantor." 9. Add patron guarantor who can also be a guarantee and save. => Save should fail with error "A guarantor cannot be a guarantee." Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy Signed-off-by: Jake Deery <jake.deery@ptfs-europe.com> All works as intended, now! Signed off Jake. Created attachment 178546 [details] [review] Bug 39014: Add new method validate_guarantor Adding a new guarantee and storing it fails with "Creating a patron" selected in TrackLastPatronActivityTriggers when guarantors are set to be mandatory. The problem comes from Koha::Patron->store called in update_lastseen. Since we validate guarantor in Koha::Patron->store but don't store them in there, new call to store fails because it doesn't receive guarantor info as parameter. As a solution, we should validate guarantors in separated method. Validation can't be moved e.g. to Koha::Patron::Relationship->store since this could lead to situations where guarantee is stored without guarantor. This patch adds new method validate_guarantor and adjusts old guarantor requirements tests to match this new method. To test: 1. Select option "Creating a patron" in "TrackLastPatronActivityTriggers" syspref. 2. Enable syspref "ChildNeedsGuarantor" and make sure you have options in "borrowerRelationship" syspref. 3. Create a guarantee patron, fill all required fields and add guarantor for patron (either add Patron guarantor or fill Non-patron guarantor inputs). 4. Attempt to save patron. => Error box is displayed with text "The following fields are wrong. Please fix them.". => Logs show error "Patron creation failed! - [Child patron needs a guarantor]". 5. Apply this patch. 6. Create a guarantee patron again and save. => Patron should be saved correctly. 7. Create a new guarantee patron but this time don't add patron guarantor or fill non-patron guarantor inputs. 8. Attempt to save patron. => Save should fail with error "A child patron needs a guarantor." 9. Add patron guarantor who can also be a guarantee and save. => Save should fail with error "A guarantor cannot be a guarantee." Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy There were some things I forgot to handle in the original patch. Patch is now rebased against current main. Created attachment 178547 [details] [review] Bug 39014: Add new method validate_guarantor Adding a new guarantee and storing it fails with "Creating a patron" selected in TrackLastPatronActivityTriggers when guarantors are set to be mandatory. The problem comes from Koha::Patron->store called in update_lastseen. Since we validate guarantor in Koha::Patron->store but don't store them in there, new call to store fails because it doesn't receive guarantor info as parameter. As a solution, we should validate guarantors in separated method. Validation can't be moved e.g. to Koha::Patron::Relationship->store since this could lead to situations where guarantee is stored without guarantor. This patch adds new method validate_guarantor and adjusts old guarantor requirements tests to match this new method. To test: 1. Select option "Creating a patron" in "TrackLastPatronActivityTriggers" syspref. 2. Enable syspref "ChildNeedsGuarantor" and make sure you have options in "borrowerRelationship" syspref. 3. Create a guarantee patron, fill all required fields and add guarantor for patron (either add Patron guarantor or fill Non-patron guarantor inputs). 4. Attempt to save patron. => Error box is displayed with text "The following fields are wrong. Please fix them.". => Logs show error "Patron creation failed! - [Child patron needs a guarantor]". 5. Apply this patch. 6. Create a guarantee patron again and save. => Patron should be saved correctly. 7. Create a new guarantee patron but this time don't add patron guarantor or fill non-patron guarantor inputs. 8. Attempt to save patron. => Save should fail with error "A child patron needs a guarantor." 9. Add patron guarantor who can also be a guarantee and save. => Save should fail with error "A guarantor cannot be a guarantee." Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy Created attachment 179572 [details] [review] Bug 39014: Add new method validate_guarantor Adding a new guarantee and storing it fails with "Creating a patron" selected in TrackLastPatronActivityTriggers when guarantors are set to be mandatory. The problem comes from Koha::Patron->store called in update_lastseen. Since we validate guarantor in Koha::Patron->store but don't store them in there, new call to store fails because it doesn't receive guarantor info as parameter. As a solution, we should validate guarantors in separated method. Validation can't be moved e.g. to Koha::Patron::Relationship->store since this could lead to situations where guarantee is stored without guarantor. This patch adds new method validate_guarantor and adjusts old guarantor requirements tests to match this new method. To test: 1. Select option "Creating a patron" in "TrackLastPatronActivityTriggers" syspref. 2. Enable syspref "ChildNeedsGuarantor" and make sure you have options in "borrowerRelationship" syspref. 3. Create a guarantee patron, fill all required fields and add guarantor for patron (either add Patron guarantor or fill Non-patron guarantor inputs). 4. Attempt to save patron. => Error box is displayed with text "The following fields are wrong. Please fix them.". => Logs show error "Patron creation failed! - [Child patron needs a guarantor]". 5. Apply this patch. 6. Create a guarantee patron again and save. => Patron should be saved correctly. 7. Create a new guarantee patron but this time don't add patron guarantor or fill non-patron guarantor inputs. 8. Attempt to save patron. => Save should fail with error "A child patron needs a guarantor." 9. Add patron guarantor who can also be a guarantee and save. => Save should fail with error "A guarantor cannot be a guarantee." Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy Created attachment 180298 [details] [review] Bug 39014: Add new method validate_guarantor Adding a new guarantee and storing it fails with "Creating a patron" selected in TrackLastPatronActivityTriggers when guarantors are set to be mandatory. The problem comes from Koha::Patron->store called in update_lastseen. Since we validate guarantor in Koha::Patron->store but don't store them in there, new call to store fails because it doesn't receive guarantor info as parameter. As a solution, we should validate guarantors in separated method. Validation can't be moved e.g. to Koha::Patron::Relationship->store since this could lead to situations where guarantee is stored without guarantor. This patch adds new method validate_guarantor and adjusts old guarantor requirements tests to match this new method. To test: 1. Select option "Creating a patron" in "TrackLastPatronActivityTriggers" syspref. 2. Enable syspref "ChildNeedsGuarantor" and make sure you have options in "borrowerRelationship" syspref. 3. Create a guarantee patron, fill all required fields and add guarantor for patron (either add Patron guarantor or fill Non-patron guarantor inputs). 4. Attempt to save patron. => Error box is displayed with text "The following fields are wrong. Please fix them.". => Logs show error "Patron creation failed! - [Child patron needs a guarantor]". 5. Apply this patch. 6. Create a guarantee patron again and save. => Patron should be saved correctly. 7. Create a new guarantee patron but this time don't add patron guarantor or fill non-patron guarantor inputs. 8. Attempt to save patron. => Save should fail with error "A child patron needs a guarantor." 9. Add patron guarantor who can also be a guarantee and save. => Save should fail with error "A guarantor cannot be a guarantee." Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy Signed-off-by: Magnus Enger <magnus@libriotech.no> Works as advertised. Created attachment 182290 [details] [review] Bug 39014: Add new method validate_guarantor Adding a new guarantee and storing it fails with "Creating a patron" selected in TrackLastPatronActivityTriggers when guarantors are set to be mandatory. The problem comes from Koha::Patron->store called in update_lastseen. Since we validate guarantor in Koha::Patron->store but don't store them in there, new call to store fails because it doesn't receive guarantor info as parameter. As a solution, we should validate guarantors in separated method. Validation can't be moved e.g. to Koha::Patron::Relationship->store since this could lead to situations where guarantee is stored without guarantor. This patch adds new method validate_guarantor and adjusts old guarantor requirements tests to match this new method. To test: 1. Select option "Creating a patron" in "TrackLastPatronActivityTriggers" syspref. 2. Enable syspref "ChildNeedsGuarantor" and make sure you have options in "borrowerRelationship" syspref. 3. Create a guarantee patron, fill all required fields and add guarantor for patron (either add Patron guarantor or fill Non-patron guarantor inputs). 4. Attempt to save patron. => Error box is displayed with text "The following fields are wrong. Please fix them.". => Logs show error "Patron creation failed! - [Child patron needs a guarantor]". 5. Apply this patch. 6. Create a guarantee patron again and save. => Patron should be saved correctly. 7. Create a new guarantee patron but this time don't add patron guarantor or fill non-patron guarantor inputs. 8. Attempt to save patron. => Save should fail with error "A child patron needs a guarantor." 9. Add patron guarantor who can also be a guarantee and save. => Save should fail with error "A guarantor cannot be a guarantee." Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy Signed-off-by: Magnus Enger <magnus@libriotech.no> Works as advertised. Signed-off-by: David Nind <david@davidnind.com> Added my sign-off, in case Magnus's can count as QA Looking here QA Comment:
Thx for your patch !
Koha::Patron->validate_guarantor( \@guarantors, $newdata{'contactname'}, $category );
validate_guarantor is a CLASS method !
Should we at least reflect that in the code by renaming to $class ? Or should we default to checking guarantor_relationships when you would pass a patron in $self ?
=head3 validate_guarantor
Koha::Patron->validate_guarantor(\@guarantors, $contactname, $category );
Validates guarantor patron.
=cut
Could you elaborate "validate guarantor patron"? Singular ?
Having the impression that we do two separate things here? FIrst we check ChildNeedsGuarantor, which is not really validating guarantors, right? And secondly we walk thru all guarantors (which is the name of the sub).
How can this test on patron work in all cases?
if ( ( $op eq 'cud-save' || $op eq 'cud-insert' ) ) {
unless ( $patron && $patron->is_guarantor ) {
[etc]
} else {
push @errors, "ERROR_guarantee_is_guarantor";
}
}
Test adding a regular patron:
[2025/05/30 09:06:36] [WARN] L291 at /usr/share/koha/members/memberentry.pl line 291.
Test modify a guarantor:
The following fields are wrong. Please fix them.
A guarantor cannot be a guarantee.
The patron is no guarantee! The test here is WRONG.
(In reply to Marcel de Rooy from comment #15) > QA Comment: > Thx for your patch ! > > Koha::Patron->validate_guarantor( \@guarantors, $newdata{'contactname'}, > $category ); > validate_guarantor is a CLASS method ! > Should we at least reflect that in the code by renaming to $class ? Or > should we default to checking guarantor_relationships when you would pass a > patron in $self ? I don't quite get what you mean with this, could you explain this some more? > =head3 validate_guarantor > Koha::Patron->validate_guarantor(\@guarantors, $contactname, $category ); > Validates guarantor patron. > =cut > Could you elaborate "validate guarantor patron"? Singular ? You're right, method should be named validate_guarantors and description of the method should be more descriptive. > Having the impression that we do two separate things here? FIrst we check > ChildNeedsGuarantor, which is not really validating guarantors, right? And > secondly we walk thru all guarantors (which is the name of the sub). > > How can this test on patron work in all cases? > if ( ( $op eq 'cud-save' || $op eq 'cud-insert' ) ) { > unless ( $patron && $patron->is_guarantor ) { > [etc] > } else { > push @errors, "ERROR_guarantee_is_guarantor"; > } > } Hmm, I guess these should be two separate methods then? One for validating guarantors and one for checking if patron needs a guarantor. > Test adding a regular patron: > [2025/05/30 09:06:36] [WARN] L291 at /usr/share/koha/members/memberentry.pl > line 291. > Test modify a guarantor: > The following fields are wrong. Please fix them. > A guarantor cannot be a guarantee. > The patron is no guarantee! The test here is WRONG. Did this happen while adding patron from Koha or while running tests? Could you provide a plan how to reproduce this? (In reply to Emmi Takkinen from comment #16) > > Test adding a regular patron: > > [2025/05/30 09:06:36] [WARN] L291 at /usr/share/koha/members/memberentry.pl > > line 291. > > Test modify a guarantor: > > The following fields are wrong. Please fix them. > > A guarantor cannot be a guarantee. > > The patron is no guarantee! The test here is WRONG. > Did this happen while adding patron from Koha or while running tests? Could > you provide a plan how to reproduce this? Adding a regular patron. May 30 is a bit long ago already :) (In reply to Marcel de Rooy from comment #17) > (In reply to Emmi Takkinen from comment #16) > > > Test adding a regular patron: > > > [2025/05/30 09:06:36] [WARN] L291 at /usr/share/koha/members/memberentry.pl > > > line 291. > > > Test modify a guarantor: > > > The following fields are wrong. Please fix them. > > > A guarantor cannot be a guarantee. > > > The patron is no guarantee! The test here is WRONG. > > Did this happen while adding patron from Koha or while running tests? Could > > you provide a plan how to reproduce this? > > Adding a regular patron. May 30 is a bit long ago already :) Ah, this happens with guarantors. There's definitely something wrong with logic in code. Good catch! Created attachment 188041 [details] [review] Bug 39014: (follow-up) Refacotring and renaming to can_be_guaranteed_by To test: 1. prove t/db_dependent/Koha/Patron.t We've also come accross with this bug. I attempted to address some of the issues Marcel pointed out. I've also added more detailed error messages, as I could not initially wrap my head around with the difference of "A guarantor cannot be a guarantee" vs "A guarantee cannot be a guarantor" :D The patch no longer applies 8-(... Applying: Bug 39014: Add new method validate_guarantor Using index info to reconstruct a base tree... M Koha/Exceptions/Patron/Relationship.pm M Koha/Patron.pm M koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt M members/memberentry.pl M t/db_dependent/Koha/Patron.t Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Koha/Patron.t Auto-merging members/memberentry.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt Auto-merging Koha/Patron.pm CONFLICT (content): Merge conflict in Koha/Patron.pm Auto-merging Koha/Exceptions/Patron/Relationship.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 39014: Add new method validate_guarantor Created attachment 188461 [details] [review] Bug 39014: Add new method validate_guarantor Adding a new guarantee and storing it fails with "Creating a patron" selected in TrackLastPatronActivityTriggers when guarantors are set to be mandatory. The problem comes from Koha::Patron->store called in update_lastseen. Since we validate guarantor in Koha::Patron->store but don't store them in there, new call to store fails because it doesn't receive guarantor info as parameter. As a solution, we should validate guarantors in separated method. Validation can't be moved e.g. to Koha::Patron::Relationship->store since this could lead to situations where guarantee is stored without guarantor. This patch adds new method validate_guarantor and adjusts old guarantor requirements tests to match this new method. To test: 1. Select option "Creating a patron" in "TrackLastPatronActivityTriggers" syspref. 2. Enable syspref "ChildNeedsGuarantor" and make sure you have options in "borrowerRelationship" syspref. 3. Create a guarantee patron, fill all required fields and add guarantor for patron (either add Patron guarantor or fill Non-patron guarantor inputs). 4. Attempt to save patron. => Error box is displayed with text "The following fields are wrong. Please fix them.". => Logs show error "Patron creation failed! - [Child patron needs a guarantor]". 5. Apply this patch. 6. Create a guarantee patron again and save. => Patron should be saved correctly. 7. Create a new guarantee patron but this time don't add patron guarantor or fill non-patron guarantor inputs. 8. Attempt to save patron. => Save should fail with error "A child patron needs a guarantor." 9. Add patron guarantor who can also be a guarantee and save. => Save should fail with error "A guarantor cannot be a guarantee." Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy Signed-off-by: Magnus Enger <magnus@libriotech.no> Works as advertised. Signed-off-by: David Nind <david@davidnind.com> Created attachment 188462 [details] [review] Bug 39014: (follow-up) Refacotring and renaming to can_be_guaranteed_by To test: 1. prove t/db_dependent/Koha/Patron.t Rebased patches. Created attachment 188463 [details] [review] Bug 39014: Add new method validate_guarantor Adding a new guarantee and storing it fails with "Creating a patron" selected in TrackLastPatronActivityTriggers when guarantors are set to be mandatory. The problem comes from Koha::Patron->store called in update_lastseen. Since we validate guarantor in Koha::Patron->store but don't store them in there, new call to store fails because it doesn't receive guarantor info as parameter. As a solution, we should validate guarantors in separated method. Validation can't be moved e.g. to Koha::Patron::Relationship->store since this could lead to situations where guarantee is stored without guarantor. This patch adds new method validate_guarantor and adjusts old guarantor requirements tests to match this new method. To test: 1. Select option "Creating a patron" in "TrackLastPatronActivityTriggers" syspref. 2. Enable syspref "ChildNeedsGuarantor" and make sure you have options in "borrowerRelationship" syspref. 3. Create a guarantee patron, fill all required fields and add guarantor for patron (either add Patron guarantor or fill Non-patron guarantor inputs). 4. Attempt to save patron. => Error box is displayed with text "The following fields are wrong. Please fix them.". => Logs show error "Patron creation failed! - [Child patron needs a guarantor]". 5. Apply this patch. 6. Create a guarantee patron again and save. => Patron should be saved correctly. 7. Create a new guarantee patron but this time don't add patron guarantor or fill non-patron guarantor inputs. 8. Attempt to save patron. => Save should fail with error "A child patron needs a guarantor." 9. Add patron guarantor who can also be a guarantee and save. => Save should fail with error "A guarantor cannot be a guarantee." Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy Signed-off-by: Magnus Enger <magnus@libriotech.no> Works as advertised. Signed-off-by: David Nind <david@davidnind.com> Created attachment 188464 [details] [review] Bug 39014: (follow-up) Refacotring and renaming to can_be_guaranteed_by To test: 1. prove t/db_dependent/Koha/Patron.t Signed-off-by: David Nind <david@davidnind.com> (In reply to Emmi Takkinen from comment #24) > Rebased patches. Thanks Emmi! Looking here Did not finish yet. Will resume next week. Created attachment 190493 [details] [review] Bug 39014: Add new method validate_guarantor Adding a new guarantee and storing it fails with "Creating a patron" selected in TrackLastPatronActivityTriggers when guarantors are set to be mandatory. The problem comes from Koha::Patron->store called in update_lastseen. Since we validate guarantor in Koha::Patron->store but don't store them in there, new call to store fails because it doesn't receive guarantor info as parameter. As a solution, we should validate guarantors in separated method. Validation can't be moved e.g. to Koha::Patron::Relationship->store since this could lead to situations where guarantee is stored without guarantor. This patch adds new method validate_guarantor and adjusts old guarantor requirements tests to match this new method. To test: 1. Select option "Creating a patron" in "TrackLastPatronActivityTriggers" syspref. 2. Enable syspref "ChildNeedsGuarantor" and make sure you have options in "borrowerRelationship" syspref. 3. Create a guarantee patron, fill all required fields and add guarantor for patron (either add Patron guarantor or fill Non-patron guarantor inputs). 4. Attempt to save patron. => Error box is displayed with text "The following fields are wrong. Please fix them.". => Logs show error "Patron creation failed! - [Child patron needs a guarantor]". 5. Apply this patch. 6. Create a guarantee patron again and save. => Patron should be saved correctly. 7. Create a new guarantee patron but this time don't add patron guarantor or fill non-patron guarantor inputs. 8. Attempt to save patron. => Save should fail with error "A child patron needs a guarantor." 9. Add patron guarantor who can also be a guarantee and save. => Save should fail with error "A guarantor cannot be a guarantee." Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy Signed-off-by: Magnus Enger <magnus@libriotech.no> Works as advertised. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 190494 [details] [review] Bug 39014: (follow-up) Refactoring and renaming to can_be_guaranteed_by To test: 1. prove t/db_dependent/Koha/Patron.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Just an observation:
members/memberentry.pl: push @errors, "ERROR_guarantor_is_guarantee"; if ( $_->invalid_guarantor ) {
push @errors, "ERROR_guarantor_is_guarantee";
$template->param( guarantor_is_guarantee => $_->guarantor );
=> Does this code not beg for renaming invalid_guarantor to something like guarantor_is_guarantee ?
Koha/Exceptions/Patron/Relationship.pm: } elsif ( $self->invalid_guarantor ) {
} elsif ( $self->invalid_guarantor ) {
$msg = sprintf("Guarantee patron cannot be a guarantor.");
=> [SAME] Does this code not beg for renaming invalid_guarantor to something like guarantor_is_guarantee ?
[FAIL] Koha/Exceptions/Patron/Relationship.pm FAIL tidiness File is not tidy, please run `perl misc/devel/tidy.pl Koha/Exceptions/Patron/Relationship.pm` [PASS] Koha/Patron.pm [FAIL] koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt FAIL tidiness File is not tidy, please run `perl misc/devel/tidy.pl koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt` [FAIL] members/memberentry.pl FAIL tidiness File is not tidy, please run `perl misc/devel/tidy.pl members/memberentry.pl` [FAIL] t/db_dependent/Koha/Patron.t FAIL tidiness File is not tidy, please run `perl misc/devel/tidy.pl t/db_dependent/Koha/Patron.t` Created attachment 190767 [details] [review] Bug 39014: Add new method validate_guarantor Adding a new guarantee and storing it fails with "Creating a patron" selected in TrackLastPatronActivityTriggers when guarantors are set to be mandatory. The problem comes from Koha::Patron->store called in update_lastseen. Since we validate guarantor in Koha::Patron->store but don't store them in there, new call to store fails because it doesn't receive guarantor info as parameter. As a solution, we should validate guarantors in separated method. Validation can't be moved e.g. to Koha::Patron::Relationship->store since this could lead to situations where guarantee is stored without guarantor. This patch adds new method validate_guarantor and adjusts old guarantor requirements tests to match this new method. To test: 1. Select option "Creating a patron" in "TrackLastPatronActivityTriggers" syspref. 2. Enable syspref "ChildNeedsGuarantor" and make sure you have options in "borrowerRelationship" syspref. 3. Create a guarantee patron, fill all required fields and add guarantor for patron (either add Patron guarantor or fill Non-patron guarantor inputs). 4. Attempt to save patron. => Error box is displayed with text "The following fields are wrong. Please fix them.". => Logs show error "Patron creation failed! - [Child patron needs a guarantor]". 5. Apply this patch. 6. Create a guarantee patron again and save. => Patron should be saved correctly. 7. Create a new guarantee patron but this time don't add patron guarantor or fill non-patron guarantor inputs. 8. Attempt to save patron. => Save should fail with error "A child patron needs a guarantor." 9. Add patron guarantor who can also be a guarantee and save. => Save should fail with error "A guarantor cannot be a guarantee." Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy Signed-off-by: Magnus Enger <magnus@libriotech.no> Works as advertised. Signed-off-by: David Nind <david@davidnind.com> Created attachment 190768 [details] [review] Bug 39014: Add new method validate_guarantor Adding a new guarantee and storing it fails with "Creating a patron" selected in TrackLastPatronActivityTriggers when guarantors are set to be mandatory. The problem comes from Koha::Patron->store called in update_lastseen. Since we validate guarantor in Koha::Patron->store but don't store them in there, new call to store fails because it doesn't receive guarantor info as parameter. As a solution, we should validate guarantors in separated method. Validation can't be moved e.g. to Koha::Patron::Relationship->store since this could lead to situations where guarantee is stored without guarantor. This patch adds new method validate_guarantor and adjusts old guarantor requirements tests to match this new method. To test: 1. Select option "Creating a patron" in "TrackLastPatronActivityTriggers" syspref. 2. Enable syspref "ChildNeedsGuarantor" and make sure you have options in "borrowerRelationship" syspref. 3. Create a guarantee patron, fill all required fields and add guarantor for patron (either add Patron guarantor or fill Non-patron guarantor inputs). 4. Attempt to save patron. => Error box is displayed with text "The following fields are wrong. Please fix them.". => Logs show error "Patron creation failed! - [Child patron needs a guarantor]". 5. Apply this patch. 6. Create a guarantee patron again and save. => Patron should be saved correctly. 7. Create a new guarantee patron but this time don't add patron guarantor or fill non-patron guarantor inputs. 8. Attempt to save patron. => Save should fail with error "A child patron needs a guarantor." 9. Add patron guarantor who can also be a guarantee and save. => Save should fail with error "A guarantor cannot be a guarantee." Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy Signed-off-by: Magnus Enger <magnus@libriotech.no> Works as advertised. Signed-off-by: David Nind <david@davidnind.com> Created attachment 190769 [details] [review] Bug 39014: (follow-up) Refactoring and renaming to can_be_guaranteed_by To test: 1. prove t/db_dependent/Koha/Patron.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 190770 [details] [review] Bug 39014: (QA follow-up) Fix tidiness (In reply to Lari Taskula from comment #37) > Created attachment 190770 [details] [review] [review] > Bug 39014: (QA follow-up) Fix tidiness You folks should be using a KTD instance in the background at least so the commits are tidy automatically! Thanks for following up so fast, Lari. I am going to set this to FQA, we don't tidy commits. (In reply to Lucas Gass (lukeg) from comment #39) > Thanks for following up so fast, Lari. I am going to set this to FQA, we > don't tidy commits. We don't want to have tidy commits. Can you squash that patch? Created attachment 190772 [details] [review] Bug 39014: Add new method validate_guarantor Adding a new guarantee and storing it fails with "Creating a patron" selected in TrackLastPatronActivityTriggers when guarantors are set to be mandatory. The problem comes from Koha::Patron->store called in update_lastseen. Since we validate guarantor in Koha::Patron->store but don't store them in there, new call to store fails because it doesn't receive guarantor info as parameter. As a solution, we should validate guarantors in separated method. Validation can't be moved e.g. to Koha::Patron::Relationship->store since this could lead to situations where guarantee is stored without guarantor. This patch adds new method validate_guarantor and adjusts old guarantor requirements tests to match this new method. To test: 1. Select option "Creating a patron" in "TrackLastPatronActivityTriggers" syspref. 2. Enable syspref "ChildNeedsGuarantor" and make sure you have options in "borrowerRelationship" syspref. 3. Create a guarantee patron, fill all required fields and add guarantor for patron (either add Patron guarantor or fill Non-patron guarantor inputs). 4. Attempt to save patron. => Error box is displayed with text "The following fields are wrong. Please fix them.". => Logs show error "Patron creation failed! - [Child patron needs a guarantor]". 5. Apply this patch. 6. Create a guarantee patron again and save. => Patron should be saved correctly. 7. Create a new guarantee patron but this time don't add patron guarantor or fill non-patron guarantor inputs. 8. Attempt to save patron. => Save should fail with error "A child patron needs a guarantor." 9. Add patron guarantor who can also be a guarantee and save. => Save should fail with error "A guarantor cannot be a guarantee." Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy Signed-off-by: Magnus Enger <magnus@libriotech.no> Works as advertised. Signed-off-by: David Nind <david@davidnind.com> Created attachment 190773 [details] [review] Bug 39014: (follow-up) Refactoring and renaming to can_be_guaranteed_by To test: 1. prove t/db_dependent/Koha/Patron.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to Lucas Gass (lukeg) from comment #39) > Thanks for following up so fast, Lari. I am going to set this to FQA, we > don't tidy commits. Squasehd. I've set this back to passed QA :) Nice work everyone! Pushed to main for 26.05 Tests are failing now:
# Failed test 'AddReturn generates a warning when can_be_guarantee type patron is missing a guarantor and ChildNeedsGuarantor'
# at /kohadevbox/koha/t/db_dependent/Circulation.t line 7787.
# didn't find a warning
# expected to find warning: (?^u:Problem updating lastseen)
ok 7 - AddIssue generates no warning when Child type patron is missing a guarantor and not ChildsNeedsGuarantor
ok 8 - AddRenewal generates no warning when Child type patron is missing a guarantor and not ChildNeedsGuarantor
ok 9 - AddReturn generates no warning when Child type patron is missing a guarantor and not ChildNeedsGuarantor
ok 10 - AddIssue generates no warning when can_be_guarantee type patron is missing a guarantor and not ChildsNeedsGuarantor
ok 11 - AddRenewal generates no warning when can_be_guarantee type patron is missing a guarantor and not ChildNeedsGuarantor
ok 12 - AddReturn generates no warning when can_be_guarantee type patron is missing a guarantor and not ChildNeedsGuarantor
ok 13 - AddIssue generates no warning when Child type patron has a guarantor and ChildsNeedsGuarantor
ok 14 - AddRenewal generates no warning when Child type patron has a guarantor and not ChildNeedsGuarantor
ok 15 - AddReturn generates no warning when Child type patron has a guarantor and not ChildNeedsGuarantor
ok 16 - AddIssue generates no warning when can_be_guarantee type patron has a guarantor and not ChildsNeedsGuarantor
ok 17 - AddRenewal generates no warning when can_be_guarantee type patron has a guarantor and not ChildNeedsGuarantor
ok 18 - AddReturn generates no warning when can_be_guarantee type patron has a guarantor and not ChildNeedsGuarantor
# Looks like you failed 6 tests of 18.
not ok 79 - ChildNeedsGuarantor
Please follow-up
Hi, the above tests were also added fairly recently by Bug 39180 and it looks like these two Bugs have had some overlapping that was left unnoticed. After Emmi's patch we no longer throw those particular exceptions, so the tests expecting them (in the form of warnings) will fail. I suppose we could just remove those tests, unless we explicitly want to notify librarians of invalid patron relationships during circulation. Created attachment 191014 [details] [review] Bug 39014: Remove unnecessary tests I added a patch to remove the tests in question. I'm not convinced this is correct. Do we want to add a warn somewhere that can be logged? For troubleshooting/support? (In reply to Lucas Gass (lukeg) from comment #48) > I added a patch to remove the tests in question. I'm not convinced this is > correct. Do we want to add a warn somewhere that can be logged? For > troubleshooting/support? I don't see much value in logging that, as misconfigured patron relationships no longer stop Koha from performing circulation actions. Having librarians alerted on the staff client is in my opinion sufficient. Librarians are notified of invalid patron relationships on the patron's "Check out" and "Details" tab, thanks to Bug 39180. As for the check-in screen, I suppose we have lost that ability. (out of scope) It would also be useful to have a report that lists all the patrons with an invalid patron relationship/alerts them on the staff main page. follow-up pushed to main (In reply to Lari Taskula from comment #49) > (In reply to Lucas Gass (lukeg) from comment #48) > > I added a patch to remove the tests in question. I'm not convinced this is > > correct. Do we want to add a warn somewhere that can be logged? For > > troubleshooting/support? > > I don't see much value in logging that, as misconfigured patron > relationships no longer stop Koha from performing circulation actions. > Having librarians alerted on the staff client is in my opinion sufficient. > Librarians are notified of invalid patron relationships on the patron's > "Check out" and "Details" tab, thanks to Bug 39180. As for the check-in > screen, I suppose we have lost that ability. > > (out of scope) It would also be useful to have a report that lists all the > patrons with an invalid patron relationship/alerts them on the staff main > page. In the new catch, we specifically handle two types of exceptions, but we don't have an else case, so if another exception is thrown, it's just going to be ignored. I kind of think if we're throwing an exception, we should always log it. The librarian may see the alert, and then dismiss it, without resolving the problem, having it in the logs ensures it can be found later. I would actually lean towards making all exceptions log, every time - if we think it warrants an exception, I think it warrants logging (In reply to Nick Clemens (kidclamp) from comment #51) > In the new catch, we specifically handle two types of exceptions, but we > don't have an else case, so if another exception is thrown, it's just going > to be ignored. The other exceptions are handled in the follow-up patch. > I kind of think if we're throwing an exception, we should always log it. The > librarian may see the alert, and then dismiss it, without resolving the > problem, having it in the logs ensures it can be found later. I would > actually lean towards making all exceptions log, every time - if we think it > warrants an exception, I think it warrants logging It would be first useful to test the amount of data that would be logged in a production environment if we had this enabled. Personally I don't think we should do it by default. Every unhandled exception must be logged, but as for caught exceptions, I'd leave it up for the developer to decide. In my opinion not every exception we have is always logworthy, for example failing to meet minimum password requirements when setting a password (in many cases we have frontend validation to avoid a HTTP request/an exception, but there are also REST API clients that just pass Koha whatever input was given by the user). But then again someone else could find that useful. Maybe a patron calls the library and the library has so awesome customer support that the system administrator checks exception logs and finds that their password contained a whitespace (Koha::Exceptions::Password::WhitespaceCharacters logged). If we did something like that, the log level should probably be at most to 'trace'. |