Description
Nick Clemens (kidclamp)
2016-05-02 22:48:35 UTC
Agreed, no userid in the import file should be ignored. Sorry to have missed this in testing! Created attachment 51123 [details] [review] Bug 16426: Add tests for ModMember - do not update userid Created attachment 51124 [details] [review] 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") 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. 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 Created attachment 51147 [details] [review] Bug 16426: Add tests for ModMember - do not update userid Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 51148 [details] [review] 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> (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 Created attachment 51274 [details] [review] k 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> 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> Pushed To Master - Should be in the May 2016 Release. Thanks! Patch pushed to 3.22.x, will be in 3.22.7 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. 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. 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. (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. Hm it worked for us - are you still seeing the issue or did you update Koha since? Pushed to 3.20.x will be in 3.20.11 |