Bug 8868 - ILS-DI: CancelHold needs to take a reserve_id
Summary: ILS-DI: CancelHold needs to take a reserve_id
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P3 normal (vote)
Assignee: Julian Maurice
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-03 11:47 UTC by Julian Maurice
Modified: 2015-06-04 23:33 UTC (History)
11 users (show)

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


Attachments
Bug 8868: ILS-DI: CancelHold needs to take a reserve_id (5.69 KB, patch)
2012-10-03 11:48 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8868: ILS-DI: CancelHold needs to take a reserve_id (5.81 KB, patch)
2013-03-19 08:06 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8868: ILS-DI: CancelHold needs to take a reserve_id (5.87 KB, patch)
2013-03-20 14:43 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 8868: ILS-DI: CancelHold needs to take a reserve_id (follow-up) (3.79 KB, patch)
2013-06-13 09:55 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8868: ILS-DI: CancelHold needs to take a reserve_id (5.59 KB, patch)
2013-09-23 08:24 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8868: ILS-DI: CancelHold needs to take a reserve_id (follow-up) (3.73 KB, patch)
2013-09-23 08:24 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8868: ILS-DI: CancelHold needs to take a reserve_id (5.61 KB, patch)
2013-11-05 14:34 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8868: ILS-DI: CancelHold needs to take a reserve_id (follow-up) (4.04 KB, patch)
2013-11-05 14:34 UTC, Julian Maurice
Details | Diff | Splinter Review
Rebased against master (4.22 KB, patch)
2014-01-30 11:23 UTC, Deichman Digital Development
Details | Diff | Splinter Review
Bug 8868: ILS-DI: CancelHold needs to take a reserve_id (follow-up) (4.82 KB, patch)
2014-03-12 09:56 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 8868: ILS-DI: CancelHold needs to take a reserve_id (follow-up) (6.66 KB, patch)
2014-03-13 13:17 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 8868: Remove CancelReserveFromId (5.13 KB, patch)
2014-03-14 09:53 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8868: Remove CancelReserveFromId (5.22 KB, patch)
2014-03-27 13:37 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 8868: ILS-DI: CancelHold needs to take a reserve_id (follow-up) (6.59 KB, patch)
2014-04-11 14:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8868: Remove CancelReserveFromId (5.27 KB, patch)
2014-04-11 14:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8868: ILS-DI: CancelHold needs to take a reserve_id (8.58 KB, patch)
2014-04-29 09:42 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8868: ILS-DI: CancelHold needs to take a reserve_id (7.63 KB, patch)
2014-05-05 12:11 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8868: ILS-DI: CancelHold needs to take a reserve_id (7.74 KB, patch)
2014-08-16 23:22 UTC, Chris Cormack
Details | Diff | Splinter Review
[PASSED QA] Bug 8868: ILS-DI: CancelHold needs to take a reserve_id (7.98 KB, patch)
2014-08-18 09:06 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8868: Follow-up: Update boostrap documentation page (1.37 KB, patch)
2014-08-18 09:07 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2012-10-03 11:47:37 UTC
CancelHold takes two parameters: patron_id and item_id.
If item_id is considered as an itemnumber, holds on title can't be canceled.
If item_id is considered as a biblionumber, all holds on this biblionumber
(for a borrower) will be canceled.

So CancelHold have to consider item_id as a reserve_id.
Comment 1 Julian Maurice 2012-10-03 11:48:49 UTC Comment hidden (obsolete)
Comment 2 Koha Team AMU 2013-03-18 14:15:33 UTC
Tried on a sandbox. Not possible :
Something went wrong !
Applying: Bug 8868: ILS-DI: CancelHold needs to take a reserve_id
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/ILSDI/Services.pm
CONFLICT (content): Merge conflict in C4/ILSDI/Services.pm
Auto-merging C4/Reserves.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 8868: ILS-DI: CancelHold needs to take a reserve_id
When you have resolved this problem run "git am --resolved".
Comment 3 Julian Maurice 2013-03-19 08:06:22 UTC Comment hidden (obsolete)
Comment 4 Paul Poulain 2013-03-20 14:42:56 UTC
Patch tested with a sandbox, by Leila and Sonia <koha.aixmarseille@gmail.com>
Comment 5 Paul Poulain 2013-03-20 14:43:13 UTC Comment hidden (obsolete)
Comment 6 Koha Team AMU 2013-03-20 14:45:10 UTC
Test ok.
Perhaps would it be better if the text of this page is changed :
http://catalogue.test3.biblibre.com/cgi-bin/koha/ilsdi.pl?service=Describe&verb=CancelHold

Now :
Paramètres
patron_id (Obligatoire)
    identifiant unique de l'adhérent; identifiant retourné par LookupPatron et AuthenticatePatron
item_id (Obligatoire)
    identifiant exemplaire

Exemple d'utilisation
ilsdi.pl?service=CancelHold&patron_id=1&item_id=1 

BUT :
no more item_id but with the patch reserve_id
Comment 7 Chris Cormack 2013-06-05 00:35:22 UTC
This seems to work well, 

1/ but could you please do a follow up fixing the text in the ilsdi.pl?service=Describe&verb=CancelHold page

2/ Also we introduce 2 new subroutines, so could we get a unit test for each one.

3/ Finally we do this

$query = qq{
            INSERT INTO old_reserves
            SELECT * FROM reserves
            WHERE reserve_id = ?
        };
        $sth = $dbh->prepare($query);
        $sth->execute($reserve_id);

        $query = qq{
            DELETE FROM reserves
            WHERE reserve_id = ?
        };
        $sth = $dbh->prepare($query);
        $sth->execute($reserve_id);

It would be safer/nicer to check the result of our execute into the old_reserves before we delete it from reserves, if that insert fails we do the delete anyway at the moment

Failing QA for 1 and 2, bonus points if you fix 3 also
Comment 8 Julian Maurice 2013-06-13 09:55:28 UTC Comment hidden (obsolete)
Comment 9 Julian Maurice 2013-06-13 09:56:08 UTC
1, 2, and 3 fixed ;)
Comment 10 I'm just a bot 2013-09-18 09:51:03 UTC
Applying: Bug 8868: ILS-DI: CancelHold needs to take a reserve_id
Using index info to reconstruct a base tree...
M	C4/ILSDI/Services.pm
M	C4/Reserves.pm
Falling back to patching base and 3-way merge...
Auto-merging C4/Reserves.pm
Auto-merging C4/ILSDI/Services.pm
CONFLICT (content): Merge conflict in C4/ILSDI/Services.pm
Patch failed at 0001 Bug 8868: ILS-DI: CancelHold needs to take a reserve_id
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.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 11 Julian Maurice 2013-09-23 08:24:25 UTC Comment hidden (obsolete)
Comment 12 Julian Maurice 2013-09-23 08:24:31 UTC Comment hidden (obsolete)
Comment 13 Julian Maurice 2013-09-23 08:24:46 UTC
All patches rebased on master
Comment 14 I'm just a bot 2013-09-23 22:06:08 UTC
Patch applied cleanly, go forth and signoff
Comment 15 I'm just a bot 2013-10-24 19:29:49 UTC
Applying: Bug 8868: ILS-DI: CancelHold needs to take a reserve_id (follow-up)
Using index info to reconstruct a base tree...
M	C4/Reserves.pm
M	t/db_dependent/Reserves.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Reserves.t
CONFLICT (content): Merge conflict in t/db_dependent/Reserves.t
Auto-merging C4/Reserves.pm
Patch failed at 0001 Bug 8868: ILS-DI: CancelHold needs to take a reserve_id (follow-up)
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.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 16 Julian Maurice 2013-11-05 14:34:29 UTC Comment hidden (obsolete)
Comment 17 Julian Maurice 2013-11-05 14:34:41 UTC Comment hidden (obsolete)
Comment 18 Deichman Digital Development 2014-01-30 11:23:29 UTC Comment hidden (obsolete)
Comment 19 Benjamin Rokseth 2014-01-30 11:45:49 UTC
ilsdi.pl?service=CancelHold&patron_id=114&item_id=47

("47" was the reserve_id of a hold)

Got the following error :

Can't use string ("1974687") as a HASH ref while "strict refs" in use at /usr/local/src/kohaclone/C4/Reserves.pm line 1724.

points to :
sub _FixPriority {
    my ( $params ) = @_;
    my $reserve_id = $params->{reserve_id};      <<<

1974687 was here the biblionumber, not the reserve_id.

The hold is correctly deleted, though.
Comment 20 Benjamin Rokseth 2014-03-12 09:56:41 UTC Comment hidden (obsolete)
Comment 21 Benjamin Rokseth 2014-03-13 13:17:23 UTC Comment hidden (obsolete)
Comment 22 Marcel de Rooy 2014-03-13 14:16:52 UTC
QA Comment:
The two routines CancelReserve and CancelReserveFromId really are twins :)
This seems not very necessary. Sorry but this is a blocker for me.

Also a more general question: Would there be a need for some kind of check if cancelling the hold via ILS-DI is allowed? Could someone misuse this to cancel reserves of someone else?

Failed QA
Comment 23 Julian Maurice 2014-03-14 09:53:03 UTC Comment hidden (obsolete)
Comment 24 Julian Maurice 2014-03-14 10:11:00 UTC
> Also a more general question: Would there be a need for some kind of check if
> cancelling the hold via ILS-DI is allowed? Could someone misuse this to
> cancel reserves of someone else?

This can certainly happen as Koha only uses IP address to trust the remote user, and IP address cannot guarantee user's identity (http://en.wikipedia.org/wiki/IP_address_spoofing).
But ILS-DI protocol doesn't provide any authentication mechanisms, so... what can we do?

Note: RenewLoan, HoldTitle and HoldItem also allows to modify database without authentication.
Comment 25 Benjamin Rokseth 2014-03-27 13:37:41 UTC Comment hidden (obsolete)
Comment 26 Benjamin Rokseth 2014-03-27 14:03:16 UTC
Might be the two patches should be squashed into one, but I leave it for QA to decide.

Regarding authentication, I personally would like to see a more united work on a RESTful API á la http://git.biblibre.com/?p=koha-restful, rather than the /svc which only addresses marc records and /ils-di which has some patron and holds calls.

Perhaps it could also implement a basic session authentication, api-key, or such.
Comment 27 Kyle M Hall 2014-04-11 14:06:02 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2014-04-11 14:06:35 UTC Comment hidden (obsolete)
Comment 29 Galen Charlton 2014-04-11 15:56:19 UTC
The second patch is weird -- it doesn't just remove CancelReserveFromId, it has the effect of *changing* CancelReserve:

- it adds a return that no callers actually check
- more importantly, it removes the code to insert the old hold into old_reserves, which is a pretty clear regression.

I'm marking failed QA on this basis.

I strongly encourage starting the patch over from scratch; a patch that adds a routine that then gets immediately removed by its follow-up is confusing, at the very least, and risks whatever merge reconciliation failure happened here.  In particular, I don't see that it is necessary to touch C4/Reserves.pm at all to implement this change; if there are concerns about CancelReserve's behavior, that should go into a separate bug.
Comment 30 Julian Maurice 2014-04-29 09:42:33 UTC Comment hidden (obsolete)
Comment 31 Julian Maurice 2014-04-29 09:46:13 UTC
Squashed all patches into one.

about CancelReserve changes
> - it adds a return that no callers actually check
It is used in tests

> more importantly, it removes the code to insert the old hold into
> old_reserves, which is a pretty clear regression.
Are you sure ? git show -b shows me that the patch only adds an if (and the return, of course)
Comment 32 Julian Maurice 2014-04-29 09:50:24 UTC
(In reply to Julian Maurice from comment #31) 
> > more importantly, it removes the code to insert the old hold into
> > old_reserves, which is a pretty clear regression.
> Are you sure ? git show -b shows me that the patch only adds an if (and the
> return, of course)
It appears you are right, in the previous patch there was some code missing in CancelReserve. I don't know what happened, but the last patch I just submitted is correct.
Comment 33 Marcel de Rooy 2014-05-01 06:50:37 UTC
QA: Looking at this one now..
Comment 34 Marcel de Rooy 2014-05-01 07:04:34 UTC
QA Comment:
You add a routine GetReserve to C4/Reserves.pm
This is used only in  C4/ILSDI/Services.pm. The returned hashref is used to check for the existence of the reserves record and to verify the borrowernumber.

It seems that you could use GetReserveInfo to do exactly the same job. This would prevent changing the Reserves module, the need of a unit test, etc. So less maintenance. The only very small overhead is the join of a few tables there; imo not too high.

Could you please adjust/simplify? Thanks.

Failed QA
Comment 35 Julian Maurice 2014-05-05 12:11:27 UTC Comment hidden (obsolete)
Comment 36 Julian Maurice 2014-05-05 12:23:10 UTC
GetReserve is already in master (added by bug 9394)
This patch was creating another GetReserve subroutine.
I only removed the GetReserve added by this patch (but still continue to use GetReserve in C4::ILSDI::Services)
Comment 37 Chris Cormack 2014-08-16 23:21:17 UTC
Comment on attachment 27947 [details] [review]
Bug 8868: ILS-DI: CancelHold needs to take a reserve_id

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

::: C4/ILSDI/Services.pm
@@ +26,4 @@
>  use C4::Branch;
>  use C4::Accounts;
>  use C4::Biblio;
> +use C4::Reserves qw(AddReserve GetReservesFromBiblionumber GetReservesFromBorrowernumber CanBookBeReserved CanItemBeReserved);

Why do we not add CancelReserve here

@@ +744,2 @@
>  
> +    C4::Reserves::CancelReserve({reserve_id => $reserve_id});

And instead do this?
Comment 38 Chris Cormack 2014-08-16 23:22:36 UTC Comment hidden (obsolete)
Comment 39 Katrin Fischer 2014-08-18 09:06:48 UTC
Created attachment 30908 [details] [review]
[PASSED QA] Bug 8868: ILS-DI: CancelHold needs to take a reserve_id

CancelHold takes two parameters: patron_id and item_id.
If item_id is considered as an itemnumber, holds on title can't be
canceled.
If item_id is considered as a biblionumber, all holds on this
biblionumber (for a borrower) will be canceled.

So CancelHold have to consider item_id as a reserve_id.

- Added subroutine C4::Reserves::GetReserve
- C4::ILSDI::Services::GetRecords now returns the reserve_id
- Fix the text in the ilsdi.pl?service=Describe&verb=CancelHold page
- Unit tests for CancelReserved and GetReserve
- Do not delete row in reserves table if insert in old_reserves fails

Signed-off-by: Leila and Sonia <koha.aixmarseille@gmail.com>
Signed-off-by: Benjamin Rokseth <bensinober@gmail.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signing off, while noting a style issue in the patch review

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes tests and QA script.
Placed and cancelled a hold using ILS-DI successfully.
Adding a follow-up to also update the ils-di documentation
page in the bootstrap theme.
Comment 40 Katrin Fischer 2014-08-18 09:07:08 UTC
Created attachment 30909 [details] [review]
Bug 8868: Follow-up: Update boostrap documentation page

Updates the bootstrap documentation page for CancelHold.
Comment 41 Katrin Fischer 2014-08-18 09:08:30 UTC
Note: this is a change in how the API currently works and people who are using it to cancel holds will need to change their programs. That said, I think it's still a good change - but maybe it should not go into lower versions and have a note in the release notes for 3.18?
Comment 42 Katrin Fischer 2014-08-18 09:20:37 UTC
Because it took me a bit to find it:
http://old.diglib.org/architectures/ilsdi/DLF_ILS_Discovery_1.1.pdf
Comment 43 Katrin Fischer 2014-08-23 13:53:50 UTC
I know it is not part of the spec - but should we maybe add the reserve_id as a new parameter to the API? This way the old behaviour could be kept (explaining that it will only work for item level holds)
Comment 44 Tomás Cohen Arazi 2014-09-18 13:10:54 UTC
Patch pushed to master.

Thanks Julian!