Bug 21465 - Cannot overlay patrons when matching by cardnumber if userid exists in file and in Koha
Summary: Cannot overlay patrons when matching by cardnumber if userid exists in file a...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
: 21951 (view as bug list)
Depends on: 12598
Blocks: 23734
  Show dependency treegraph
 
Reported: 2018-10-01 18:59 UTC by Kelly McElligott
Modified: 2020-11-30 21:45 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21465: Don't throw duplicate userid error if userid belongs to the matched patron (11.10 KB, patch)
2018-10-02 20:07 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21465: Don't throw duplicate userid error if userid belongs to the matched patron (10.08 KB, patch)
2018-10-03 17:13 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21465: Don't throw duplicate userid error if userid belongs to the matched patron (10.13 KB, patch)
2018-12-05 16:26 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 21465: Don't throw duplicate userid error if userid belongs to the matched patron (10.22 KB, patch)
2018-12-10 21:02 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kelly McElligott 2018-10-01 18:59:10 UTC
In the Koha version of 18.05 using the patron import tool, if a patron has the same userid and cardnumber, and a library attempts of overlaying an updated file on patron information into Koha, an error will occur.   

Test Plan
1. Create patron csv according to the download starter CSV in Koha.
2. Patrons were assigned a card number by the library and this card number was also used for their User ID. 
3. Import the Patron CSV into Koha.
4. Adjust this same Patron CSV file adding/updating an address, adding/updating a field of data.
5. Use this new file to import into Koha
6 Choose to match on the cardnumber
7. Choose to overwrite the existing one with this file. 
8. Koha will produce an error saying that the userid already in use by another patron. 

Different Scenario
Do Steps 1-5 the same
6. Choose to match on the User Id
7. Choose to overwrite the existing one with this file.
8.  Koha will do the overwrite correctly.

Libraries do tend to give patrons a cardnumber and also use that as the patron's userid. It would appear that matching on the userid works, however, this should not work this way, it should be able to match on either and produce same results- a successful overlay.
Comment 1 Nick Clemens 2018-10-02 19:47:42 UTC
This appears to have been intentional according to tests, but I think the  tests and assumption were wrong.

Essentially if not matching by userid and a userid is inthe file we try to create a new user and see if the user id is valid. It isn't, of course, because the patron exists. has_valid_userid is supposed to ignore a duplicate for the existing borrower, but we avoid that check by creating a new borrower!?!?

Patch coming
Comment 2 Nick Clemens 2018-10-02 20:07:15 UTC Comment hidden (obsolete)
Comment 3 Nick Clemens 2018-10-03 17:13:16 UTC Comment hidden (obsolete)
Comment 4 Owen Leonard 2018-12-05 16:26:00 UTC
Created attachment 82877 [details] [review]
Bug 21465: Don't throw duplicate userid error if userid belongs to the matched patron

To test:
 1 - Export your patrons
    a - Create a report 'SELECT * FROM borrowers'
    b - Run and save the report as csv (check your delimiter)
    c - Delete the borrowernumebr column
 2 - Use the Patron Import tool to import the csv from above
 3 - Set matching to 'cardnumber'
 4 - Set 'If matching record is already in the borrowers table:' to
Overwrite
 5 - Import
 6 - None are import because of matchign userid (their own)
 7 - Apply patch
 8 - Repeat
 9 - Patrons are successfully overwritten
10 - prove -v t/db_dependent/Koha/Patrons/Import.t
11 - prove -v t/db_dependent/Koha/Patrons.t

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 5 Owen Leonard 2018-12-05 16:26:48 UTC
The tests aren't successful but as far as I can tell the errors are the same as without the patch, so not relevant.
Comment 6 Katrin Fischer 2018-12-06 08:49:05 UTC
Is this is a duplicate of bug 21951?
Comment 7 Nick Clemens 2018-12-07 19:12:40 UTC
*** Bug 21951 has been marked as a duplicate of this bug. ***
Comment 8 Nick Clemens 2018-12-07 20:44:04 UTC
(In reply to Katrin Fischer from comment #6)
> Is this is a duplicate of bug 21951?

Other way round, only because there are tests here

All tests pass for me
Comment 9 Jonathan Druart 2018-12-10 20:48:58 UTC
Nick, did you investigate the new warnings your tests add?

Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$handle_3a> line 2.
Comment 10 Jonathan Druart 2018-12-10 21:02:10 UTC
Comes from eval { $patron->set(\%borrower)->store }; in import_patrons, it complains on invalid patron attribute's values (login_attempts, lost, etc.)

Related to bug 21761.
Comment 11 Jonathan Druart 2018-12-10 21:02:50 UTC
Created attachment 83033 [details] [review]
Bug 21465: Don't throw duplicate userid error if userid belongs to the matched patron

To test:
 1 - Export your patrons
    a - Create a report 'SELECT * FROM borrowers'
    b - Run and save the report as csv (check your delimiter)
    c - Delete the borrowernumebr column
 2 - Use the Patron Import tool to import the csv from above
 3 - Set matching to 'cardnumber'
 4 - Set 'If matching record is already in the borrowers table:' to
Overwrite
 5 - Import
 6 - None are import because of matchign userid (their own)
 7 - Apply patch
 8 - Repeat
 9 - Patrons are successfully overwritten
10 - prove -v t/db_dependent/Koha/Patrons/Import.t
11 - prove -v t/db_dependent/Koha/Patrons.t

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 12 Nick Clemens 2018-12-11 20:55:51 UTC
Awesome work all!

Pushed to master for 19.05
Comment 13 Martin Renvoize 2018-12-12 11:00:42 UTC
Pushed to 18.11.x for 18.11.01
Comment 14 Fridolin Somers 2019-01-02 09:35:40 UTC Comment hidden (obsolete)
Comment 15 Fridolin Somers 2019-01-02 09:58:58 UTC
Depends on Bug 12598 not in 17.11.x
Comment 16 Jesse Maseto 2019-01-08 17:36:27 UTC
This was backported to 18.05.x for 18.05.07
Comment 17 Nick Clemens 2019-01-16 20:59:48 UTC
(In reply to Jesse Maseto from comment #16)
> This was backported to 18.05.x for 18.05.07

I think it is in 18.05.08?
Comment 18 Lucas Gass 2019-01-18 21:06:44 UTC
(In reply to Nick Clemens from comment #17)
> (In reply to Jesse Maseto from comment #16)
> > This was backported to 18.05.x for 18.05.07
> 
> I think it is in 18.05.08?

Yes, 18.05.08 NOT 18.05.07
Comment 19 Jonathan Druart 2019-11-19 10:54:47 UTC
(In reply to Jonathan Druart from comment #9)
> Nick, did you investigate the new warnings your tests add?
> 
> Argument "" isn't numeric in numeric eq (==) at
> /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$handle_3a> line 2.

(In reply to Jonathan Druart from comment #10)
> Comes from eval { $patron->set(\%borrower)->store }; in import_patrons, it
> complains on invalid patron attribute's values (login_attempts, lost, etc.)
> 
> Related to bug 21761.

See bug 23734