Bug 12876 - Reserve in waiting/transfer status may be cancelled by user
Summary: Reserve in waiting/transfer status may be cancelled by user
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Rafal Kopaczka
QA Contact: Testopia
URL:
Keywords:
Depends on: 12873
Blocks:
  Show dependency treegraph
 
Reported: 2014-09-05 09:28 UTC by Rafal Kopaczka
Modified: 2017-08-15 10:59 UTC (History)
4 users (show)

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


Attachments
[PATCH] Bug 12876 - Reserve in waiting/transfer status may be cancelled by user User may cancel his own reservation at waiting or in transit status through calling opac-modrequest.pl. Cancel button is disabled in interface but possibility to cancel should (5.32 KB, patch)
2014-09-05 12:59 UTC, Rafal Kopaczka
Details | Diff | Splinter Review
Bug 12876 - Reserve in waiting/transfer status may be cancelled by user (5.09 KB, patch)
2014-09-05 14:35 UTC, Rafal Kopaczka
Details | Diff | Splinter Review
0001-Bug-12876-Reserve-in-waiting-transfer-status-may-be-.patch (5.10 KB, patch)
2014-09-05 14:42 UTC, Rafal Kopaczka
Details | Diff | Splinter Review
Bug 12876 - Reserve in waiting/transfer status may be cancelled by user (5.14 KB, patch)
2014-09-05 15:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12876: Improve unit tests for CanReserveBeCanceledFromOpac (2.03 KB, patch)
2014-09-05 15:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 12876 - Reserve in waiting/transfer status may be cancelled by user (5.33 KB, patch)
2014-09-07 21:43 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 12876: Improve unit tests for CanReserveBeCanceledFromOpac (2.09 KB, patch)
2014-09-07 21:44 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 12876: (followup) remove useless diags (2.09 KB, patch)
2014-09-24 00:27 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Rafal Kopaczka 2014-09-05 09:28:28 UTC
User may cancel his own reservation at waiting or in transit status through calling opac-modrequest.pl. Cancel button is disabled in interface but possibility to cancel should be checked also in opac-moderequest.pl, before calling CancelReserve().
Comment 1 Rafal Kopaczka 2014-09-05 09:30:44 UTC
Damm, wrong number...
Comment 2 Rafal Kopaczka 2014-09-05 09:34:28 UTC
Moved to security since it depends on other bug from security.
Comment 3 Rafal Kopaczka 2014-09-05 12:59:46 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2014-09-05 14:10:03 UTC
Rafal,
Thank to provide a patch.
I have 3 remarks:
1/ Please have a look at the wiki patch for the commit message formatting: http://wiki.koha-community.org/wiki/Commit_messages

2/ I think the routine name is a little bit ambiguous. We could believe that your are checking the permission for a librarian.

3/ If I am not wrong (which is possible...), a reserve in transit have the found value set to 'T'. Easier to check than calling GetTransfers.
Comment 5 Rafal Kopaczka 2014-09-05 14:24:12 UTC
(In reply to Jonathan Druart from comment #4)
> Rafal,
> Thank to provide a patch.
> I have 3 remarks:
> 1/ Please have a look at the wiki patch for the commit message formatting:
> http://wiki.koha-community.org/wiki/Commit_messages
> 
> 2/ I think the routine name is a little bit ambiguous. We could believe that
> your are checking the permission for a librarian.
What name do you suggest then? CanReserveBeCanceledFromOpac ?

> 
> 3/ If I am not wrong (which is possible...), a reserve in transit have the
> found value set to 'T'. Easier to check than calling GetTransfers.
I think you are true with this. I was suggested by opac-user.pl with checking for transfers. My library don't use transfers, so I'm not familiar with it at all. :)
Comment 6 Jonathan Druart 2014-09-05 14:26:49 UTC
(In reply to Rafal Kopaczka from comment #5)
> > 2/ I think the routine name is a little bit ambiguous. We could believe that
> > your are checking the permission for a librarian.
> What name do you suggest then? CanReserveBeCanceledFromOpac ?

Yep, certainly better :)
Comment 7 Rafal Kopaczka 2014-09-05 14:35:41 UTC Comment hidden (obsolete)
Comment 8 Rafal Kopaczka 2014-09-05 14:42:33 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2014-09-05 15:42:36 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2014-09-05 15:42:40 UTC Comment hidden (obsolete)
Comment 11 Katrin Fischer 2014-09-05 18:41:04 UTC
Sorry, I can't get this to apply:
Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y
Applying: Bug 12876 - Reserve in waiting/transfer status may be cancelled by user
fatal: sha1 information is lacking or useless (opac/opac-modrequest.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 12876 - Reserve in waiting/transfer status may be cancelled by user
The copy of the patch that failed is found in:
   /home/katrin/kohaclone/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 12 Katrin Fischer 2014-09-05 18:41:39 UTC
Missed the dependency :)
Comment 13 Katrin Fischer 2014-09-07 21:43:57 UTC
Created attachment 31440 [details] [review]
[PASSED QA] Bug 12876 - Reserve in waiting/transfer status may be cancelled by user

User may cancel his own reservation at waiting or in transit status
through calling opac-modrequest.pl. Cancel button is disabled in
interface but possibility to cancel should be checked also in
opac-moderequest.pl, before calling CancelReserve().
Similar situation is with opac-modrequest-suspend.pl

This patch provides new soubroutine to chceck if user can cancel given
reserve. It's possible only when he's owner of hold and hold isn't in
transfer or waiting status.

Additionaly there are new test for this function in Reserves.t

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests, QA script and new tests.
Works as described, tested with:
.../cgi-bin/koha/opac-modrequest.pl?reserve_id=XXX
Comment 14 Katrin Fischer 2014-09-07 21:44:12 UTC
Created attachment 31441 [details] [review]
[PASSED QA] Bug 12876: Improve unit tests for CanReserveBeCanceledFromOpac

This patch fix the subroutine name and add a restriction on the
arguments: both argument are mandatory!

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 15 Tomás Cohen Arazi (tcohen) 2014-09-24 00:27:34 UTC
Created attachment 31846 [details] [review]
Bug 12876: (followup) remove useless diags

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 16 Tomás Cohen Arazi (tcohen) 2014-09-24 00:30:24 UTC
Patches pushed to master.

Thanks Rafal!
Comment 17 Galen Charlton 2014-10-01 21:47:22 UTC
Pushed to 3.16.x for inclusion in 3.16.4.
Comment 18 Galen Charlton 2014-10-01 21:47:33 UTC
Pushed to 3.16.x for inclusion in 3.16.4.
Comment 19 Fridolin Somers 2014-10-02 12:10:21 UTC
Pushed to 3.14.x, will be in 3.14.11