[Bug 33940] New: Move get_cardnumber_length and checkcardnumber to Koha
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 Bug ID: 33940 Summary: Move get_cardnumber_length and checkcardnumber to Koha Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: ASSIGNED Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: jonathan.druart+koha@gmail.com Reporter: jonathan.druart+koha@gmail.com QA Contact: testopia@bugs.koha-community.org C4::Members::get_cardnumber_length and C4::Members::checkcardnumber must be moved to the Koha namespace -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |16846 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16846 [Bug 16846] Move patron related code to Koha::Patron -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=30825 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 --- Comment #1 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 152109 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152109&action=edit Bug 33940: Move C4::Members cardnumber sub to Koha::Policy::Patrons::Cardnumber Test plan: The idea here is to confirm this patch does not introduce regression. For that you will play with the CardnumberLength syspref and create a new user, modify an existing one, and check that the UI does not let you modify an invalid cardnumber. The onboarding process and the patron import tool will also have to be tested -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 --- Comment #2 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Actually you will need to edit the DOM of the patron edit page, to remove the min and max length on the cardnumber's input. Otherwise the form will be validate client-side and you won't reach the changes made by this patch. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 --- Comment #3 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Will add tiny rebase for bug 33937 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 --- Comment #4 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- OK C4/Members.pm WARN Koha/Patrons/Import.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 199, now: 201) OK Koha/Policy/Patrons/Cardnumber.pm WARN installer/onboarding.pl WARN tidiness The file is less tidy than before (bad/messy lines before: 60, now: 61) OK members/memberentry.pl WARN opac/opac-memberentry.pl WARN tidiness The file is less tidy than before (bad/messy lines before: 167, now: 169) OK t/Members/cardnumber.t WARN t/db_dependent/Koha/Policy/Patrons/Cardnumber.t WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 45) OK t/db_dependent/Members.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 --- Comment #5 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Wont add tidy stuff :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 --- Comment #6 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- - # If cardnumber is null, we assume they're allowed. - return 0 unless defined $cardnumber; + return Koha::Result::Boolean->new(0)->add_message( { message => "is_empty" } ) + unless defined $cardnumber; The former routine returned 0 meaning OK. The new routine returns 0 meaning false. You are changing behavior. Mabye I just dont wanna use cardnumber. Not sure if it is needed to make a distinction here btw between null and empty string. Currently, message => "is_empty" should read is_undefined. We miss tests for this situation. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #152109|0 |1 is obsolete| | --- Comment #7 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 153758 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153758&action=edit Bug 33940: Move C4::Members cardnumber sub to Koha::Policy::Patrons::Cardnumber Test plan: The idea here is to confirm this patch does not introduce regression. For that you will play with the CardnumberLength syspref and create a new user, modify an existing one, and check that the UI does not let you modify an invalid cardnumber. The onboarding process and the patron import tool will also have to be tested -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 --- Comment #8 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Marcel de Rooy from comment #6)
- # If cardnumber is null, we assume they're allowed. - return 0 unless defined $cardnumber;
+ return Koha::Result::Boolean->new(0)->add_message( { message => "is_empty" } ) + unless defined $cardnumber;
The former routine returned 0 meaning OK. The new routine returns 0 meaning false. You are changing behavior. Mabye I just dont wanna use cardnumber.
I don't understand the problem. I am also changing if with unless... - if ( my $error_code = checkcardnumber($cardnumber) ) { + unless ( $is_cardnumber_valid ) {
Not sure if it is needed to make a distinction here btw between null and empty string. Currently, message => "is_empty" should read is_undefined.
We miss tests for this situation.
empty string will be caught by invalid_length unless defined (returning is_empty) Rereading the code: - # If cardnumber is null, we assume they're allowed. - return 0 unless defined $cardnumber; + return Koha::Result::Boolean->new(0)->add_message( { message => "is_empty" } ) + unless defined $cardnumber; So there might be a change in behaviour here. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Adding field length to comments is dangerous + my ( $min, $max ) = ( 0, $field_size ); # borrowers.cardnumber is a nullable varchar(20) Pref and DB: Maximum cannot be bigger than the database field size of 32. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 --- Comment #10 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 153759 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153759&action=edit Bug 33940: Consider NULL as valid -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 --- Comment #11 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Marcel de Rooy from comment #9)
Adding field length to comments is dangerous
+ my ( $min, $max ) = ( 0, $field_size ); # borrowers.cardnumber is a nullable varchar(20)
Pref and DB: Maximum cannot be bigger than the database field size of 32.
This is not an addition. This has been moved from existing code. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Testing experience: Have a borrower with empty card number (NULL in db). Set length pref to 1. Edit borrower. See the message should be 1. Save. It accepts the empty cardnumber. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #11)
(In reply to Marcel de Rooy from comment #9)
Adding field length to comments is dangerous
+ my ( $min, $max ) = ( 0, $field_size ); # borrowers.cardnumber is a nullable varchar(20)
Pref and DB: Maximum cannot be bigger than the database field size of 32.
This is not an addition. This has been moved from existing code.
I know. But it is not true anymore. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Marcel de Rooy from comment #12)
Testing experience:
Have a borrower with empty card number (NULL in db). Set length pref to 1. Edit borrower. See the message should be 1. Save. It accepts the empty cardnumber.
Still valid. Allows saving a space too. But fields remains NULL. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 --- Comment #15 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Marcel de Rooy from comment #14)
(In reply to Marcel de Rooy from comment #12)
Testing experience:
Have a borrower with empty card number (NULL in db). Set length pref to 1. Edit borrower. See the message should be 1. Save. It accepts the empty cardnumber.
Still valid. Allows saving a space too. But fields remains NULL.
But it needs a weird combination of sysprefs. Could be a separate bug.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |34338 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34338 [Bug 34338] Cardnumbers should be trimmed before insertion -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153758|0 |1 is obsolete| | --- Comment #16 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 153763 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153763&action=edit Bug 33940: Move C4::Members cardnumber sub to Koha::Policy::Patrons::Cardnumber Test plan: The idea here is to confirm this patch does not introduce regression. For that you will play with the CardnumberLength syspref and create a new user, modify an existing one, and check that the UI does not let you modify an invalid cardnumber. The onboarding process and the patron import tool will also have to be tested Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153759|0 |1 is obsolete| | --- Comment #17 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 153764 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153764&action=edit Bug 33940: Consider NULL as valid Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 --- Comment #18 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 153766 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153766&action=edit Bug 33940: (QA follow-up) Remove obsolete length of 20 Currently 32. Could be 43 tomorrow :) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 --- Comment #19 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 153768 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153768&action=edit Bug 33940: Fix selfreg please squash with first patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 --- Comment #20 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #19)
Created attachment 153768 [details] [review] Bug 33940: Fix selfreg
please squash with first patch
Good catch. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 --- Comment #21 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 153769 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153769&action=edit Bug 33940: Fix messages we sent to templates please squash with the first patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |ASSIGNED --- Comment #22 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Hum, found too many things here. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 --- Comment #23 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 153770 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153770&action=edit Bug 33940: Fix what we send to memberentry -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153763|0 |1 is obsolete| | Attachment #153764|0 |1 is obsolete| | Attachment #153766|0 |1 is obsolete| | Attachment #153768|0 |1 is obsolete| | Attachment #153769|0 |1 is obsolete| | Attachment #153770|0 |1 is obsolete| | --- Comment #24 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 153771 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153771&action=edit Bug 33940: Move C4::Members cardnumber sub to Koha::Policy::Patrons::Cardnumber Test plan: The idea here is to confirm this patch does not introduce regression. For that you will play with the CardnumberLength syspref and create a new user, modify an existing one, and check that the UI does not let you modify an invalid cardnumber. The onboarding process and the patron import tool will also have to be tested Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Bug 33940: Fix selfreg please squash with first patch Bug 33940: Fix messages we sent to templates please squash with the first patch Bug 33940: Fix what we send to memberentry -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 --- Comment #25 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 153772 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153772&action=edit Bug 33940: Consider NULL as valid Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 --- Comment #26 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 153773 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153773&action=edit Bug 33940: (QA follow-up) Remove obsolete length of 20 Currently 32. Could be 43 tomorrow :) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |34414 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34414 [Bug 34414] Remove DBD::Mock -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153771|0 |1 is obsolete| | Attachment #153772|0 |1 is obsolete| | Attachment #153773|0 |1 is obsolete| | --- Comment #27 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 155687 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155687&action=edit Bug 33940: Move C4::Members cardnumber sub to Koha::Policy::Patrons::Cardnumber Test plan: The idea here is to confirm this patch does not introduce regression. For that you will play with the CardnumberLength syspref and create a new user, modify an existing one, and check that the UI does not let you modify an invalid cardnumber. The onboarding process and the patron import tool will also have to be tested Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Bug 33940: Fix selfreg please squash with first patch Bug 33940: Fix messages we sent to templates please squash with the first patch Bug 33940: Fix what we send to memberentry Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 --- Comment #28 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 155688 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155688&action=edit Bug 33940: Consider NULL as valid Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 --- Comment #29 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 155689 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155689&action=edit Bug 33940: (QA follow-up) Remove obsolete length of 20 Currently 32. Could be 43 tomorrow :) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |23.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 --- Comment #30 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 23.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m --- Comment #31 from Fridolin Somers <fridolin.somers@biblibre.com> --- Enhancement not pushed to 23.05.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33940 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|Pushed to master |RESOLVED -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org