Description
Nicole C. Engard
2012-12-19 13:32:36 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. I think it might need a library/libraries sponsoring it. The feature would be useful! Created attachment 66615 [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 Created attachment 66617 [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 I have applied the patch followed the test plan and it works fine! 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. Readonly: see bug 16588 Koha::Patrons->merge: See 15336, it adds a Koha::Acquisition::Bookseller->merge_with #consistency 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! (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. 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 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 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. 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. *** Bug 19567 has been marked as a duplicate of this bug. *** Created attachment 70474 [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 Created attachment 70477 [details] [review] Bug 9302 [QA Followup] - Copy merged patrons to deletedborrowers table Created attachment 70478 [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> Created attachment 70479 [details] [review] Bug 9302 [QA Followup] Copy merged patrons to deletedborrowers table Signed-off-by: Owen Leonard <oleonard@myacpl.org> Patch tested with a sandbox, by Ed Veal <eveal@mckinneytexas.org> 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> 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> Sorry, please rebase. 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> 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> 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 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> 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> Created attachment 73647 [details] [review] Bug 9302 [QA Followup]: Merge should be a transaction Created attachment 73648 [details] [review] Bug 9302 [QA Followup]: Merge should be a transaction Created attachment 73649 [details] [review] Bug 9302 [QA Followup]: Dumper plugin isn't used 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> 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> Created attachment 73662 [details] [review] Bug 9302: (QA follow-up) Merge should be a transaction Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 73663 [details] [review] Bug 9302: (QA follow-up) Dumper plugin isn't used Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 73664 [details] [review] Bug 9302: (QA follow-up) reverse try/catch block Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 73668 [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> 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> 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) 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"> > 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. 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> 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> 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> Created attachment 74442 [details] [review] Bug 9302: (QA follow-up) Merge should be a transaction Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 74443 [details] [review] Bug 9302: (QA follow-up) Dumper plugin isn't used Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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> Created attachment 74445 [details] [review] Bug 9302: Add aq tables to mapping Created attachment 74446 [details] [review] Bug 9302: Use new 'edit borrowers' permission Created attachment 74447 [details] [review] Bug 9302: Make the patron search show/hide more links work Created attachment 74448 [details] [review] Bug 9302: Add button directing use to keeper's patron record Created attachment 74449 [details] [review] Bug 9302: Use patron-title.inc Created attachment 74450 [details] [review] Bug 9302: Wrap display_names block line with a span tag Created attachment 74451 [details] [review] Bug 9302: Update div classes 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 (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. Created attachment 74585 [details] [review] Bug 9302: Add error messages, correct number of unit tests in subtest 'Test Koha::Patrons::merge' 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
(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. Created attachment 74653 [details] [review] Bug 9302: Add error message if keeper patron is invalid (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. Pushed to master for 18.05, thanks to everybody involved! (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. (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? > 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 ; )
(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! |