Bug 34117 - Duplicate patron sets dateenrolled incorrectly
Summary: Duplicate patron sets dateenrolled incorrectly
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Emily Lamancusa
QA Contact: Katrin Fischer
URL:
Keywords:
: 33872 (view as bug list)
Depends on: 30718
Blocks: 34435
  Show dependency treegraph
 
Reported: 2023-06-23 16:16 UTC by Andrew Fuerste-Henry
Modified: 2023-12-28 20:47 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.03,22.11.09


Attachments
Bug 34117: Add ymd to date_from_string when duplicating patron (1.16 KB, patch)
2023-06-23 20:25 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 34117: Add ymd to date_from_string when duplicating patron (1.22 KB, patch)
2023-06-24 14:34 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 34117: Remove side effect from get_expiry_date (1.69 KB, patch)
2023-07-27 13:43 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 34117: Remove side effect from get_expiry_date (1.75 KB, patch)
2023-07-27 13:58 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 34117: Remove side effect from get_expiry_date (1.80 KB, patch)
2023-07-27 14:41 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 34117: Add unit tests (2.68 KB, patch)
2023-07-27 14:48 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 34117: Remove side effect from get_expiry_date (1.85 KB, patch)
2023-07-29 19:41 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 34117: Add unit tests (2.81 KB, patch)
2023-07-29 19:41 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2023-06-23 16:16:33 UTC
When duplicating a patron, the new patron's registration date on memberentry.pl defaults to a date that is current date + the enrollment period for the category of the patron being duplicated. The registration date should simply default to today's date.

To recreate:
- have or make a patron
- duplicate that patron
- on the memberentry page for your new patron, before you save them, scroll down to Registration Date and see that it's defaulting to a date in the future
Comment 1 Andrew Fuerste-Henry 2023-06-23 16:17:04 UTC
I noticed this first in a 22.11 system and confirmed it on master.
Comment 2 Lucas Gass 2023-06-23 20:25:21 UTC
Created attachment 152656 [details] [review]
Bug 34117: Add ymd to date_from_string when duplicating patron

To test:
- have or make a patron
- duplicate that patron
- on the memberentry page for your new patron, before you save them, scroll down to Registration Date and see that it's defaulting to a date in the future
- Apply patch and restart_all
- Try duplicating a patron again.
- Date should correctly set to today
Comment 3 ByWater Sandboxes 2023-06-24 14:34:21 UTC
Created attachment 152657 [details] [review]
Bug 34117: Add ymd to date_from_string when duplicating patron

To test:
- have or make a patron
- duplicate that patron
- on the memberentry page for your new patron, before you save them, scroll down to Registration Date and see that it's defaulting to a date in the future
- Apply patch and restart_all
- Try duplicating a patron again.
- Date should correctly set to today

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 4 Laura Escamilla 2023-07-18 12:47:00 UTC
*** Bug 33872 has been marked as a duplicate of this bug. ***
Comment 5 Emily Lamancusa 2023-07-18 13:31:51 UTC
Adding a dependency on 30718 as it looks like this bug was a side effect from the first patch on that one - that patch changed $data{dateenrolled} from a string to an object (causing get_expiry_date to update the original object rather than creating its own from a string).
Comment 6 Jonathan Druart 2023-07-18 14:02:15 UTC
Just reading the diff my feeling is that this is a candidate to side-effects. If the problem is coming from get_expiry_date then I would suggest to pass the ymd version without modifying $data{'dateenrolled'}.

See also the previous if, where we pass dt_from_string (and so the script will have 2 different values in $data{dateenrolled})
Comment 7 Jonathan Druart 2023-07-18 14:07:38 UTC
Sorry, I haven't seen Emily's comment!

There is something weird here as well:
105 sub get_expiry_date {
109         $date = dt_from_string( $date ) unless ref $date;

Which means get_expiry_date should deal with DateTime object correctly.
Comment 8 Emily Lamancusa 2023-07-18 15:20:58 UTC
(In reply to Jonathan Druart from comment #7)
> Sorry, I haven't seen Emily's comment!
> 
> There is something weird here as well:
> 105 sub get_expiry_date {
> 109         $date = dt_from_string( $date ) unless ref $date;
> 
> Which means get_expiry_date should deal with DateTime object correctly.

Line 109 keeps the original object if one was passed, and creates a new object if a string was passed, right? And then in the next line...

110 return $date->add( months => $self->enrolmentperiod, end_of_month => 'limit' );

According to https://metacpan.org/pod/DateTime#Math-Methods:
"Like the set methods, math related methods always return the object itself, to allow for chaining"

So unless I'm misunderstanding the Perl from line 109, if get_expiry_date is passed an object, it updates and returns that same object, which memberentry.pl doesn't take into account in line 725:
$data{dateexpiry} = $category->get_expiry_date( $data{dateenrolled} );
Comment 9 Jonathan Druart 2023-07-20 13:23:43 UTC
(In reply to Emily Lamancusa from comment #8)
> (In reply to Jonathan Druart from comment #7)
> > Sorry, I haven't seen Emily's comment!
> > 
> > There is something weird here as well:
> > 105 sub get_expiry_date {
> > 109         $date = dt_from_string( $date ) unless ref $date;
> > 
> > Which means get_expiry_date should deal with DateTime object correctly.
> 
> Line 109 keeps the original object if one was passed, and creates a new
> object if a string was passed, right? And then in the next line...
> 
> 110 return $date->add( months => $self->enrolmentperiod, end_of_month =>
> 'limit' );
> 
> According to https://metacpan.org/pod/DateTime#Math-Methods:
> "Like the set methods, math related methods always return the object itself,
> to allow for chaining"
> 
> So unless I'm misunderstanding the Perl from line 109, if get_expiry_date is
> passed an object, it updates and returns that same object, which
> memberentry.pl doesn't take into account in line 725:
> $data{dateexpiry} = $category->get_expiry_date( $data{dateenrolled} );


DateTime modifies the original object.

For instance:
use Koha::DateUtils qw( dt_from_string );
my $dt = dt_from_string;
say $dt; 
say $dt->add( days => 2 ); 
say $dt; 

2023-07-20T13:14:51
2023-07-22T13:14:51
2023-07-22T13:14:51



So, if a DateTime object is passed to get_expiry_date
108         $date ||= dt_from_string;
=> $date is still the variable passed in parameter
109         $date = dt_from_string( $date ) unless ref $date;
=> same
110         return $date->add( months => $self->enrolmentperiod, end_of_month => 'limit' );
=> We return the object we passed (modified)
Comment 10 Jonathan Druart 2023-07-20 13:25:53 UTC
Ok so maybe I get it now. If that was the original problem then the correct fix is (in get_expiry_date):

-        $date = dt_from_string( $date ) unless ref $date;
+        $date = ref $date ? $date->clone() : dt_from_string( $date );

If confirmed, the same should be apply to get_password_expiry_date.

If all that does not make sense, please ignore me.
Comment 11 Emily Lamancusa 2023-07-20 14:36:21 UTC
Yes, that's the idea I was trying to get across :)

That fix looks viable to me! Changing the behavior of a method seems like it would also come with a side effect risk, but that would be easy to check for with a git grep of the method call. New bug to make that adjustment to get_expiry_date and get_password_expiry_date? (anywhere else we use that logic that should get updated?)
Comment 12 Jonathan Druart 2023-07-20 14:57:01 UTC
If the change fixes the problem originally describe here, we should make the change here, and obsolete the existing patch.
Comment 13 Emily Lamancusa 2023-07-27 13:43:16 UTC
Created attachment 153977 [details] [review]
Bug 34117: Remove side effect from get_expiry_date

If get_expiry_date is passed a DateTime object as a parameter,
it modifies and returns the original object. When memberentry.pl
prefills the input fields for duplicating a patron, it passes the
enrollment date object to get_expiry_date. This causes the enrollment
date object to be modified with the expiry date value.

This patch modifies get_expiry_date to clone the DateTime object that it
receives as a parameter and return the clone, so that references to an
enrollment date object can be passed in safely.

To test:
1. Have or make a patron
2. Duplicate that patron
3. Before saving the new patron, scroll down to Registration Date and
   see that it's defaulting to a date in the future.
4. Apply patch and restart_all
5. Try duplicating a patron again
6. Registration Date should correctly set to today
Comment 14 ByWater Sandboxes 2023-07-27 13:58:43 UTC
Created attachment 153982 [details] [review]
Bug 34117: Remove side effect from get_expiry_date

If get_expiry_date is passed a DateTime object as a parameter,
it modifies and returns the original object. When memberentry.pl
prefills the input fields for duplicating a patron, it passes the
enrollment date object to get_expiry_date. This causes the enrollment
date object to be modified with the expiry date value.

This patch modifies get_expiry_date to clone the DateTime object that it
receives as a parameter and return the clone, so that references to an
enrollment date object can be passed in safely.

To test:
1. Have or make a patron
2. Duplicate that patron
3. Before saving the new patron, scroll down to Registration Date and
   see that it's defaulting to a date in the future.
4. Apply patch and restart_all
5. Try duplicating a patron again
6. Registration Date should correctly set to today

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 15 Lucas Gass 2023-07-27 14:41:44 UTC
Created attachment 153991 [details] [review]
Bug 34117: Remove side effect from get_expiry_date

If get_expiry_date is passed a DateTime object as a parameter,
it modifies and returns the original object. When memberentry.pl
prefills the input fields for duplicating a patron, it passes the
enrollment date object to get_expiry_date. This causes the enrollment
date object to be modified with the expiry date value.

This patch modifies get_expiry_date to clone the DateTime object that it
receives as a parameter and return the clone, so that references to an
enrollment date object can be passed in safely.

To test:
1. Have or make a patron
2. Duplicate that patron
3. Before saving the new patron, scroll down to Registration Date and
   see that it's defaulting to a date in the future.
4. Apply patch and restart_all
5. Try duplicating a patron again
6. Registration Date should correctly set to today

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 16 Emily Lamancusa 2023-07-27 14:48:21 UTC
Created attachment 153992 [details] [review]
Bug 34117: Add unit tests

To test:
prove t/db_dependent/Koha/Patron/Categories.t
Comment 17 Emily Lamancusa 2023-07-27 14:49:20 UTC
Y'all are too fast! :D

Added unit tests - setting back to signed off.
Comment 18 Katrin Fischer 2023-07-29 19:41:31 UTC
Created attachment 154061 [details] [review]
Bug 34117: Remove side effect from get_expiry_date

If get_expiry_date is passed a DateTime object as a parameter,
it modifies and returns the original object. When memberentry.pl
prefills the input fields for duplicating a patron, it passes the
enrollment date object to get_expiry_date. This causes the enrollment
date object to be modified with the expiry date value.

This patch modifies get_expiry_date to clone the DateTime object that it
receives as a parameter and return the clone, so that references to an
enrollment date object can be passed in safely.

To test:
1. Have or make a patron
2. Duplicate that patron
3. Before saving the new patron, scroll down to Registration Date and
   see that it's defaulting to a date in the future.
4. Apply patch and restart_all
5. Try duplicating a patron again
6. Registration Date should correctly set to today

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 19 Katrin Fischer 2023-07-29 19:41:35 UTC
Created attachment 154062 [details] [review]
Bug 34117: Add unit tests

To test:
prove t/db_dependent/Koha/Patron/Categories.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 20 Katrin Fischer 2023-07-29 19:42:54 UTC
I perltidied the unit test file a little to make the QA test tools green. Emily: remember to run "qa" on your branch before submitting and have a look at how to use the our new perltidy file with your editor of choice (https://wiki.koha-community.org/wiki/Perltidy) :)
Comment 21 Emily Lamancusa 2023-08-09 13:14:23 UTC
(In reply to Katrin Fischer from comment #20)
> I perltidied the unit test file a little to make the QA test tools green.
> Emily: remember to run "qa" on your branch before submitting and have a look
> at how to use the our new perltidy file with your editor of choice
> (https://wiki.koha-community.org/wiki/Perltidy) :)

Thanks for the reminder! :)
Comment 22 Tomás Cohen Arazi 2023-08-15 08:24:39 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 23 Fridolin Somers 2023-08-17 19:07:36 UTC
Pushed to 23.05.x for 23.05.03
Comment 24 Pedro Amorim 2023-08-18 13:46:23 UTC
Nice work everyone!

Pushed to 22.11.x for next release