[Bug 34117] New: Duplicate patron sets dateenrolled incorrectly
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Bug ID: 34117 Summary: Duplicate patron sets dateenrolled incorrectly Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Patrons Assignee: koha-bugs@lists.koha-community.org Reporter: andrewfh@dubcolib.org QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 --- Comment #1 from Andrew Fuerste-Henry <andrewfh@dubcolib.org> --- I noticed this first in a 22.11 system and confirmed it on master. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 --- Comment #2 from Lucas Gass <lucas@bywatersolutions.com> --- Created attachment 152656 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152656&action=edit 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch CC| |lucas@bywatersolutions.com Assignee|koha-bugs@lists.koha-commun |lucas@bywatersolutions.com |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 ByWater Sandboxes <bws.sandboxes@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #152656|0 |1 is obsolete| | --- Comment #3 from ByWater Sandboxes <bws.sandboxes@gmail.com> --- Created attachment 152657 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152657&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Andrew Fuerste-Henry <andrewfh@dubcolib.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=33872 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Katie Bliss <kebliss@dmpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kebliss@dmpl.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Laura Escamilla <Laura.escamilla@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |catrina@bywatersolutions.co | |m --- Comment #4 from Laura Escamilla <Laura.escamilla@bywatersolutions.com> --- *** Bug 33872 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |emily.lamancusa@montgomeryc | |ountymd.gov Depends on| |30718 --- Comment #5 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- 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). Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30718 [Bug 30718] Use flatpickr's altInput option everywhere -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com --- Comment #6 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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}) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 --- Comment #7 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 --- Comment #8 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- (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} ); -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 --- Comment #9 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 --- Comment #10 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 --- Comment #11 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- 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?) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 --- Comment #12 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- If the change fixes the problem originally describe here, we should make the change here, and obsolete the existing patch. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|lucas@bywatersolutions.com |jonathan.druart+koha@gmail. | |com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #152657|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|jonathan.druart+koha@gmail. |emily.lamancusa@montgomeryc |com |ountymd.gov -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 --- Comment #13 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- Created attachment 153977 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153977&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff Patch complexity|Trivial patch |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 ByWater Sandboxes <bws.sandboxes@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153977|0 |1 is obsolete| | --- Comment #14 from ByWater Sandboxes <bws.sandboxes@gmail.com> --- Created attachment 153982 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153982&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Andrew Fuerste-Henry <andrewfh@dubcolib.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153982|0 |1 is obsolete| | --- Comment #15 from Lucas Gass <lucas@bywatersolutions.com> --- Created attachment 153991 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153991&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 --- Comment #16 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- Created attachment 153992 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153992&action=edit Bug 34117: Add unit tests To test: prove t/db_dependent/Koha/Patron/Categories.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Signed Off --- Comment #17 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- Y'all are too fast! :D Added unit tests - setting back to signed off. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |34435 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34435 [Bug 34435] get_password_expiry_date should not modify its parameter -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |katrin.fischer@bsz-bw.de |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153991|0 |1 is obsolete| | --- Comment #18 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 154061 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=154061&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153992|0 |1 is obsolete| | --- Comment #19 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 154062 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=154062&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 --- Comment #20 from Katrin Fischer <katrin.fischer@bsz-bw.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) :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 --- Comment #21 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- (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! :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Lisette Scheer <lisette.scheer@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lisette.scheer@bywatersolut | |ions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |23.11.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 --- Comment #22 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 23.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|23.11.00 |23.11.00,23.05.03 released in| | Status|Pushed to master |Pushed to stable CC| |fridolin.somers@biblibre.co | |m --- Comment #23 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 23.05.x for 23.05.03 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |Pushed to oldstable Version(s)|23.11.00,23.05.03 |23.11.00,23.05.03,22.11.09 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 --- Comment #24 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Nice work everyone! Pushed to 22.11.x for next release -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34117 Rebecca Coert <rcoert@arlingtonva.us> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |rcoert@arlingtonva.us -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org