https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39943
--- Comment #25 from Lari Taskula <[email protected]> --- Awesome work, really impressive. This will be highly appreciated in many Finnish libraries. QA notes: 1. prove t/db_dependent/Koha/Patrons.t fails with DBIx::Class::Row::store_column(): No such column 'pin' on Koha::Schema::Result::Deletedborrower at /home/koha/Koha/Koha/Patron.pm line 1497 I see that you later attached a follow up patch adding the column to deletedborrowers, I've also attached a schema patch to make this test pass. 2. Should we enfore an absolute minimum pin length? From Koha::AuthUtils::is_pin_valid() I noticed that you left out the fallback value of 3 that the is_password_valid() method had. With these patches we default to a minimum of 4 by the atomicupdate (which is OK), but setting MinPinLength 0 is possible while on the other hand at least 1 digit is enforced by Koha::AuthUtils::is_pin_valid() regex step "non_digits". I think that like is_password_valid(), enforcing a fallback of 3 digits if MinPinLength < 3 would be reasonable. See my patches tagged with (proposal). 3. Your patch "Bug 39943: allow_pin_code field schema [DO NOT PUSH]" contains a change to kohastructure.sql likely meant for the previous patch. The new default value and the nullability will be dropped if this change is not pushed. 4. GUI questions: I think we should display the PIN code field at: - staff/cgi-bin/koha/members/memberentry.pl?op=edit_form&destination=circ&borrowernumber=123 (after password) - self registration (I could see libraries wanting to store patron's PIN code during self registration. Btw check syspref PatronSelfRegistrationBorrowerUnwantedField, pin is missing from this list and probably the other similar sysprefs as well. See Koha::Database::Columns->columns{'borrowers'}) These additions can be moved into new Bugs so that they won't stop this feature from moving on. Other very minor patch diff issues that stood out when reading your changes line by line: 5. Patch "Bug 39943: Add PIN_CHANGE letter" contains changes to OpenAPI spec, likely meant for the later patch "Bug 39943: patron/{patron_id}/pin endpoint" 6. Patch "Bug 39943: patron/{patron_id}/pin endpoint" 6.1 introduces Koha/Exceptions/Pin.pm late as it is already used by the previous patch. 6.2 has a strange looking endpoint edit in "api/v1/swagger/paths/patrons_pin.yaml", probably related to note 5 These do not change the end result nor the functionality of the feature but leave the mentioned patches slightly disorganized. Ran the following unit tests and they passed: 7. t/db_dependent/Auth/pin.t 8. t/db_dependent/api/v1/patrons_pin.t Followed the test plan you've written, and the feature works as advertised. I've added tests for set_pin() and is_pin_valid(). I'm still marking this as Failed QA due to note 3 which should be a trivial fix. Also I would like to hear your take on note 2 about the absolute minimum pin length. Again, really good work. Thank you so much for this. -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
