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
I noticed this first in a 22.11 system and confirmed it on master.
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
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>
*** Bug 33872 has been marked as a duplicate of this bug. ***
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).
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})
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.
(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} );
(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)
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.
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?)
If the change fixes the problem originally describe here, we should make the change here, and obsolete the existing patch.
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
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>
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>
Created attachment 153992 [details] [review] Bug 34117: Add unit tests To test: prove t/db_dependent/Koha/Patron/Categories.t
Y'all are too fast! :D Added unit tests - setting back to signed off.
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>
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>
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) :)
(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! :)
Pushed to master for 23.11. Nice work everyone, thanks!
Pushed to 23.05.x for 23.05.03
Nice work everyone! Pushed to 22.11.x for next release