Bug 19936

Summary: Move Check_userid and Generate_Userid to Koha::Patron
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: enhancement    
Priority: P5 - low CC: josef.moravec, katrin.fischer, liz, nick, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 12001    
Bug Blocks: 16846, 13655, 21848, 22386    
Attachments: Bug 19936: Replace Check_userid - just move the tests
Bug 19936: Add the Koha::Patron->has_valid_userid method
Bug 19936: Replace Check_userid - Update the occurrences
Bug 19936: Remove the subroutine Check_Userid
Bug 19936: Move and add tests for Generate_Userid
Bug 19936: Add the Koha::Patron->generate_userid method
Bug 19936: Adapt tests
Bug 19936: Replace Generate_Userid - Update the occurrences
Bug 19936: Remove the subroutine Generate_Userid
Bug 19936: Remove warnings from tests
Bug 19936: Replace Check_userid - just move the tests
Bug 19936: Add the Koha::Patron->has_valid_userid method
Bug 19936: Replace Check_userid - Update the occurrences
Bug 19936: Remove the subroutine Check_Userid
Bug 19936: Move and add tests for Generate_Userid
Bug 19936: Add the Koha::Patron->generate_userid method
Bug 19936: Adapt tests
Bug 19936: Replace Generate_Userid - Update the occurrences
Bug 19936: Remove the subroutine Generate_Userid
Bug 19936: Remove warnings from tests
Bug 19936: Replace Check_userid - just move the tests
Bug 19936: Add the Koha::Patron->has_valid_userid method
Bug 19936: Replace Check_userid - Update the occurrences
Bug 19936: Remove the subroutine Check_Userid
Bug 19936: Move and add tests for Generate_Userid
Bug 19936: Add the Koha::Patron->generate_userid method
Bug 19936: Adapt tests
Bug 19936: Replace Generate_Userid - Update the occurrences
Bug 19936: Remove the subroutine Generate_Userid
Bug 19936: Remove warnings from tests
Bug 19936: Reuse existing userid if none provided
Bug 19936: Fix number of tests
Bug 19936: Replace Check_userid - just move the tests
Bug 19936: Add the Koha::Patron->has_valid_userid method
Bug 19936: Replace Check_userid - Update the occurrences
Bug 19936: Remove the subroutine Check_Userid
Bug 19936: Move and add tests for Generate_Userid
Bug 19936: Add the Koha::Patron->generate_userid method
Bug 19936: Adapt tests
Bug 19936: Replace Generate_Userid - Update the occurrences
Bug 19936: Remove the subroutine Generate_Userid
Bug 19936: Remove warnings from tests
Bug 19936: (follow-up) Replace Check_userid - Update the occurrences
Bug 19936: Reuse existing userid if none provided
Bug 19936: Replace Check_userid - just move the tests
Bug 19936: Add the Koha::Patron->has_valid_userid method
Bug 19936: Replace Check_userid - Update the occurrences
Bug 19936: Remove the subroutine Check_Userid
Bug 19936: Move and add tests for Generate_Userid
Bug 19936: Add the Koha::Patron->generate_userid method
Bug 19936: Adapt tests
Bug 19936: Replace Generate_Userid - Update the occurrences
Bug 19936: Remove the subroutine Generate_Userid
Bug 19936: Remove warnings from tests
Bug 19936: Replace Check_userid - Update the occurrences
Bug 19936: Reuse existing userid if none provided
Bug 19936: Replace Check_userid - just move the tests
Bug 19936: Add the Koha::Patron->has_valid_userid method
Bug 19936: Replace Check_userid - Update the occurrences
Bug 19936: Remove the subroutine Check_Userid
Bug 19936: Move and add tests for Generate_Userid
Bug 19936: Add the Koha::Patron->generate_userid method
Bug 19936: Adapt tests
Bug 19936: Replace Generate_Userid - Update the occurrences
Bug 19936: Remove the subroutine Generate_Userid
Bug 19936: Remove warnings from tests
Bug 19936: Replace Check_userid - Update the occurrences
Bug 19936: Reuse existing userid if none provided
Bug 19936: Replace Check_userid - just move the tests
Bug 19936: Add the Koha::Patron->has_valid_userid method
Bug 19936: Replace Check_userid - Update the occurrences
Bug 19936: Remove the subroutine Check_Userid
Bug 19936: Move and add tests for Generate_Userid
Bug 19936: Add the Koha::Patron->generate_userid method
Bug 19936: Adapt tests
Bug 19936: Replace Generate_Userid - Update the occurrences
Bug 19936: Remove the subroutine Generate_Userid
Bug 19936: Remove warnings from tests
Bug 19936: Replace Check_userid - Update the occurrences
Bug 19936: Reuse existing userid if none provided

Description Jonathan Druart 2018-01-09 14:33:29 UTC

    
Comment 1 Jonathan Druart 2018-01-09 14:53:27 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
Comment 2 Jonathan Druart 2018-01-09 14:53:30 UTC
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
Comment 3 Jonathan Druart 2018-01-09 14:53:33 UTC
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
Comment 4 Jonathan Druart 2018-01-09 14:53:36 UTC
Created attachment 70368 [details] [review]
Bug 19936: Remove the subroutine Check_Userid

It is no longer in user, we can remove it.
Comment 5 Jonathan Druart 2018-01-09 14:53:40 UTC
Created attachment 70369 [details] [review]
Bug 19936: Move and add tests for Generate_Userid
Comment 6 Jonathan Druart 2018-01-09 14:53:43 UTC
Created attachment 70370 [details] [review]
Bug 19936: Add the Koha::Patron->generate_userid method
Comment 7 Jonathan Druart 2018-01-09 14:53:47 UTC
Created attachment 70371 [details] [review]
Bug 19936: Adapt tests

And prove that we are not cheating
Comment 8 Jonathan Druart 2018-01-09 14:53:51 UTC
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
Comment 9 Jonathan Druart 2018-01-09 14:53:55 UTC
Created attachment 70373 [details] [review]
Bug 19936: Remove the subroutine Generate_Userid
Comment 10 Jonathan Druart 2018-01-09 14:53:59 UTC
Created attachment 70374 [details] [review]
Bug 19936: Remove warnings from tests

C4::Members did not "use warnings;"
Comment 11 Jonathan Druart 2018-02-16 14:34:13 UTC
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
Comment 12 Jonathan Druart 2018-02-16 14:34:16 UTC
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
Comment 13 Jonathan Druart 2018-02-16 14:34:19 UTC
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
Comment 14 Jonathan Druart 2018-02-16 14:34:22 UTC
Created attachment 71790 [details] [review]
Bug 19936: Remove the subroutine Check_Userid

It is no longer in user, we can remove it.
Comment 15 Jonathan Druart 2018-02-16 14:34:26 UTC
Created attachment 71791 [details] [review]
Bug 19936: Move and add tests for Generate_Userid
Comment 16 Jonathan Druart 2018-02-16 14:34:29 UTC
Created attachment 71792 [details] [review]
Bug 19936: Add the Koha::Patron->generate_userid method
Comment 17 Jonathan Druart 2018-02-16 14:34:33 UTC
Created attachment 71793 [details] [review]
Bug 19936: Adapt tests

And prove that we are not cheating
Comment 18 Jonathan Druart 2018-02-16 14:34:36 UTC
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
Comment 19 Jonathan Druart 2018-02-16 14:34:40 UTC
Created attachment 71795 [details] [review]
Bug 19936: Remove the subroutine Generate_Userid
Comment 20 Jonathan Druart 2018-02-16 14:34:43 UTC
Created attachment 71796 [details] [review]
Bug 19936: Remove warnings from tests

C4::Members did not "use warnings;"
Comment 21 Jonathan Druart 2018-02-23 16:48:40 UTC
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
Comment 22 Jonathan Druart 2018-02-23 16:48:44 UTC
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
Comment 23 Jonathan Druart 2018-02-23 16:48:48 UTC
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
Comment 24 Jonathan Druart 2018-02-23 16:48:51 UTC
Created attachment 72132 [details] [review]
Bug 19936: Remove the subroutine Check_Userid

It is no longer in user, we can remove it.
Comment 25 Jonathan Druart 2018-02-23 16:48:54 UTC
Created attachment 72133 [details] [review]
Bug 19936: Move and add tests for Generate_Userid
Comment 26 Jonathan Druart 2018-02-23 16:48:57 UTC
Created attachment 72134 [details] [review]
Bug 19936: Add the Koha::Patron->generate_userid method
Comment 27 Jonathan Druart 2018-02-23 16:49:01 UTC
Created attachment 72135 [details] [review]
Bug 19936: Adapt tests

And prove that we are not cheating
Comment 28 Jonathan Druart 2018-02-23 16:49:04 UTC
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
Comment 29 Jonathan Druart 2018-02-23 16:49:07 UTC
Created attachment 72137 [details] [review]
Bug 19936: Remove the subroutine Generate_Userid
Comment 30 Jonathan Druart 2018-02-23 16:49:11 UTC
Created attachment 72138 [details] [review]
Bug 19936: Remove warnings from tests

C4::Members did not "use warnings;"
Comment 31 Jonathan Druart 2018-02-23 16:49:14 UTC
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
Comment 32 Jonathan Druart 2018-02-23 16:50:48 UTC
Created attachment 72140 [details] [review]
Bug 19936: Fix number of tests

will be squashed later, but lazy to reupload the whole patchset
Comment 33 Jonathan Druart 2018-02-23 17:15:31 UTC
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
Comment 34 Jonathan Druart 2018-02-23 17:15:35 UTC
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
Comment 35 Jonathan Druart 2018-02-23 17:15:38 UTC
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
Comment 36 Jonathan Druart 2018-02-23 17:15:41 UTC
Created attachment 72144 [details] [review]
Bug 19936: Remove the subroutine Check_Userid

It is no longer in user, we can remove it.
Comment 37 Jonathan Druart 2018-02-23 17:15:45 UTC
Created attachment 72145 [details] [review]
Bug 19936: Move and add tests for Generate_Userid
Comment 38 Jonathan Druart 2018-02-23 17:15:48 UTC
Created attachment 72146 [details] [review]
Bug 19936: Add the Koha::Patron->generate_userid method
Comment 39 Jonathan Druart 2018-02-23 17:15:52 UTC
Created attachment 72147 [details] [review]
Bug 19936: Adapt tests

And prove that we are not cheating
Comment 40 Jonathan Druart 2018-02-23 17:15:55 UTC
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
Comment 41 Jonathan Druart 2018-02-23 17:15:58 UTC
Created attachment 72149 [details] [review]
Bug 19936: Remove the subroutine Generate_Userid
Comment 42 Jonathan Druart 2018-02-23 17:16:02 UTC
Created attachment 72150 [details] [review]
Bug 19936: Remove warnings from tests

C4::Members did not "use warnings;"
Comment 43 Jonathan Druart 2018-02-23 17:18:09 UTC
Created attachment 72151 [details] [review]
Bug 19936: (follow-up) Replace Check_userid - Update the occurrences
Comment 44 Jonathan Druart 2018-02-23 17:18:13 UTC
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
Comment 45 Jonathan Druart 2018-02-23 17:18:54 UTC
Rebased on top of bug 12001

+ 2 bugs fixed.
Comment 46 Josef Moravec 2018-04-04 05:42:15 UTC
Sorry, could you rebase please?
Comment 47 Jonathan Druart 2018-04-04 13:50:18 UTC
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
Comment 48 Jonathan Druart 2018-04-04 13:50:23 UTC
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
Comment 49 Jonathan Druart 2018-04-04 13:50:28 UTC
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
Comment 50 Jonathan Druart 2018-04-04 13:50:34 UTC
Created attachment 73633 [details] [review]
Bug 19936: Remove the subroutine Check_Userid

It is no longer in user, we can remove it.
Comment 51 Jonathan Druart 2018-04-04 13:50:43 UTC
Created attachment 73634 [details] [review]
Bug 19936: Move and add tests for Generate_Userid
Comment 52 Jonathan Druart 2018-04-04 13:50:48 UTC
Created attachment 73635 [details] [review]
Bug 19936: Add the Koha::Patron->generate_userid method
Comment 53 Jonathan Druart 2018-04-04 13:50:54 UTC
Created attachment 73636 [details] [review]
Bug 19936: Adapt tests

And prove that we are not cheating
Comment 54 Jonathan Druart 2018-04-04 13:51:00 UTC
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
Comment 55 Jonathan Druart 2018-04-04 13:51:12 UTC
Created attachment 73638 [details] [review]
Bug 19936: Remove the subroutine Generate_Userid
Comment 56 Jonathan Druart 2018-04-04 13:51:17 UTC
Created attachment 73639 [details] [review]
Bug 19936: Remove warnings from tests

C4::Members did not "use warnings;"
Comment 57 Jonathan Druart 2018-04-04 13:51:23 UTC
Created attachment 73640 [details] [review]
Bug 19936: Replace Check_userid - Update the occurrences
Comment 58 Jonathan Druart 2018-04-04 13:51:28 UTC
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
Comment 59 Josef Moravec 2018-04-10 21:37:32 UTC
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>
Comment 60 Josef Moravec 2018-04-10 21:37:39 UTC
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>
Comment 61 Josef Moravec 2018-04-10 21:37:45 UTC
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>
Comment 62 Josef Moravec 2018-04-10 21:37:50 UTC
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>
Comment 63 Josef Moravec 2018-04-10 21:37:54 UTC
Created attachment 73967 [details] [review]
Bug 19936: Move and add tests for Generate_Userid

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 64 Josef Moravec 2018-04-10 21:37:59 UTC
Created attachment 73968 [details] [review]
Bug 19936: Add the Koha::Patron->generate_userid method

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 65 Josef Moravec 2018-04-10 21:38:06 UTC
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>
Comment 66 Josef Moravec 2018-04-10 21:38:10 UTC
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>
Comment 67 Josef Moravec 2018-04-10 21:38:15 UTC
Created attachment 73971 [details] [review]
Bug 19936: Remove the subroutine Generate_Userid

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 68 Josef Moravec 2018-04-10 21:38:19 UTC
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>
Comment 69 Josef Moravec 2018-04-10 21:38:23 UTC
Created attachment 73973 [details] [review]
Bug 19936: Replace Check_userid - Update the occurrences

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 70 Josef Moravec 2018-04-10 21:38:28 UTC
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>
Comment 71 Katrin Fischer 2018-04-12 10:28:11 UTC
Giving this a try.
Comment 72 Katrin Fischer 2018-04-12 12:25:43 UTC
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>
Comment 73 Katrin Fischer 2018-04-12 12:25:47 UTC
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>
Comment 74 Katrin Fischer 2018-04-12 12:25:51 UTC
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>
Comment 75 Katrin Fischer 2018-04-12 12:25:55 UTC
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>
Comment 76 Katrin Fischer 2018-04-12 12:25:59 UTC
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>
Comment 77 Katrin Fischer 2018-04-12 12:26:04 UTC
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>
Comment 78 Katrin Fischer 2018-04-12 12:26:08 UTC
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>
Comment 79 Katrin Fischer 2018-04-12 12:26:12 UTC
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>
Comment 80 Katrin Fischer 2018-04-12 12:26:16 UTC
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>
Comment 81 Katrin Fischer 2018-04-12 12:26:20 UTC
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>
Comment 82 Katrin Fischer 2018-04-12 12:26:24 UTC
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>
Comment 83 Katrin Fischer 2018-04-12 12:26:29 UTC
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>
Comment 84 Katrin Fischer 2018-04-12 12:28:31 UTC
Preserves current behaviour - including remaining bugs :D (bug 13655)
Comment 85 Jonathan Druart 2018-04-12 15:06:05 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 86 Nick Clemens 2018-05-14 19:34:30 UTC
Enhancement, not backported for 17.11.x
Comment 87 Liz Rea 2018-06-15 05:06:03 UTC
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.
Comment 88 Jonathan Druart 2018-06-18 13:41:44 UTC
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.
Comment 89 Jonathan Druart 2018-06-18 13:49:26 UTC
(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).