Bug 14310 - Add ability to suspend and resume individual holds from the patron holds table
Summary: Add ability to suspend and resume individual holds from the patron holds table
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on: 13918
Blocks:
  Show dependency treegraph
 
Reported: 2015-06-01 15:57 UTC by Kyle M Hall
Modified: 2017-06-14 22:11 UTC (History)
8 users (show)

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


Attachments
Bug 14310 - Suspend and resume indvidual holds from patron holds table (12.55 KB, patch)
2015-06-01 16:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14310 - Suspend and resume indvidual holds from patron holds table (12.38 KB, patch)
2015-06-01 16:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14310 - Suspend and resume indvidual holds from patron holds table (12.37 KB, patch)
2015-06-01 16:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14310 - Suspend and resume indvidual holds from patron holds table (12.40 KB, patch)
2015-06-01 16:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14310 - Suspend and resume indvidual holds from patron holds table (12.40 KB, patch)
2015-06-01 16:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14310 - Suspend and resume indvidual holds from patron holds table (12.40 KB, patch)
2015-06-09 16:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14310 - Suspend and resume indvidual holds from patron holds table (12.52 KB, patch)
2015-06-09 16:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14310 - Suspend and resume indvidual holds from patron holds table (27.44 KB, patch)
2015-06-09 16:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 14310 - Suspend and resume indvidual holds from patron holds table (12.70 KB, patch)
2015-06-09 17:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14310 - Suspend and resume indvidual holds from patron holds table (8.86 KB, patch)
2015-10-16 15:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14310 [QA Followup] - Add unit tests (2.27 KB, patch)
2015-10-16 15:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14310 - Suspend and resume indvidual holds from patron holds table (12.34 KB, patch)
2015-10-23 13:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14310 - Suspend and resume indvidual holds from patron holds table (12.34 KB, patch)
2015-10-23 13:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14310 [QA Followup] - Add unit tests (2.27 KB, patch)
2015-10-23 13:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14310 [QA Followup] - Enable translations, set svc script to be executable (4.19 KB, patch)
2015-10-26 10:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14310 - Suspend and resume indvidual holds from patron holds table (12.34 KB, patch)
2015-10-30 14:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14310 [QA Followup] - Add unit tests (2.27 KB, patch)
2015-10-30 14:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14310 [QA Followup] - Enable translations, set svc script to be executable (4.19 KB, patch)
2015-10-30 14:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14310 [QA Followup] - Deal with error conditions (5.06 KB, patch)
2015-10-30 14:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14310 [QA Followup] - Deal with error conditions (4.89 KB, patch)
2015-10-30 14:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14310 [QA Followup] - Use output_with_http_headers, return 403s (4.07 KB, patch)
2016-01-19 12:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14310 [QA Followup] - Adapt existing code to use new methods (7.58 KB, patch)
2016-01-19 12:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14310 [QA Followup] - Use output_with_http_headers, return 403s (3.39 KB, patch)
2016-01-19 12:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14310 [QA Followup] - Adapt existing code to use new methods (7.58 KB, patch)
2016-01-19 12:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14310 [QA Followup] - Adapt existing code to use new methods (7.89 KB, patch)
2016-01-19 12:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14310 [QA Followup] - Adapt existing code to use new methods (8.70 KB, patch)
2016-01-19 12:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14310 - Suspend and resume indvidual holds from patron holds table (12.40 KB, patch)
2016-01-19 13:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14310 [QA Followup] - Add unit tests (2.34 KB, patch)
2016-01-19 13:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14310 [QA Followup] - Enable translations, set svc script to be executable (4.37 KB, patch)
2016-01-19 13:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14310 [QA Followup] - Deal with error conditions (4.96 KB, patch)
2016-01-19 13:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14310 [QA Followup] - Use output_with_http_headers, return 403s (3.46 KB, patch)
2016-01-19 13:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14310 [QA Followup] - Adapt existing code to use new methods (8.77 KB, patch)
2016-01-19 13:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14310 [QA Follow] - Remove FIXME, add explanation comment instead (1.03 KB, patch)
2016-01-19 14:16 UTC, Kyle M Hall
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 2015-06-01 15:57:41 UTC

    
Comment 1 Kyle M Hall 2015-06-01 16:02:58 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2015-06-01 16:04:45 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2015-06-01 16:07:59 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2015-06-01 16:15:04 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2015-06-01 16:17:19 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2015-06-09 16:33:21 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2015-06-09 16:55:23 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2015-06-09 16:59:22 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2015-06-09 17:33:50 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2015-06-23 09:45:00 UTC
Blocked by bug 13517 which is blocked by 13853.
Comment 11 Jonathan Druart 2015-08-05 10:44:50 UTC
Kyle, I am afraid that there is a lack of tests for the 2 new methods added to Koha::Hold
Comment 12 Kyle M Hall 2015-10-16 15:04:18 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2015-10-16 15:04:25 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2015-10-22 09:36:48 UTC
CONFLICT (content): Merge conflict in t/db_dependent/Hold.t
Comment 15 Kyle M Hall 2015-10-22 13:23:52 UTC
Not getting any merge conflicts. Did you apply 13517 and 13918 first?
Comment 16 Jonathan Druart 2015-10-23 09:32:30 UTC
(In reply to Kyle M Hall from comment #15)
> Not getting any merge conflicts. Did you apply 13517 and 13918 first?

Oops, no, sorry about that. I thought they were already pushed.

1/ It seems that you forgot to include some files to this patch, please check.

2/ $hold->suspend_hold
sounds a bit redundant :)
Comment 17 Jonathan Druart 2015-10-23 09:34:28 UTC
Looking at the obsolete patches, the 2 following files don't exist in the last patch set:
 svc/hold/resume                                    |   48 +++++++++++
 svc/hold/suspend                                   |   51 ++++++++++++
Comment 18 Kyle M Hall 2015-10-23 13:34:42 UTC
(In reply to Jonathan Druart from comment #17)
> Looking at the obsolete patches, the 2 following files don't exist in the
> last patch set:
>  svc/hold/resume                                    |   48 +++++++++++
>  svc/hold/suspend                                   |   51 ++++++++++++

Scripts got lost in last rebase. Will fix!
Comment 19 Kyle M Hall 2015-10-23 13:35:18 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2015-10-23 13:45:25 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall 2015-10-23 13:45:32 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2015-10-23 14:16:18 UTC
(In reply to Jonathan Druart from comment #16)
> (In reply to Kyle M Hall from comment #15)
> > Not getting any merge conflicts. Did you apply 13517 and 13918 first?
> 
> Oops, no, sorry about that. I thought they were already pushed.
> 
> 1/ It seems that you forgot to include some files to this patch, please
> check.

Fixed!

> 2/ $hold->suspend_hold
> sounds a bit redundant :)

Agreed, but the column is already named 'suspend' so I didn't want to conflict with that.
Comment 23 Katrin Fischer 2015-10-25 14:44:14 UTC
Comment on attachment 43831 [details] [review]
Bug 14310 - Suspend and resume indvidual holds from patron holds table

Review of attachment 43831 [details] [review]:
-----------------------------------------------------------------

Hi Kyle,

I have patches from the 2 dependent bugs and this one. QA script is clean, but code review and testing showed 2 big problems:

1) Suspending holds doesn't work for me, clicking on Suspend with or without a date - nothing happens.
Tested from /cgi-bin/koha/members/moremember.pl?borrowernumber=1
Firebug:
Post: reserve_id=407&suspend_until=31%2F10%2F2015
Response: <!-- TEMPLATE FILE: 500.tt --> error page.

2) Almost all the new strings are not translatable, because they are trapped inside the .js file.

::: koha-tmpl/intranet-tmpl/prog/en/js/holds.js
@@ +129,5 @@
> +                            if ( oObj.found ) {
> +                                return "";
> +                            } else if ( oObj.suspend == 1 ) {
> +                                return "<a class='hold-resume btn btn-link' id='resume" + oObj.reserve_id + "' style='display: inline; white-space: nowrap;'>"
> +                                     + "<i class='icon-play'></i> " + _("Resume") + "</a>";

.js files are not translatable - we need to move the strings to the templates or find another solution to this.

@@ +132,5 @@
> +                                return "<a class='hold-resume btn btn-link' id='resume" + oObj.reserve_id + "' style='display: inline; white-space: nowrap;'>"
> +                                     + "<i class='icon-play'></i> " + _("Resume") + "</a>";
> +                            } else {
> +                                return "<a class='hold-suspend btn btn-link' id='suspend" + oObj.reserve_id + "' style='display: inline; white-space: nowrap;'>"
> +                                     + "<i class='icon-pause'></i> " + _("Suspend") + "</a>";

Same as above, and many more below.
Comment 24 Kyle M Hall 2015-10-26 10:40:21 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2015-10-27 08:50:02 UTC
CONFLICT (content): Merge conflict in t/db_dependent/Hold.t


I think the possible errors should be handle. For instance: An hold suspended but has been deleted in the meantime, the suspend until date is not valid, etc.
Comment 26 Kyle M Hall 2015-10-30 14:34:29 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall 2015-10-30 14:34:37 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2015-10-30 14:34:40 UTC Comment hidden (obsolete)
Comment 29 Kyle M Hall 2015-10-30 14:34:43 UTC Comment hidden (obsolete)
Comment 30 Kyle M Hall 2015-10-30 14:35:23 UTC Comment hidden (obsolete)
Comment 31 Jonathan Druart 2016-01-14 14:16:36 UTC
QA comments:

1/ The correct mimetype for json is application/json (https://www.ietf.org/rfc/rfc4627.txt).
Maybe it would be easier to use C4::Output::output_with_http_headers

2/
+if ( $auth_status ne "ok" ) {
+    exit 0;
+}

It might make more sense to return a 403.

3/ I am a bit worried to add different ways to suspend/resume hold, the others (in C4::Reserves) are mainly used and the behaviors could differ. Does the plan is to adapt the existing code to use these new methods?
Comment 32 Kyle M Hall 2016-01-19 12:18:32 UTC Comment hidden (obsolete)
Comment 33 Kyle M Hall 2016-01-19 12:18:38 UTC Comment hidden (obsolete)
Comment 34 Kyle M Hall 2016-01-19 12:23:11 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2016-01-19 12:23:18 UTC Comment hidden (obsolete)
Comment 36 Kyle M Hall 2016-01-19 12:28:22 UTC Comment hidden (obsolete)
Comment 37 Kyle M Hall 2016-01-19 12:37:04 UTC Comment hidden (obsolete)
Comment 38 Jonathan Druart 2016-01-19 13:50:26 UTC
Comment on attachment 46862 [details] [review]
Bug 14310 [QA Followup] - Adapt existing code to use new methods

Review of attachment 46862 [details] [review]:
-----------------------------------------------------------------

::: C4/Reserves.pm
@@ +1178,4 @@
>              } else {
> +                # FIXME: Why are we doing this? Should this be resuming the hold,
> +                # or maybe suspending it indefinitely?
> +                $hold->set( { suspend_until => undef } )->store();

Indeed, this is weird.
If suspend_until is defined, it should be a date.
Comment 39 Jonathan Druart 2016-01-19 13:59:12 UTC
Created attachment 46870 [details] [review]
Bug 14310 - Suspend and resume indvidual holds from patron holds table

This enhancment adds the ability to suspend and resume individual holds
from the holds table on circulation.pl and moremember.pl.

The interface is inspired/cribbed from the same feature already
available in the opac.

Test Plan:
1) Apply this patch
2) Find a patron with holds
3) Suspend a hold with no resume date
4) Resume the suspended hold
5) Suspend a hold with a resume date
6) Resume the suspended hold

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Cathi Wiggins <CWIGGINS@ci.arcadia.ca.us>

Signed-off-by: Megan Wianecki <mwianecki@mplmain.mtpl.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 40 Jonathan Druart 2016-01-19 13:59:18 UTC
Created attachment 46871 [details] [review]
Bug 14310 [QA Followup] - Add unit tests

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 41 Jonathan Druart 2016-01-19 13:59:22 UTC
Created attachment 46872 [details] [review]
Bug 14310 [QA Followup] - Enable translations, set svc script to be executable

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 42 Jonathan Druart 2016-01-19 13:59:25 UTC
Created attachment 46873 [details] [review]
Bug 14310 [QA Followup] - Deal with error conditions

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 43 Jonathan Druart 2016-01-19 13:59:29 UTC
Created attachment 46874 [details] [review]
Bug 14310 [QA Followup] - Use output_with_http_headers, return 403s

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 44 Jonathan Druart 2016-01-19 13:59:32 UTC
Created attachment 46875 [details] [review]
Bug 14310 [QA Followup] - Adapt existing code to use new methods

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 45 Kyle M Hall 2016-01-19 14:07:00 UTC
(In reply to Jonathan Druart from comment #38)
> Comment on attachment 46862 [details] [review] [review]
> Bug 14310 [QA Followup] - Adapt existing code to use new methods
> 
> Review of attachment 46862 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/Reserves.pm
> @@ +1178,4 @@
> >              } else {
> > +                # FIXME: Why are we doing this? Should this be resuming the hold,
> > +                # or maybe suspending it indefinitely?
> > +                $hold->set( { suspend_until => undef } )->store();
> 
> Indeed, this is weird.
> If suspend_until is defined, it should be a date.

I've found the one and only path that executes this code. If you have a suspended hold with a resume date, and clear the resume date, then click "Update hold(s)" the action is to change the suspension from one with a resume date to an indefinite suspension! I'll update the FIXME to document this instead.
Comment 46 Kyle M Hall 2016-01-19 14:16:16 UTC
Created attachment 46876 [details] [review]
Bug 14310 [QA Follow] - Remove FIXME, add explanation comment instead
Comment 47 Brendan Gallagher 2016-01-27 06:21:54 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks!