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".

Patch tested with a sandbox, by Leila and Sonia <koha.aixmarseille@gmail.com>

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

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

1, 2, and 3 fixed ;)

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".

All patches rebased on master

Patch applied cleanly, go forth and signoff

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".

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.

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

> 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.

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.

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.

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)

(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.

QA: Looking at this one now..

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

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 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?

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.

Created attachment 30909 [details] [review]
Bug 8868: Follow-up: Update boostrap documentation page

Updates the bootstrap documentation page for CancelHold.

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?

Because it took me a bit to find it:
http://old.diglib.org/architectures/ilsdi/DLF_ILS_Discovery_1.1.pdf

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)

Patch pushed to master.

Thanks Julian!

Have to say (after such a long time) that item_id is a misleading parameter. It would be easier to use a biblionumber, itemnumber or reserve_id.
Reason to open a new report ?