Description
Jonathan Druart
2018-01-09 14:33:29 UTC
Created attachment 70365 [details] [review] Bug 19936: Replace Check_userid - just move the tests This patch moves the tests related to Check_userid out of Members.t The tests must pass with only this patch applied Created attachment 70366 [details] [review] Bug 19936: Add the Koha::Patron->has_valid_userid method Reuse how C4::Members::Check_Userid works and adapt it to write Koha::Patron->check_userid Adapt the tests to use this new method. The tests still pass, we can adapt the different callers Created attachment 70367 [details] [review] Bug 19936: Replace Check_userid - Update the occurrences We previously prove that the method and the subroutine were equivalent, we know update the controller calls. Test plan: - Add and update a patron with different variations of userid (automatically generated or not) - Import patrons with and without userid, as well as with existing userid Created attachment 70368 [details] [review] Bug 19936: Remove the subroutine Check_Userid It is no longer in user, we can remove it. Created attachment 70369 [details] [review] Bug 19936: Move and add tests for Generate_Userid Created attachment 70370 [details] [review] Bug 19936: Add the Koha::Patron->generate_userid method Created attachment 70371 [details] [review] Bug 19936: Adapt tests And prove that we are not cheating Created attachment 70372 [details] [review] Bug 19936: Replace Generate_Userid - Update the occurrences Test plan: Same test plan as previous patch: add/update/import patrons and watch the userid Created attachment 70373 [details] [review] Bug 19936: Remove the subroutine Generate_Userid Created attachment 70374 [details] [review] Bug 19936: Remove warnings from tests C4::Members did not "use warnings;" Created attachment 71787 [details] [review] Bug 19936: Replace Check_userid - just move the tests This patch moves the tests related to Check_userid out of Members.t The tests must pass with only this patch applied Created attachment 71788 [details] [review] Bug 19936: Add the Koha::Patron->has_valid_userid method Reuse how C4::Members::Check_Userid works and adapt it to write Koha::Patron->check_userid Adapt the tests to use this new method. The tests still pass, we can adapt the different callers Created attachment 71789 [details] [review] Bug 19936: Replace Check_userid - Update the occurrences We previously prove that the method and the subroutine were equivalent, we know update the controller calls. Test plan: - Add and update a patron with different variations of userid (automatically generated or not) - Import patrons with and without userid, as well as with existing userid Created attachment 71790 [details] [review] Bug 19936: Remove the subroutine Check_Userid It is no longer in user, we can remove it. Created attachment 71791 [details] [review] Bug 19936: Move and add tests for Generate_Userid Created attachment 71792 [details] [review] Bug 19936: Add the Koha::Patron->generate_userid method Created attachment 71793 [details] [review] Bug 19936: Adapt tests And prove that we are not cheating Created attachment 71794 [details] [review] Bug 19936: Replace Generate_Userid - Update the occurrences Test plan: Same test plan as previous patch: add/update/import patrons and watch the userid Created attachment 71795 [details] [review] Bug 19936: Remove the subroutine Generate_Userid Created attachment 71796 [details] [review] Bug 19936: Remove warnings from tests C4::Members did not "use warnings;" Created attachment 72129 [details] [review] Bug 19936: Replace Check_userid - just move the tests This patch moves the tests related to Check_userid out of Members.t The tests must pass with only this patch applied Created attachment 72130 [details] [review] Bug 19936: Add the Koha::Patron->has_valid_userid method Reuse how C4::Members::Check_Userid works and adapt it to write Koha::Patron->check_userid Adapt the tests to use this new method. The tests still pass, we can adapt the different callers Created attachment 72131 [details] [review] Bug 19936: Replace Check_userid - Update the occurrences We previously prove that the method and the subroutine were equivalent, we know update the controller calls. Test plan: - Add and update a patron with different variations of userid (automatically generated or not) - Import patrons with and without userid, as well as with existing userid Created attachment 72132 [details] [review] Bug 19936: Remove the subroutine Check_Userid It is no longer in user, we can remove it. Created attachment 72133 [details] [review] Bug 19936: Move and add tests for Generate_Userid Created attachment 72134 [details] [review] Bug 19936: Add the Koha::Patron->generate_userid method Created attachment 72135 [details] [review] Bug 19936: Adapt tests And prove that we are not cheating Created attachment 72136 [details] [review] Bug 19936: Replace Generate_Userid - Update the occurrences Test plan: Same test plan as previous patch: add/update/import patrons and watch the userid Created attachment 72137 [details] [review] Bug 19936: Remove the subroutine Generate_Userid Created attachment 72138 [details] [review] Bug 19936: Remove warnings from tests C4::Members did not "use warnings;" Created attachment 72139 [details] [review] Bug 19936: Reuse existing userid if none provided Found this regression when working on other patches: If you edit a patron and blank the userid field, it will be regenerated with an incremented value (firstname.surname will be firstname.surname1) This is because we use a non-existing patron and ->in_storage in has_valid_userid is always false. The trick here is to backup the value, generate the userid, then reset userid to the previous value. As the POD says, it will be fix later, when AddMember and ModMember will be replaced with Koha::Patron->store Created attachment 72140 [details] [review] Bug 19936: Fix number of tests will be squashed later, but lazy to reupload the whole patchset Created attachment 72141 [details] [review] Bug 19936: Replace Check_userid - just move the tests This patch moves the tests related to Check_userid out of Members.t The tests must pass with only this patch applied Created attachment 72142 [details] [review] Bug 19936: Add the Koha::Patron->has_valid_userid method Reuse how C4::Members::Check_Userid works and adapt it to write Koha::Patron->check_userid Adapt the tests to use this new method. The tests still pass, we can adapt the different callers Created attachment 72143 [details] [review] Bug 19936: Replace Check_userid - Update the occurrences We previously prove that the method and the subroutine were equivalent, we know update the controller calls. Test plan: - Add and update a patron with different variations of userid (automatically generated or not) - Import patrons with and without userid, as well as with existing userid Created attachment 72144 [details] [review] Bug 19936: Remove the subroutine Check_Userid It is no longer in user, we can remove it. Created attachment 72145 [details] [review] Bug 19936: Move and add tests for Generate_Userid Created attachment 72146 [details] [review] Bug 19936: Add the Koha::Patron->generate_userid method Created attachment 72147 [details] [review] Bug 19936: Adapt tests And prove that we are not cheating Created attachment 72148 [details] [review] Bug 19936: Replace Generate_Userid - Update the occurrences Test plan: Same test plan as previous patch: add/update/import patrons and watch the userid Created attachment 72149 [details] [review] Bug 19936: Remove the subroutine Generate_Userid Created attachment 72150 [details] [review] Bug 19936: Remove warnings from tests C4::Members did not "use warnings;" Created attachment 72151 [details] [review] Bug 19936: (follow-up) Replace Check_userid - Update the occurrences Created attachment 72152 [details] [review] Bug 19936: Reuse existing userid if none provided Found this regression when working on other patches: If you edit a patron and blank the userid field, it will be regenerated with an incremented value (firstname.surname will be firstname.surname1) This is because we use a non-existing patron and ->in_storage in has_valid_userid is always false. The trick here is to backup the value, generate the userid, then reset userid to the previous value. As the POD says, it will be fix later, when AddMember and ModMember will be replaced with Koha::Patron->store Rebased on top of bug 12001 + 2 bugs fixed. Sorry, could you rebase please? Created attachment 73630 [details] [review] Bug 19936: Replace Check_userid - just move the tests This patch moves the tests related to Check_userid out of Members.t The tests must pass with only this patch applied Created attachment 73631 [details] [review] Bug 19936: Add the Koha::Patron->has_valid_userid method Reuse how C4::Members::Check_Userid works and adapt it to write Koha::Patron->check_userid Adapt the tests to use this new method. The tests still pass, we can adapt the different callers Created attachment 73632 [details] [review] Bug 19936: Replace Check_userid - Update the occurrences We previously prove that the method and the subroutine were equivalent, we know update the controller calls. Test plan: - Add and update a patron with different variations of userid (automatically generated or not) - Import patrons with and without userid, as well as with existing userid Created attachment 73633 [details] [review] Bug 19936: Remove the subroutine Check_Userid It is no longer in user, we can remove it. Created attachment 73634 [details] [review] Bug 19936: Move and add tests for Generate_Userid Created attachment 73635 [details] [review] Bug 19936: Add the Koha::Patron->generate_userid method Created attachment 73636 [details] [review] Bug 19936: Adapt tests And prove that we are not cheating Created attachment 73637 [details] [review] Bug 19936: Replace Generate_Userid - Update the occurrences Test plan: Same test plan as previous patch: add/update/import patrons and watch the userid Created attachment 73638 [details] [review] Bug 19936: Remove the subroutine Generate_Userid Created attachment 73639 [details] [review] Bug 19936: Remove warnings from tests C4::Members did not "use warnings;" Created attachment 73640 [details] [review] Bug 19936: Replace Check_userid - Update the occurrences Created attachment 73641 [details] [review] Bug 19936: Reuse existing userid if none provided Found this regression when working on other patches: If you edit a patron and blank the userid field, it will be regenerated with an incremented value (firstname.surname will be firstname.surname1) This is because we use a non-existing patron and ->in_storage in has_valid_userid is always false. The trick here is to backup the value, generate the userid, then reset userid to the previous value. As the POD says, it will be fix later, when AddMember and ModMember will be replaced with Koha::Patron->store Created attachment 73963 [details] [review] Bug 19936: Replace Check_userid - just move the tests This patch moves the tests related to Check_userid out of Members.t The tests must pass with only this patch applied Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 73964 [details] [review] Bug 19936: Add the Koha::Patron->has_valid_userid method Reuse how C4::Members::Check_Userid works and adapt it to write Koha::Patron->check_userid Adapt the tests to use this new method. The tests still pass, we can adapt the different callers Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 73965 [details] [review] Bug 19936: Replace Check_userid - Update the occurrences We previously prove that the method and the subroutine were equivalent, we know update the controller calls. Test plan: - Add and update a patron with different variations of userid (automatically generated or not) - Import patrons with and without userid, as well as with existing userid Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 73966 [details] [review] Bug 19936: Remove the subroutine Check_Userid It is no longer in user, we can remove it. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 73967 [details] [review] Bug 19936: Move and add tests for Generate_Userid Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 73968 [details] [review] Bug 19936: Add the Koha::Patron->generate_userid method Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 73969 [details] [review] Bug 19936: Adapt tests And prove that we are not cheating Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 73970 [details] [review] Bug 19936: Replace Generate_Userid - Update the occurrences Test plan: Same test plan as previous patch: add/update/import patrons and watch the userid Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 73971 [details] [review] Bug 19936: Remove the subroutine Generate_Userid Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 73972 [details] [review] Bug 19936: Remove warnings from tests C4::Members did not "use warnings;" Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 73973 [details] [review] Bug 19936: Replace Check_userid - Update the occurrences Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 73974 [details] [review] Bug 19936: Reuse existing userid if none provided Found this regression when working on other patches: If you edit a patron and blank the userid field, it will be regenerated with an incremented value (firstname.surname will be firstname.surname1) This is because we use a non-existing patron and ->in_storage in has_valid_userid is always false. The trick here is to backup the value, generate the userid, then reset userid to the previous value. As the POD says, it will be fix later, when AddMember and ModMember will be replaced with Koha::Patron->store Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Giving this a try. Created attachment 74071 [details] [review] Bug 19936: Replace Check_userid - just move the tests This patch moves the tests related to Check_userid out of Members.t The tests must pass with only this patch applied Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 74072 [details] [review] Bug 19936: Add the Koha::Patron->has_valid_userid method Reuse how C4::Members::Check_Userid works and adapt it to write Koha::Patron->check_userid Adapt the tests to use this new method. The tests still pass, we can adapt the different callers Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 74073 [details] [review] Bug 19936: Replace Check_userid - Update the occurrences We previously prove that the method and the subroutine were equivalent, we know update the controller calls. Test plan: - Add and update a patron with different variations of userid (automatically generated or not) - Import patrons with and without userid, as well as with existing userid Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 74074 [details] [review] Bug 19936: Remove the subroutine Check_Userid It is no longer in user, we can remove it. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 74075 [details] [review] Bug 19936: Move and add tests for Generate_Userid Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 74076 [details] [review] Bug 19936: Add the Koha::Patron->generate_userid method Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 74077 [details] [review] Bug 19936: Adapt tests And prove that we are not cheating Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 74078 [details] [review] Bug 19936: Replace Generate_Userid - Update the occurrences Test plan: Same test plan as previous patch: add/update/import patrons and watch the userid Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 74079 [details] [review] Bug 19936: Remove the subroutine Generate_Userid Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 74080 [details] [review] Bug 19936: Remove warnings from tests C4::Members did not "use warnings;" Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 74081 [details] [review] Bug 19936: Replace Check_userid - Update the occurrences Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 74082 [details] [review] Bug 19936: Reuse existing userid if none provided Found this regression when working on other patches: If you edit a patron and blank the userid field, it will be regenerated with an incremented value (firstname.surname will be firstname.surname1) This is because we use a non-existing patron and ->in_storage in has_valid_userid is always false. The trick here is to backup the value, generate the userid, then reset userid to the previous value. As the POD says, it will be fix later, when AddMember and ModMember will be replaced with Koha::Patron->store Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Preserves current behaviour - including remaining bugs :D (bug 13655) Pushed to master for 18.05, thanks to everybody involved! Enhancement, not backported for 17.11.x Since this patch trying to import patrons seems to be noisy, lots of Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1017, <$handle> line 512. example line that has this problem, with header, private stuff removed: cardnumber,firstname,surname,address,address2,city,zipcode,email,phone,B_phone,branchcode,patron_attributes,borrowernotes,categorycode 100000090,"borrower surname",firstname,"Po Box xxxxx"," City",,,firstname.surname@institution.co.nz,,123456,BRANCHCODE,GROUP:STAFF,,STAFF It doesn't seem to affect the performance, patrons still get imported, but it sure does fill up the logs. Hi Liz, I have tried with your line (with sample data, replaced "STAFF" with "S" and "BRANCHCODE" with "CPL", I have also created the patron's attribute "GROUP"). I see these 2 lines: Use of uninitialized value $uploadborrowers in concatenation (.) or string at /home/vagrant/kohaclone/tools/import_borrowers.pl line 109. CGI::param called in list context from package CGI::Compile::ROOT::home_vagrant_kohaclone_tools_import_borrowers_2epl line 123, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436. But not the one you are referring to. Let me know if you have more detail to provide and if you recreate on a devbox, or with the sample data. (In reply to Liz Rea from comment #87) > Since this patch trying to import patrons seems to be noisy, lots of You did not tell if you were using the UI of the command line script (I tried with both). |