Bug 14570 - Make it possible to add multiple guarantors to a record
Summary: Make it possible to add multiple guarantors to a record
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Kyle M Hall
QA Contact:
URL:
Whiteboard:
Keywords:
: 1700 (view as bug list)
Depends on: 14577 17790
Blocks: 19382 20211 20691
  Show dependency treegraph
 
Reported: 2015-07-20 22:36 UTC by Jesse Weaver
Modified: 2019-04-24 15:06 UTC (History)
16 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
This features 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 person has no patron record in Koha ). This feature retains the existing behavior for importing guarantors during patron imports. Guarantors can no longer be viewed/added/updated from the REST API. Reports that utilize borrowers.guarantorid will need to be updated.
Version(s) released in:


Attachments
Bug 14570 - Make it possible to add multiple guarantors to a record (108.32 KB, patch)
2016-06-14 16:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Update Schema (6.22 KB, patch)
2016-06-14 16:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Make it possible to add multiple guarantors to a record (109.62 KB, patch)
2016-06-14 17:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Update Schema (6.22 KB, patch)
2016-06-14 17:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Make it possible to add multiple guarantors to a record (110.98 KB, patch)
2016-06-14 18:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Update Schema (6.22 KB, patch)
2016-06-14 18:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Update Schema (6.95 KB, patch)
2016-12-12 18:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Make it possible to add multiple guarantors to a record (107.06 KB, patch)
2016-12-12 18:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Proposal for initial state of add guarantor form (5.53 KB, image/png)
2016-12-13 14:45 UTC, Owen Leonard
Details
Bug 14570 - Make it possible to add multiple guarantors to a record (107.60 KB, patch)
2016-12-15 14:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Make it possible to add multiple guarantors to a record (107.35 KB, patch)
2016-12-22 15:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Make it possible to add multiple guarantors to a record (108.53 KB, patch)
2016-12-22 17:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Make it possible to add multiple guarantors to a record (108.93 KB, patch)
2016-12-23 11:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Make it possible to add multiple guarantors to a record (108.97 KB, patch)
2017-01-09 16:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Update Schema (6.95 KB, patch)
2017-01-09 16:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Followup (1.13 KB, patch)
2017-01-10 11:50 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14570 - Make it possible to add multiple guarantors to a record (109.29 KB, patch)
2017-01-10 16:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Update Schema (6.95 KB, patch)
2017-01-10 16:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Followup (1.13 KB, patch)
2017-01-10 16:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Make it possible to add multiple guarantors to a record (109.32 KB, patch)
2017-01-11 17:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Update Schema (6.95 KB, patch)
2017-01-11 17:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Followup (1.13 KB, patch)
2017-01-11 17:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Make it possible to add multiple guarantors to a record (109.29 KB, patch)
2017-01-11 17:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Update Schema (6.95 KB, patch)
2017-01-11 17:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Followup (1.13 KB, patch)
2017-01-11 17:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Make it possible to add multiple guarantors to a record (109.17 KB, patch)
2017-01-13 11:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Update Schema (6.95 KB, patch)
2017-01-13 11:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Followup (1.13 KB, patch)
2017-01-13 11:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Make it possible to add multiple guarantors to a record (109.20 KB, patch)
2017-01-13 11:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Update Schema (6.95 KB, patch)
2017-01-13 11:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Followup (1.13 KB, patch)
2017-01-13 11:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Make it possible to add multiple guarantors to a record (109.46 KB, patch)
2017-01-13 19:41 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14570 - Update Schema (6.95 KB, patch)
2017-01-13 19:41 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14570 - Followup (1.13 KB, patch)
2017-01-13 19:41 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14570 - Make it possible to add multiple guarantors to a record (109.26 KB, patch)
2017-01-13 19:46 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14570 - Update Schema (6.95 KB, patch)
2017-01-13 19:46 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14570 - Followup (1.13 KB, patch)
2017-01-13 19:46 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14570 - Make it possible to add multiple guarantors to a record (109.26 KB, patch)
2017-01-13 19:54 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14570 - Update Schema (6.95 KB, patch)
2017-01-13 19:54 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14570 - Followup (1.13 KB, patch)
2017-01-13 19:54 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 14570 - Make it possible to add multiple guarantors to a record (107.66 KB, patch)
2017-04-20 11:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 14570 - Update Schema (7.06 KB, patch)
2017-04-20 11:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 14570 - Followup (1.25 KB, patch)
2017-04-20 11:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Make it possible to add multiple guarantors to a record (106.09 KB, patch)
2017-06-09 10:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Followup (1.23 KB, patch)
2017-06-09 10:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Update Schema (6.95 KB, patch)
2017-06-09 10:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Make it possible to add multiple guarantors to a record (110.33 KB, patch)
2018-02-02 18:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Update Schema (6.97 KB, patch)
2018-02-02 18:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Make it possible to add multiple guarantors to a record (110.43 KB, patch)
2018-02-02 18:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Update Schema (6.97 KB, patch)
2018-02-02 18:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Make it possible to add multiple guarantors to a record (109.82 KB, patch)
2018-03-26 14:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Update Schema (7.05 KB, patch)
2018-03-26 14:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Make it possible to add multiple guarantors to a record (109.80 KB, patch)
2018-04-03 13:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Update Schema (7.05 KB, patch)
2018-04-03 13:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Make it possible to add multiple guarantors to a record (109.72 KB, patch)
2018-05-01 15:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Update Schema (7.05 KB, patch)
2018-05-01 15:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 [QA Followup] - Fix QA issues with POD (806 bytes, patch)
2018-05-01 15:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 [QA Followup] - Fix QA issues with POD (1.43 KB, patch)
2018-05-01 15:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 [QA Followup] - Show add guarantor pane for correct patron types, fix "Add child" button (2.26 KB, patch)
2018-05-01 16:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Make it possible to add multiple guarantors to a record (110.20 KB, patch)
2018-05-02 13:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Update Schema (7.05 KB, patch)
2018-05-02 13:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 [QA Followup] - Fix QA issues with POD (1.43 KB, patch)
2018-05-02 13:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 [QA Followup] - Show add guarantor pane for correct patron types, fix "Add child" button (2.26 KB, patch)
2018-05-02 13:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Make it possible to add multiple guarantors to a record (109.74 KB, patch)
2018-07-17 10:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 [QA Followup] - Fix QA issues with POD (1.42 KB, patch)
2018-07-17 10:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 [QA Followup] - Show add guarantor pane for correct patron types, fix "Add child" button (2.25 KB, patch)
2018-07-17 10:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 - Update schema files [Do Not Push] (7.06 KB, patch)
2018-07-17 10:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 [QA Followup] - Fix issue with only one new guarantor being saved (1.39 KB, patch)
2018-07-17 10:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 [QA Followup]: Make delete label clickable (1.26 KB, patch)
2018-07-17 10:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570: Make it possible to add multiple guarantors to a record (109.73 KB, patch)
2018-07-17 14:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 [QA Followup]: Fix QA issues with POD (1.42 KB, patch)
2018-07-17 14:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 [QA Followup]: Show add guarantor pane for correct patron types, fix "Add child" button (2.25 KB, patch)
2018-07-17 14:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570: Update schema files [Do Not Push] (7.13 KB, patch)
2018-07-17 14:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 [QA Followup]: Fix issue with only one new guarantor being saved (1.38 KB, patch)
2018-07-17 14:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 [QA Followup]: Make delete label clickable (1.26 KB, patch)
2018-07-17 14:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 [QA Followup]: Rename relationships table to borrower_relationships (4.84 KB, patch)
2018-07-17 14:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 [QA Followup]: Add Koha::Patron::add_gurarantor (4.73 KB, patch)
2018-07-17 14:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 [QA Followup]: change instance of param to multi_param (921 bytes, patch)
2018-07-17 14:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 [QA Followup]: Don't remove existing columns for the time being, pending review of request 3 (1.73 KB, patch)
2018-07-17 14:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570: Make it possible to add multiple guarantors to a record (109.73 KB, patch)
2018-07-18 13:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 [QA Followup]: Fix QA issues with POD (1.42 KB, patch)
2018-07-18 13:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 [QA Followup]: Show add guarantor pane for correct patron types, fix "Add child" button (2.25 KB, patch)
2018-07-18 13:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 [QA Followup]: Fix issue with only one new guarantor being saved (1.38 KB, patch)
2018-07-18 13:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 [QA Followup]: Make delete label clickable (1.26 KB, patch)
2018-07-18 13:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 [QA Followup]: Rename relationships table to borrower_relationships (4.84 KB, patch)
2018-07-18 13:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 [QA Followup]: Add Koha::Patron::add_gurarantor (4.73 KB, patch)
2018-07-18 13:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 [QA Followup]: change instance of param to multi_param (921 bytes, patch)
2018-07-18 13:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570 [QA Followup]: Remove ability to add multiple non-koha patrons as guarantors, bring back contact fields (39.81 KB, patch)
2018-07-18 13:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570: Update schema files [Do Not Push] (4.40 KB, patch)
2018-07-18 13:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570: Make it possible to add multiple guarantors to a record (113.37 KB, patch)
2018-07-18 13:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570: Update schema files [Do Not Push] (4.40 KB, patch)
2018-07-18 13:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570: Make it possible to add multiple guarantors to a record (112.26 KB, patch)
2018-07-26 14:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570: Update schema files [Do Not Push] (4.40 KB, patch)
2018-07-26 14:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570: Make it possible to add multiple guarantors to a record (111.98 KB, patch)
2018-07-31 18:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14570: Update schema files [Do Not Push] (4.40 KB, patch)
2018-07-31 18:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14570: Remove double declaration of $patron in opac-user.pl (712 bytes, patch)
2018-07-31 18:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14570: Make it possible to add multiple guarantors to a record (118.38 KB, patch)
2018-10-18 13:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570: Update schema files [Do Not Push] (5.69 KB, patch)
2018-10-18 14:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570: Make it possible to add multiple guarantors to a record (118.22 KB, patch)
2018-11-05 13:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570: Update schema files [Do Not Push] (4.95 KB, patch)
2018-11-05 13:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570: Make it possible to add multiple guarantors to a record (117.90 KB, patch)
2019-02-20 20:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570: Update schema files [Do Not Push] (6.12 KB, patch)
2019-02-20 20:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14570: (QA Follow-up) Misc changes based on QA feedback (4.01 KB, patch)
2019-02-21 11:44 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 M. Tompsett 2017-01-13 19:41:09 UTC Comment hidden (obsolete)
Comment 40 M. Tompsett 2017-01-13 19:41:14 UTC Comment hidden (obsolete)
Comment 41 M. Tompsett 2017-01-13 19:41:20 UTC Comment hidden (obsolete)
Comment 42 M. Tompsett 2017-01-13 19:46:15 UTC Comment hidden (obsolete)
Comment 43 M. Tompsett 2017-01-13 19:46:22 UTC Comment hidden (obsolete)
Comment 44 M. Tompsett 2017-01-13 19:46:27 UTC Comment hidden (obsolete)
Comment 45 M. Tompsett 2017-01-13 19:54:26 UTC Comment hidden (obsolete)
Comment 46 M. Tompsett 2017-01-13 19:54:32 UTC Comment hidden (obsolete)
Comment 47 M. Tompsett 2017-01-13 19:54:39 UTC Comment hidden (obsolete)
Comment 48 M. 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!