Bug 7641 - Add ability to suspend reserves.
Summary: Add ability to suspend reserves.
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: 3.8
Hardware: All All
: P5 - low critical (vote)
Assignee: Kyle M Hall
QA Contact: Ian Walls
URL:
Keywords:
: 7946 (view as bug list)
Depends on:
Blocks: 7711 5609 7951 8084
  Show dependency treegraph
 
Reported: 2012-03-02 18:55 UTC by Kyle M Hall
Modified: 2019-06-27 09:24 UTC (History)
9 users (show)

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


Attachments
Suspend Reserves (38.00 KB, patch)
2012-03-02 19:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7641: Suspend Reserves (38.49 KB, patch)
2012-03-02 19:51 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 7641 - Add ability to suspend reserves - Followup - Switch from C4::Dates to $KohaDates (3.66 KB, patch)
2012-03-29 15:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7641 - Add ability to suspend reserves - Followup - Switch from C4::Dates to $KohaDates (3.66 KB, patch)
2012-03-29 15:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7641 - Add ability to suspend reserves - Followup - Switch from C4::Dates to $KohaDates (3.66 KB, patch)
2012-03-29 15:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
screenshot (22.55 KB, image/png)
2012-04-03 06:22 UTC, Katrin Fischer
Details
Bug 7641 - Add ability to suspend reserves - Followup (6.21 KB, patch)
2012-04-04 17:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7641 - Followup - Suspend Until not set on by suspend button. (6.44 KB, patch)
2012-04-13 14:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7641 - Followup - Suspend Until not set on by suspend button. (6.96 KB, patch)
2012-04-13 16:31 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 7641 - Followup - Suspend Until not set on by suspend button. (6.33 KB, patch)
2012-04-17 12:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7641 - Followup - Suspend Until not set on by suspend button. (6.40 KB, patch)
2012-05-07 18:45 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 7641 - Followup - Suspend Until not set on by suspend button. (6.91 KB, patch)
2012-05-07 19:25 UTC, Liz Rea
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 2012-03-02 18:55:56 UTC
Suspension of holds

Users should be able to place a hold 'on suspension'.  This means the hold will preserve it's place in the queue, but it will be skipped over to the next non-suspended hold when the item becomes available.  Upon un-suspension, the hold will resume it's original place in the queue.

There shall be a button to suspend all holds for a user at once, both in the staff client and the OPAC (if OPAC holds management is enabled)

Auto unsuspend holds by date

For users who know the date when they will be able to receive holds again, there shall be an available box to specify this date at suspension time.  When that date rolls around, the hold will be automatically resumed.  This box shall also be available when suspending multiple holds at once.  This box shall be available in the staff client, and in the OPAC (if OPAC holds management is enabled).

The use of this features shall be toggleable by a system preference.
Comment 1 Kyle M Hall 2012-03-02 19:00:52 UTC Comment hidden (obsolete)
Comment 2 Nicole C. Engard 2012-03-02 19:51:37 UTC Comment hidden (obsolete)
Comment 3 Nicole C. Engard 2012-03-02 19:55:01 UTC
I forgot to mention that I did test it from the patron record, from the bib record, and from the opac
Comment 4 Paul Poulain 2012-03-28 14:42:40 UTC
This patch is marked as blocked by 7711, that has a "NEW" status. Does it mean it should not be pushed before something is submitted for 7711 ? Why has it been signed-off in this case ?

I'm OK to QA it, but I need some enlightment ;-)
Comment 5 Kyle M Hall 2012-03-28 14:52:47 UTC
I think this is backwards. I think 7711 is blocked by 7641, rather than the other way around. I'll fix that.

(In reply to comment #4)
> This patch is marked as blocked by 7711, that has a "NEW" status. Does it
> mean it should not be pushed before something is submitted for 7711 ? Why
> has it been signed-off in this case ?
> 
> I'm OK to QA it, but I need some enlightment ;-)
Comment 6 Paul Poulain 2012-03-29 13:04:41 UTC
tests  comments:
* I tested many cases, could not find any problem
* a possible enhancement could be for a patron to suspend holds one by one from the opac-user page. The common use-case is a patron that is unavailable for a long trip, another is "i placed a hold on volume 1 and 2, but want to suspend volume 2 until I have volume 1 in my hands". but that could be a future ENH

QA comments:
* There are 2 unconditional warnings in the logs:
[Thu Mar 29 14:46:24 2012] [error] [client 127.0.0.1] [Thu Mar 29 14:46:24 2012] opac-modrequest-suspend.pl: C4::Reserves::SuspendAll( borrowernumber => 7852, biblionumber => , suspend_until => , suspend => 0 ) at /home/paul/koha.dev/koha-community/C4/Reserves.pm line 1562., referer: http://lecannet/cgi-bin/koha/opac-user.pl
I've removed them in a follow-up

* not worth failing QA because the rule is still at early stage, but a follow-up is requested for :
+    my $query = "UPDATE reserves SET suspend = 0, suspend_until = NULL WHERE DATE( suspend_until ) < DATE( CURDATE() )"; CURDATE is a mysqlism (see http://wiki.koha-community.org/wiki/PostgreSQL => CAST(now as date)

* MARC BALMER (our mysqlism specialist :-) ) putting you in the loop: this patch contains 
+        "UPDATE reserves SET suspend = NOT suspend,
+        suspend_until = CASE WHEN suspend = 0 THEN NULL ELSE suspend_until END
=> is it a mysqlism or no ? If yes, add it to your wiki page

* prove translatable_templates and valid_templates are OK

* misc/cronjobs/holds/auto_unsuspend_holds.pl had use strict/use warning commented, I've uncommented them in a follow-up, works fine
* opac/opac-modrequest-suspend.pl and reserve/modrequest_suspendall.pl passes perlcritic

* the 
+            <td>[% IF ( reservloo.suspend ) %]Suspended [% IF ( reservloo.suspend_until ) %] until [% reservloo.suspend_until %][% END %][% END %]</td>
should use kohaDates, not
+        $getreserv{suspend_until} = C4::Dates->new( $num_res->{'suspend_until'}, "iso")->output("syspref");;

I'll pass QA & push because everything is OK except thig kohaDates display, but please provide a follow-up ASAP
Comment 7 Marc Balmer 2012-03-29 13:09:55 UTC
PostgreSQL has the CASE statement as well.  I will check if it is compatible with the standard and MySQL.  For now, I say treat CASE as non mysqlism, I took a note to recheck this file in case it is one
Comment 8 Paul Poulain 2012-03-29 13:11:38 UTC
patch pushed.

(In reply to comment #7)
> PostgreSQL has the CASE statement as well.  I will check if it is compatible
> with the standard and MySQL.  For now, I say treat CASE as non mysqlism, I
> took a note to recheck this file in case it is one

thx marc for the very quick reply !
Comment 9 Kyle M Hall 2012-03-29 15:50:47 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2012-03-29 15:51:52 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2012-03-29 15:52:51 UTC Comment hidden (obsolete)
Comment 12 Katrin Fischer 2012-04-03 06:22:42 UTC
Created attachment 8795 [details]
screenshot

I am not so happy with the display here, especially the last button:
'Resume All Suspended All Requests'
Also the new buttons don't follow the new capitalization rules :(
Comment 13 Kyle M Hall 2012-04-04 17:43:21 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2012-04-04 17:45:12 UTC
(In reply to comment #12)

Wow, how did that slip by! Also, I was just following the capitalization of the existing button. The new followup addresses these issues.

> Created attachment 8795 [details]
> screenshot
> 
> I am not so happy with the display here, especially the last button:
> 'Resume All Suspended All Requests'
> Also the new buttons don't follow the new capitalization rules :(
Comment 15 Ian Walls 2012-04-04 17:47:14 UTC
Followup is mostly template changes to wording, with an implementation of KohaDates.  Marking as Signed Off and Passed QA in one go.
Comment 16 Paul Poulain 2012-04-06 15:24:02 UTC
the follow-up has been pushed on master
(not on new/bug_7641, for an unknown reason, I can't push it on this branch)
Comment 17 Greg 2012-04-11 21:39:17 UTC
Have begun testing the suspend holds on our test Koha server.

After creating and suspending a hold on 2 different patron records, can no longer access those patron records.

The patrons affected by this so far are ADKINS, DORIS J (22003000177152) and STANCLIFF, JUNE (22003000000073).

Instead we get this error message:

Software error:

Template process failed: undef error - : filter not found at /home/koha/kohaclone/C4/Templates.pm line 127.

For help, please send mail to the webmaster (webmaster@kwala), giving this error message and the time and date of the error. 

June Stancliff
Comment 18 Kyle M Hall 2012-04-12 16:12:44 UTC
I am unable to recreate your error. Perhaps it involves the $KohaDates filter somehow?
Comment 19 Liz Rea 2012-04-12 18:45:51 UTC
Kyle - 

This happens after you try to "Suspend all holds"

I was able to recreate with "suspend all"
Comment 20 Owen Leonard 2012-04-12 18:46:21 UTC
(In reply to comment #0)

> The use of this features shall be toggleable by a system preference.

Looks like this is missing.
Comment 21 Katrin Fischer 2012-04-12 18:53:20 UTC
It should be possible to turn this feature off by a system preference like it is possible for other hold related features. I filed a separate bug - bug 7951.
Comment 22 Chris Cormack 2012-04-12 19:00:15 UTC
The 500 error will need to be fixed before release
Comment 23 Owen Leonard 2012-04-12 19:02:08 UTC
Should I be able to see the date until which the hold is suspended? If I suspend a hold from request.pl and specify a date I don't see it when request.pl reloads and I don't see it on the holds tab of moremember.pl or circulation.pl.

If I "suspend all" with a date from moremember.pl I do see a date in the form field on request.pl, so I assume this is the correct behavior.

I think this feature could have used some more testing.
Comment 24 Kyle M Hall 2012-04-13 13:16:30 UTC
(In reply to comment #22)
> The 500 error will need to be fixed before release

The problem appears to be that the KohaDates plugin is not being included in moremember.pl and circulation.pl. I tested this be removing the filter on the suspend_until date and adding it to a different date outside that code. I still got the error.

Chris, it appears that you wrote the KohaDates plugin. Do you have any idea why the plugin wouldn't be included?
Comment 25 Kyle M Hall 2012-04-13 14:45:54 UTC Comment hidden (obsolete)
Comment 26 Liz Rea 2012-04-13 16:31:31 UTC Comment hidden (obsolete)
Comment 27 Liz Rea 2012-04-13 16:32:28 UTC
Also tested that the cronjob unsuspends the holds when the suspension date has passed.
Comment 28 Katrin Fischer 2012-04-14 09:59:31 UTC
*** Bug 7946 has been marked as a duplicate of this bug. ***
Comment 29 Ian Walls 2012-04-16 14:26:23 UTC
Adds unconditional warn to ModReserve.

ToggleSuspend uses C4::Dates instead of Koha::Calendar and DateTime; we're moving away from C4::Dates, so best not to introduce new usage.

Marking Failed QA until these issues are resolved.
Comment 30 Kyle M Hall 2012-04-17 12:27:17 UTC Comment hidden (obsolete)
Comment 31 Kyle M Hall 2012-04-17 12:31:10 UTC
--- NOTE: clicking suspend all holds without setting a date will mean the holds must be manually unsuspended. I'm not sure this is intentional?

Yes, this is intentional. In fact, the ability to set a date can be turned off entirely, using the system preference AutoResumeSuspendedHolds.
Comment 32 Nicole C. Engard 2012-05-07 18:45:32 UTC Comment hidden (obsolete)
Comment 33 Liz Rea 2012-05-07 19:25:31 UTC Comment hidden (obsolete)
Comment 34 Ian Walls 2012-05-10 15:33:32 UTC
Adds more consideration for NULL suspend_untils, and formats them in ISO with Koha::DateUtils::output_pref.  Modifies display layer to use KohaDates, including in passed value in form.  Looks like all the necessary input points are properly scrubbed for this formatting, so it should be fine.

Marking Passed QA.
Comment 35 Paul Poulain 2012-05-14 12:26:25 UTC
The problem with follow-up that are submitted after a feature has been pushed, is that:
* the bugzilla entry becomes looonnggg and very hard to read and understand.
* I can't push this follow-up as usual.

Here is what i usually do:
* git checkout new/bug_7641
* git bz apply 7641
* testing
* pushing on the branch
* git checkout master
* git merge the branch into master

Unfortunately, for this follow-up, I can't do that, because some other changes in the code make it valid only for branch master.

So I could only push it directly on master, which would be confusing a lot.

That's why I'd preffered, from far, a separate bug entry, with it's own number.

Could you please file a new entry, with a clean description, and the patch. You can also link the new bug to this one and switch this one to pushed to master and the follow-up passed QA.

thanks
Comment 36 Jared Camins-Esakov 2012-12-31 00:58:39 UTC
There have been no further reports of problems so I am marking this bug resolved.