Bug 25769 - Patron self modification triggers change request for date of birth to null
Summary: Patron self modification triggers change request for date of birth to null
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-06-16 11:59 UTC by Kyle M Hall
Modified: 2020-07-22 17:34 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.02, 19.11.08, 19.05.13


Attachments
Bug 25769: Patron self modification triggers change request for date of birth to null (1.93 KB, patch)
2020-06-16 12:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25769: Patron self modification triggers change request for date of birth to null (1.98 KB, patch)
2020-06-17 09:49 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 25769: Patron self modification triggers change request for date of birth to null (2.04 KB, patch)
2020-06-22 07:36 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2020-06-16 11:59:32 UTC
If PatronSelfModificationBorrowerUnwantedField contains dateofbirth, all patron self modification requests will attempt to set dateofbirth to null instead of ignoring the field.
Comment 1 Kyle M Hall 2020-06-16 12:06:33 UTC
Created attachment 105908 [details] [review]
Bug 25769: Patron self modification triggers change request for date of birth to null

If PatronSelfModificationBorrowerUnwantedField contains dateofbirth, all
patron self modification requests will attempt to set dateofbirth to
null instead of ignoring the field.

Test Plan:
1) Add dateofbirth to PatronSelfModificationBorrowerUnwantedField
2) Run a patron self modification
3) Note the request sets dateofbirth to null on the staff side
4) Apply this patch
5) Restart all the things!
6) Run another self-modification
7) Note of the staff side that dateofbirth is unaffected
Comment 2 Emmi Takkinen 2020-06-17 09:49:37 UTC
Created attachment 105938 [details] [review]
Bug 25769: Patron self modification triggers change request for date of birth to null

If PatronSelfModificationBorrowerUnwantedField contains dateofbirth, all
patron self modification requests will attempt to set dateofbirth to
null instead of ignoring the field.

Test Plan:
1) Add dateofbirth to PatronSelfModificationBorrowerUnwantedField
2) Run a patron self modification
3) Note the request sets dateofbirth to null on the staff side
4) Apply this patch
5) Restart all the things!
6) Run another self-modification
7) Note of the staff side that dateofbirth is unaffected

Signed-off-by: Emmi Takkinen <emmi.takkinen@outlook.com>
Comment 3 Martin Renvoize 2020-06-19 11:36:07 UTC
Comment on attachment 105938 [details] [review]
Bug 25769: Patron self modification triggers change request for date of birth to null

Review of attachment 105938 [details] [review]:
-----------------------------------------------------------------

::: opac/opac-memberentry.pl
@@ +484,5 @@
>  
> +    if ( defined $borrower{'dateofbirth'} ) {
> +        my $dob_dt;
> +        $dob_dt = eval { dt_from_string( $borrower{'dateofbirth'} ); }
> +            if ( $borrower{'dateofbirth'} );

You're now testing if $borrower{'dateofbirth'} is defined above.. do you still need to test it here too?
Comment 4 Martin Renvoize 2020-06-19 11:36:53 UTC
Failing QA whilst I wait for a reply.
Comment 5 Kyle M Hall 2020-06-19 16:12:17 UTC
(In reply to Martin Renvoize from comment #3)
> Comment on attachment 105938 [details] [review] [review]
> Bug 25769: Patron self modification triggers change request for date of
> birth to null
> 
> Review of attachment 105938 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: opac/opac-memberentry.pl
> @@ +484,5 @@
> >  
> > +    if ( defined $borrower{'dateofbirth'} ) {
> > +        my $dob_dt;
> > +        $dob_dt = eval { dt_from_string( $borrower{'dateofbirth'} ); }
> > +            if ( $borrower{'dateofbirth'} );
> 
> You're now testing if $borrower{'dateofbirth'} is defined above.. do you
> still need to test it here too?

I suppose not! Followup will be forthcoming.
Comment 6 Kyle M Hall 2020-06-19 16:19:37 UTC
(In reply to Martin Renvoize from comment #3)
> Comment on attachment 105938 [details] [review] [review]
> Bug 25769: Patron self modification triggers change request for date of
> birth to null
> 
> Review of attachment 105938 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: opac/opac-memberentry.pl
> @@ +484,5 @@
> >  
> > +    if ( defined $borrower{'dateofbirth'} ) {
> > +        my $dob_dt;
> > +        $dob_dt = eval { dt_from_string( $borrower{'dateofbirth'} ); }
> > +            if ( $borrower{'dateofbirth'} );
> 
> You're now testing if $borrower{'dateofbirth'} is defined above.. do you
> still need to test it here too?

After examining the code, yes we do still need it. This new check tests to see if the dateofbirth param is part of the form. If it *is*, it still may be blank. Without this check, we'd set the dateofbirth to the current date.
Comment 7 Martin Renvoize 2020-06-22 07:33:53 UTC
(In reply to Kyle M Hall from comment #6)
> (In reply to Martin Renvoize from comment #3)
> > Comment on attachment 105938 [details] [review] [review] [review]
> > Bug 25769: Patron self modification triggers change request for date of
> > birth to null
> > 
> > Review of attachment 105938 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: opac/opac-memberentry.pl
> > @@ +484,5 @@
> > >  
> > > +    if ( defined $borrower{'dateofbirth'} ) {
> > > +        my $dob_dt;
> > > +        $dob_dt = eval { dt_from_string( $borrower{'dateofbirth'} ); }
> > > +            if ( $borrower{'dateofbirth'} );
> > 
> > You're now testing if $borrower{'dateofbirth'} is defined above.. do you
> > still need to test it here too?
> 
> After examining the code, yes we do still need it. This new check tests to
> see if the dateofbirth param is part of the form. If it *is*, it still may
> be blank. Without this check, we'd set the dateofbirth to the current date.

Ah, of course.. good catch.
Comment 8 Martin Renvoize 2020-06-22 07:36:25 UTC
Created attachment 106146 [details] [review]
Bug 25769: Patron self modification triggers change request for date of birth to null

If PatronSelfModificationBorrowerUnwantedField contains dateofbirth, all
patron self modification requests will attempt to set dateofbirth to
null instead of ignoring the field.

Test Plan:
1) Add dateofbirth to PatronSelfModificationBorrowerUnwantedField
2) Run a patron self modification
3) Note the request sets dateofbirth to null on the staff side
4) Apply this patch
5) Restart all the things!
6) Run another self-modification
7) Note of the staff side that dateofbirth is unaffected

Signed-off-by: Emmi Takkinen <emmi.takkinen@outlook.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2020-06-22 07:36:53 UTC
Works as expected, passes qa scripts.

Passing QA
Comment 10 Jonathan Druart 2020-06-24 13:33:15 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 11 Lucas Gass 2020-07-10 15:24:17 UTC
backported to 20.05.x for 20.05.02
Comment 12 Aleisha Amohia 2020-07-12 22:24:57 UTC
backported to 19.11.x for 19.11.08
Comment 13 Victor Grousset/tuxayo 2020-07-22 17:34:28 UTC
Backported to 19.05.x branch for 19.05.13