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.
Created attachment 27104 [details] [review] 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>
See also bug 12085
Created attachment 27129 [details] [review] [SIGNED-OFF] 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
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>
Pushed to master.
Pushed to 3.14.x, will be in 3.14.10