Bug 34338 - Cardnumbers should be trimmed before insertion
Summary: Cardnumbers should be trimmed before insertion
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 33940
Blocks:
  Show dependency treegraph
 
Reported: 2023-07-21 09:36 UTC by Jonathan Druart
Modified: 2023-07-27 13:14 UTC (History)
1 user (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 34338: Do not allow spaces in cardnumber (8.39 KB, patch)
2023-07-21 10:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34338: Do not allow leading or trailing whitespaces in cardnumber (9.03 KB, patch)
2023-07-27 12:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34338: Do not allow leading or trailing whitespaces in cardnumber (9.21 KB, patch)
2023-07-27 13:14 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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