Bug 26059 - Create guarantor/guarantee links on patron import
Summary: Create guarantor/guarantee links on patron import
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on: 14570
Blocks: 27755
  Show dependency treegraph
 
Reported: 2020-07-24 14:54 UTC by Andrew Fuerste-Henry
Modified: 2021-02-23 15:58 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.03,20.05.09


Attachments
Bug 26059: Create guarantor/guarantee links on patron import (5.06 KB, patch)
2020-07-24 16:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26059: Create guarantor/guarantee links on patron import (5.12 KB, patch)
2020-07-24 16:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26059: Create guarantor/guarantee links on patron import (5.16 KB, patch)
2020-07-29 14:46 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 26059: Create guarantor/guarantee links on patron import (5.23 KB, patch)
2020-08-03 12:47 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26059: Create guarantor/guarantee links on patron import (5.27 KB, patch)
2020-08-15 10:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26059: Add unit tests (4.66 KB, patch)
2020-08-19 14:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26059: Add unit tests (4.64 KB, patch)
2020-08-19 14:32 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 Andrew Fuerste-Henry 2020-07-24 14:54:13 UTC
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.
Comment 1 Kyle M Hall 2020-07-24 16:18:21 UTC
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.
Comment 2 Kyle M Hall 2020-07-24 16:25:51 UTC
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
Comment 3 Kyle M Hall 2020-07-24 16:57:42 UTC
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
Comment 4 Amit Gupta 2020-07-29 14:46:58 UTC
Created attachment 107505 [details] [review]
Bug 26059: Create guarantor/guarantee links on patron import

Work perfectly.
Comment 5 Andrew Fuerste-Henry 2020-08-03 12:47:54 UTC
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>
Comment 6 Katrin Fischer 2020-08-15 10:57:45 UTC
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 7 Jonathan Druart 2020-08-18 13:02:00 UTC
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.
Comment 8 Kyle M Hall 2020-08-18 13:05:57 UTC
$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.
Comment 9 Jonathan Druart 2020-08-18 15:55:07 UTC
ok, I did not notice we were in the "unless $borrowernumber" block.

Can you provide tests for this change please?
Comment 10 Kyle M Hall 2020-08-19 14:31:24 UTC
Created attachment 108634 [details] [review]
Bug 26059: Add unit tests
Comment 11 Kyle M Hall 2020-08-19 14:32:39 UTC
Created attachment 108635 [details] [review]
Bug 26059: Add unit tests
Comment 12 Martha Fuerst 2021-02-09 13:57:23 UTC
Making squeaky noises - we're still interested in this.

Thanks!

-Marti Fuerst
Comment 13 Jonathan Druart 2021-02-12 11:18:46 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 14 Fridolin Somers 2021-02-19 14:46:57 UTC
Pushed to 20.11.x for 20.11.03
Comment 15 Andrew Fuerste-Henry 2021-02-22 22:38:04 UTC
Pushed to 20.05.x for 20.05.09
Comment 16 Victor Grousset/tuxayo 2021-02-23 15:58:31 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.