Bug 9302 - Add ability to merge patron records
Summary: Add ability to merge patron records
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low new feature
Assignee: Kyle M Hall (khall)
QA Contact: Tomás Cohen Arazi (tcohen)
URL:
Keywords:
: 19567 (view as bug list)
Depends on:
Blocks: 20636 21041 21113 21648 26594
  Show dependency tree / graph
 
Reported: 2012-12-19 13:32 UTC by Nicole C. Engard
Modified: 2020-10-01 12:12 UTC (History)
19 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Koha now has the ability to merge patron accounts! To merge patrons, perform a patron search, select two or more patrons then click the 'Merge' button. Next, choose which patron you want to keep. Circulation data (checkouts, holds, fines, etc.) will be transferred to the remaining patron record.
Version(s) released in:
Circulation function:

Attachments
Bug 9302 - Add ability to merge patron records (21.91 KB, patch)
2017-08-30 14:15 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302 - Add ability to merge patron records (20.96 KB, patch)
2017-08-30 14:24 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302 - Add ability to merge patron records (20.83 KB, patch)
2017-10-26 13:27 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302 - Add ability to merge patron records (20.77 KB, patch)
2018-01-12 17:51 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302 [QA Followup] - Copy merged patrons to deletedborrowers table (605 bytes, patch)
2018-01-12 19:23 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302 - Add ability to merge patron records (20.82 KB, patch)
2018-01-12 19:29 UTC, Owen Leonard 		Details | Diff | Splinter Review
Bug 9302 [QA Followup] Copy merged patrons to deletedborrowers table (652 bytes, patch)
2018-01-12 19:29 UTC, Owen Leonard 		Details | Diff | Splinter Review
Bug 9302 - Add ability to merge patron records (20.87 KB, patch)
2018-01-14 18:27 UTC, ByWaterSandboxes 		Details | Diff | Splinter Review
Bug 9302 [QA Followup] Copy merged patrons to deletedborrowers table (760 bytes, patch)
2018-01-14 18:27 UTC, ByWaterSandboxes 		Details | Diff | Splinter Review
Bug 9302 - Add ability to merge patron records (21.14 KB, patch)
2018-02-22 11:21 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302 [QA Followup] Copy merged patrons to deletedborrowers table (761 bytes, patch)
2018-02-22 11:21 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302: Add ability to merge patron records (21.19 KB, patch)
2018-04-04 15:11 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302 [QA Followup]: Copy merged patrons to deletedborrowers table (784 bytes, patch)
2018-04-04 15:11 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302 [QA Followup]: Merge should be a transaction (2.01 KB, patch)
2018-04-04 15:11 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302 [QA Followup]: Merge should be a transaction (5.88 KB, patch)
2018-04-04 15:28 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302 [QA Followup]: Dumper plugin isn't used (1.03 KB, patch)
2018-04-04 15:30 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302: Add ability to merge patron records (21.19 KB, patch)
2018-04-04 19:58 UTC, Tomás Cohen Arazi (tcohen) 		Details | Diff | Splinter Review
Bug 9302: (QA follow-up) Copy merged patrons to deletedborrowers table (765 bytes, patch)
2018-04-04 19:58 UTC, Tomás Cohen Arazi (tcohen) 		Details | Diff | Splinter Review
Bug 9302: (QA follow-up) Merge should be a transaction (5.90 KB, patch)
2018-04-04 19:58 UTC, Tomás Cohen Arazi (tcohen) 		Details | Diff | Splinter Review
Bug 9302: (QA follow-up) Dumper plugin isn't used (1.07 KB, patch)
2018-04-04 19:59 UTC, Tomás Cohen Arazi (tcohen) 		Details | Diff | Splinter Review
Bug 9302: (QA follow-up) reverse try/catch block (889 bytes, patch)
2018-04-04 19:59 UTC, Tomás Cohen Arazi (tcohen) 		Details | Diff | Splinter Review
Bug 9302: (QA follow-up) Consistency follow-up (14.22 KB, patch)
2018-04-04 22:43 UTC, Tomás Cohen Arazi (tcohen) 		Details | Diff | Splinter Review
Bug 9302: (QA follow-up) Consistency follow-up (14.33 KB, patch)
2018-04-05 10:32 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302: Add ability to merge patron records (21.17 KB, patch)
2018-04-18 15:27 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302: Add ability to merge patron records (21.17 KB, patch)
2018-04-18 15:27 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302: (QA follow-up) Copy merged patrons to deletedborrowers table (781 bytes, patch)
2018-04-18 15:27 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302: (QA follow-up) Merge should be a transaction (5.94 KB, patch)
2018-04-18 15:28 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302: (QA follow-up) Dumper plugin isn't used (1.08 KB, patch)
2018-04-18 15:28 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302: (QA follow-up) Consistency follow-up (14.33 KB, patch)
2018-04-18 15:28 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302: Add aq tables to mapping (1.29 KB, patch)
2018-04-18 15:28 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302: Use new 'edit borrowers' permission (2.28 KB, patch)
2018-04-18 15:28 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302: Make the patron search show/hide more links work (2.07 KB, patch)
2018-04-18 15:28 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302: Add button directing use to keeper's patron record (986 bytes, patch)
2018-04-18 15:28 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302: Use patron-title.inc (2.32 KB, patch)
2018-04-18 15:28 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302: Wrap display_names block line with a span tag (1.28 KB, patch)
2018-04-18 15:28 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302: Update div classes (1.15 KB, patch)
2018-04-18 15:28 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302: Add error messages, correct number of unit tests in subtest 'Test Koha::Patrons::merge' (6.13 KB, patch)
2018-04-19 19:02 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 9302: Add error message if keeper patron is invalid (2.91 KB, patch)
2018-04-20 15:48 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Show Obsolete (24) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2012-12-19 13:32:36 UTC 
It would be great if there were a merge patrons feature. If you accidentally end up with one patron with two cards it would be nice to merge their records together so that you don't lose their history or holds or anything.
Comment 1 Cab Vinton 2017-02-26 09:02:31 UTC 
Yikes. Surprised to see so little interest in this feature.

Our staff are not perfect & I've identified dozens of duplicate accounts in our own system.

The bib record merge tool offers a useful template for accomplishing something like this.
Comment 2 Katrin Fischer 2017-06-05 10:38:44 UTC 
I think it might need a library/libraries sponsoring it. The feature would be useful!
Comment 3 Kyle M Hall (khall) 2017-08-30 14:15:20 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall (khall) 2017-08-30 14:24:29 UTC Comment hidden (obsolete)
Comment 5 Hugo Agud 2017-09-04 07:01:31 UTC 
I have applied the patch followed the test plan and it works fine!
Comment 6 Katrin Fischer 2017-09-04 09:03:49 UTC 
Hi Hugo, please add your sign-off line to patche that you mark as signed off. Instructions on how to do this can be found on the wiki.
Comment 7 Jonathan Druart 2017-09-07 17:43:33 UTC 
Readonly: see bug 16588

Koha::Patrons->merge: See 15336, it adds a Koha::Acquisition::Bookseller->merge_with #consistency
Comment 8 Tomás Cohen Arazi (tcohen) 2017-09-11 16:03:05 UTC 
Kyle, I'm failing it so it is obvious that some changes are needed for its inclusion!

(In reply to Jonathan Druart from comment #7)
> Readonly: see bug 16588

We are trying explicitly to get rid of Readonly, dicussion on that bug.

> Koha::Patrons->merge: See 15336, it adds a
> Koha::Acquisition::Bookseller->merge_with #consistency

Self explanatory!
Comment 9 Kyle M Hall (khall) 2017-10-26 13:26:53 UTC 
(In reply to Tomás Cohen Arazi from comment #8)
> Kyle, I'm failing it so it is obvious that some changes are needed for its
> inclusion!
> 
> (In reply to Jonathan Druart from comment #7)
> > Readonly: see bug 16588
> 
> We are trying explicitly to get rid of Readonly, dicussion on that bug.

Will fix!

> > Koha::Patrons->merge: See 15336, it adds a
> > Koha::Acquisition::Bookseller->merge_with #consistency
> 
> Self explanatory!

This one isn't so easy for me ; )
Bookseller->merge_with is a method for a single object, and acts upon it.
Koha::Patrons->merge is a method that accepts a patron, and a list of patrons to merge into. I think it's actually better because the parameters make is clear which patron will be retained. With Bookseller->merge, I cannot tell which Bookseller will be removed and which will be kept without reading the code.
Comment 10 Kyle M Hall (khall) 2017-10-26 13:27:18 UTC 
Created attachment 68606 [details] [review]
Bug 9302 - Add ability to merge patron records

It would be great if there were a merge patrons feature. If you accidentally end up with one patron with two cards it would be nice to merge their records together so that you don't lose their history or holds or anything.

This patch adds a basic patron merge feature. It attempts to relink all patron related tables from the patron(s) to be merged. It does not attempt to relink librarian account related tables at this time. This feature does not attempt to automatically resolve issues such as duplicate holds. Such a feature could build upon this one though.

Test Plan:
1) Apply this patch
2) Find two or more patrons
3) Perform a patron search that will bring them up on the same page of results,
   or add them all to a list of patrons.
4) Use the 'merge' button to begin the merging process
5) Choose a patron to keep
6) Verify the deleted patrons data ( checkouts, holds, etc )
   are now linked to the kept patron
Comment 11 Josef Moravec 2017-10-29 20:19:17 UTC 
Kyle, it's nice feature, and I really would like to have it in Koha.

But qa tools are complaining about this:

 FAIL	Koha/Patrons.pm
   FAIL	  valid
		Useless use of anonymous hash ({}) in void context 

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/members/merge-patrons.tt
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 30)
		forbidden pattern: tab char (line 33)
		forbidden pattern: tab char (line 50)
		forbidden pattern: tab char (line 39)
		forbidden pattern: tab char (line 42)
		forbidden pattern: tab char (line 49)
		forbidden pattern: tab char (line 36)
		forbidden pattern: tab char (line 123)
		forbidden pattern: tab char (line 46)
		forbidden pattern: tab char (line 51)
		forbidden pattern: tab char (line 32)
		forbidden pattern: tab char (line 41)
		forbidden pattern: tab char (line 40)
		forbidden pattern: tab char (line 55)
		forbidden pattern: tab char (line 34)
		forbidden pattern: tab char (line 122)
		forbidden pattern: tab char (line 53)
		forbidden pattern: tab char (line 127)
		forbidden pattern: tab char (line 38)
		forbidden pattern: tab char (line 37)
		forbidden pattern: tab char (line 35)
		forbidden pattern: tab char (line 126)
		forbidden pattern: tab char (line 48)
		forbidden pattern: tab char (line 43)
		forbidden pattern: tab char (line 29)
		forbidden pattern: tab char (line 52)
		forbidden pattern: tab char (line 125)
		forbidden pattern: tab char (line 124)
		forbidden pattern: tab char (line 31)
		forbidden pattern: tab char (line 47)
		forbidden pattern: tab char (line 54)
		forbidden pattern: tab char (line 45)
		forbidden pattern: tab char (line 44)
		forbidden pattern: tab char (line 121)
		forbidden pattern: tab char (line 120)

 FAIL	members/merge-patrons.pl
   FAIL	  valid
		Useless use of anonymous hash ({}) in void context 

 FAIL	t/db_dependent/Patrons.t
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 149)
		forbidden pattern: tab char (line 148)
   FAIL	  valid
		Useless use of anonymous hash ({}) in void context
Comment 12 John DeLooper 2017-11-02 21:44:03 UTC 
I too would love to see this kind of feature added, and am glad to see patches in progress. I think a process for merging creates a smoother experience than the current looking up duplicates and choosing which one to delete.
Comment 13 Owen Leonard 2017-11-09 16:11:37 UTC 
This doesn't work for me at all. When I select the patron record to keep and submit the form, I'm redirected to a page which says "Patron records merged into ()" and the records have not been merged.
Comment 14 Nick Clemens (kidclamp) 2018-01-12 16:52:58 UTC 
*** Bug 19567 has been marked as a duplicate of this bug. ***
Comment 15 Kyle M Hall (khall) 2018-01-12 17:51:34 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall (khall) 2018-01-12 19:23:03 UTC Comment hidden (obsolete)
Comment 17 Owen Leonard 2018-01-12 19:29:12 UTC Comment hidden (obsolete)
Comment 18 Owen Leonard 2018-01-12 19:29:15 UTC Comment hidden (obsolete)
Comment 19 ByWaterSandboxes 2018-01-14 18:27:09 UTC 
Patch tested with a sandbox, by Ed Veal <eveal@mckinneytexas.org>
Comment 20 ByWaterSandboxes 2018-01-14 18:27:32 UTC 
Created attachment 70482 [details] [review]
Bug 9302 - Add ability to merge patron records

It would be great if there were a merge patrons feature. If you
accidentally end up with one patron with two cards it would be nice to
merge their records together so that you don't lose their history or
holds or anything.

This patch adds a basic patron merge feature. It attempts to relink all
patron related tables from the patron(s) to be merged. It does not
attempt to relink librarian account related tables at this time. This
feature does not attempt to automatically resolve issues such as
duplicate holds. Such a feature could build upon this one though.

Test Plan:
1) Apply this patch
2) Find two or more patrons
3) Perform a patron search that will bring them up on the same page of
   results, or add them all to a list of patrons.
4) Use the 'merge' button to begin the merging process
5) Choose a patron to keep
6) Verify the deleted patrons data ( checkouts, holds, etc )
   are now linked to the kept patron

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Ed Veal <eveal@mckinneytexas.org>
Comment 21 ByWaterSandboxes 2018-01-14 18:27:35 UTC 
Created attachment 70483 [details] [review]
Bug 9302 [QA Followup] Copy merged patrons to deletedborrowers table

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Ed Veal <eveal@mckinneytexas.org>
Signed-off-by: BWS Sandboxes <ByWaterSandboxes@gmail.com>
Comment 22 Tomás Cohen Arazi (tcohen) 2018-02-20 17:37:04 UTC 
Sorry, please rebase.
Comment 23 Kyle M Hall (khall) 2018-02-22 11:21:01 UTC 
Created attachment 72082 [details] [review]
Bug 9302 - Add ability to merge patron records

It would be great if there were a merge patrons feature. If you
accidentally end up with one patron with two cards it would be nice to
merge their records together so that you don't lose their history or
holds or anything.

This patch adds a basic patron merge feature. It attempts to relink all
patron related tables from the patron(s) to be merged. It does not
attempt to relink librarian account related tables at this time. This
feature does not attempt to automatically resolve issues such as
duplicate holds. Such a feature could build upon this one though.

Test Plan:
1) Apply this patch
2) Find two or more patrons
3) Perform a patron search that will bring them up on the same page of
   results, or add them all to a list of patrons.
4) Use the 'merge' button to begin the merging process
5) Choose a patron to keep
6) Verify the deleted patrons data ( checkouts, holds, etc )
   are now linked to the kept patron

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Ed Veal <eveal@mckinneytexas.org>
Comment 24 Kyle M Hall (khall) 2018-02-22 11:21:09 UTC 
Created attachment 72083 [details] [review]
Bug 9302 [QA Followup] Copy merged patrons to deletedborrowers table

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Ed Veal <eveal@mckinneytexas.org>
Signed-off-by: BWS Sandboxes <ByWaterSandboxes@gmail.com>
Comment 25 Tomás Cohen Arazi (tcohen) 2018-04-04 14:30:39 UTC 
I tested the patches and they work as expected. Great job!

Some remarks:

Major: I'd say the ->merge method should execute code wrapped by a transaction. There are risky operations taking place in there.

Minor: To my taste, I'd prefer ->merge to be a Koha::Patron method. i.e. no 'keeper' param, as you want to merge the patron list with $self. Also, I'm not sure about borrowernumbers, they could just be Koha::Patron objects passed, and simplify the logic in ->merge.

Tests could be placed in t/db_dependent/Koha/Patrons.t
Comment 26 Kyle M Hall (khall) 2018-04-04 15:11:06 UTC 
Created attachment 73645 [details] [review]
Bug 9302: Add ability to merge patron records

It would be great if there were a merge patrons feature. If you
accidentally end up with one patron with two cards it would be nice to
merge their records together so that you don't lose their history or
holds or anything.

This patch adds a basic patron merge feature. It attempts to relink all
patron related tables from the patron(s) to be merged. It does not
attempt to relink librarian account related tables at this time. This
feature does not attempt to automatically resolve issues such as
duplicate holds. Such a feature could build upon this one though.

Test Plan:
1) Apply this patch
2) Find two or more patrons
3) Perform a patron search that will bring them up on the same page of
   results, or add them all to a list of patrons.
4) Use the 'merge' button to begin the merging process
5) Choose a patron to keep
6) Verify the deleted patrons data ( checkouts, holds, etc )
   are now linked to the kept patron

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Ed Veal <eveal@mckinneytexas.org>
Comment 27 Kyle M Hall (khall) 2018-04-04 15:11:14 UTC 
Created attachment 73646 [details] [review]
Bug 9302 [QA Followup]: Copy merged patrons to deletedborrowers table

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Ed Veal <eveal@mckinneytexas.org>
Signed-off-by: BWS Sandboxes <ByWaterSandboxes@gmail.com>
Comment 28 Kyle M Hall (khall) 2018-04-04 15:11:18 UTC 
Created attachment 73647 [details] [review]
Bug 9302 [QA Followup]: Merge should be a transaction
Comment 29 Kyle M Hall (khall) 2018-04-04 15:28:35 UTC 
Created attachment 73648 [details] [review]
Bug 9302 [QA Followup]: Merge should be a transaction
Comment 30 Kyle M Hall (khall) 2018-04-04 15:30:29 UTC 
Created attachment 73649 [details] [review]
Bug 9302 [QA Followup]: Dumper plugin isn't used
Comment 31 Tomás Cohen Arazi (tcohen) 2018-04-04 19:58:33 UTC 
Created attachment 73660 [details] [review]
Bug 9302: Add ability to merge patron records

It would be great if there were a merge patrons feature. If you
accidentally end up with one patron with two cards it would be nice to
merge their records together so that you don't lose their history or
holds or anything.

This patch adds a basic patron merge feature. It attempts to relink all
patron related tables from the patron(s) to be merged. It does not
attempt to relink librarian account related tables at this time. This
feature does not attempt to automatically resolve issues such as
duplicate holds. Such a feature could build upon this one though.

Test Plan:
1) Apply this patch
2) Find two or more patrons
3) Perform a patron search that will bring them up on the same page of
   results, or add them all to a list of patrons.
4) Use the 'merge' button to begin the merging process
5) Choose a patron to keep
6) Verify the deleted patrons data ( checkouts, holds, etc )
   are now linked to the kept patron

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Ed Veal <eveal@mckinneytexas.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 32 Tomás Cohen Arazi (tcohen) 2018-04-04 19:58:43 UTC 
Created attachment 73661 [details] [review]
Bug 9302: (QA follow-up) Copy merged patrons to deletedborrowers table

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Ed Veal <eveal@mckinneytexas.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 33 Tomás Cohen Arazi (tcohen) 2018-04-04 19:58:52 UTC 
Created attachment 73662 [details] [review]
Bug 9302: (QA follow-up) Merge should be a transaction

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 34 Tomás Cohen Arazi (tcohen) 2018-04-04 19:59:04 UTC 
Created attachment 73663 [details] [review]
Bug 9302: (QA follow-up) Dumper plugin isn't used

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 35 Tomás Cohen Arazi (tcohen) 2018-04-04 19:59:11 UTC Comment hidden (obsolete)
Comment 36 Tomás Cohen Arazi (tcohen) 2018-04-04 22:43:56 UTC Comment hidden (obsolete)
Comment 37 Kyle M Hall (khall) 2018-04-05 10:32:57 UTC 
Created attachment 73679 [details] [review]
Bug 9302: (QA follow-up) Consistency follow-up

This patch moves the Koha::Patrons->merge method into
Koha::Patron->merge_with in the line of the discussed implementation for
bug 15336. I agree with that implementation so I provide this follow-up.

Tests are adjusted, the controller script is adapted too. The behaviour
remains.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 38 Jonathan Druart 2018-04-12 17:34:42 UTC 
1. How did you generate $RESULTSET_PATRON_ID_MAPPING?
How are we going to keep it up-to-date?

2. Looks like acquisition tables are missing.

3. Since bug 18403 we should use CAN_user_borrowers_edit_borrowers instead of CAN_user_borrowers

(continuing later)
Comment 39 Jonathan Druart 2018-04-16 20:22:24 UTC 
4. on /members/merge-patrons.pl?id=19&id=5&id=45&id=21 the [+][-] links in the header do nothing (https://screenshots.firefox.com/kq2LyGsluEr0ir7F/pro.kohadev.org)

5. After this confirmation screen: https://screenshots.firefox.com/h9NQITzesVqeVQm6/pro.kohadev.org
I see these 3 holds:
https://screenshots.firefox.com/ZfjbD6PZWOBYv3Je/pro.kohadev.org

It's not clear to me what happened.

6. On this screen (result of the merge), we are left alone (back/return button is missing?)

7. flagsrequired   => { borrowers => 1 }
must be borrowers => 'edit_borrowers'

8. 
  <td>[% p.firstname | html %] [% p.surname | html %]</td>
and 
  <a href="moremember.pl?borrowernumber=[% keeper.id %]">[% keeper.firstname %] [% keeper.surname %] ([% keeper.cardnumber | html %])</a>

must be replaced with a call to patron-include.inc

9. TT, in the display_names block, you need to surround the strings with a html tag (for translation)

10. Not sure about that part, I am going to ping Owen:
 60 <div id="doc2" class="yui-t7">
 61    <div id="bd">
 62         <div id="yui-main">
Comment 40 Owen Leonard 2018-04-17 13:47:52 UTC 
> 10. Not sure about that part, I am going to ping Owen:
>  60 <div id="doc2" class="yui-t7">
>  61    <div id="bd">
>  62         <div id="yui-main">

I recommend this be replaced with:

    <div class="main container-fluid">
        <div class="row">
            <div class="col-md-10 col-md-offset-1 col-lg-8 col-lg-offset-2">

I'll work on updating the interface patterns page (https://wiki.koha-community.org/wiki/Interface_patterns#Page_layouts) on the wiki.

Please also move the JavaScript block to the footer. At the top of the page:

[% SET footerjs = 1 %]

Just before [% INCLUDE 'intranet-bottom.inc' %]:

[% MACRO jsinclude BLOCK %]
   ...your <script>
[% END %]

Note that you should leave off the script "type" attribute, and the "//<![CDATA[" markers are now obsolete.
Comment 41 Kyle M Hall (khall) 2018-04-18 15:27:31 UTC 
Created attachment 74439 [details] [review]
Bug 9302: Add ability to merge patron records

It would be great if there were a merge patrons feature. If you
accidentally end up with one patron with two cards it would be nice to
merge their records together so that you don't lose their history or
holds or anything.

This patch adds a basic patron merge feature. It attempts to relink all
patron related tables from the patron(s) to be merged. It does not
attempt to relink librarian account related tables at this time. This
feature does not attempt to automatically resolve issues such as
duplicate holds. Such a feature could build upon this one though.

Test Plan:
1) Apply this patch
2) Find two or more patrons
3) Perform a patron search that will bring them up on the same page of
   results, or add them all to a list of patrons.
4) Use the 'merge' button to begin the merging process
5) Choose a patron to keep
6) Verify the deleted patrons data ( checkouts, holds, etc )
   are now linked to the kept patron

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Ed Veal <eveal@mckinneytexas.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 42 Kyle M Hall (khall) 2018-04-18 15:27:48 UTC 
Created attachment 74440 [details] [review]
Bug 9302: Add ability to merge patron records

It would be great if there were a merge patrons feature. If you
accidentally end up with one patron with two cards it would be nice to
merge their records together so that you don't lose their history or
holds or anything.

This patch adds a basic patron merge feature. It attempts to relink all
patron related tables from the patron(s) to be merged. It does not
attempt to relink librarian account related tables at this time. This
feature does not attempt to automatically resolve issues such as
duplicate holds. Such a feature could build upon this one though.

Test Plan:
1) Apply this patch
2) Find two or more patrons
3) Perform a patron search that will bring them up on the same page of
   results, or add them all to a list of patrons.
4) Use the 'merge' button to begin the merging process
5) Choose a patron to keep
6) Verify the deleted patrons data ( checkouts, holds, etc )
   are now linked to the kept patron

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Ed Veal <eveal@mckinneytexas.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 43 Kyle M Hall (khall) 2018-04-18 15:27:57 UTC 
Created attachment 74441 [details] [review]
Bug 9302: (QA follow-up) Copy merged patrons to deletedborrowers table

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Ed Veal <eveal@mckinneytexas.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 44 Kyle M Hall (khall) 2018-04-18 15:28:01 UTC 
Created attachment 74442 [details] [review]
Bug 9302: (QA follow-up) Merge should be a transaction

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 45 Kyle M Hall (khall) 2018-04-18 15:28:05 UTC 
Created attachment 74443 [details] [review]
Bug 9302: (QA follow-up) Dumper plugin isn't used

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 46 Kyle M Hall (khall) 2018-04-18 15:28:10 UTC 
Created attachment 74444 [details] [review]
Bug 9302: (QA follow-up) Consistency follow-up

This patch moves the Koha::Patrons->merge method into
Koha::Patron->merge_with in the line of the discussed implementation for
bug 15336. I agree with that implementation so I provide this follow-up.

Tests are adjusted, the controller script is adapted too. The behaviour
remains.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 47 Kyle M Hall (khall) 2018-04-18 15:28:14 UTC 
Created attachment 74445 [details] [review]
Bug 9302: Add aq tables to mapping
Comment 48 Kyle M Hall (khall) 2018-04-18 15:28:18 UTC 
Created attachment 74446 [details] [review]
Bug 9302: Use new 'edit borrowers' permission
Comment 49 Kyle M Hall (khall) 2018-04-18 15:28:23 UTC 
Created attachment 74447 [details] [review]
Bug 9302: Make the patron search show/hide more links work
Comment 50 Kyle M Hall (khall) 2018-04-18 15:28:27 UTC 
Created attachment 74448 [details] [review]
Bug 9302: Add button directing use to keeper's patron record
Comment 51 Kyle M Hall (khall) 2018-04-18 15:28:31 UTC 
Created attachment 74449 [details] [review]
Bug 9302: Use patron-title.inc
Comment 52 Kyle M Hall (khall) 2018-04-18 15:28:35 UTC 
Created attachment 74450 [details] [review]
Bug 9302: Wrap display_names block line with a span tag
Comment 53 Kyle M Hall (khall) 2018-04-18 15:28:39 UTC 
Created attachment 74451 [details] [review]
Bug 9302: Update div classes
Comment 54 Jonathan Druart 2018-04-18 19:35:53 UTC 
Thanks for the follow-ups.

(In reply to Jonathan Druart from comment #38)
> 1. How did you generate $RESULTSET_PATRON_ID_MAPPING?
> How are we going to keep it up-to-date?

Answer still needed.

(In reply to Jonathan Druart from comment #39)
> 4. on /members/merge-patrons.pl?id=19&id=5&id=45&id=21 the [+][-] links in
> the header do nothing
> (https://screenshots.firefox.com/kq2LyGsluEr0ir7F/pro.kohadev.org)
> 
> 5. After this confirmation screen:
> https://screenshots.firefox.com/h9NQITzesVqeVQm6/pro.kohadev.org
> I see these 3 holds:
> https://screenshots.firefox.com/ZfjbD6PZWOBYv3Je/pro.kohadev.org
> 
> It's not clear to me what happened.

Answering to myself:
So it's not the holds that have been transferred in the sense we use it usually, that was I was confused.


Errors handling should be polished a bit. For instance I am seeing these screens when id or keeper does not match valid borrowernumbers:
https://screenshots.firefox.com/nIgsc8xOpLHvuqXC/pro.kohadev.org
https://screenshots.firefox.com/VTKYZNqBS3L8B5cq/pro.kohadev.org
https://screenshots.firefox.com/gEGiMi16yknuyrZU/pro.kohadev.org
Comment 55 Kyle M Hall (khall) 2018-04-19 18:45:39 UTC 
(In reply to Jonathan Druart from comment #54)
> Thanks for the follow-ups.
> 
> (In reply to Jonathan Druart from comment #38)
> > 1. How did you generate $RESULTSET_PATRON_ID_MAPPING?
> > How are we going to keep it up-to-date?

I went through the tables looking for columns that represent borrowernumbers. It's pretty easy for columns with foreign keys to borrowers.borrowernumber, but not all the columns have a borrowernumber. We should just add that to the qa process. If a new column is added that is a borrowernumber, it should be added to the mapping. We could move the mapping itself its own perl module to make it more obvious if you wish.
Comment 56 Kyle M Hall (khall) 2018-04-19 19:02:58 UTC 
Created attachment 74585 [details] [review]
Bug 9302: Add error messages, correct number of unit tests in subtest 'Test Koha::Patrons::merge'
Comment 57 Kyle M Hall (khall) 2018-04-19 19:04:39 UTC 
This followup adds errors for the first and last situations. The middle one only occurs if you apply the patch set but don't restart plack ( at least for me ) so it's not a valid production error afaict.

> Errors handling should be polished a bit. For instance I am seeing these
> screens when id or keeper does not match valid borrowernumbers:
> https://screenshots.firefox.com/nIgsc8xOpLHvuqXC/pro.kohadev.org
> https://screenshots.firefox.com/VTKYZNqBS3L8B5cq/pro.kohadev.org
> https://screenshots.firefox.com/gEGiMi16yknuyrZU/pro.kohadev.org
Comment 58 Jonathan Druart 2018-04-19 19:28:36 UTC 
(In reply to Kyle M Hall from comment #57)
> This followup adds errors for the first and last situations. The middle one
> only occurs if you apply the patch set but don't restart plack ( at least
> for me ) so it's not a valid production error afaict.
> 
> > Errors handling should be polished a bit. For instance I am seeing these
> > screens when id or keeper does not match valid borrowernumbers:
> > https://screenshots.firefox.com/nIgsc8xOpLHvuqXC/pro.kohadev.org
> > https://screenshots.firefox.com/VTKYZNqBS3L8B5cq/pro.kohadev.org
> > https://screenshots.firefox.com/gEGiMi16yknuyrZU/pro.kohadev.org

IIRC it happened when keeper was invalid.
Comment 59 Kyle M Hall (khall) 2018-04-20 15:48:05 UTC 
Created attachment 74653 [details] [review]
Bug 9302: Add error message if keeper patron is invalid
Comment 60 Jonathan Druart 2018-04-20 16:44:17 UTC 
(In reply to Kyle M Hall from comment #55)
> (In reply to Jonathan Druart from comment #54)
> > Thanks for the follow-ups.
> > 
> > (In reply to Jonathan Druart from comment #38)
> > > 1. How did you generate $RESULTSET_PATRON_ID_MAPPING?
> > > How are we going to keep it up-to-date?
> 
> I went through the tables looking for columns that represent
> borrowernumbers. It's pretty easy for columns with foreign keys to
> borrowers.borrowernumber, but not all the columns have a borrowernumber. We
> should just add that to the qa process. If a new column is added that is a
> borrowernumber, it should be added to the mapping. We could move the mapping
> itself its own perl module to make it more obvious if you wish.

I am going to push as it but I am not convinced by this approach.
We will not have any tests to alert us if we forgot to update this mapping.
Comment 61 Jonathan Druart 2018-04-20 16:45:31 UTC 
Pushed to master for 18.05, thanks to everybody involved!
Comment 62 Kyle M Hall (khall) 2018-04-23 11:01:27 UTC 
(In reply to Jonathan Druart from comment #60)
> (In reply to Kyle M Hall from comment #55)
> > (In reply to Jonathan Druart from comment #54)
> > > Thanks for the follow-ups.
> > > 
> > > (In reply to Jonathan Druart from comment #38)
> > > > 1. How did you generate $RESULTSET_PATRON_ID_MAPPING?
> > > > How are we going to keep it up-to-date?
> > 
> > I went through the tables looking for columns that represent
> > borrowernumbers. It's pretty easy for columns with foreign keys to
> > borrowers.borrowernumber, but not all the columns have a borrowernumber. We
> > should just add that to the qa process. If a new column is added that is a
> > borrowernumber, it should be added to the mapping. We could move the mapping
> > itself its own perl module to make it more obvious if you wish.
> 
> I am going to push as it but I am not convinced by this approach.
> We will not have any tests to alert us if we forgot to update this mapping.

I'm sure we can do two things with unit tests:
1) Automate checking that all borrowernumber fields that are foreign keys to borrowers.borrowernumber are in our mapping
2) Automate testing all non-fk fields that are *named* borrowernumber are in our mapping.

As for non-fk fields that are not named bororwernumber, I don't think there is anything we can do.
Comment 63 Tomás Cohen Arazi (tcohen) 2018-04-23 11:03:56 UTC 
(In reply to Kyle M Hall from comment #62)
> (In reply to Jonathan Druart from comment #60)
> > (In reply to Kyle M Hall from comment #55)
> > > (In reply to Jonathan Druart from comment #54)
> > > > Thanks for the follow-ups.
> > > > 
> > > > (In reply to Jonathan Druart from comment #38)
> > > > > 1. How did you generate $RESULTSET_PATRON_ID_MAPPING?
> > > > > How are we going to keep it up-to-date?
> > > 
> > > I went through the tables looking for columns that represent
> > > borrowernumbers. It's pretty easy for columns with foreign keys to
> > > borrowers.borrowernumber, but not all the columns have a borrowernumber. We
> > > should just add that to the qa process. If a new column is added that is a
> > > borrowernumber, it should be added to the mapping. We could move the mapping
> > > itself its own perl module to make it more obvious if you wish.
> > 
> > I am going to push as it but I am not convinced by this approach.
> > We will not have any tests to alert us if we forgot to update this mapping.
> 
> I'm sure we can do two things with unit tests:
> 1) Automate checking that all borrowernumber fields that are foreign keys to
> borrowers.borrowernumber are in our mapping
> 2) Automate testing all non-fk fields that are *named* borrowernumber are in
> our mapping.
> 
> As for non-fk fields that are not named bororwernumber, I don't think there
> is anything we can do.

Can we use the is_boolean approach on the screens files? is_patron_id?
Comment 64 Kyle M Hall (khall) 2018-04-23 11:05:56 UTC 
> Can we use the is_boolean approach on the screens files? is_patron_id?

We could be I don't think it solves the root problem. A developer who would forget to add a new column to the merge patron's mapping is the same developer that would forget to set is_patron_id in the schema file ; )
Comment 65 Kyle M Hall (khall) 2018-04-23 13:26:38 UTC 
(In reply to Kyle M Hall from comment #64)
> > Can we use the is_boolean approach on the screens files? is_patron_id?
> 
> We could be I don't think it solves the root problem. A developer who would
> forget to add a new column to the merge patron's mapping is the same
> developer that would forget to set is_patron_id in the schema file ; )

I just want to clarify I think that would be an excellent enhancement that would definitely simplify the code!