Bug 12079

Summary: CheckReserves() and _Findgroupreserve do not return reserve_id
Product: Koha Reporter: Galen Charlton <gmcharlt>
Component: Hold requestsAssignee: Galen Charlton <gmc>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: bgkriegel, fridolin.somers, gmcharlt, katrin.fischer, kyle, pasi.kallinen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 12079: ensure that CheckReserves() includes reserve_id in its response
[SIGNED-OFF] Bug 12079: ensure that CheckReserves() includes reserve_id in its response
[PASSED QA] Bug 12079: ensure that CheckReserves() includes reserve_id in its response

Description Galen Charlton 2014-04-14 18:38:53 UTC
The hold requests returned by _Findgroupreserve do not include reserve_id, which means that the request returned by CheckReserves also does not include reserve_id.

If they did, bug 11947 would not have occurred (though the patch for bug 11947 should not be preempted by this bug, IMO).

All routines that return hold requests should return the reserve_id to avoid the potential for breakage by other routines that rely on reserve_id.
Comment 1 Galen Charlton 2014-04-14 18:48:31 UTC Comment hidden (obsolete)
Comment 2 paxed 2014-04-15 09:32:05 UTC
See also bug 12085
Comment 3 Bernardo Gonzalez Kriegel 2014-04-15 13:42:56 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2014-04-17 15:08:17 UTC
Created attachment 27257 [details] [review]
[PASSED QA] Bug 12079: ensure that CheckReserves() includes reserve_id in its response

This patch modifies _Findgroupreserve so that its one caller,
CheckReserves(), would include the reserve_id field in the
hold request it returns.

Failure to include reserve_id in every circumstance resulted
in bug 11947.  This patch is therefore a complementary fix for
that bug, but is not meant to preempt the direct fix for
that bug.

To test:

[1] Verify that t/db_dependent/Reserves.t passes.
[2] Verify that the following test plan taken from
    the patch for bug 11947 works for this patch
    *without* applying the patch for 11947:

* have a few borrowers, say 4.
* have a biblio with a single item (you can scale this up, it should
  work just the same.)
* issue the item to borrower A
* have borrowers B, C, and D place a hold on the item
* return the item, acknowledge that it'll be put aside for B.
* view the holds on the item.

Without the patch:
* the hold priorities in the UI end up being "waiting, 2, 1" when they
  should be "waiting, 1, 2".
* in the database "reserves" table, they're really "0, 2, 3" when they
  should be "0, 1, 2".

With the patch:
* the hold priorities in the UI end up being "waiting, 1, 2"
* in the database, they're "0, 1, 2"

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Work as described. No koha-qa errors. Test pass

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 5 Galen Charlton 2014-04-17 16:39:54 UTC
Pushed to master.
Comment 6 Fridolin Somers 2014-08-06 09:51:28 UTC
Pushed to 3.14.x, will be in 3.14.10