Bug 16426 - Import borrowers tool warns for blank and/or existing userids
Summary: Import borrowers tool warns for blank and/or existing userids
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P3 major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 15840
Blocks:
  Show dependency treegraph
 
Reported: 2016-05-02 22:48 UTC by Nick Clemens
Modified: 2017-12-07 22:16 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 16426: Add tests for ModMember - do not update userid (1.81 KB, patch)
2016-05-03 08:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16426: follow-up of bug 15840 - correctly manage userid while inserting patrons (3.92 KB, patch)
2016-05-03 08:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16426: Add tests for ModMember - do not update userid (1.87 KB, patch)
2016-05-03 14:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16426: follow-up of bug 15840 - correctly manage userid while inserting patrons (3.98 KB, patch)
2016-05-03 14:59 UTC, Nick Clemens
Details | Diff | Splinter Review
k (4.05 KB, patch)
2016-05-05 23:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 16426: Add tests for ModMember - do not update userid (1.94 KB, patch)
2016-05-05 23:18 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 16426: follow-up of bug 15840 - correctly manage userid while inserting patrons (4.05 KB, patch)
2016-05-05 23:19 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2016-05-02 22:48:35 UTC
Bug 15840 added warns for existing userids, however these warning are now triggering when borrowers are imported with:

1 - no userid
2 - existing userids even if they belong to the matching user

To recreate:
Find a borrower with a userid in your system
Create an import file with that patron and a new patron with no user id
Try to import and match on userid
Errors are generated and no patrons are updated or imported

Added comments From IRC:
<wizzyrea> if there is no userid, we should ignore it.
<wizzyrea> or optionally create one
Comment 1 Katrin Fischer 2016-05-03 06:35:26 UTC
Agreed, no userid in the import file should be ignored. Sorry to have missed this in testing!
Comment 2 Jonathan Druart 2016-05-03 08:06:47 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2016-05-03 08:06:52 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2016-05-03 08:08:38 UTC
Important note: this patch assumes that borrowers.userid should not be undef, 0, or ''. If everybody agrees with that, we will have to add an updatedb entry to check if there are no rows in the DB matching that, otherwise an update of a patron without userid will fail.
Comment 5 Nick Clemens 2016-05-03 12:49:43 UTC
This seems to be working well, two questions:

1 - If I am matching on userid, and import a patron with no userid in the file but the cardnumber is already used the warning says "Cardnumber ###### is not a valid cardnumber"  
It would be clearer if it said 'this cardnumber is already used'

2 - If I am mathching on cardnumber and include an existing username in the file I am not warned, but the username is replaced with a generated one (surname.firstname) - I think there should be a warning since the udername doesn't match the submitted data, or that line should fail
Comment 6 Nick Clemens 2016-05-03 14:59:33 UTC Comment hidden (obsolete)
Comment 7 Nick Clemens 2016-05-03 14:59:39 UTC Comment hidden (obsolete)
Comment 8 Nick Clemens 2016-05-03 15:02:56 UTC
(In reply to Nick Clemens from comment #5)
> This seems to be working well, two questions:
moving these to separate bug reports as they were present before 15840

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16433

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16434
Comment 9 Katrin Fischer 2016-05-05 23:17:38 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2016-05-05 23:18:57 UTC
Created attachment 51275 [details] [review]
[PASSED QA] Bug 16426: Add tests for ModMember - do not update userid

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Katrin Fischer 2016-05-05 23:19:43 UTC
Created attachment 51276 [details] [review]
[PASSED QA] Bug 16426: follow-up of bug 15840 - correctly manage userid while inserting patrons

Bug 15840 tried to fix a bug but makes things more complicated than it
was before.
If an userid is not provided for 1 or more rows of the csv file, it
should not be updated. However, if a userid is provided and it already
used by an other patron, the import should fail for this row (but not
crash!).

Test plan:
0/ Create a patron with a userid=your_userid
1/ Use the import patron tool to update this userid
=> userid should have been updated
2/ Update another data and do not provide the userid
=> data should have been updated and not the userid
3/ Update another data and provide the userid, but set it to an empty
string, or '0'
=> data should have been updated and not the userid
4/ Update another patron, and set userid=your_userid
=> Update should fail and an error whouls be displayed ("already used by
another patron")

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Brendan Gallagher 2016-05-06 03:42:37 UTC
Pushed To Master - Should be in the May 2016 Release.  Thanks!
Comment 13 Julian Maurice 2016-05-09 08:36:10 UTC
Patch pushed to 3.22.x, will be in 3.22.7
Comment 14 Florent Mara 2016-05-10 02:06:35 UTC
While rebasing bug 12598, I backported this patch and it broke my tests.

(unless I missed something) With this patch, either overwrite_cardnumber is set to 0 and the duplicate borrower number will be caught as an 'already_in_db' feedback - before reaching the new code and get to the next input line. 

OR overwrite_cardnumber is set to 1 and duplicates are overwritten anyway. 

In other words, there does not appear to be any duplicate userid check anymore with the current code.

In bug 12598, I relocated the new code block above 'if ($borrowernumber) {' to restore functionality.
Comment 15 Craig Dean 2016-05-12 00:18:17 UTC
I have a customer affected by this bug and thus can't import any new users. Looking at the previous releases it seems that 3.22.7 won't be out until closer to the end of the month. This is not really an acceptable wait to be able to provision new users.

1. Is there any chance that the 3.22.7 will be released sooner?
2. Is there anything I can do to import this patch now, without breaking future updates. Given then I am a Systems Administrator and not a developer.
Comment 16 Katrin Fischer 2016-05-12 05:11:25 UTC
If you are importing new borrowers it will will help to add a userid column with some unique key to your import file - for example the cardnumber or something similar. That will let you import the data.
Comment 17 Craig Dean 2016-05-24 21:19:00 UTC
(In reply to Katrin Fischer from comment #16)
> If you are importing new borrowers it will will help to add a userid column
> with some unique key to your import file - for example the cardnumber or
> something similar. That will let you import the data.

Unfortunately that did not work. It does not matter what I put in the userid column it will not import the users.
Comment 18 Katrin Fischer 2016-06-08 21:54:07 UTC
Hm it worked for us - are you still seeing the issue or did you update Koha since?
Comment 19 Chris Cormack 2016-06-19 20:55:58 UTC
Pushed to 3.20.x will be in 3.20.11