Bug 12079 - CheckReserves() and _Findgroupreserve do not return reserve_id
Summary: CheckReserves() and _Findgroupreserve do not return reserve_id
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Galen Charlton
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-04-14 18:38 UTC by Galen Charlton
Modified: 2015-06-04 23:30 UTC (History)
6 users (show)

See Also:
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 (3.48 KB, patch)
2014-04-14 18:48 UTC, Galen Charlton
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12079: ensure that CheckReserves() includes reserve_id in its response (3.60 KB, patch)
2014-04-15 13:42 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[PASSED QA] Bug 12079: ensure that CheckReserves() includes reserve_id in its response (3.65 KB, patch)
2014-04-17 15:08 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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