Bug 14570

Summary: Make it possible to add multiple guarantors to a record
Product: Koha Reporter: Jesse Weaver <jweaver>
Component: PatronsAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: new feature    
Priority: P5 - low CC: agustinmoyano, amy, bdaeuber, clackman, dcook, fridolin.somers, george, gmcharlt, jdemuth, jonathan.druart, josef.moravec, katrin.fischer, kyle.m.hall, kyle, m.de.rooy, margaret, martin.renvoize, mtompset, nick, nicole, oleonard, tomascohen, wizzyrea
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12446
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25161
Change sponsored?: Sponsored Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This development adds the ability for a patron to have an unlimited number of guarantors in any combination of existing Koha patrons and manually added guarantors ( e.g. the guarantor has no patron record in Koha ). This feature retains the existing behaviour for importing guarantors during patron imports; However, guarantors can no longer be viewed, added or updated via the REST API. Reports that utilize `borrowers.guarantorid` will need to be updated.
Version(s) released in:
19.11.00
Bug Depends on: 14577, 17790    
Bug Blocks: 25645, 25854, 12446, 19382, 20211, 20691, 23655, 23778, 23905, 24113, 24215, 24263, 24317, 24666, 25654, 25742, 25914, 26059, 26995, 30472, 30534    
Attachments: Bug 14570 - Make it possible to add multiple guarantors to a record
Bug 14570 - Update Schema
Bug 14570 - Make it possible to add multiple guarantors to a record
Bug 14570 - Update Schema
Bug 14570 - Make it possible to add multiple guarantors to a record
Bug 14570 - Update Schema
Bug 14570 - Update Schema
Bug 14570 - Make it possible to add multiple guarantors to a record
Proposal for initial state of add guarantor form
Bug 14570 - Make it possible to add multiple guarantors to a record
Bug 14570 - Make it possible to add multiple guarantors to a record
Bug 14570 - Make it possible to add multiple guarantors to a record
Bug 14570 - Make it possible to add multiple guarantors to a record
Bug 14570 - Make it possible to add multiple guarantors to a record
Bug 14570 - Update Schema
Bug 14570 - Followup
Bug 14570 - Make it possible to add multiple guarantors to a record
Bug 14570 - Update Schema
Bug 14570 - Followup
Bug 14570 - Make it possible to add multiple guarantors to a record
Bug 14570 - Update Schema
Bug 14570 - Followup
Bug 14570 - Make it possible to add multiple guarantors to a record
Bug 14570 - Update Schema
Bug 14570 - Followup
Bug 14570 - Make it possible to add multiple guarantors to a record
Bug 14570 - Update Schema
Bug 14570 - Followup
Bug 14570 - Make it possible to add multiple guarantors to a record
Bug 14570 - Update Schema
Bug 14570 - Followup
Bug 14570 - Make it possible to add multiple guarantors to a record
Bug 14570 - Update Schema
Bug 14570 - Followup
Bug 14570 - Make it possible to add multiple guarantors to a record
Bug 14570 - Update Schema
Bug 14570 - Followup
Bug 14570 - Make it possible to add multiple guarantors to a record
Bug 14570 - Update Schema
Bug 14570 - Followup
[SIGNED-OFF] Bug 14570 - Make it possible to add multiple guarantors to a record
[SIGNED-OFF] Bug 14570 - Update Schema
[SIGNED-OFF] Bug 14570 - Followup
Bug 14570 - Make it possible to add multiple guarantors to a record
Bug 14570 - Followup
Bug 14570 - Update Schema
Bug 14570 - Make it possible to add multiple guarantors to a record
Bug 14570 - Update Schema
Bug 14570 - Make it possible to add multiple guarantors to a record
Bug 14570 - Update Schema
Bug 14570 - Make it possible to add multiple guarantors to a record
Bug 14570 - Update Schema
Bug 14570 - Make it possible to add multiple guarantors to a record
Bug 14570 - Update Schema
Bug 14570 - Make it possible to add multiple guarantors to a record
Bug 14570 - Update Schema
Bug 14570 [QA Followup] - Fix QA issues with POD
Bug 14570 [QA Followup] - Fix QA issues with POD
Bug 14570 [QA Followup] - Show add guarantor pane for correct patron types, fix "Add child" button
Bug 14570 - Make it possible to add multiple guarantors to a record
Bug 14570 - Update Schema
Bug 14570 [QA Followup] - Fix QA issues with POD
Bug 14570 [QA Followup] - Show add guarantor pane for correct patron types, fix "Add child" button
Bug 14570 - Make it possible to add multiple guarantors to a record
Bug 14570 [QA Followup] - Fix QA issues with POD
Bug 14570 [QA Followup] - Show add guarantor pane for correct patron types, fix "Add child" button
Bug 14570 - Update schema files [Do Not Push]
Bug 14570 [QA Followup] - Fix issue with only one new guarantor being saved
Bug 14570 [QA Followup]: Make delete label clickable
Bug 14570: Make it possible to add multiple guarantors to a record
Bug 14570 [QA Followup]: Fix QA issues with POD
Bug 14570 [QA Followup]: Show add guarantor pane for correct patron types, fix "Add child" button
Bug 14570: Update schema files [Do Not Push]
Bug 14570 [QA Followup]: Fix issue with only one new guarantor being saved
Bug 14570 [QA Followup]: Make delete label clickable
Bug 14570 [QA Followup]: Rename relationships table to borrower_relationships
Bug 14570 [QA Followup]: Add Koha::Patron::add_gurarantor
Bug 14570 [QA Followup]: change instance of param to multi_param
Bug 14570 [QA Followup]: Don't remove existing columns for the time being, pending review of request 3
Bug 14570: Make it possible to add multiple guarantors to a record
Bug 14570 [QA Followup]: Fix QA issues with POD
Bug 14570 [QA Followup]: Show add guarantor pane for correct patron types, fix "Add child" button
Bug 14570 [QA Followup]: Fix issue with only one new guarantor being saved
Bug 14570 [QA Followup]: Make delete label clickable
Bug 14570 [QA Followup]: Rename relationships table to borrower_relationships
Bug 14570 [QA Followup]: Add Koha::Patron::add_gurarantor
Bug 14570 [QA Followup]: change instance of param to multi_param
Bug 14570 [QA Followup]: Remove ability to add multiple non-koha patrons as guarantors, bring back contact fields
Bug 14570: Update schema files [Do Not Push]
Bug 14570: Make it possible to add multiple guarantors to a record
Bug 14570: Update schema files [Do Not Push]
Bug 14570: Make it possible to add multiple guarantors to a record
Bug 14570: Update schema files [Do Not Push]
Bug 14570: Make it possible to add multiple guarantors to a record
Bug 14570: Update schema files [Do Not Push]
Bug 14570: Remove double declaration of $patron in opac-user.pl
Bug 14570: Make it possible to add multiple guarantors to a record
Bug 14570: Update schema files [Do Not Push]
Bug 14570: Make it possible to add multiple guarantors to a record
Bug 14570: Update schema files [Do Not Push]
Bug 14570: Make it possible to add multiple guarantors to a record
Bug 14570: Update schema files [Do Not Push]
Bug 14570: (QA Follow-up) Misc changes based on QA feedback
Bug 14570: Make it possible to add multiple guarantors to a record
Bug 14570: Make it possible to add multiple guarantors to a record
Bug 14570: (QA Follow-up) Misc changes based on QA feedback
Bug 14570: Update schema files [Do Not Push]
Bug 14570: Make it possible to add multiple guarantors to a record
Bug 14570: Update schema files [Do Not Push]
Bug 14570: Make it possible to add multiple guarantors to a record
Bug 14570: Update schema files [Do Not Push]
Bug 14570: (follow-up) fix table name in j2a.pl
Bug 14570: (follow-up) fix qa FAIL tests
Bug 14570: (follow-up) Fix tests
Bug 14570: (QA follow-up) fix sample borrowers, remove guarantorid)
Bug 14570: (follow-up) Fix patron type when "add child" button is pressed
Bug 14570: Make it possible to add multiple guarantors to a record
Bug 14570: Update schema files [Do Not Push]
Bug 14570: (follow-up) fix table name in j2a.pl
Bug 14570: (follow-up) fix qa FAIL tests
Bug 14570: (follow-up) Fix tests
Bug 14570: (QA follow-up) fix sample borrowers, remove guarantorid)
Bug 14570: (follow-up) Fix patron type when "add child" button is pressed
Bug 14570: Make it possible to add multiple guarantors to a record
Bug 14570: (follow-up) fix table name in j2a.pl
Bug 14570: (follow-up) fix qa FAIL tests
Bug 14570: (follow-up) Fix tests
Bug 14570: (QA follow-up) fix sample borrowers, remove guarantorid)
Bug 14570: (follow-up) Fix patron type when "add child" button is pressed
Bug 14570: Update schema files [Do Not Push]
Bug 14570: (QA follow-up) Fix issues in Guarantee context
Bug 14570: (QA follow-up) Fix issues in Guarantor context
Bug 14570: Add error handling to Koha::Patron->add_guarantor
Bug 14570: Remove wrong parameter in add_guarantor call
Bug 14570: (QA follow-up) Consistency displaying the list of guarantors
Bug 14570: Add a UNIQUE constraint on guarantor_id+guarantee_id
Bug 14570: Add error handling to Koha::Patron::Relationship->store
Bug 14570: Remove wrong parameter in add_guarantor call
Bug 14570: (QA follow-up) Consistency displaying the list of guarantors
Bug 14570: Add a UNIQUE constraint on guarantor_id+guarantee_id
Bug 14570: Add error handling to Koha::Patron::Relationship->store
Bug 14570: Remove wrong parameter in add_guarantor call
Bug 14570: (QA follow-up) Consistency displaying the list of guarantors
Bug 14570: (QA follow-up) Fix tests
Bug 14570: (QA follow-up) Fix tests
Bug 14570: Make it possible to add multiple guarantors to a record
Bug 14570: (follow-up) fix table name in j2a.pl
Bug 14570: (follow-up) fix qa FAIL tests
Bug 14570: (follow-up) Fix tests
Bug 14570: (QA follow-up) fix sample borrowers, remove guarantorid)
Bug 14570: (follow-up) Fix patron type when "add child" button is pressed
Bug 14570: Update schema files [Do Not Push]
Bug 14570: (QA follow-up) Fix issues in Guarantee context
Bug 14570: (QA follow-up) Fix issues in Guarantor context
Bug 14570: Add a UNIQUE constraint on guarantor_id+guarantee_id
Bug 14570: Add error handling to Koha::Patron::Relationship->store
Bug 14570: Remove wrong parameter in add_guarantor call
Bug 14570: (QA follow-up) Consistency displaying the list of guarantors
Bug 14570: (QA follow-up) Fix tests
Bug 14570: Make it possible to add multiple guarantors to a record
Bug 14570: (follow-up) fix table name in j2a.pl
Bug 14570: (follow-up) fix qa FAIL tests
Bug 14570: (follow-up) Fix tests
Bug 14570: (QA follow-up) fix sample borrowers, remove guarantorid)
Bug 14570: (follow-up) Fix patron type when "add child" button is pressed
Bug 14570: Update schema files [Do Not Push]
Bug 14570: (QA follow-up) Fix issues in Guarantee context
Bug 14570: (QA follow-up) Fix issues in Guarantor context
Bug 14570: Add a UNIQUE constraint on guarantor_id+guarantee_id
Bug 14570: Add error handling to Koha::Patron::Relationship->store
Bug 14570: Remove wrong parameter in add_guarantor call
Bug 14570: (QA follow-up) Consistency displaying the list of guarantors
Bug 14570: (QA follow-up) Fix tests
Bug 14570: Make it possible to add multiple guarantors to a record
Bug 14570: (follow-up) fix table name in j2a.pl
Bug 14570: (follow-up) fix qa FAIL tests
Bug 14570: (follow-up) Fix tests
Bug 14570: (QA follow-up) fix sample borrowers, remove guarantorid)
Bug 14570: (follow-up) Fix patron type when "add child" button is pressed
Bug 14570: Update schema files [Do Not Push]
Bug 14570: (QA follow-up) Fix issues in Guarantee context
Bug 14570: (QA follow-up) Fix issues in Guarantor context
Bug 14570: Add a UNIQUE constraint on guarantor_id+guarantee_id
Bug 14570: Add error handling to Koha::Patron::Relationship->store
Bug 14570: Remove wrong parameter in add_guarantor call
Bug 14570: (QA follow-up) Consistency displaying the list of guarantors
Bug 14570: (QA follow-up) Fix tests
Bug 14570: (QA follow-up) Consistency displaying the list of guarantors
Bug 14570: (QA follow-up) Fix tests
Bug 14570: Handle missing 'relationship' in atomicupdate (bad data)
Bug 14570: (QA follow-up) Call Koha::Patron->find in scalar context
Bug 14570: Add about.pl entry for wrong relationship data

Description Jesse Weaver 2015-07-20 22:36:10 UTC

    
Comment 1 HB-NEKLS 2015-10-16 01:24:59 UTC
I just came into Bugzilla tonight looking to see if this feature had ever been requested and found this bug. Is this under development yet? What's been proposed? 

Also, how will this data be stored in the database? Any discussion/thoughts about creating a separate guarantor table and no longer storing the guarantor borrowernumber in the guarantee's borrower record?
Comment 2 Katrin Fischer 2015-10-24 23:54:11 UTC
I think in order to have n:n relationships separate tables will be needed. Would also like to see some more information about this!
Comment 3 Katrin Fischer 2015-10-29 07:28:43 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2016-06-14 16:45:04 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2016-06-14 16:45:16 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2016-06-14 17:06:38 UTC
Test plan will be forthcoming.
Comment 7 Kyle M Hall 2016-06-14 17:50:58 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2016-06-14 17:51:09 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2016-06-14 18:09:17 UTC
Working on unit tests.
Comment 10 Kyle M Hall 2016-06-14 18:22:19 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2016-06-14 18:22:27 UTC Comment hidden (obsolete)
Comment 12 Kyle M Hall 2016-12-12 18:30:22 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2016-12-12 18:30:32 UTC Comment hidden (obsolete)
Comment 14 Owen Leonard 2016-12-13 14:45:39 UTC
Created attachment 58159 [details]
Proposal for initial state of add guarantor form

It seems that now the search for patrons to select as a guarantor includes patrons with a 'Child' type patron category. Is that intentional? Shouldn't they be excluded?

I notice also that when you select a patron to be a guarantor, the patron number is no longer a link to the patron record. I think it would make more sense to not show the borrowernumber at all, but make the guarantor name a link:

<a href="moredetail.pl?borrowernumber=0000">Surname, Firstname (cardnumber)</a>

I think it's confusing that there are two buttons, 'Add guarantor', 'Set to patron.' They could potentially mean the same thing. I think we might have the initial state of this form have only two links: 'Search to add guarantor' and 'Add guarantor manually' (see attached image).

Clicking the 'Add manually' link would display the form along with a submit button, and the "Clear" link would be shown.

The manual add form should prevent the user from submitting empty data.

Sorry, I know that's a lot, and I'm willing to help with follow-ups if folks think these are good ideas.
Comment 15 Kyle M Hall 2016-12-15 14:02:54 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2016-12-22 14:21:59 UTC
All in all I like what you are proposing! Can you add a followup for your suggestions?

(In reply to Owen Leonard from comment #14)
> Created attachment 58159 [details]
> Proposal for initial state of add guarantor form
> 
> It seems that now the search for patrons to select as a guarantor includes
> patrons with a 'Child' type patron category. Is that intentional? Shouldn't
> they be excluded?
> 
> I notice also that when you select a patron to be a guarantor, the patron
> number is no longer a link to the patron record. I think it would make more
> sense to not show the borrowernumber at all, but make the guarantor name a
> link:
> 
> <a href="moredetail.pl?borrowernumber=0000">Surname, Firstname
> (cardnumber)</a>
> 
> I think it's confusing that there are two buttons, 'Add guarantor', 'Set to
> patron.' They could potentially mean the same thing. I think we might have
> the initial state of this form have only two links: 'Search to add
> guarantor' and 'Add guarantor manually' (see attached image).
> 
> Clicking the 'Add manually' link would display the form along with a submit
> button, and the "Clear" link would be shown.
> 
> The manual add form should prevent the user from submitting empty data.
> 
> Sorry, I know that's a lot, and I'm willing to help with follow-ups if folks
> think these are good ideas.
Comment 17 Kyle M Hall 2016-12-22 15:32:14 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2016-12-22 17:36:17 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2016-12-23 11:20:36 UTC Comment hidden (obsolete)
Comment 20 Owen Leonard 2017-01-03 14:14:32 UTC
Auto-merging Koha/Schema/Result/BorrowerModification.pm
CONFLICT (content): Merge conflict in Koha/Schema/Result/BorrowerModification.pm
Failed to merge in the changes.
Comment 21 Kyle M Hall 2017-01-09 16:23:47 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2017-01-09 16:23:54 UTC Comment hidden (obsolete)
Comment 23 Nick Clemens 2017-01-10 11:50:23 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2017-01-10 16:28:39 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2017-01-10 16:28:46 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2017-01-10 16:28:49 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall 2017-01-11 17:16:35 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2017-01-11 17:16:46 UTC Comment hidden (obsolete)
Comment 29 Kyle M Hall 2017-01-11 17:16:49 UTC Comment hidden (obsolete)
Comment 30 Kyle M Hall 2017-01-11 17:34:11 UTC Comment hidden (obsolete)
Comment 31 Kyle M Hall 2017-01-11 17:34:19 UTC Comment hidden (obsolete)
Comment 32 Kyle M Hall 2017-01-11 17:34:23 UTC Comment hidden (obsolete)
Comment 33 Kyle M Hall 2017-01-13 11:50:58 UTC Comment hidden (obsolete)
Comment 34 Kyle M Hall 2017-01-13 11:51:06 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2017-01-13 11:51:10 UTC Comment hidden (obsolete)
Comment 36 Kyle M Hall 2017-01-13 11:56:03 UTC Comment hidden (obsolete)
Comment 37 Kyle M Hall 2017-01-13 11:56:10 UTC Comment hidden (obsolete)
Comment 38 Kyle M Hall 2017-01-13 11:56:13 UTC Comment hidden (obsolete)
Comment 39 Mark Tompsett 2017-01-13 19:41:09 UTC Comment hidden (obsolete)
Comment 40 Mark Tompsett 2017-01-13 19:41:14 UTC Comment hidden (obsolete)
Comment 41 Mark Tompsett 2017-01-13 19:41:20 UTC Comment hidden (obsolete)
Comment 42 Mark Tompsett 2017-01-13 19:46:15 UTC Comment hidden (obsolete)
Comment 43 Mark Tompsett 2017-01-13 19:46:22 UTC Comment hidden (obsolete)
Comment 44 Mark Tompsett 2017-01-13 19:46:27 UTC Comment hidden (obsolete)
Comment 45 Mark Tompsett 2017-01-13 19:54:26 UTC Comment hidden (obsolete)
Comment 46 Mark Tompsett 2017-01-13 19:54:32 UTC Comment hidden (obsolete)
Comment 47 Mark Tompsett 2017-01-13 19:54:39 UTC Comment hidden (obsolete)
Comment 48 Mark Tompsett 2017-01-13 19:55:27 UTC
Rebased, and corrected atomic update filename. :)
Comment 49 Kyle M Hall 2017-04-20 11:17:27 UTC Comment hidden (obsolete)
Comment 50 Kyle M Hall 2017-04-20 11:17:36 UTC Comment hidden (obsolete)
Comment 51 Kyle M Hall 2017-04-20 11:17:41 UTC Comment hidden (obsolete)
Comment 52 Jonathan Druart 2017-04-20 18:46:56 UTC
Very quick code review, did not test much:

1/ QA tests fail:
 FAIL   Koha/Patron/Relationships.pm
   FAIL   pod
                *** ERROR: 
                 in file Koha/Patron/Relationships.pm
                Spurious =cut command

 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
   FAIL   forbidden patterns
                forbidden pattern: Contains old bootstrap style for buttons (see bug 16239) (line 535)
                forbidden pattern: Contains old bootstrap style for buttons (see bug 16239) (line 536)

2/ Can set several fathers from the interface, but when I save only one is kept. Sounds like we should handle that from the interface and not display more than 2 guarantors (and not more than 1 with the same relationship).

3/ In misc/cronjobs/j2a.pl
+    $dbh->{AutoCommit} = 0;
You should use ->txn_begin
Comment 53 Jonathan Druart 2017-04-20 18:47:18 UTC
What about comment 14?
Comment 54 Owen Leonard 2017-04-21 11:52:07 UTC
(In reply to Jonathan Druart from comment #52)
> Sounds like we should handle that from the interface and not display
> more than 2 guarantors (and not more than 1 with the same relationship).

I don't think we should restrict how many guarantors an account has nor how many of the same relationships appear.
Comment 55 Kyle M Hall 2017-06-09 10:29:50 UTC Comment hidden (obsolete)
Comment 56 Kyle M Hall 2017-06-09 10:29:59 UTC Comment hidden (obsolete)
Comment 57 Kyle M Hall 2017-06-09 10:30:04 UTC Comment hidden (obsolete)
Comment 58 Kyle M Hall 2018-02-02 18:50:35 UTC Comment hidden (obsolete)
Comment 59 Kyle M Hall 2018-02-02 18:50:44 UTC Comment hidden (obsolete)
Comment 60 Kyle M Hall 2018-02-02 18:59:47 UTC Comment hidden (obsolete)
Comment 61 Kyle M Hall 2018-02-02 18:59:57 UTC Comment hidden (obsolete)
Comment 62 Kyle M Hall 2018-02-02 19:02:21 UTC
> 
> I think it's confusing that there are two buttons, 'Add guarantor', 'Set to
> patron.' They could potentially mean the same thing. I think we might have
> the initial state of this form have only two links: 'Search to add
> guarantor' and 'Add guarantor manually' (see attached image).
> 
> Clicking the 'Add manually' link would display the form along with a submit
> button, and the "Clear" link would be shown.
> 
> The manual add form should prevent the user from submitting empty data.
> 
> Sorry, I know that's a lot, and I'm willing to help with follow-ups if folks
> think these are good ideas.

I'm absolutely down with that. Are you able to post a followup for that?
Comment 63 Kyle M Hall 2018-03-26 14:11:44 UTC Comment hidden (obsolete)
Comment 64 Kyle M Hall 2018-03-26 14:11:55 UTC Comment hidden (obsolete)
Comment 65 Katrin Fischer 2018-03-31 07:26:42 UTC
Hi Kyle, patch doesn't apply because of recent changes to the patrons api. Can you please rebase?
Comment 66 Kyle M Hall 2018-04-03 13:15:21 UTC Comment hidden (obsolete)
Comment 67 Kyle M Hall 2018-04-03 13:15:29 UTC Comment hidden (obsolete)
Comment 68 Katrin Fischer 2018-04-15 11:01:02 UTC
Comment on attachment 73583 [details] [review]
Bug 14570 - Make it possible to add multiple guarantors to a record

Review of attachment 73583 [details] [review]:
-----------------------------------------------------------------

Hi Kyle,

this is a huge patch with several consequences. Lots to test. Not nearly done yet.

1) Release notes
Should state consequences to:
- patron import (no change, but can only import with one guarantor?)
- LDAP and SIP (lines where changed, but only in POD?)
- patron REST API (guarantor information can't be changed or asked for)
- Reports using borrowers.guarantorid have to be changed/rewritten
...

2) QA script
 FAIL	opac/opac-user.pl
   FAIL	  valid
		"my" variable $patron masks earlier declaration in same scope

 FAIL	Koha/Patron/Relationships.pm
   FAIL	  pod 
		Spurious =cut command
		*** ERROR: 
		 in file Koha/Patron/Relationships.pm
   FAIL	  pod coverage
		POD is missing for 'object_class'

 FAIL	Koha/Patron.pm
   FAIL	  pod coverage
		POD is missing for 'has_permission'

3) New unit tests pass.

4) Tests in GUI

Testing this, something doesn't seem to be quite right. 

a) I tried adding a guarantor from a Teacher (Professional) and from a Kid (Child) account. Both don't offer the form to me.

b) Using the 'add child' from an existing patron (Adult), a new form opens for an adult patron category, the guarantor information is empty.


5) Code review (splinter):

::: Koha/Patron.pm
@@ +119,5 @@
>  
>      return scalar Koha::Patron::Images->find( $self->borrowernumber );
>  }
>  
> +=head3 library

Please add a description here too, not just an empty entry. QA tools will be happy with this, but the missing POD never fixed.

@@ +135,5 @@
>  
> +Returns the set of relationships for the patrons that are guarantors for this patron.
> +
> +This is returned instead of a Koha::Patron object because the guarantor
> +may not exist as a patron in Koha. If this is true, the guarantors name

This is quite a change - is it really implemented in this patch set? What's the use case? Just a name is not a lot to locate the person if needed.

::: koha-tmpl/intranet-tmpl/prog/en/includes/members-toolbar.inc
@@ -17,5 @@
>      [% IF CAN_user_borrowers_edit_borrowers %]
>          [% IF patron.is_adult AND Koha.Preference("borrowerRelationship") %]
>              <a id="addchild" class="btn btn-default btn-sm" href="/cgi-bin/koha/members/memberentry.pl?op=add&amp;guarantorid=[% patron.borrowernumber %]"><i class="fa fa-plus"></i> Add child</a>
>          [% END %]
> -        [% IF CAN_user_borrowers_edit_borrowers %]

This change doesn't look like it was intentional/related. Please check.
Comment 69 Kyle M Hall 2018-05-01 15:07:25 UTC Comment hidden (obsolete)
Comment 70 Kyle M Hall 2018-05-01 15:07:34 UTC Comment hidden (obsolete)
Comment 71 Kyle M Hall 2018-05-01 15:07:37 UTC Comment hidden (obsolete)
Comment 72 Kyle M Hall 2018-05-01 15:36:34 UTC Comment hidden (obsolete)
Comment 73 Kyle M Hall 2018-05-01 16:03:19 UTC
> 1) Release notes
> Should state consequences to:
> - patron import (no change, but can only import with one guarantor?)
> - LDAP and SIP (lines where changed, but only in POD?)
> - patron REST API (guarantor information can't be changed or asked for)
> - Reports using borrowers.guarantorid have to be changed/rewritten

Added. Left out LDAP/SIP since there is literally no change in behavior. I updated the documentation since it's there but it should really be removed from the codebase altogether.

> 2) QA script
>  FAIL	opac/opac-user.pl
>    FAIL	  valid
> 		"my" variable $patron masks earlier declaration in same scope
> 
>  FAIL	Koha/Patron/Relationships.pm
>    FAIL	  pod 
> 		Spurious =cut command
> 		*** ERROR: 
> 		 in file Koha/Patron/Relationships.pm
>    FAIL	  pod coverage
> 		POD is missing for 'object_class'
> 
>  FAIL	Koha/Patron.pm
>    FAIL	  pod coverage
> 		POD is missing for 'has_permission'

Fixed! QA script is giving me invalid TT syntax failures for some reason. xt/tt_valid.t passes and everything works in actual testing.

 
> a) I tried adding a guarantor from a Teacher (Professional) and from a Kid
> (Child) account. Both don't offer the form to me.
> 
> b) Using the 'add child' from an existing patron (Adult), a new form opens
> for an adult patron category, the guarantor information is empty.

Fixed!
 
> 
> 5) Code review (splinter):
> 
> ::: Koha/Patron.pm
> @@ +119,5 @@
> >  
> >      return scalar Koha::Patron::Images->find( $self->borrowernumber );
> >  }
> >  
> > +=head3 library
> 
> Please add a description here too, not just an empty entry. QA tools will be
> happy with this, but the missing POD never fixed.

Fixed!
 
> @@ +135,5 @@
> >  
> > +Returns the set of relationships for the patrons that are guarantors for this patron.
> > +
> > +This is returned instead of a Koha::Patron object because the guarantor
> > +may not exist as a patron in Koha. If this is true, the guarantors name
> 
> This is quite a change - is it really implemented in this patch set? What's
> the use case? Just a name is not a lot to locate the person if needed.

It's not a change, it's a new subroutine. I'm not sure I understand what you mean. The use case is explained in the comment. We can't return Koha::Patron's because not every guarantor is one. So instead we return the guarantor relationships from which a Koha::Patron can be gotten if there is one, otherwise the guarantor info ( name, relationship, etc ) can be read directly from the guarantor relationship object.

> ::: koha-tmpl/intranet-tmpl/prog/en/includes/members-toolbar.inc
> @@ -17,5 @@
> >      [% IF CAN_user_borrowers_edit_borrowers %]
> >          [% IF patron.is_adult AND Koha.Preference("borrowerRelationship") %]
> >              <a id="addchild" class="btn btn-default btn-sm" href="/cgi-bin/koha/members/memberentry.pl?op=add&amp;guarantorid=[% patron.borrowernumber %]"><i class="fa fa-plus"></i> Add child</a>
> >          [% END %]
> > -        [% IF CAN_user_borrowers_edit_borrowers %]
> 
> This change doesn't look like it was intentional/related. Please check.

This change is intentional. Looks at the  IF above it. We have two nested if statements checking the same thing. There was no need for the inner one so I removed it.
Comment 74 Kyle M Hall 2018-05-01 16:04:11 UTC Comment hidden (obsolete)
Comment 75 Kyle M Hall 2018-05-02 13:18:31 UTC Comment hidden (obsolete)
Comment 76 Kyle M Hall 2018-05-02 13:18:41 UTC Comment hidden (obsolete)
Comment 77 Kyle M Hall 2018-05-02 13:18:45 UTC Comment hidden (obsolete)
Comment 78 Kyle M Hall 2018-05-02 13:18:50 UTC Comment hidden (obsolete)
Comment 79 Jonathan Druart 2018-06-19 16:28:29 UTC
Some more QA comments, I have not checked if previous requests have been addressed.

1. Bug - add or edit a patron and add 2 fathers, one from an existing patron and the other not. Only 1 is saved.

2. On the edit patron view, the "Delete" checkbox does not have the correct label (i.e. when you click on it, it will not check/uncheck the checkbox).
(related to comment 52, 2/)

3.
a. It is not clear to me why you removed the contact* fields from the borrowers table. It looks like they were also used when no guarantor existed.
b. Moreover `git grep contactfirstname|grep -v altcontactfirstname` returns occurrences that should be removed (if we really want to remove them)
c. The fields are not removed from borrowermodification and deletedborrowers (from the atomic update)

d. IMPORTANT - Should not we finally keep these fields and force the guarantors to be existing patrons? We could have only 1 "contact" and several guarantors. It seems that it will simplify a lot the code (for instance Koha::Patron->guarantors would return Koha::Patrons, and Koha::Patron::Relationship[s] would stay simple Koha::Object[s])

4. Should not the table name "relationships" be more specific? ("borrower_relationships"?)

5. add_guarantors should be moved to Koha::Patron

6. Koha::Patron->siblings is weird, we should not need to refetch the patron list.
Comment 80 Kyle M Hall 2018-07-17 10:17:40 UTC Comment hidden (obsolete)
Comment 81 Kyle M Hall 2018-07-17 10:17:57 UTC Comment hidden (obsolete)
Comment 82 Kyle M Hall 2018-07-17 10:18:04 UTC Comment hidden (obsolete)
Comment 83 Kyle M Hall 2018-07-17 10:18:10 UTC Comment hidden (obsolete)
Comment 84 Kyle M Hall 2018-07-17 10:18:17 UTC Comment hidden (obsolete)
Comment 85 Kyle M Hall 2018-07-17 10:18:23 UTC Comment hidden (obsolete)
Comment 86 Kyle M Hall 2018-07-17 14:24:56 UTC Comment hidden (obsolete)
Comment 87 Kyle M Hall 2018-07-17 14:25:16 UTC Comment hidden (obsolete)
Comment 88 Kyle M Hall 2018-07-17 14:25:21 UTC Comment hidden (obsolete)
Comment 89 Kyle M Hall 2018-07-17 14:25:27 UTC Comment hidden (obsolete)
Comment 90 Kyle M Hall 2018-07-17 14:25:32 UTC Comment hidden (obsolete)
Comment 91 Kyle M Hall 2018-07-17 14:25:37 UTC Comment hidden (obsolete)
Comment 92 Kyle M Hall 2018-07-17 14:25:43 UTC Comment hidden (obsolete)
Comment 93 Kyle M Hall 2018-07-17 14:25:48 UTC Comment hidden (obsolete)
Comment 94 Kyle M Hall 2018-07-17 14:25:53 UTC Comment hidden (obsolete)
Comment 95 Kyle M Hall 2018-07-17 14:25:59 UTC Comment hidden (obsolete)
Comment 96 Kyle M Hall 2018-07-17 14:26:25 UTC
(In reply to Jonathan Druart from comment #79)
> Some more QA comments, I have not checked if previous requests have been
> addressed.
> 
> 1. Bug - add or edit a patron and add 2 fathers, one from an existing patron
> and the other not. Only 1 is saved.

Fixed in followup.

> 2. On the edit patron view, the "Delete" checkbox does not have the correct
> label (i.e. when you click on it, it will not check/uncheck the checkbox).
> (related to comment 52, 2/)

Fixed in followup.

> 3.
> a. It is not clear to me why you removed the contact* fields from the
> borrowers table. It looks like they were also used when no guarantor existed.
> b. Moreover `git grep contactfirstname|grep -v altcontactfirstname` returns
> occurrences that should be removed (if we really want to remove them)
> c. The fields are not removed from and deletedborrowers
> (from the atomic update)
> 
> d. IMPORTANT - Should not we finally keep these fields and force the
> guarantors to be existing patrons? We could have only 1 "contact" and
> several guarantors. It seems that it will simplify a lot the code (for
> instance Koha::Patron->guarantors would return Koha::Patrons, and
> Koha::Patron::Relationship[s] would stay simple Koha::Object[s])

I'm going to have to ruminate on this and find out what the sponsors requirements are. I like how it would simplify the code, but it would also provide limitations that have already been overcome in the current patch set.

> 4. Should not the table name "relationships" be more specific?
> ("borrower_relationships"?)

Done in followup

> 5. add_guarantors should be moved to Koha::Patron

Done in followup

> 6. Koha::Patron->siblings is weird, we should not need to refetch the patron
> list.

I have no idea how to accomplish that. As far as I know there is no way to merge two dbic resultsets.
Comment 97 Kyle M Hall 2018-07-18 13:28:46 UTC Comment hidden (obsolete)
Comment 98 Kyle M Hall 2018-07-18 13:29:01 UTC Comment hidden (obsolete)
Comment 99 Kyle M Hall 2018-07-18 13:29:06 UTC Comment hidden (obsolete)
Comment 100 Kyle M Hall 2018-07-18 13:29:12 UTC Comment hidden (obsolete)
Comment 101 Kyle M Hall 2018-07-18 13:29:17 UTC Comment hidden (obsolete)
Comment 102 Kyle M Hall 2018-07-18 13:29:23 UTC Comment hidden (obsolete)
Comment 103 Kyle M Hall 2018-07-18 13:29:28 UTC Comment hidden (obsolete)
Comment 104 Kyle M Hall 2018-07-18 13:29:34 UTC Comment hidden (obsolete)
Comment 105 Kyle M Hall 2018-07-18 13:29:40 UTC Comment hidden (obsolete)
Comment 106 Kyle M Hall 2018-07-18 13:29:45 UTC Comment hidden (obsolete)
Comment 107 Kyle M Hall 2018-07-18 13:33:29 UTC
Squashed patch for ease of reading the entire change set: https://github.com/bywatersolutions/bywater-koha-devel/commit/be99e2aa23c3896e9c0aaedf464a5141d676556d
Comment 108 Kyle M Hall 2018-07-18 13:41:22 UTC
Created attachment 77077 [details] [review]
Bug 14570: Make it possible to add multiple guarantors to a record

This patch adds the ability to set an unlimited number of guarantors
for a given patron. As before, each guarantor may be linked to another
Koha patron, and all the behavior that applies to a given guarantor
remains the same.

Test Plan:
 1) Apply this patch
 2) Run updatedatabase.pl
 3) Find some patrons with guarantors, verify the still have their guarantor
 4) Test adding and removing guarantors on a patron record, both Koha users and not
 5) Verify the "Add child" button works
 6) Verify NoIssuesChargeGuarantees still works
 7) Verify tools/cleanborrowers.pl will not delete a guarantor
 8) Verify the guarantors are displayed on moremember.pl
 9) Verify the guarantor is removed by members/update-child.pl
10) Verify the guarantor is removed by misc/cronjobs/j2a.pl
11) Verify import patrons converts guarantor_id, relationship, contactfirstname,
  and contactsurname into a guarantor
12) prove t/Patron.t
13) prove t/db_dependent/Circulation.t
14) prove t/db_dependent/Circulation/NoIssuesChargeGuarantees.t
15) prove t/db_dependent/Items.t
16) prove t/db_dependent/Koha/Patrons.t
17) prove t/db_dependent/Members.t
18) prove t/db_dependent/Patron/Relationships.t

Signed-off-by: Kim Peine <kmpeine@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 109 Kyle M Hall 2018-07-18 13:41:35 UTC
Created attachment 77078 [details] [review]
Bug 14570: Update schema files [Do Not Push]
Comment 110 Jonathan Druart 2018-07-20 13:51:05 UTC
CONFLICT (content): Merge conflict in t/db_dependent/Members.t
CONFLICT (content): Merge conflict in members/update-child.pl
CONFLICT (content): Merge conflict in members/memberentry.pl
CONFLICT (content): Merge conflict in Koha/Item.pm
CONFLICT (content): Merge conflict in C4/Members.pm
Comment 111 Kyle M Hall 2018-07-26 14:02:42 UTC
Created attachment 77290 [details] [review]
Bug 14570: Make it possible to add multiple guarantors to a record

This patch adds the ability to set an unlimited number of guarantors
for a given patron. As before, each guarantor may be linked to another
Koha patron, and all the behavior that applies to a given guarantor
remains the same.

Test Plan:
 1) Apply this patch
 2) Run updatedatabase.pl
 3) Find some patrons with guarantors, verify the still have their guarantor
 4) Test adding and removing guarantors on a patron record, both Koha users and not
 5) Verify the "Add child" button works
 6) Verify NoIssuesChargeGuarantees still works
 7) Verify tools/cleanborrowers.pl will not delete a guarantor
 8) Verify the guarantors are displayed on moremember.pl
 9) Verify the guarantor is removed by members/update-child.pl
10) Verify the guarantor is removed by misc/cronjobs/j2a.pl
11) Verify import patrons converts guarantor_id, relationship, contactfirstname,
  and contactsurname into a guarantor
12) prove t/Patron.t
13) prove t/db_dependent/Circulation.t
14) prove t/db_dependent/Circulation/NoIssuesChargeGuarantees.t
15) prove t/db_dependent/Items.t
16) prove t/db_dependent/Koha/Patrons.t
17) prove t/db_dependent/Members.t
18) prove t/db_dependent/Patron/Relationships.t

Signed-off-by: Kim Peine <kmpeine@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 112 Kyle M Hall 2018-07-26 14:02:56 UTC
Created attachment 77291 [details] [review]
Bug 14570: Update schema files [Do Not Push]
Comment 113 Jonathan Druart 2018-07-31 18:38:55 UTC
(In reply to Kyle M Hall from comment #96)
> > d. IMPORTANT - Should not we finally keep these fields and force the
> > guarantors to be existing patrons? We could have only 1 "contact" and
> > several guarantors. It seems that it will simplify a lot the code (for
> > instance Koha::Patron->guarantors would return Koha::Patrons, and
> > Koha::Patron::Relationship[s] would stay simple Koha::Object[s])
> 
> I'm going to have to ruminate on this and find out what the sponsors
> requirements are. I like how it would simplify the code, but it would also
> provide limitations that have already been overcome in the current patch set.

What did you finally decide?
Comment 114 Jonathan Druart 2018-07-31 18:40:24 UTC
Created attachment 77346 [details] [review]
Bug 14570: Make it possible to add multiple guarantors to a record

This patch adds the ability to set an unlimited number of guarantors
for a given patron. As before, each guarantor may be linked to another
Koha patron, and all the behavior that applies to a given guarantor
remains the same.

Test Plan:
 1) Apply this patch
 2) Run updatedatabase.pl
 3) Find some patrons with guarantors, verify the still have their guarantor
 4) Test adding and removing guarantors on a patron record, both Koha users and not
 5) Verify the "Add child" button works
 6) Verify NoIssuesChargeGuarantees still works
 7) Verify tools/cleanborrowers.pl will not delete a guarantor
 8) Verify the guarantors are displayed on moremember.pl
 9) Verify the guarantor is removed by members/update-child.pl
10) Verify the guarantor is removed by misc/cronjobs/j2a.pl
11) Verify import patrons converts guarantor_id, relationship, contactfirstname,
  and contactsurname into a guarantor
12) prove t/Patron.t
13) prove t/db_dependent/Circulation.t
14) prove t/db_dependent/Circulation/NoIssuesChargeGuarantees.t
15) prove t/db_dependent/Items.t
16) prove t/db_dependent/Koha/Patrons.t
17) prove t/db_dependent/Members.t
18) prove t/db_dependent/Patron/Relationships.t

Signed-off-by: Kim Peine <kmpeine@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 115 Jonathan Druart 2018-07-31 18:40:31 UTC
Created attachment 77347 [details] [review]
Bug 14570: Update schema files [Do Not Push]
Comment 116 Jonathan Druart 2018-07-31 18:40:37 UTC
Created attachment 77348 [details] [review]
Bug 14570: Remove double declaration of $patron in opac-user.pl
Comment 117 Jonathan Druart 2018-07-31 19:07:43 UTC
- Are you sure we need the change to Koha::Object?

- koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
  [% To.json(data) %] must have the html filter

- changes to opac-user.tt are wrong (not needed)

- There are too many (unneeded) changes made to members/update-child.pl, it will conflict with bug 20226 (not blocker)

- misc/cronjobs/j2a.pl transaction must be done using txn_begin/txn_commit/txn_rollback

- C4::Installer::load_sql returned the following errors while attempting to load /home/vagrant/kohaclone/installer/data/mysql/atomicupdate/bug_14570.sql:

I do not know what the error is.

- Charset/collate:
relationship VARCHAR(100) COLLATE utf8_unicode_ci NOT NULL,
=> this is wrong

- same in the atomic update
Comment 118 Jonathan Druart 2018-07-31 19:10:03 UTC
I cannot test it, when I save a new patron with a guarantor I get 
No property new_guarantor_surname for Koha::Patron at /usr/share/perl5/Exception/Class/Base.pm line 73

When I click "Add Child", I land on a "Add patron (Adult)" page.

I would suggest to request another signoff after the re-submission.
Comment 119 Jonathan Druart 2018-08-02 15:27:38 UTC
Should be rebased on top of bug 18635.
Comment 120 Kyle M Hall 2018-10-18 13:45:24 UTC Comment hidden (obsolete)
Comment 121 Kyle M Hall 2018-10-18 14:06:19 UTC Comment hidden (obsolete)
Comment 122 Kyle M Hall 2018-11-05 13:13:39 UTC
Created attachment 81932 [details] [review]
Bug 14570: Make it possible to add multiple guarantors to a record

This patch adds the ability to set an unlimited number of guarantors
for a given patron. As before, each guarantor may be linked to another
Koha patron, and all the behavior that applies to a given guarantor
remains the same.

Test Plan:
 1) Apply this patch
 2) Run updatedatabase.pl
 3) Find some patrons with guarantors, verify the still have their guarantor
 4) Test adding and removing guarantors on a patron record, both Koha users and not
 5) Verify the "Add child" button works
 6) Verify NoIssuesChargeGuarantees still works
 7) Verify tools/cleanborrowers.pl will not delete a guarantor
 8) Verify the guarantors are displayed on moremember.pl
 9) Verify the guarantor is removed by members/update-child.pl
10) Verify the guarantor is removed by misc/cronjobs/j2a.pl
11) Verify import patrons converts guarantor_id, relationship, contactfirstname,
  and contactsurname into a guarantor
12) prove t/Patron.t
13) prove t/db_dependent/Circulation.t
14) prove t/db_dependent/Circulation/NoIssuesChargeGuarantees.t
15) prove t/db_dependent/Items.t
16) prove t/db_dependent/Koha/Patrons.t
17) prove t/db_dependent/Members.t
18) prove t/db_dependent/Patron/Relationships.t

Signed-off-by: Kim Peine <kmpeine@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 123 Kyle M Hall 2018-11-05 13:13:55 UTC
Created attachment 81933 [details] [review]
Bug 14570: Update schema files [Do Not Push]
Comment 124 Jonathan Druart 2018-12-13 18:52:38 UTC
Did you address comment 117 and comment 118?
Comment 125 Katrin Fischer 2019-02-15 07:21:34 UTC
Hi Kyle, 

patch doesn't apply. :( Please also take a look at comments 117 and 118.

I'm adding Tomas to get an opinion on the API note I saw in the text for release notes too. "Guarantors can no longer be viewed/added/updated from the REST API."
Comment 126 Kyle M Hall 2019-02-20 20:03:35 UTC
Created attachment 85411 [details] [review]
Bug 14570: Make it possible to add multiple guarantors to a record

This patch adds the ability to set an unlimited number of guarantors
for a given patron. As before, each guarantor may be linked to another
Koha patron, and all the behavior that applies to a given guarantor
remains the same.

Test Plan:
 1) Apply this patch
 2) Run updatedatabase.pl
 3) Find some patrons with guarantors, verify the still have their guarantor
 4) Test adding and removing guarantors on a patron record, both Koha users and not
 5) Verify the "Add child" button works
 6) Verify NoIssuesChargeGuarantees still works
 7) Verify tools/cleanborrowers.pl will not delete a guarantor
 8) Verify the guarantors are displayed on moremember.pl
 9) Verify the guarantor is removed by members/update-child.pl
10) Verify the guarantor is removed by misc/cronjobs/j2a.pl
11) Verify import patrons converts guarantor_id, relationship, contactfirstname,
  and contactsurname into a guarantor
12) prove t/Patron.t
13) prove t/db_dependent/Circulation.t
14) prove t/db_dependent/Circulation/NoIssuesChargeGuarantees.t
15) prove t/db_dependent/Items.t
16) prove t/db_dependent/Koha/Patrons.t
17) prove t/db_dependent/Members.t
18) prove t/db_dependent/Patron/Relationships.t

Signed-off-by: Kim Peine <kmpeine@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 127 Kyle M Hall 2019-02-20 20:03:51 UTC
Created attachment 85412 [details] [review]
Bug 14570: Update schema files [Do Not Push]
Comment 128 Kyle M Hall 2019-02-21 11:44:18 UTC
Created attachment 85431 [details] [review]
Bug 14570: (QA Follow-up) Misc changes based on QA feedback
Comment 129 Katrin Fischer 2019-02-21 16:35:50 UTC
Hi Kyle, could you reply to 117/118 marking the points fixed/commenting? Thx!
Comment 130 Owen Leonard 2019-04-24 15:04:37 UTC
I'm marking this "Failed QA" to get it out of the queue until Comment 117 and Comment 118 are responded to.
Comment 131 Owen Leonard 2019-04-24 15:06:10 UTC
Also needs to be rebased!
Comment 132 Kyle M Hall 2019-05-17 12:15:36 UTC
Created attachment 89889 [details] [review]
Bug 14570: Make it possible to add multiple guarantors to a record

This patch adds the ability to set an unlimited number of guarantors
for a given patron. As before, each guarantor may be linked to another
Koha patron, and all the behavior that applies to a given guarantor
remains the same.

Test Plan:
 1) Apply this patch
 2) Run updatedatabase.pl
 3) Find some patrons with guarantors, verify the still have their guarantor
 4) Test adding and removing guarantors on a patron record, both Koha users and not
 5) Verify the "Add child" button works
 6) Verify NoIssuesChargeGuarantees still works
 7) Verify tools/cleanborrowers.pl will not delete a guarantor
 8) Verify the guarantors are displayed on moremember.pl
 9) Verify the guarantor is removed by members/update-child.pl
10) Verify the guarantor is removed by misc/cronjobs/j2a.pl
11) Verify import patrons converts guarantor_id, relationship, contactfirstname,
  and contactsurname into a guarantor
12) prove t/Patron.t
13) prove t/db_dependent/Circulation.t
14) prove t/db_dependent/Circulation/NoIssuesChargeGuarantees.t
15) prove t/db_dependent/Items.t
16) prove t/db_dependent/Koha/Patrons.t
17) prove t/db_dependent/Members.t
18) prove t/db_dependent/Patron/Relationships.t

Signed-off-by: Kim Peine <kmpeine@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 133 Kyle M Hall 2019-05-17 12:17:09 UTC
Created attachment 89890 [details] [review]
Bug 14570: Make it possible to add multiple guarantors to a record

This patch adds the ability to set an unlimited number of guarantors
for a given patron. As before, each guarantor may be linked to another
Koha patron, and all the behavior that applies to a given guarantor
remains the same.

Test Plan:
 1) Apply this patch
 2) Run updatedatabase.pl
 3) Find some patrons with guarantors, verify the still have their guarantor
 4) Test adding and removing guarantors on a patron record, both Koha users and not
 5) Verify the "Add child" button works
 6) Verify NoIssuesChargeGuarantees still works
 7) Verify tools/cleanborrowers.pl will not delete a guarantor
 8) Verify the guarantors are displayed on moremember.pl
 9) Verify the guarantor is removed by members/update-child.pl
10) Verify the guarantor is removed by misc/cronjobs/j2a.pl
11) Verify import patrons converts guarantor_id, relationship, contactfirstname,
  and contactsurname into a guarantor
12) prove t/Patron.t
13) prove t/db_dependent/Circulation.t
14) prove t/db_dependent/Circulation/NoIssuesChargeGuarantees.t
15) prove t/db_dependent/Items.t
16) prove t/db_dependent/Koha/Patrons.t
17) prove t/db_dependent/Members.t
18) prove t/db_dependent/Patron/Relationships.t

Signed-off-by: Kim Peine <kmpeine@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 134 Kyle M Hall 2019-05-17 12:17:24 UTC
Created attachment 89891 [details] [review]
Bug 14570: (QA Follow-up) Misc changes based on QA feedback
Comment 135 Kyle M Hall 2019-05-17 12:17:29 UTC
Created attachment 89892 [details] [review]
Bug 14570: Update schema files [Do Not Push]
Comment 136 Kyle M Hall 2019-05-17 12:22:42 UTC
(In reply to Jonathan Druart from comment #117)
> - Are you sure we need the change to Koha::Object?

Fixed in followup

> - koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
>   [% To.json(data) %] must have the html filter

Fixed in followup

> 
> - changes to opac-user.tt are wrong (not needed)

Fixed in rebase. Please point out which lines you are referring to in the future.

> - There are too many (unneeded) changes made to members/update-child.pl, it
> will conflict with bug 20226 (not blocker)

There are currently very few changes to this script. Maybe this was fixed in a previous rebase.

> - misc/cronjobs/j2a.pl transaction must be done using
> txn_begin/txn_commit/txn_rollback

Fixed in followup
 
> - C4::Installer::load_sql returned the following errors while attempting to
> load /home/vagrant/kohaclone/installer/data/mysql/atomicupdate/bug_14570.sql:
> 
> I do not know what the error is.

?
 
> - Charset/collate:
> relationship VARCHAR(100) COLLATE utf8_unicode_ci NOT NULL,
> => this is wrong
>
> - same in the atomic update

Fixed in followup
Comment 137 Kyle M Hall 2019-05-17 16:31:40 UTC
Created attachment 89909 [details] [review]
Bug 14570: Make it possible to add multiple guarantors to a record

This patch adds the ability to set an unlimited number of guarantors
for a given patron. As before, each guarantor may be linked to another
Koha patron, and all the behavior that applies to a given guarantor
remains the same.

Test Plan:
 1) Apply this patch
 2) Run updatedatabase.pl
 3) Find some patrons with guarantors, verify the still have their guarantor
 4) Test adding and removing guarantors on a patron record, both Koha users and not
 5) Verify the "Add child" button works
 6) Verify NoIssuesChargeGuarantees still works
 7) Verify tools/cleanborrowers.pl will not delete a guarantor
 8) Verify the guarantors are displayed on moremember.pl
 9) Verify the guarantor is removed by members/update-child.pl
10) Verify the guarantor is removed by misc/cronjobs/j2a.pl
11) Verify import patrons converts guarantor_id, relationship, contactfirstname,
  and contactsurname into a guarantor
12) prove t/Patron.t
13) prove t/db_dependent/Circulation.t
14) prove t/db_dependent/Circulation/NoIssuesChargeGuarantees.t
15) prove t/db_dependent/Items.t
16) prove t/db_dependent/Koha/Patrons.t
17) prove t/db_dependent/Members.t
18) prove t/db_dependent/Patron/Relationships.t

Signed-off-by: Kim Peine <kmpeine@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 138 Kyle M Hall 2019-05-17 16:33:46 UTC
Created attachment 89910 [details] [review]
Bug 14570: Update schema files [Do Not Push]
Comment 139 Kyle M Hall 2019-05-17 16:34:13 UTC
New patch has the followup squashed in for simplicity.
Comment 140 Kyle M Hall 2019-05-17 16:41:49 UTC
Created attachment 89911 [details] [review]
Bug 14570: Make it possible to add multiple guarantors to a record

This patch adds the ability to set an unlimited number of guarantors
for a given patron. As before, each guarantor may be linked to another
Koha patron, and all the behavior that applies to a given guarantor
remains the same.

Test Plan:
 1) Apply this patch
 2) Run updatedatabase.pl
 3) Find some patrons with guarantors, verify the still have their guarantor
 4) Test adding and removing guarantors on a patron record, both Koha users and not
 5) Verify the "Add child" button works
 6) Verify NoIssuesChargeGuarantees still works
 7) Verify tools/cleanborrowers.pl will not delete a guarantor
 8) Verify the guarantors are displayed on moremember.pl
 9) Verify the guarantor is removed by members/update-child.pl
10) Verify the guarantor is removed by misc/cronjobs/j2a.pl
11) Verify import patrons converts guarantor_id, relationship, contactfirstname,
  and contactsurname into a guarantor
12) prove t/Patron.t
13) prove t/db_dependent/Circulation.t
14) prove t/db_dependent/Circulation/NoIssuesChargeGuarantees.t
15) prove t/db_dependent/Items.t
16) prove t/db_dependent/Koha/Patrons.t
17) prove t/db_dependent/Members.t
18) prove t/db_dependent/Patron/Relationships.t

Signed-off-by: Kim Peine <kmpeine@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 141 Kyle M Hall 2019-05-17 16:42:00 UTC
Created attachment 89912 [details] [review]
Bug 14570: Update schema files [Do Not Push]
Comment 142 Agustín Moyano 2019-05-18 00:15:23 UTC
Created attachment 89921 [details] [review]
Bug 14570: (follow-up) fix table name in j2a.pl
Comment 143 Liz Rea 2019-05-18 01:32:44 UTC
This patch seems to break inserting sample patrons, because the sample patrons file has a guarantorid field and this patch removes it.
Comment 144 Agustín Moyano 2019-05-18 02:53:39 UTC
Created attachment 89922 [details] [review]
Bug 14570: (follow-up) fix qa FAIL tests
Comment 145 Agustín Moyano 2019-05-18 02:56:23 UTC
(In reply to Liz Rea from comment #143)
> This patch seems to break inserting sample patrons, because the sample
> patrons file has a guarantorid field and this patch removes it.

Yes, I had the same problem.. to retest, I had to 
1. git reset --hard master 
2. reset_all
3. apply again patches 
4. updatedatabase.pl

Agustín
Comment 146 Agustín Moyano 2019-05-21 03:08:48 UTC
Created attachment 89929 [details] [review]
Bug 14570: (follow-up) Fix tests
Comment 147 Liz Rea 2019-05-22 16:46:15 UTC
Koha can't be installed freshly with the sample patrons with this patch:


Optional data added

    sample_patrons.sql

    DBD::mysql::st execute failed: Unknown column 'guarantorid' in 'field list' at /usr/share/perl5/DBIx/RunSQL.pm line 273.

So probably we need to update the sample patrons to not include that field, if we are going to push this.

Liz
Comment 148 Liz Rea 2019-05-23 18:23:40 UTC
Created attachment 90048 [details] [review]
Bug 14570: (QA follow-up) fix sample borrowers, remove guarantorid)
Comment 149 Liz Rea 2019-05-23 18:57:03 UTC
Happened to notice also, after applying the updates supplied with this patch, that the Account.t test fails due to a lack of guarantorid. There may be other tests that will fail as well.

Liz
Comment 150 Kyle M Hall 2019-05-24 17:43:12 UTC
Liz, you are referring to t/db_dependent/Koha/Account.t right? I just tried it pre and post patch with the database update and it passes in both cases! This was with a fresh koha-testing-docker instance.


(In reply to Liz Rea from comment #149)
> Happened to notice also, after applying the updates supplied with this
> patch, that the Account.t test fails due to a lack of guarantorid. There may
> be other tests that will fail as well.
> 
> Liz
Comment 151 Liz Rea 2019-05-24 20:00:19 UTC
That's messed up - possibly I had a chicken and egg thing going. I'll try it again - and if I don't get to it soon it's probably fine.
Comment 152 Agustín Moyano 2019-06-04 16:07:41 UTC
Sorry to anounce that yesterday I applied this patches over a fresh master, and the problem of add child that shows "Add Patron (Adult)" returned... 

I'll do it again today just to be sure
Comment 153 Martin Renvoize 2019-07-01 17:19:28 UTC
I hit this error during DB update.. seems harmless but perhaps we should be handling it better.

DEV atomic update: bug_14570.perl                                                                                                                             
DBD::mysql::db do failed: Column 'relationship' cannot be null [for Statement "                                                                               
            INSERT INTO borrower_relationships ( guarantor_id, guarantee_id, relationship )                                                                   
            SELECT guarantorid, borrowernumber, relationship FROM borrowers WHERE guarantorid IS NOT NULL;                                                    
        "] at (eval 1396) line 24.
Comment 154 Agustín Moyano 2019-07-03 17:38:35 UTC
Created attachment 91231 [details] [review]
Bug 14570: (follow-up) Fix patron type when "add child" button is pressed
Comment 155 Agustín Moyano 2019-07-10 13:05:56 UTC
Created attachment 91439 [details] [review]
Bug 14570: Make it possible to add multiple guarantors to a record

This patch adds the ability to set an unlimited number of guarantors
for a given patron. As before, each guarantor may be linked to another
Koha patron, and all the behavior that applies to a given guarantor
remains the same.

Test Plan:
 1) Apply this patch
 2) Run updatedatabase.pl
 3) Find some patrons with guarantors, verify the still have their guarantor
 4) Test adding and removing guarantors on a patron record, both Koha users and not
 5) Verify the "Add child" button works
 6) Verify NoIssuesChargeGuarantees still works
 7) Verify tools/cleanborrowers.pl will not delete a guarantor
 8) Verify the guarantors are displayed on moremember.pl
 9) Verify the guarantor is removed by members/update-child.pl
10) Verify the guarantor is removed by misc/cronjobs/j2a.pl
11) Verify import patrons converts guarantor_id, relationship, contactfirstname,
  and contactsurname into a guarantor
12) prove t/Patron.t
13) prove t/db_dependent/Circulation.t
14) prove t/db_dependent/Circulation/NoIssuesChargeGuarantees.t
15) prove t/db_dependent/Items.t
16) prove t/db_dependent/Koha/Patrons.t
17) prove t/db_dependent/Members.t
18) prove t/db_dependent/Patron/Relationships.t

Signed-off-by: Kim Peine <kmpeine@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 156 Agustín Moyano 2019-07-10 13:06:04 UTC
Created attachment 91440 [details] [review]
Bug 14570: Update schema files [Do Not Push]
Comment 157 Agustín Moyano 2019-07-10 13:06:09 UTC
Created attachment 91441 [details] [review]
Bug 14570: (follow-up) fix table name in j2a.pl
Comment 158 Agustín Moyano 2019-07-10 13:06:15 UTC
Created attachment 91442 [details] [review]
Bug 14570: (follow-up) fix qa FAIL tests
Comment 159 Agustín Moyano 2019-07-10 13:06:20 UTC
Created attachment 91443 [details] [review]
Bug 14570: (follow-up) Fix tests
Comment 160 Agustín Moyano 2019-07-10 13:06:26 UTC
Created attachment 91444 [details] [review]
Bug 14570: (QA follow-up) fix sample borrowers, remove guarantorid)
Comment 161 Agustín Moyano 2019-07-10 13:06:32 UTC
Created attachment 91445 [details] [review]
Bug 14570: (follow-up) Fix patron type when "add child" button is pressed
Comment 162 Liz Rea 2019-07-22 16:05:28 UTC
Apologies, this seems to not apply:

Auto-merging members/deletemem.pl
CONFLICT (content): Merge conflict in members/deletemem.pl

Patch failed at 0001 Bug 14570: Make it possible to add multiple guarantors to a record
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Bug 14570 - Make it possible to add multiple guarantors to a record

will have a look to resolve that but you may beat me!

Liz
Comment 163 Kyle M Hall 2019-07-22 16:56:32 UTC
Created attachment 91694 [details] [review]
Bug 14570: Make it possible to add multiple guarantors to a record

This patch adds the ability to set an unlimited number of guarantors
for a given patron. As before, each guarantor may be linked to another
Koha patron, and all the behavior that applies to a given guarantor
remains the same.

Test Plan:
 1) Apply this patch
 2) Run updatedatabase.pl
 3) Find some patrons with guarantors, verify the still have their guarantor
 4) Test adding and removing guarantors on a patron record, both Koha users and not
 5) Verify the "Add child" button works
 6) Verify NoIssuesChargeGuarantees still works
 7) Verify tools/cleanborrowers.pl will not delete a guarantor
 8) Verify the guarantors are displayed on moremember.pl
 9) Verify the guarantor is removed by members/update-child.pl
10) Verify the guarantor is removed by misc/cronjobs/j2a.pl
11) Verify import patrons converts guarantor_id, relationship, contactfirstname,
  and contactsurname into a guarantor
12) prove t/Patron.t
13) prove t/db_dependent/Circulation.t
14) prove t/db_dependent/Circulation/NoIssuesChargeGuarantees.t
15) prove t/db_dependent/Items.t
16) prove t/db_dependent/Koha/Patrons.t
17) prove t/db_dependent/Members.t
18) prove t/db_dependent/Patron/Relationships.t

Signed-off-by: Kim Peine <kmpeine@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 164 Kyle M Hall 2019-07-22 16:56:47 UTC
Created attachment 91695 [details] [review]
Bug 14570: (follow-up) fix table name in j2a.pl
Comment 165 Kyle M Hall 2019-07-22 16:56:52 UTC
Created attachment 91696 [details] [review]
Bug 14570: (follow-up) fix qa FAIL tests
Comment 166 Kyle M Hall 2019-07-22 16:56:56 UTC
Created attachment 91697 [details] [review]
Bug 14570: (follow-up) Fix tests
Comment 167 Kyle M Hall 2019-07-22 16:57:01 UTC
Created attachment 91698 [details] [review]
Bug 14570: (QA follow-up) fix sample borrowers, remove guarantorid)
Comment 168 Kyle M Hall 2019-07-22 16:57:06 UTC
Created attachment 91699 [details] [review]
Bug 14570: (follow-up) Fix patron type when "add child" button is pressed
Comment 169 Kyle M Hall 2019-07-22 16:57:11 UTC
Created attachment 91700 [details] [review]
Bug 14570: Update schema files [Do Not Push]
Comment 170 Tomás Cohen Arazi 2019-08-09 19:30:35 UTC
I will take on this one on monday.

I see some usability issues, specially when adding/removing guarantors/guarantees.

Guarantee context
- 'Search' should be 'fa-plus Add' for adding a guarantor.
- There's no way to remove a guarantor
- A guarantor can be added multiple times to the same guarantee (is that a feature?)
- There's no way to set the relationship
- Labels seem to be wrong: 'Organization #' and 'Organization name'

Guarantor context
- I still find it confusing that 'Add child' only shows for 'adult' categories, so 'board' cannot have a guarantee? This is something already in Koha, not this bug's responsibility. but worth mentioning.
- Shouldn't we rename 'Add child' into 'Add guarantee'? I know this bug is just about changing a 1-1 relationship into 1-n, but worth considering, even as a follow-up bug. Related to the previous comment.
- 'Cancel' should just be 'Delete', as it shows for already existing relations, not just the ones being added (a change that can be cancelled). Maybe have 'Delete' for the current relations, and 'cancel/delete' for the ones being added.

General code review
- Most of it follows the guidelines, even if it is old code already. It has got attention and several follow-ups have adjusted it forward, which is great.
- I expected to find Koha::Patron->guarantees and Koha::Patron->guarantors. I read the discussion and know it is because they are not always proper Koha patrons, but just some metadata about them [1]. It still smells.
- In line with recent community agreements, (after this patches have been written) we should try to make this guarantors/guarantees related objects prefetchable. Not difficult to do, just need to take care of naming the relations correctly to follow the guidelines.

Out of the scope of this bug
- Lets move out of sysprefs for the relations! Have a full CRUD for those, and a internationalization layer!
- [1] Can't we think of 'virtual' patrons? so patrons get created, but just not considered proper patrons that can circulate, etc. Food for thought, but it would allow us to add nice search forms to discover relations, etc.
Comment 171 Tomás Cohen Arazi 2019-08-09 19:38:28 UTC Comment hidden (obsolete)
Comment 172 Kyle M Hall 2019-08-12 18:13:53 UTC
(In reply to Tomás Cohen Arazi from comment #170)
> Guarantee context
> - 'Search' should be 'fa-plus Add' for adding a guarantor.

Fixed in followup!

> - There's no way to remove a guarantor

You need to check the "Remove" checkbox and save the patron ( I renamed it from Delete in this upcoming followup ).

> - A guarantor can be added multiple times to the same guarantee (is that a
> feature?)

Fixed in followup!

> - There's no way to set the relationship

It can be set at the time you add a guarantor. To edit it you just remove and re-add the same guarantor

> - Labels seem to be wrong: 'Organization #' and 'Organization name'

Fixed in followup!
Comment 173 Kyle M Hall 2019-08-12 18:15:09 UTC
Created attachment 92157 [details] [review]
Bug 14570: (QA follow-up) Fix issues in Guarantee context
Comment 174 Tomás Cohen Arazi 2019-08-12 18:16:09 UTC
(In reply to Tomás Cohen Arazi from comment #171)
> Maybe add
> - Koha::Guarantee(s)
> - Koha::Guarantor(s)
> which provide a ->patron method.

Forget about this one, I agree with Jonathan we should simplify this a lot.
Comment 175 Kyle M Hall 2019-08-12 18:20:55 UTC
(In reply to Tomás Cohen Arazi from comment #170)
> Guarantor context
> - I still find it confusing that 'Add child' only shows for 'adult'
> categories, so 'board' cannot have a guarantee? This is something already in
> Koha, not this bug's responsibility. but worth mentioning.

Yep, that's outside the scope of this bug. I think there is another report for that. If not, there should be!

> - Shouldn't we rename 'Add child' into 'Add guarantee'? I know this bug is
> just about changing a 1-1 relationship into 1-n, but worth considering, even
> as a follow-up bug. Related to the previous comment.

I think that's simple enough to accomplish in this bug report with a followup.

> - 'Cancel' should just be 'Delete', as it shows for already existing
> relations, not just the ones being added (a change that can be cancelled).
> Maybe have 'Delete' for the current relations, and 'cancel/delete' for the
> ones being added.

Changed in followup to "Remove" to match the other update from "Delete" to "Remove".
Comment 176 Kyle M Hall 2019-08-12 18:21:23 UTC
Created attachment 92158 [details] [review]
Bug 14570: (QA follow-up) Fix issues in Guarantor context
Comment 177 Tomás Cohen Arazi 2019-08-12 19:50:58 UTC
Created attachment 92161 [details] [review]
Bug 14570: Add error handling to Koha::Patron->add_guarantor

This patch adds checks on the values for the 'relationship'. This is
done to avoid future problems when migrating relationships from the
plain text syspref into (why not) a proper table. And to preserve
consistency.

Tests are added for both the new exceptions and the changes to
Koha::Patron.

To test:
- Apply this patches
- Run:
  $ kshell
 k$ prove t/Koha/Exceptions.t \
          t/db_dependent/Koha/Patron.t
=> SUCCESS: Tests pass!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 178 Tomás Cohen Arazi 2019-08-12 19:51:04 UTC
Created attachment 92162 [details] [review]
Bug 14570: Remove wrong parameter in add_guarantor call

This patch just removes an unused parameter.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 179 Tomás Cohen Arazi 2019-08-12 20:14:40 UTC
Created attachment 92163 [details] [review]
Bug 14570: (QA follow-up) Consistency displaying the list of guarantors

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 180 Tomás Cohen Arazi 2019-08-13 17:13:14 UTC
Created attachment 92180 [details] [review]
Bug 14570: Add a UNIQUE constraint on guarantor_id+guarantee_id

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 181 Tomás Cohen Arazi 2019-08-13 17:13:23 UTC
Created attachment 92181 [details] [review]
Bug 14570: Add error handling to Koha::Patron::Relationship->store

This patch adds checks on the values for the 'relationship'. This is
done to avoid future problems when migrating relationships from the
plain text syspref into (why not) a proper table. And to preserve
consistency.

There's also catching on possible broken constraints and throwing a new
exception

Tests are added for both the new exceptions and the changes to
Koha::Patron::Relationship.

To test:
- Apply this patches
- Run:
  $ kshell
 k$ prove t/Koha/Exceptions.t \
          t/db_dependent/Koha/Patron.t \
          t/db_dependent/Koha/Patron/Relationship.t
=> SUCCESS: Tests pass!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 182 Tomás Cohen Arazi 2019-08-13 17:13:33 UTC
Created attachment 92182 [details] [review]
Bug 14570: Remove wrong parameter in add_guarantor call

This patch just removes an unused parameter.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 183 Tomás Cohen Arazi 2019-08-13 17:13:44 UTC
Created attachment 92183 [details] [review]
Bug 14570: (QA follow-up) Consistency displaying the list of guarantors

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 184 Kyle M Hall 2019-08-15 13:35:30 UTC
Created attachment 92232 [details] [review]
Bug 14570: Add a UNIQUE constraint on guarantor_id+guarantee_id

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 185 Kyle M Hall 2019-08-15 13:35:42 UTC
Created attachment 92233 [details] [review]
Bug 14570: Add error handling to Koha::Patron::Relationship->store

This patch adds checks on the values for the 'relationship'. This is
done to avoid future problems when migrating relationships from the
plain text syspref into (why not) a proper table. And to preserve
consistency.

There's also catching on possible broken constraints and throwing a new
exception

Tests are added for both the new exceptions and the changes to
Koha::Patron::Relationship.

To test:
- Apply this patches
- Run:
  $ kshell
 k$ prove t/Koha/Exceptions.t \
          t/db_dependent/Koha/Patron.t \
          t/db_dependent/Koha/Patron/Relationship.t
=> SUCCESS: Tests pass!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 186 Kyle M Hall 2019-08-15 13:35:47 UTC
Created attachment 92234 [details] [review]
Bug 14570: Remove wrong parameter in add_guarantor call

This patch just removes an unused parameter.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 187 Kyle M Hall 2019-08-15 13:35:52 UTC
Created attachment 92235 [details] [review]
Bug 14570: (QA follow-up) Consistency displaying the list of guarantors

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 188 Tomás Cohen Arazi 2019-08-16 17:44:02 UTC
Created attachment 92277 [details] [review]
Bug 14570: (QA follow-up) Fix tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 189 Tomás Cohen Arazi 2019-08-16 17:53:15 UTC
Created attachment 92278 [details] [review]
Bug 14570: (QA follow-up) Fix tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 190 Agustín Moyano 2019-08-16 18:05:43 UTC
Created attachment 92279 [details] [review]
Bug 14570: Make it possible to add multiple guarantors to a record

This patch adds the ability to set an unlimited number of guarantors
for a given patron. As before, each guarantor may be linked to another
Koha patron, and all the behavior that applies to a given guarantor
remains the same.

Test Plan:
 1) Apply this patch
 2) Run updatedatabase.pl
 3) Find some patrons with guarantors, verify the still have their guarantor
 4) Test adding and removing guarantors on a patron record, both Koha users and not
 5) Verify the "Add child" button works
 6) Verify NoIssuesChargeGuarantees still works
 7) Verify tools/cleanborrowers.pl will not delete a guarantor
 8) Verify the guarantors are displayed on moremember.pl
 9) Verify the guarantor is removed by members/update-child.pl
10) Verify the guarantor is removed by misc/cronjobs/j2a.pl
11) Verify import patrons converts guarantor_id, relationship, contactfirstname,
  and contactsurname into a guarantor
12) prove t/Patron.t
13) prove t/db_dependent/Circulation.t
14) prove t/db_dependent/Circulation/NoIssuesChargeGuarantees.t
15) prove t/db_dependent/Items.t
16) prove t/db_dependent/Koha/Patrons.t
17) prove t/db_dependent/Members.t
18) prove t/db_dependent/Patron/Relationships.t

Signed-off-by: Kim Peine <kmpeine@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 191 Agustín Moyano 2019-08-16 18:05:57 UTC
Created attachment 92280 [details] [review]
Bug 14570: (follow-up) fix table name in j2a.pl

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 192 Agustín Moyano 2019-08-16 18:06:06 UTC
Created attachment 92281 [details] [review]
Bug 14570: (follow-up) fix qa FAIL tests

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 193 Agustín Moyano 2019-08-16 18:06:16 UTC
Created attachment 92282 [details] [review]
Bug 14570: (follow-up) Fix tests

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 194 Agustín Moyano 2019-08-16 18:06:39 UTC
Created attachment 92283 [details] [review]
Bug 14570: (QA follow-up) fix sample borrowers, remove guarantorid)

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 195 Agustín Moyano 2019-08-16 18:06:50 UTC
Created attachment 92284 [details] [review]
Bug 14570: (follow-up) Fix patron type when "add child" button is pressed

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 196 Agustín Moyano 2019-08-16 18:07:07 UTC
Created attachment 92285 [details] [review]
Bug 14570: Update schema files [Do Not Push]

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 197 Agustín Moyano 2019-08-16 18:07:21 UTC
Created attachment 92286 [details] [review]
Bug 14570: (QA follow-up) Fix issues in Guarantee context

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 198 Agustín Moyano 2019-08-16 18:07:32 UTC
Created attachment 92287 [details] [review]
Bug 14570: (QA follow-up) Fix issues in Guarantor context

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 199 Agustín Moyano 2019-08-16 18:07:47 UTC
Created attachment 92288 [details] [review]
Bug 14570: Add a UNIQUE constraint on guarantor_id+guarantee_id

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 200 Agustín Moyano 2019-08-16 18:08:00 UTC
Created attachment 92289 [details] [review]
Bug 14570: Add error handling to Koha::Patron::Relationship->store

This patch adds checks on the values for the 'relationship'. This is
done to avoid future problems when migrating relationships from the
plain text syspref into (why not) a proper table. And to preserve
consistency.

There's also catching on possible broken constraints and throwing a new
exception

Tests are added for both the new exceptions and the changes to
Koha::Patron::Relationship.

To test:
- Apply this patches
- Run:
  $ kshell
 k$ prove t/Koha/Exceptions.t \
          t/db_dependent/Koha/Patron.t \
          t/db_dependent/Koha/Patron/Relationship.t
=> SUCCESS: Tests pass!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 201 Agustín Moyano 2019-08-16 18:08:12 UTC
Created attachment 92290 [details] [review]
Bug 14570: Remove wrong parameter in add_guarantor call

This patch just removes an unused parameter.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 202 Agustín Moyano 2019-08-16 18:08:25 UTC
Created attachment 92291 [details] [review]
Bug 14570: (QA follow-up) Consistency displaying the list of guarantors

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 203 Agustín Moyano 2019-08-16 18:08:38 UTC
Created attachment 92292 [details] [review]
Bug 14570: (QA follow-up) Fix tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 204 Liz Rea 2019-08-16 19:20:00 UTC
Created attachment 92298 [details] [review]
Bug 14570: Make it possible to add multiple guarantors to a record

This patch adds the ability to set an unlimited number of guarantors
for a given patron. As before, each guarantor may be linked to another
Koha patron, and all the behavior that applies to a given guarantor
remains the same.

Test Plan:
 1) Apply this patch
 2) Run updatedatabase.pl
 3) Find some patrons with guarantors, verify the still have their guarantor
 4) Test adding and removing guarantors on a patron record, both Koha users and not
 5) Verify the "Add child" button works
 6) Verify NoIssuesChargeGuarantees still works
 7) Verify tools/cleanborrowers.pl will not delete a guarantor
 8) Verify the guarantors are displayed on moremember.pl
 9) Verify the guarantor is removed by members/update-child.pl
10) Verify the guarantor is removed by misc/cronjobs/j2a.pl
11) Verify import patrons converts guarantor_id, relationship, contactfirstname,
  and contactsurname into a guarantor
12) prove t/Patron.t
13) prove t/db_dependent/Circulation.t
14) prove t/db_dependent/Circulation/NoIssuesChargeGuarantees.t
15) prove t/db_dependent/Items.t
16) prove t/db_dependent/Koha/Patrons.t
17) prove t/db_dependent/Members.t
18) prove t/db_dependent/Patron/Relationships.t

Signed-off-by: Kim Peine <kmpeine@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 205 Liz Rea 2019-08-16 19:20:30 UTC
Created attachment 92299 [details] [review]
Bug 14570: (follow-up) fix table name in j2a.pl

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 206 Liz Rea 2019-08-16 19:20:35 UTC
Created attachment 92300 [details] [review]
Bug 14570: (follow-up) fix qa FAIL tests

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 207 Liz Rea 2019-08-16 19:20:41 UTC
Created attachment 92301 [details] [review]
Bug 14570: (follow-up) Fix tests

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 208 Liz Rea 2019-08-16 19:20:49 UTC
Created attachment 92302 [details] [review]
Bug 14570: (QA follow-up) fix sample borrowers, remove guarantorid)

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 209 Liz Rea 2019-08-16 19:20:56 UTC
Created attachment 92303 [details] [review]
Bug 14570: (follow-up) Fix patron type when "add child" button is pressed

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 210 Liz Rea 2019-08-16 19:21:02 UTC
Created attachment 92304 [details] [review]
Bug 14570: Update schema files [Do Not Push]

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 211 Liz Rea 2019-08-16 19:21:11 UTC
Created attachment 92305 [details] [review]
Bug 14570: (QA follow-up) Fix issues in Guarantee context

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 212 Liz Rea 2019-08-16 19:21:19 UTC
Created attachment 92306 [details] [review]
Bug 14570: (QA follow-up) Fix issues in Guarantor context

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 213 Liz Rea 2019-08-16 19:21:25 UTC
Created attachment 92307 [details] [review]
Bug 14570: Add a UNIQUE constraint on guarantor_id+guarantee_id

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 214 Liz Rea 2019-08-16 19:21:31 UTC
Created attachment 92308 [details] [review]
Bug 14570: Add error handling to Koha::Patron::Relationship->store

This patch adds checks on the values for the 'relationship'. This is
done to avoid future problems when migrating relationships from the
plain text syspref into (why not) a proper table. And to preserve
consistency.

There's also catching on possible broken constraints and throwing a new
exception

Tests are added for both the new exceptions and the changes to
Koha::Patron::Relationship.

To test:
- Apply this patches
- Run:
  $ kshell
 k$ prove t/Koha/Exceptions.t \
          t/db_dependent/Koha/Patron.t \
          t/db_dependent/Koha/Patron/Relationship.t
=> SUCCESS: Tests pass!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 215 Liz Rea 2019-08-16 19:21:37 UTC
Created attachment 92309 [details] [review]
Bug 14570: Remove wrong parameter in add_guarantor call

This patch just removes an unused parameter.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 216 Liz Rea 2019-08-16 19:21:43 UTC
Created attachment 92310 [details] [review]
Bug 14570: (QA follow-up) Consistency displaying the list of guarantors

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 217 Liz Rea 2019-08-16 19:21:50 UTC
Created attachment 92311 [details] [review]
Bug 14570: (QA follow-up) Fix tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 218 Tomás Cohen Arazi 2019-08-16 19:45:50 UTC
Created attachment 92312 [details] [review]
Bug 14570: Make it possible to add multiple guarantors to a record

This patch adds the ability to set an unlimited number of guarantors
for a given patron. As before, each guarantor may be linked to another
Koha patron, and all the behavior that applies to a given guarantor
remains the same.

Test Plan:
 1) Apply this patch
 2) Run updatedatabase.pl
 3) Find some patrons with guarantors, verify the still have their guarantor
 4) Test adding and removing guarantors on a patron record, both Koha users and not
 5) Verify the "Add child" button works
 6) Verify NoIssuesChargeGuarantees still works
 7) Verify tools/cleanborrowers.pl will not delete a guarantor
 8) Verify the guarantors are displayed on moremember.pl
 9) Verify the guarantor is removed by members/update-child.pl
10) Verify the guarantor is removed by misc/cronjobs/j2a.pl
11) Verify import patrons converts guarantor_id, relationship, contactfirstname,
  and contactsurname into a guarantor
12) prove t/Patron.t
13) prove t/db_dependent/Circulation.t
14) prove t/db_dependent/Circulation/NoIssuesChargeGuarantees.t
15) prove t/db_dependent/Items.t
16) prove t/db_dependent/Koha/Patrons.t
17) prove t/db_dependent/Members.t
18) prove t/db_dependent/Patron/Relationships.t

Signed-off-by: Kim Peine <kmpeine@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 219 Tomás Cohen Arazi 2019-08-16 19:45:57 UTC
Created attachment 92313 [details] [review]
Bug 14570: (follow-up) fix table name in j2a.pl

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 220 Tomás Cohen Arazi 2019-08-16 19:46:03 UTC
Created attachment 92314 [details] [review]
Bug 14570: (follow-up) fix qa FAIL tests

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 221 Tomás Cohen Arazi 2019-08-16 19:46:10 UTC
Created attachment 92315 [details] [review]
Bug 14570: (follow-up) Fix tests

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 222 Tomás Cohen Arazi 2019-08-16 19:46:16 UTC
Created attachment 92316 [details] [review]
Bug 14570: (QA follow-up) fix sample borrowers, remove guarantorid)

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 223 Tomás Cohen Arazi 2019-08-16 19:46:24 UTC
Created attachment 92317 [details] [review]
Bug 14570: (follow-up) Fix patron type when "add child" button is pressed

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 224 Tomás Cohen Arazi 2019-08-16 19:46:32 UTC
Created attachment 92318 [details] [review]
Bug 14570: Update schema files [Do Not Push]

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 225 Tomás Cohen Arazi 2019-08-16 19:46:39 UTC
Created attachment 92319 [details] [review]
Bug 14570: (QA follow-up) Fix issues in Guarantee context

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 226 Tomás Cohen Arazi 2019-08-16 19:46:45 UTC
Created attachment 92320 [details] [review]
Bug 14570: (QA follow-up) Fix issues in Guarantor context

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 227 Tomás Cohen Arazi 2019-08-16 19:46:53 UTC
Created attachment 92321 [details] [review]
Bug 14570: Add a UNIQUE constraint on guarantor_id+guarantee_id

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 228 Tomás Cohen Arazi 2019-08-16 19:46:59 UTC
Created attachment 92322 [details] [review]
Bug 14570: Add error handling to Koha::Patron::Relationship->store

This patch adds checks on the values for the 'relationship'. This is
done to avoid future problems when migrating relationships from the
plain text syspref into (why not) a proper table. And to preserve
consistency.

There's also catching on possible broken constraints and throwing a new
exception

Tests are added for both the new exceptions and the changes to
Koha::Patron::Relationship.

To test:
- Apply this patches
- Run:
  $ kshell
 k$ prove t/Koha/Exceptions.t \
          t/db_dependent/Koha/Patron.t \
          t/db_dependent/Koha/Patron/Relationship.t
=> SUCCESS: Tests pass!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 229 Tomás Cohen Arazi 2019-08-16 19:47:06 UTC
Created attachment 92323 [details] [review]
Bug 14570: Remove wrong parameter in add_guarantor call

This patch just removes an unused parameter.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 230 Tomás Cohen Arazi 2019-08-16 19:47:12 UTC
Created attachment 92324 [details] [review]
Bug 14570: (QA follow-up) Consistency displaying the list of guarantors

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 231 Tomás Cohen Arazi 2019-08-16 19:47:19 UTC
Created attachment 92325 [details] [review]
Bug 14570: (QA follow-up) Fix tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 232 Martin Renvoize 2019-08-19 07:54:11 UTC
I'm afraid the DB update fails for a few test runs I've tried.

borrowers.relationship has no constraints to it so it's very possible we have guarantor_relationships without a valid relationship and so the db update fails.

We need to come up with a solution for this before I can push the patch series.. I'm not sure whether to reduce the 'NOT NULL' constraint on the new table or set a default or note the issue as an error somewhere and populate the table with some sort of placeholder relationship whilst libraries work through fixing up the bad data..

Thoughts?
Comment 233 Tomás Cohen Arazi 2019-08-19 09:02:26 UTC
(In reply to Martin Renvoize from comment #232)
> I'm afraid the DB update fails for a few test runs I've tried.
> 
> borrowers.relationship has no constraints to it so it's very possible we
> have guarantor_relationships without a valid relationship and so the db
> update fails.
> 
> We need to come up with a solution for this before I can push the patch
> series.. I'm not sure whether to reduce the 'NOT NULL' constraint on the new
> table or set a default or note the issue as an error somewhere and populate
> the table with some sort of placeholder relationship whilst libraries work
> through fixing up the bad data..
> 
> Thoughts?

There's no semantic on the relationship field ATM, so I think it being just a label, we should not fail because of it.

If we were to add some semantics, we would have a separate table, with the relationship description and probably some columns for describing rules. If such was the situation, I would pick a placeholder as you say, so we don't fail, and even have an entry in about.pl about those problematic cases (maybe a note field with the original value).
Comment 234 Tomás Cohen Arazi 2019-08-19 15:19:54 UTC
Created attachment 92346 [details] [review]
Bug 14570: (QA follow-up) Consistency displaying the list of guarantors

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 235 Tomás Cohen Arazi 2019-08-19 15:20:02 UTC
Created attachment 92347 [details] [review]
Bug 14570: (QA follow-up) Fix tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 236 Tomás Cohen Arazi 2019-08-19 15:20:09 UTC
Created attachment 92348 [details] [review]
Bug 14570: Handle missing 'relationship' in atomicupdate (bad data)

This patch adds two steps to the upgrade process:
- Handle the pathologic case in which there's a guarantorid and no
'relationship'.
- Performs a cleanup on the relevant columns when the guarantor data
contains a guarantorid, and is then migrated into the
borrower_relationships table. The original code left the information
in the borrowers table, so no clear separation between guarantors as
patrons and the virtual ones.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 237 Tomás Cohen Arazi 2019-08-19 15:20:16 UTC
Created attachment 92349 [details] [review]
Bug 14570: (QA follow-up) Call Koha::Patron->find in scalar context

This patch prevents opac/opac-memberentry.pl from exploding.

To test:
- Login with a known user in the OPAC
- Open the opac/opac-memberentry.pl page
=> FAIL: It explodes
- Apply this patch
- Restart all the things just in case:
  $ restart_all
- Reload the page
=> SUCCESS: Things work!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 238 Tomás Cohen Arazi 2019-08-19 15:20:23 UTC
Created attachment 92350 [details] [review]
Bug 14570: Add about.pl entry for wrong relationship data

This patch adds a check and an entry to the about.pl, for bad
relationship data. The checks are:

- 'relationship' containing '_bad_data' (generated by the atomicupdate
  on undef relationships.
- 'relationship' values that are not in the borrowerRelationship
syspref.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 239 Martin Renvoize 2019-08-20 15:00:59 UTC
Nice work!

Pushed to master for 19.11.00
Comment 240 Fridolin Somers 2019-09-24 07:08:42 UTC
Enhancement not pushed to 19.05.x
Comment 241 Jonathan Druart 2019-11-26 19:29:34 UTC
Something wrong happening on the about page:

https://snipboard.io/QmBPeJ.jpg

MariaDB [koha_kohadev]> select * from systempreferences where variable="borrowerRelationship";
+----------------------+---------------+---------+------------------------------------------------------------------------------------+------+
| variable             | value         | options | explanation                                                                        | type |
+----------------------+---------------+---------+------------------------------------------------------------------------------------+------+
| borrowerRelationship | father|mother |         | Define valid relationships between a guarantor & a guarantee (separated by | or ,) | free |
+----------------------+---------------+---------+------------------------------------------------------------------------------------+------+
1 row in set (0.00 sec)

MariaDB [koha_kohadev]> select distinct relationship from borrowers;
+--------------+
| relationship |
+--------------+
| NULL         |
+--------------+
1 row in set (0.01 sec)

MariaDB [koha_kohadev]> select * from borrower_relationships;
Empty set (0.00 sec)
Comment 242 Martin Renvoize 2019-11-27 10:43:12 UTC
Sponsorship lines moved into git commit for release notes and about page
Comment 243 Katrin Fischer 2019-12-29 22:33:32 UTC
It looks like this broke the non-English installers. Can we please get a fix? Filed as bug 24317.
Comment 244 Katrin Fischer 2019-12-29 22:41:45 UTC
It looks like guarantorid still appears in Koha/REST/V1/Patrons.pm although we state that it cannot be updated with the API now.
Comment 245 David Cook 2020-11-10 03:41:55 UTC
(In reply to Tomás Cohen Arazi from comment #238)
> Created attachment 92350 [details] [review] [review]
> Bug 14570: Add about.pl entry for wrong relationship data
> 
> This patch adds a check and an entry to the about.pl, for bad
> relationship data. The checks are:
> 
> - 'relationship' containing '_bad_data' (generated by the atomicupdate
>   on undef relationships.
> - 'relationship' values that are not in the borrowerRelationship
> syspref.
> 
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Unfortunately, this doesn't give sysadmins any idea about how to fix this problem.
Comment 246 David Cook 2020-11-10 03:47:47 UTC
(In reply to David Cook from comment #245)
> (In reply to Tomás Cohen Arazi from comment #238)
> > Created attachment 92350 [details] [review] [review] [review]
> > Bug 14570: Add about.pl entry for wrong relationship data
> > 
> > This patch adds a check and an entry to the about.pl, for bad
> > relationship data. The checks are:
> > 
> > - 'relationship' containing '_bad_data' (generated by the atomicupdate
> >   on undef relationships.
> > - 'relationship' values that are not in the borrowerRelationship
> > syspref.
> > 
> > Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
> 
> Unfortunately, this doesn't give sysadmins any idea about how to fix this
> problem.

For instance, I see a borrower_relationships.relationship column, but then there's also a borrowers.relationship column in the database. There are no constraints indicating the relationship between these two columns.

Trying to fix this via the UI isn't possible as the AJAX call is getting 500 errors.

I know I'll resolve it, but I've also been working on Koha for nearly 10 years now...
Comment 247 David Cook 2020-11-10 04:02:44 UTC
(In reply to David Cook from comment #246)
> Trying to fix this via the UI isn't possible as the AJAX call is getting 500
> errors.
> 
Of course, it could be that the problem is local, so I'm just going to stop posting for the time being...
Comment 248 David Cook 2020-11-10 04:27:28 UTC
Ok guarantor search broken due to local changes... I'll fix that.

As for borrower_relationships.relationship vs borrowers.relationship, it looks like borrowers.relationship is no longer used? Why don't we just delete that column then?

And with borrower_relationships it's the relationship that the guarantor has to the guarantee... all right.
Comment 249 Jonathan Druart 2020-11-10 08:35:55 UTC
(In reply to David Cook from comment #248)
> As for borrower_relationships.relationship vs borrowers.relationship, it
> looks like borrowers.relationship is no longer used? Why don't we just
> delete that column then?


Hum yes, I am coming to the same conclusion. Can someone deal with on a separate bug report please?
Comment 250 David Cook 2020-11-11 00:57:06 UTC
(In reply to Jonathan Druart from comment #249)
> Hum yes, I am coming to the same conclusion. Can someone deal with on a
> separate bug report please?

I'll raise the bug report. Cheers.
Comment 251 Jonathan Druart 2022-04-21 09:26:29 UTC
It seems that the following occurrences of "guarantorid" should have been removed:

koha-tmpl/intranet-tmpl/prog/en/includes/patronfields.inc:     [%- CASE 'guarantorid' -%]<span>Guarantor ID</span>

koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt:                                        <a class="btn btn-default btn-xs" href="memberentry.pl?op=modify&amp;borrowernumber=[% patron.borrowernumber | html %]&amp;step=1&amp;guarantorid=[% guarantor.borrowernumber | html %]"><i class="fa fa-pencil"></i> Edit</a>

t/db_dependent/Koha/Patrons.t:                guarantorid => $guarantor->borrowernumber

t/db_dependent/Koha/Patrons.t:                guarantorid => $guarantor->borrowernumber

t/lib/TestBuilder.pm:        # skip when FK points to itself: e.g. borrowers:guarantorid