Bug 34338

Summary: Cardnumbers should be trimmed before insertion
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: In Discussion --- QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: m.de.rooy
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on: 33940    
Bug Blocks:    
Attachments: Bug 34338: Do not allow spaces in cardnumber
Bug 34338: Do not allow leading or trailing whitespaces in cardnumber
Bug 34338: Do not allow leading or trailing whitespaces in cardnumber

Description Jonathan Druart 2023-07-21 09:36:58 UTC
You can set cardnumber to NULL even if not allowed
Comment 1 Jonathan Druart 2023-07-21 09:46:38 UTC
  commit 785af7310daa4633359755add9474e81f108d022
  Bug 6521 - allow blank cardnumbers to not trigger "already in use"

+    # If the cardnumber is blank, treat it as null.
+    $newdata{'cardnumber'} = undef if $newdata{'cardnumber'} =~ /^\s*$/;


What's the expected behaviour?
Comment 2 Jonathan Druart 2023-07-21 10:16:34 UTC
Created attachment 153774 [details] [review]
Bug 34338: Do not allow spaces in cardnumber
Comment 3 Jonathan Druart 2023-07-21 10:16:48 UTC
Hum see comment 1. What's the expected behaviour? Should we keep the "if blank treat as null" in memberentry.pl for staff, but reject in other places?
Comment 4 Marcel de Rooy 2023-07-27 12:14:08 UTC
(In reply to Jonathan Druart from comment #3)
> Hum see comment 1. What's the expected behaviour? Should we keep the "if
> blank treat as null" in memberentry.pl for staff, but reject in other places?

No we should reject it too.
But something strange is going on here in the direct context:

    my $new_barcode = $newdata{'cardnumber'};
    Koha::Plugins->call( 'patron_barcode_transform', \$new_barcode );

    $newdata{'cardnumber'} = $new_barcode;

That is interfering with our new logic here.
Does not look good to me.
Comment 5 Marcel de Rooy 2023-07-27 12:18:22 UTC
koha-tmpl/intranet-tmpl/prog/en/includes/onboarding_messages.inc
Card number contains whitespaces

koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
Cardnumber already contains whitespaces.

koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt
[% ELSIF cardnumber_not_trimmed %]
<a href="#borrower_cardnumber"><strong>The entered card number contains whitespaces.</strong></a>

Three different strings.
And they are not exact. We dont like leading or trailing whitespace.
Comment 6 Jonathan Druart 2023-07-27 12:29:50 UTC
Created attachment 153967 [details] [review]
Bug 34338: Do not allow leading or trailing whitespaces in cardnumber
Comment 7 Jonathan Druart 2023-07-27 12:32:41 UTC
(In reply to Marcel de Rooy from comment #5)
> koha-tmpl/intranet-tmpl/prog/en/includes/onboarding_messages.inc
> Card number contains whitespaces
> 
> koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
> Cardnumber already contains whitespaces.
> 
> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt
> [% ELSIF cardnumber_not_trimmed %]
> <a href="#borrower_cardnumber"><strong>The entered card number contains
> whitespaces.</strong></a>
> 
> Three different strings.
> And they are not exact. We dont like leading or trailing whitespace.

Fixed. There are still 2 different messages, to match the other strings in the same files.

(In reply to Marcel de Rooy from comment #4)
> (In reply to Jonathan Druart from comment #3)
> > Hum see comment 1. What's the expected behaviour? Should we keep the "if
> > blank treat as null" in memberentry.pl for staff, but reject in other places?
> 
> No we should reject it too.

It's not clear to me if we want to introduce a behaviour change here.


> But something strange is going on here in the direct context:
> 
>     my $new_barcode = $newdata{'cardnumber'};
>     Koha::Plugins->call( 'patron_barcode_transform', \$new_barcode );
> 
>     $newdata{'cardnumber'} = $new_barcode;
> 
> That is interfering with our new logic here.
> Does not look good to me.

I don't understand what you mean, can you explain a bit more?
Comment 8 Marcel de Rooy 2023-07-27 12:43:32 UTC
(In reply to Jonathan Druart from comment #7)
> It's not clear to me if we want to introduce a behaviour change here.
Please explain that will be the benefits of keeping this 'behavior'.

> > But something strange is going on here in the direct context:
> > 
> >     my $new_barcode = $newdata{'cardnumber'};
> >     Koha::Plugins->call( 'patron_barcode_transform', \$new_barcode );
> > 
> >     $newdata{'cardnumber'} = $new_barcode;
> > 
> > That is interfering with our new logic here.
> > Does not look good to me.
> 
> I don't understand what you mean, can you explain a bit more?

Plugin is called apparently based on cardnumber. But stores info in cardnumber. Should be barcode, right?
Comment 9 Marcel de Rooy 2023-07-27 12:45:03 UTC
If there are no plugins, cardnumber is undef ??
Comment 10 Jonathan Druart 2023-07-27 12:49:42 UTC
(In reply to Marcel de Rooy from comment #8)
> (In reply to Jonathan Druart from comment #7)
> > It's not clear to me if we want to introduce a behaviour change here.
> Please explain that will be the benefits of keeping this 'behavior'.
> 
> > > But something strange is going on here in the direct context:
> > > 
> > >     my $new_barcode = $newdata{'cardnumber'};
> > >     Koha::Plugins->call( 'patron_barcode_transform', \$new_barcode );
> > > 
> > >     $newdata{'cardnumber'} = $new_barcode;
> > > 
> > > That is interfering with our new logic here.
> > > Does not look good to me.
> > 
> > I don't understand what you mean, can you explain a bit more?
> 
> Plugin is called apparently based on cardnumber. But stores info in
> cardnumber. Should be barcode, right?

barcode==cardnumber here

(In reply to Marcel de Rooy from comment #9)
> If there are no plugins, cardnumber is undef ??

No, it's just not modified.
Comment 11 Jonathan Druart 2023-07-27 13:14:23 UTC
Created attachment 153972 [details] [review]
Bug 34338: Do not allow leading or trailing whitespaces in cardnumber