I am not seeing this error. When I click Suspend, this is the entry that gets inserted into the action log: $VAR1 = {'waitingdate' => undef,'itemnumber' => undef,'timestamp' => '2017-04-18 23:54:43','notificationdate' => undef,'priority' => '1','biblionumber' => '21','expirationdate' => undef,'cancellationdate' => undef,'reminderdate' => undef,'found' => undef,'reservedate' => '2017-04-18','suspend_until' => bless( {'locale' => bless( {'default_time_format_length' => 'medium','en_complete_name' => 'English United States','native_territory' => 'United States','default_date_format_length' => 'medium','native_language' => 'English','en_territory' => 'United States','native_complete_name' => 'English United States','en_language' => 'English','id' => 'en_US'}, 'DateTime::Locale::en_US' ),'tz' => bless( {'name' => 'UTC'}, 'DateTime::TimeZone::UTC' ),'utc_rd_days' => 736440,'utc_rd_secs' => 0,'local_rd_secs' => 0,'offset_modifier' => 0,'local_rd_days' => 736440,'rd_nanosecs' => 0,'local_c' => {'year' => 2017,'month' => 4,'day' => 21,'day_of_quarter' => 21,'quarter' => 2,'hour' => 0,'minute' => 0,'day_of_year' => 111,'day_of_week' => 5,'second' => 0},'formatter' => undef,'utc_year' => 2018}, 'DateTime' ),'lowestPriority' => '0','branchcode' => 'CPL','suspend' => 1,'reserve_id' => '232','reservenotes' => '','itemtype' => undef,'borrowernumber' => '51'}; It's a little long, sure, but it doesn't separate for each new line like in your attachment. (In reply to Aleisha Amohia from comment #1) > I am not seeing this error. > When I click Suspend, this is the entry that gets inserted into the action > log: > > $VAR1 = {'waitingdate' => undef,'itemnumber' => undef,'timestamp' => > '2017-04-18 23:54:43','notificationdate' => undef,'priority' => > '1','biblionumber' => '21','expirationdate' => undef,'cancellationdate' => > undef,'reminderdate' => undef,'found' => undef,'reservedate' => > '2017-04-18','suspend_until' => bless( {'locale' => bless( > {'default_time_format_length' => 'medium','en_complete_name' => 'English > United States','native_territory' => 'United > States','default_date_format_length' => 'medium','native_language' => > 'English','en_territory' => 'United States','native_complete_name' => > 'English United States','en_language' => 'English','id' => 'en_US'}, > 'DateTime::Locale::en_US' ),'tz' => bless( {'name' => 'UTC'}, > 'DateTime::TimeZone::UTC' ),'utc_rd_days' => 736440,'utc_rd_secs' => > 0,'local_rd_secs' => 0,'offset_modifier' => 0,'local_rd_days' => > 736440,'rd_nanosecs' => 0,'local_c' => {'year' => 2017,'month' => 4,'day' => > 21,'day_of_quarter' => 21,'quarter' => 2,'hour' => 0,'minute' => > 0,'day_of_year' => 111,'day_of_week' => 5,'second' => 0},'formatter' => > undef,'utc_year' => 2018}, 'DateTime' ),'lowestPriority' => '0','branchcode' > => 'CPL','suspend' => 1,'reserve_id' => '232','reservenotes' => > '','itemtype' => undef,'borrowernumber' => '51'}; > > It's a little long, sure, but it doesn't separate for each new line like in > your attachment. My formatting came from the fact that I queried action_logs via the command line, using mysql's '\G' query terminator, which puts in the line breaks. Apparently your 'locale' variable is fairly compact... mine isn't. In any case 'suspend_until' is still refers to a datetime object which is dumped in its entirity, that *may* get huge, and none of it is information that the user will need or want. I agree it's a bit much and possibly hard to understand for people writing reports. Could we shorten the information to a normal timestamp like format? Created attachment 63962 [details] [review] Bug 18382 - action_logs entry for module HOLDS, action SUSPEND is spammy When a hold is suspended, 'suspend_until' is represented as a DateTime object, complete with locale. All told, this is about 800 lines of text. Test Plan: 1) Enable the HoldsLog syspref 2) Add a hold on a record/item 3) Suspend the hold with a date to resume 4) Note the massive amount of date in the suspend_until field 5) Apply this patch 6) Suspend another hold with a date to resume 7) Note the log has an acutal date in the suspend_until field Check the logs using module 'Holds' and Action 'Suspend' With this patch you can no longer suspend without a date set. Internal server error: Can't call method "ymd" on an undefined value at /home/vagrant/kohaclone/Koha/Hold.pm line 65. Created attachment 68281 [details] [review] Bug 18382: [FOLLOW-UP] Ensure user can still enter an empty date Follow above test plan Also check that you can suspend with no date Created attachment 68363 [details] [review] Bug 18382 - action_logs entry for module HOLDS, action SUSPEND is spammy When a hold is suspended, 'suspend_until' is represented as a DateTime object, complete with locale. All told, this is about 800 lines of text. Test Plan: 1) Enable the HoldsLog syspref 2) Add a hold on a record/item 3) Suspend the hold with a date to resume 4) Note the massive amount of date in the suspend_until field 5) Apply this patch 6) Suspend another hold with a date to resume 7) Note the log has an acutal date in the suspend_until field Check the logs using module 'Holds' and Action 'Suspend' Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 68364 [details] [review] Bug 18382: [FOLLOW-UP] Ensure user can still enter an empty date Follow above test plan Also check that you can suspend with no date Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Your solution is not limited to action_logs. You truncate, so to speak, already at the holds level. Please add your fix at the hold unblessed level when sending it to the logs. Squash the patches too. Thanks. Created attachment 72610 [details] [review] Bug 18382: action_logs entry for module HOLDS, action SUSPEND is spammy When a hold is suspended, 'suspend_until' is represented as a DateTime object, complete with locale. All told, this is about 800 lines of text. Test Plan: 1) Enable the HoldsLog syspref 2) Add a hold on a record/item 3) Suspend the hold with a date to resume 4) Note the massive amount of date in the suspend_until field 5) Apply this patch 6) Suspend another hold with a date to resume 7) Note the log has an acutal date in the suspend_until field Check the logs using module 'Holds' and Action 'Suspend' Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> (In reply to Marcel de Rooy from comment #9) > Your solution is not limited to action_logs. > You truncate, so to speak, already at the holds level. > Please add your fix at the hold unblessed level when sending it to the logs. > Squash the patches too. > Thanks. I'm not sure I understand your request. Can you give me a bit more detail? Thanks! (In reply to Kyle M Hall from comment #11) > (In reply to Marcel de Rooy from comment #9) > > Your solution is not limited to action_logs. > > You truncate, so to speak, already at the holds level. > > Please add your fix at the hold unblessed level when sending it to the logs. > > Squash the patches too. > > Thanks. > > I'm not sure I understand your request. Can you give me a bit more detail? > Thanks! Yes, it was rather cryptic ;) I overlooked the truncate statement in the start of the sub. So just ignore it. Created attachment 73149 [details] [review] Bug 18382: action_logs entry for module HOLDS, action SUSPEND is spammy When a hold is suspended, 'suspend_until' is represented as a DateTime object, complete with locale. All told, this is about 800 lines of text. Test Plan: 1) Enable the HoldsLog syspref 2) Add a hold on a record/item 3) Suspend the hold with a date to resume 4) Note the massive amount of date in the suspend_until field 5) Apply this patch 6) Suspend another hold with a date to resume 7) Note the log has an acutal date in the suspend_until field Check the logs using module 'Holds' and Action 'Suspend' Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 73150 [details] [review] Bug 18382: (QA follow-up) Replace ->ymd by ->datetime The first patch makes Hold.t fail on: not ok 8 - Hold is suspended with a date, truncation takes place automatically Failed test 'Hold is suspended with a date, truncation takes place automatically' at Hold.t line 94. got: '2018-03-22' expected: '2018-03-22T00:00:00' We could remove the midnight time there, but we also could replace ymd by datetime. The cpan doc tells us: Same as $dt->ymd('-') . 'T' . $dt->hms(':') Test plan: With this patch, run Hold.t again. It should pass now. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> The change reads wrong, please adjust. 94 if ( defined $dt ){ 95 $self->suspend_until( $dt->datetime ); 96 } else { 97 $self->suspend_until( $dt ); 98 } I think we should clean the existing dirty entries: select from action_logs where suspend_until, dump the structure, modify the date and update it. (In reply to Jonathan Druart from comment #15) > The change reads wrong, please adjust. > > 94 if ( defined $dt ){ > 95 $self->suspend_until( $dt->datetime ); > 96 } else { > 97 $self->suspend_until( $dt ); > 98 } Can you explain what you mean by it "reads wrong"? Thanks! Created attachment 73317 [details] [review] Bug 18382 [QA Followup]: Don't set suspend_until if dt if undefined Kyle, I was not expecting your follow-up patch to work, I was expecting that ->suspend_hold would not update suspend_until What is supposed to do: $hold->suspend_hold( $dt ); $hold->suspend_hold; ? I would expect suspend_until to be set to undef Created attachment 73343 [details] [review] Bug 18382: Add a test This is what I am expecting but I may be wrong. Created attachment 73420 [details] [review] Bug 18382 [QA Followup]: Make new unit test pass Pushed to master for 18.05, thanks to everybody involved! Enhancement, not backporting for 17.11.x |
Created attachment 61842 [details] Sample Action logs entry for suspend. When a hold is suspended, 'suspend_until' is represented as a DateTime object, complete with locale. All told, this is about 800 lines of text. See the attached 'action_logs.out' To replicate: To test: - Enable the HoldsLog syspref - Add a hold on a record/item - Click on the <Suspend> button Check the logs using module 'Holds' and Action 'Suspend'