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?

I think in order to have n:n relationships separate tables will be needed. Would also like to see some more information about this!

Test plan will be forthcoming.

Working on unit tests.

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.

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.

Auto-merging Koha/Schema/Result/BorrowerModification.pm
CONFLICT (content): Merge conflict in Koha/Schema/Result/BorrowerModification.pm
Failed to merge in the changes.

Rebased, and corrected atomic update filename. :)

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

What about comment 14?

(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.

> 
> 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?

Hi Kyle, patch doesn't apply because of recent changes to the patrons api. Can you please rebase?

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.

> 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.

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.

(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.

Squashed patch for ease of reading the entire change set: https://github.com/bywatersolutions/bywater-koha-devel/commit/be99e2aa23c3896e9c0aaedf464a5141d676556d

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>

Created attachment 77078 [details] [review]
Bug 14570: Update schema files [Do Not Push]

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

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>

Created attachment 77291 [details] [review]
Bug 14570: Update schema files [Do Not Push]

(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?

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>

Created attachment 77347 [details] [review]
Bug 14570: Update schema files [Do Not Push]

Created attachment 77348 [details] [review]
Bug 14570: Remove double declaration of $patron in opac-user.pl

- 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

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.

Should be rebased on top of bug 18635.

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>

Created attachment 81933 [details] [review]
Bug 14570: Update schema files [Do Not Push]

Did you address comment 117 and comment 118?

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."

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>

Created attachment 85412 [details] [review]
Bug 14570: Update schema files [Do Not Push]

Created attachment 85431 [details] [review]
Bug 14570: (QA Follow-up) Misc changes based on QA feedback

Hi Kyle, could you reply to 117/118 marking the points fixed/commenting? Thx!

I'm marking this "Failed QA" to get it out of the queue until Comment 117 and Comment 118 are responded to.

Also needs to be rebased!

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>

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>

Created attachment 89891 [details] [review]
Bug 14570: (QA Follow-up) Misc changes based on QA feedback

Created attachment 89892 [details] [review]
Bug 14570: Update schema files [Do Not Push]

(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

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>

Created attachment 89910 [details] [review]
Bug 14570: Update schema files [Do Not Push]

New patch has the followup squashed in for simplicity.

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>

Created attachment 89912 [details] [review]
Bug 14570: Update schema files [Do Not Push]

Created attachment 89921 [details] [review]
Bug 14570: (follow-up) fix table name in j2a.pl

This patch seems to break inserting sample patrons, because the sample patrons file has a guarantorid field and this patch removes it.

Created attachment 89922 [details] [review]
Bug 14570: (follow-up) fix qa FAIL tests

(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

Created attachment 89929 [details] [review]
Bug 14570: (follow-up) Fix tests

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

Created attachment 90048 [details] [review]
Bug 14570: (QA follow-up) fix sample borrowers, remove guarantorid)

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

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

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.

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

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.

Created attachment 91231 [details] [review]
Bug 14570: (follow-up) Fix patron type when "add child" button is pressed

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>

Created attachment 91440 [details] [review]
Bug 14570: Update schema files [Do Not Push]

Created attachment 91441 [details] [review]
Bug 14570: (follow-up) fix table name in j2a.pl

Created attachment 91442 [details] [review]
Bug 14570: (follow-up) fix qa FAIL tests

Created attachment 91443 [details] [review]
Bug 14570: (follow-up) Fix tests

Created attachment 91444 [details] [review]
Bug 14570: (QA follow-up) fix sample borrowers, remove guarantorid)

Created attachment 91445 [details] [review]
Bug 14570: (follow-up) Fix patron type when "add child" button is pressed

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

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>

Created attachment 91695 [details] [review]
Bug 14570: (follow-up) fix table name in j2a.pl

Created attachment 91696 [details] [review]
Bug 14570: (follow-up) fix qa FAIL tests

Created attachment 91697 [details] [review]
Bug 14570: (follow-up) Fix tests

Created attachment 91698 [details] [review]
Bug 14570: (QA follow-up) fix sample borrowers, remove guarantorid)

Created attachment 91699 [details] [review]
Bug 14570: (follow-up) Fix patron type when "add child" button is pressed

Created attachment 91700 [details] [review]
Bug 14570: Update schema files [Do Not Push]

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.

(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!

Created attachment 92157 [details] [review]
Bug 14570: (QA follow-up) Fix issues in Guarantee context

(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.

(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".

Created attachment 92158 [details] [review]
Bug 14570: (QA follow-up) Fix issues in Guarantor context

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

Created attachment 92277 [details] [review]
Bug 14570: (QA follow-up) Fix tests

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

Created attachment 92278 [details] [review]
Bug 14570: (QA follow-up) Fix tests

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

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>

Created attachment 92280 [details] [review]
Bug 14570: (follow-up) fix table name in j2a.pl

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

Created attachment 92281 [details] [review]
Bug 14570: (follow-up) fix qa FAIL tests

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

Created attachment 92282 [details] [review]
Bug 14570: (follow-up) Fix tests

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

Created attachment 92283 [details] [review]
Bug 14570: (QA follow-up) fix sample borrowers, remove guarantorid)

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

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>

Created attachment 92285 [details] [review]
Bug 14570: Update schema files [Do Not Push]

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

Created attachment 92286 [details] [review]
Bug 14570: (QA follow-up) Fix issues in Guarantee context

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

Created attachment 92287 [details] [review]
Bug 14570: (QA follow-up) Fix issues in Guarantor context

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

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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?

(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).

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>

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>

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>

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>

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>

Nice work!

Pushed to master for 19.11.00

Enhancement not pushed to 19.05.x

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)

Sponsorship lines moved into git commit for release notes and about page

It looks like this broke the non-English installers. Can we please get a fix? Filed as bug 24317.

It looks like guarantorid still appears in Koha/REST/V1/Patrons.pm although we state that it cannot be updated with the API now.

(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.

(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...

(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...

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.

(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?

(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.

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