Bug 11445 - it is possible for duplicate hold notifications to be sent
Summary: it is possible for duplicate hold notifications to be sent
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-12-25 17:26 UTC by Galen Charlton
Modified: 2015-06-04 23:23 UTC (History)
2 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 11445: regression test for duplicate hold notifications (2.43 KB, patch)
2013-12-25 17:48 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 11445: avoid sending duplicate hold waiting notifications (1.82 KB, patch)
2013-12-25 17:48 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 11445: regression test for duplicate hold notifications (2.52 KB, patch)
2013-12-29 02:40 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11445: avoid sending duplicate hold waiting notifications (1.87 KB, patch)
2013-12-29 02:40 UTC, Chris Cormack
Details | Diff | Splinter Review
[PASSED QA] Bug 11445: regression test for duplicate hold notifications (2.59 KB, patch)
2013-12-29 07:58 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11445: avoid sending duplicate hold waiting notifications (1.99 KB, patch)
2013-12-29 07:58 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 Galen Charlton 2013-12-25 17:26:19 UTC
If an item that fills a hold request is checked in twice and the hold is confirmed each time, it is possible for multiple hold notifications to be sent to the patron  even if the item was already marked as waiting on the hold shelf after the first checkin.

This is because ModReserveAffect() includes a call to GetReserveInfo() that was never updated when the holds API was updated to use reserve_id.  Consequently, ModReserveAffect fails to correctly determine if the item was already waiting.
Comment 1 Galen Charlton 2013-12-25 17:48:35 UTC Comment hidden (obsolete)
Comment 2 Galen Charlton 2013-12-25 17:48:45 UTC Comment hidden (obsolete)
Comment 3 Chris Cormack 2013-12-29 02:40:26 UTC Comment hidden (obsolete)
Comment 4 Chris Cormack 2013-12-29 02:40:41 UTC Comment hidden (obsolete)
Comment 5 Katrin Fischer 2013-12-29 07:58:22 UTC
Created attachment 23852 [details] [review]
[PASSED QA] Bug 11445: regression test for duplicate hold notifications

This patch implements a regression test for verifying that
duplicate hold notifications aren't sent if ModReserveAffect() is
called repeatedly (as might happen if a circ operator accidentally
checks in an item and confirms its hold more than once).

Note that the test depends on the fact that _koha_notify_reserve()
defaults to sending a HOLD_PRINT letter if the borrower has not
specified an email or SMS hold notification.

To test:

[1] Run prove -v t/db_dependent/Reserves.t
[2] The 'patron not notified a second time (bug 11445)' test
    should fail.
[3] Apply the main patch and run prove -v t/db_dependent/Reserves.t
    again.  This time all tests should pass.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 6 Katrin Fischer 2013-12-29 07:58:39 UTC
Created attachment 23853 [details] [review]
[PASSED QA] Bug 11445: avoid sending duplicate hold waiting notifications

This patch fixes a problem where a patron could receive duplicate
hold waiting notifications.  For example, this could happen if a
circ operator checked in an item more than once and confirmed the
same hold each time.

To test:

[1] Set up a test patron that received hold waiting notifications.
[2] Put an item on hold for the patron, then check the item in
    and confirm the hold.  Verify that a hold notification is
    sent (or inspect the message_queue table).
[3] Check the item in again and confirm the hold again.  A duplicate
    hold notification will be generated.
[4] Apply the patch.
[5] Repeat steps 2 and 3.  This time, only one notification should be
    generated.
[6] Verify that prove -v t/db_dependent/Reserves.t passes.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests and QA script.
Works as described.
Comment 7 Galen Charlton 2013-12-30 15:11:13 UTC
Pushed to master.
Comment 8 Fridolin Somers 2014-01-27 13:21:49 UTC
Patches pushed to 3.14.x, will be in 3.14.3.