C4::Members::checkuniquemember was not really nicely written, was only used once and was not covered by tests. I think it does not make sense to keep such complexity and have this code in the subroutine/method.
Created attachment 53334 [details] [review] Bug 16909: Koha::Patrons - Remove checkuniquemember C4::Members::checkuniquemember was not really nicely written, was only used once and was not covered by tests. I think it does not make sense to keep such complexity and have this code in the subroutine/method. Looking at this patch it seems that what this subroutine did can be done easily in the pl script in few lines. Test plan: 1/ Create 2 organisations with the same "surname": you should get a warning. 2/ Create 2 patrons (non-organisation) with the same surname/firstname/date of birth, you should get a warning
Hi Jonathan, If a duplicate entry is found, the link "View existing record" and the button "It's a duplicate" always redirect to a borrowernumber=1 and not to the borrowernumber found.
Created attachment 53345 [details] [review] Bug 16909: Koha::Patrons - Remove checkuniquemember C4::Members::checkuniquemember was not really nicely written, was only used once and was not covered by tests. I think it does not make sense to keep such complexity and have this code in the subroutine/method. Looking at this patch it seems that what this subroutine did can be done easily in the pl script in few lines. Test plan: 1/ Create 2 organisations with the same "surname": you should get a warning. 2/ Create 2 patrons (non-organisation) with the same surname/firstname/date of birth, you should get a warning
(In reply to Marc Véron from comment #2) > Hi Jonathan, > > If a duplicate entry is found, the link "View existing record" and the > button "It's a duplicate" always redirect to a borrowernumber=1 and not to > the borrowernumber found. Yes good catch. I can now remove the FIXME I added, I understand the 2 variables :)
Created attachment 53350 [details] [review] Bug 16909: Koha::Patrons - Remove checkuniquemember C4::Members::checkuniquemember was not really nicely written, was only used once and was not covered by tests. I think it does not make sense to keep such complexity and have this code in the subroutine/method. Looking at this patch it seems that what this subroutine did can be done easily in the pl script in few lines. Test plan: 1/ Create 2 organisations with the same "surname": you should get a warning. 2/ Create 2 patrons (non-organisation) with the same surname/firstname/date of birth, you should get a warning Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 53445 [details] [review] Bug 16909: Koha::Patrons - Remove checkuniquemember C4::Members::checkuniquemember was not really nicely written, was only used once and was not covered by tests. I think it does not make sense to keep such complexity and have this code in the subroutine/method. Looking at this patch it seems that what this subroutine did can be done easily in the pl script in few lines. Test plan: 1/ Create 2 organisations with the same "surname": you should get a warning. 2/ Create 2 patrons (non-organisation) with the same surname/firstname/date of birth, you should get a warning Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pushed to master for 16.11, thanks Jonathan!
Wrote to bug 16889 by mistake. Twice. This patch broke add new member. When pressing save, we cycle back to the page. Not sure what to do with the status and importance, but put in "blocker" since, well, it is kind of a big problem.
(In reply to Blou from comment #8) > Wrote to bug 16889 by mistake. Twice. > > This patch broke add new member. When pressing save, we cycle back to the > page. > > Not sure what to do with the status and importance, but put in "blocker" > since, well, it is kind of a big problem. See Bug 16941 - Can not add new patron in staff client for the solution.