|
Description
Kyle M Hall (khall)
2015-06-01 15:57:41 UTC
Created attachment 39750 [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 Created attachment 39751 [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 Created attachment 39752 [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 Created attachment 39753 [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 Created attachment 39754 [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 Created attachment 40024 [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 Created attachment 40030 [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 Created attachment 40031 [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 Created attachment 40034 [details] [review] [SIGNED-OFF] 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> Blocked by bug 13517 which is blocked by 13853. Kyle, I am afraid that there is a lack of tests for the 2 new methods added to Koha::Hold Created attachment 43525 [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> Created attachment 43526 [details] [review] Bug 14310 [QA Followup] - Add unit tests CONFLICT (content): Merge conflict in t/db_dependent/Hold.t Not getting any merge conflicts. Did you apply 13517 and 13918 first? (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 :) 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 ++++++++++++ (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! Created attachment 43830 [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> Created attachment 43831 [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> Created attachment 43832 [details] [review] Bug 14310 [QA Followup] - Add unit tests (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 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. Created attachment 43978 [details] [review] Bug 14310 [QA Followup] - Enable translations, set svc script to be executable 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. Created attachment 44203 [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> Created attachment 44204 [details] [review] Bug 14310 [QA Followup] - Add unit tests Created attachment 44205 [details] [review] Bug 14310 [QA Followup] - Enable translations, set svc script to be executable Created attachment 44206 [details] [review] Bug 14310 [QA Followup] - Deal with error conditions Created attachment 44207 [details] [review] Bug 14310 [QA Followup] - Deal with error conditions 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? Created attachment 46857 [details] [review] Bug 14310 [QA Followup] - Use output_with_http_headers, return 403s Created attachment 46858 [details] [review] Bug 14310 [QA Followup] - Adapt existing code to use new methods Created attachment 46859 [details] [review] Bug 14310 [QA Followup] - Use output_with_http_headers, return 403s Created attachment 46860 [details] [review] Bug 14310 [QA Followup] - Adapt existing code to use new methods Created attachment 46861 [details] [review] Bug 14310 [QA Followup] - Adapt existing code to use new methods Created attachment 46862 [details] [review] Bug 14310 [QA Followup] - Adapt existing code to use new methods 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. 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> Created attachment 46871 [details] [review] Bug 14310 [QA Followup] - Add unit tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> 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> Created attachment 46873 [details] [review] Bug 14310 [QA Followup] - Deal with error conditions Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> 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> 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> (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. Created attachment 46876 [details] [review] Bug 14310 [QA Follow] - Remove FIXME, add explanation comment instead Pushed to Master - Should be in the May 2016 release. Thanks! |