The change to allow multiple guarantors was meant to maintain the ability to import patrons with a Koha patron guarantor, but is not working as intended.
I found a couple problems: A) Adding the guarantor simply doesn't work B) We have two columns named 'relationship', one for the borrowers table, and one for the guarantor relationships table. This clearly doesn't work and will cause confusion. The one for the guarantor relationships table should be renamed.
Created attachment 107360 [details] [review] Bug 26059: Create guarantor/guarantee links on patron import The change to allow multiple guarantors was meant to maintain the ability to import patrons with a Koha patron guarantor, but is not working as intended. A) Adding the guarantor simply doesn't work B) We have two columns named 'relationship', one for the borrowers table, and one for the guarantor relationships table. This clearly doesn't work and will cause confusion. The one for the guarantor relationships table should be renamed. C) guarantor_firstname and guarantor_surname in the CSV file do nothing and should be removed. This patch also fixes a minor issue that causes warnings like: CGI::param called in list context from /kohadevbox/koha/tools/import_borrowers.pl line 124 Test Plan: 1) Create a CSV with contents like: cardnumber,surname,firstname,branchcode,categorycode,guarantor_relationship,guarantor_id bloop,gloop,froop,MPL,J,father,48 brim,flim,zim,MPL,J,father,48 2) Attempt to upload this file, ensure you have a borrowernumber 48 that can have guarantors 3) Note the accounts are not linked 4) Apply this patch 5) Restart all the things! 6) Upload the file again 7) The patrons should now be linked! 8) Download the starter CSV file 9) Note the second relationship column is now guarantor_relationship 10) Note the columns guarantor_firstname and guarantor_surname are no longer present
Created attachment 107364 [details] [review] Bug 26059: Create guarantor/guarantee links on patron import The change to allow multiple guarantors was meant to maintain the ability to import patrons with a Koha patron guarantor, but is not working as intended. A) Adding the guarantor simply doesn't work B) We have two columns named 'relationship', one for the borrowers table, and one for the guarantor relationships table. This clearly doesn't work and will cause confusion. The one for the guarantor relationships table should be renamed. C) guarantor_firstname and guarantor_surname in the CSV file do nothing and should be removed. This patch also fixes a minor issue that causes warnings like: CGI::param called in list context from /kohadevbox/koha/tools/import_borrowers.pl line 124 Test Plan: 1) Create a CSV with contents like: cardnumber,surname,firstname,branchcode,categorycode,guarantor_relationship,guarantor_id bloop,gloop,froop,MPL,J,father,48 brim,flim,zim,MPL,J,father,48 2) Attempt to upload this file, ensure you have a borrowernumber 48 that can have guarantors 3) Note the accounts are not linked 4) Apply this patch 5) Restart all the things! 6) Upload the file again 7) The patrons should now be linked! 8) Download the starter CSV file 9) Note the second relationship column is now guarantor_relationship 10) Note the columns guarantor_firstname and guarantor_surname are no longer present
Created attachment 107505 [details] [review] Bug 26059: Create guarantor/guarantee links on patron import Work perfectly.
Created attachment 107703 [details] [review] Bug 26059: Create guarantor/guarantee links on patron import The change to allow multiple guarantors was meant to maintain the ability to import patrons with a Koha patron guarantor, but is not working as intended. A) Adding the guarantor simply doesn't work B) We have two columns named 'relationship', one for the borrowers table, and one for the guarantor relationships table. This clearly doesn't work and will cause confusion. The one for the guarantor relationships table should be renamed. C) guarantor_firstname and guarantor_surname in the CSV file do nothing and should be removed. This patch also fixes a minor issue that causes warnings like: CGI::param called in list context from /kohadevbox/koha/tools/import_borrowers.pl line 124 Test Plan: 1) Create a CSV with contents like: cardnumber,surname,firstname,branchcode,categorycode,guarantor_relationship,guarantor_id bloop,gloop,froop,MPL,J,father,48 brim,flim,zim,MPL,J,father,48 2) Attempt to upload this file, ensure you have a borrowernumber 48 that can have guarantors 3) Note the accounts are not linked 4) Apply this patch 5) Restart all the things! 6) Upload the file again 7) The patrons should now be linked! 8) Download the starter CSV file 9) Note the second relationship column is now guarantor_relationship 10) Note the columns guarantor_firstname and guarantor_surname are no longer present Signed-off-by: Amit Gupta <amit.gupta@informaticsglobal.com> Signed-off-by: Marti Fuerst <mfuerst@hmcpl.org>
Created attachment 108319 [details] [review] Bug 26059: Create guarantor/guarantee links on patron import The change to allow multiple guarantors was meant to maintain the ability to import patrons with a Koha patron guarantor, but is not working as intended. A) Adding the guarantor simply doesn't work B) We have two columns named 'relationship', one for the borrowers table, and one for the guarantor relationships table. This clearly doesn't work and will cause confusion. The one for the guarantor relationships table should be renamed. C) guarantor_firstname and guarantor_surname in the CSV file do nothing and should be removed. This patch also fixes a minor issue that causes warnings like: CGI::param called in list context from /kohadevbox/koha/tools/import_borrowers.pl line 124 Test Plan: 1) Create a CSV with contents like: cardnumber,surname,firstname,branchcode,categorycode,guarantor_relationship,guarantor_id bloop,gloop,froop,MPL,J,father,48 brim,flim,zim,MPL,J,father,48 2) Attempt to upload this file, ensure you have a borrowernumber 48 that can have guarantors 3) Note the accounts are not linked 4) Apply this patch 5) Restart all the things! 6) Upload the file again 7) The patrons should now be linked! 8) Download the starter CSV file 9) Note the second relationship column is now guarantor_relationship 10) Note the columns guarantor_firstname and guarantor_surname are no longer present Signed-off-by: Amit Gupta <amit.gupta@informaticsglobal.com> Signed-off-by: Marti Fuerst <mfuerst@hmcpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment on attachment 108319 [details] [review] Bug 26059: Create guarantor/guarantee links on patron import Review of attachment 108319 [details] [review]: ----------------------------------------------------------------- ::: Koha/Patrons/Import.pm @@ +354,4 @@ > Koha::Patron->new(\%borrower)->store; > }; > unless ( $@ ) { > + $borrowernumber = $patron->id; Is this line really needed? If so please explain.
$patron is scoped to a different sibling block and doesn't survive to reach the code where the relationship is set. $borrowernumber is scoped to the parent block. Since we only need the borrowernumber, it was simpler to set $borrowernumber than to re-engineer $patron to be in the parent block. (In reply to Jonathan Druart from comment #7) > Comment on attachment 108319 [details] [review] [review] > Bug 26059: Create guarantor/guarantee links on patron import > > Review of attachment 108319 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: Koha/Patrons/Import.pm > @@ +354,4 @@ > > Koha::Patron->new(\%borrower)->store; > > }; > > unless ( $@ ) { > > + $borrowernumber = $patron->id; > > Is this line really needed? If so please explain.
ok, I did not notice we were in the "unless $borrowernumber" block. Can you provide tests for this change please?
Created attachment 108634 [details] [review] Bug 26059: Add unit tests
Created attachment 108635 [details] [review] Bug 26059: Add unit tests
Making squeaky noises - we're still interested in this. Thanks! -Marti Fuerst
Pushed to master for 21.05, thanks to everybody involved!
Pushed to 20.11.x for 20.11.03
Pushed to 20.05.x for 20.05.09
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.