|
Description
Jonathan Druart
2023-06-07 13:36:50 UTC
Created attachment 152109 [details] [review] 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 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. 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
Wont add tidy stuff :) - # 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.
Created attachment 153758 [details] [review] 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 (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. 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. Created attachment 153759 [details] [review] Bug 33940: Consider NULL as valid (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. 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. (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. (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. (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.. Created attachment 153763 [details] [review] 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> Created attachment 153764 [details] [review] Bug 33940: Consider NULL as valid Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 153766 [details] [review] 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> Created attachment 153768 [details] [review] Bug 33940: Fix selfreg please squash with first patch (In reply to Jonathan Druart from comment #19) > Created attachment 153768 [details] [review] [review] > Bug 33940: Fix selfreg > > please squash with first patch Good catch. Created attachment 153769 [details] [review] Bug 33940: Fix messages we sent to templates please squash with the first patch Hum, found too many things here. Created attachment 153770 [details] [review] Bug 33940: Fix what we send to memberentry Created attachment 153771 [details] [review] 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 Created attachment 153772 [details] [review] Bug 33940: Consider NULL as valid Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 153773 [details] [review] 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> Created attachment 155687 [details] [review] 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> Created attachment 155688 [details] [review] 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> Created attachment 155689 [details] [review] 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> Pushed to master for 23.11. Nice work everyone, thanks! Enhancement not pushed to 23.05.x |