Bug 5144

Summary: Duplicate holds allowed if patron clicks back button after placing hold
Product: Koha Reporter: Liz Rea <wizzyrea>
Component: OPACAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Bugs List <koha-bugs>
Severity: normal    
Priority: P5 - low CC: cbrannon, chris, gwilliams, jonathan.druart, katrin.fischer, kyle.m.hall, kyle, nengard, nicole, tomascohen, wizzyrea
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9809
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7710
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 5144 [1] - Unit test
Bug 5144 [2] - Duplicate holds allowed if patron clicks back button after placing hold
Bug 5144 [2] - Duplicate holds allowed if patron clicks back button after placing hold
Bug 5144 [1] - Unit test
Bug 5144 [2] - Duplicate holds allowed if patron clicks back button after placing hold
Bug 5144 [3] - Add message for patron
Bug 5144 [1] - Unit test
Bug 5144 [2] - Duplicate holds allowed if patron clicks back button after placing hold
Bug 5144 [3] - Add message for patron
Bug 5144 [1] - Unit test
Bug 5144 [2] - Duplicate holds allowed if patron clicks back button after placing hold
Bug 5144 [3] - Add message for patron
Bug 5144 [4] [QA Followup] - Fix message error and language used
Bug 5144 [4] [QA Followup] - Fix message error and language used
Bug 5144 - Duplicate holds allowed if patron clicks back button after placing hold
Bug 5144 - Duplicate holds allowed if patron clicks back button after placing hold
Bug 5144 - Duplicate holds allowed if patron clicks back button after placing hold
Bug 5144 - Duplicate holds allowed if patron clicks back button after placing hold
Bug 5144 [QA Followup] - Remove duplicated line of code
Bug 5144 [QA Followup] - Add unit test
Bug 5144 [QA Followup] - Add unit test
Bug 5144 - Duplicate holds allowed if patron clicks back button after placing hold
Bug 5144 - Add unit test
Bug 5144 - Duplicate holds allowed if patron clicks back button after placing hold
Bug 5144 - Add unit test
Bug 5144 - Duplicate holds allowed if patron clicks back button after placing hold
Bug 5144 - Add unit test
Bug 5144 - Duplicate holds allowed if patron clicks back button after placing hold
Bug 5144: Add test on carp message
Bug 5144: Don't display the failed_holds param in the url if not needed

Description Liz Rea 2010-08-17 19:14:13 UTC
Here is a short screencast: http://screencast.com/t/ZGZhMDkwMjYt

This shows that if a patron places a reserve, then clicks the back button, and clicks place hold again, duplicate reserves will be created. This happens even though the patron is alerted that they cannot place an additional reserve on the same item.

Here is what the staff side looks like: http://screencast.com/t/ODEzNmY3

The behavior of these duplicate reserves on the items reserve list is that both are displayed, but if you try to delete one, both will be deleted and the patron loses the reserve. See above screencast.
Comment 1 Liz Rea 2014-01-18 06:49:10 UTC
This bug is still valid.
Comment 2 Nicole C. Engard 2014-03-17 17:43:22 UTC
This is still true in 3.14 and master.
Comment 3 Kyle M Hall 2014-06-04 14:51:49 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2014-06-04 14:52:06 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2014-06-04 14:53:40 UTC
Note, the additional changes in Reserves.t were required due to the fact that the unit test was exploiting the fact that AddReserve wasn't checking for duplicate reserves!
Comment 6 Kyle M Hall 2014-06-04 14:55:44 UTC Comment hidden (obsolete)
Comment 7 Christopher Brannon 2014-06-04 17:06:05 UTC
Kyle,
I don't understand the reference to "test 36".

You applied this patch to our production server.  While it appear to be working in that it prevents a duplicate hold, there are a couple of unexpected behaviors.

1) When you back up to the previous screen in the OPAC, the radio button for "A specific copy" is still selected, but nothing is showing.  If you click on the already selected radio button, the items appear.  I don't recall if this was a pre-existing condition.

2) When you select something else and try to place the hold, it just bounces you to the screen showing the hold you have already placed.  There needs to be a warning that you cannot place multiple holds on the same biblio record.  There is simply no explanation, and patrons can assume the hold did work.

I think the issue in point 2 needs to be addressed before this patch is signed off.

Christopher
Comment 8 Kyle M Hall 2015-01-06 14:52:19 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2015-01-06 14:52:25 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2015-01-06 14:52:28 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2015-01-06 15:30:31 UTC Comment hidden (obsolete)
Comment 12 Kyle M Hall 2015-01-06 15:30:37 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2015-01-06 15:30:40 UTC Comment hidden (obsolete)
Comment 14 Owen Leonard 2015-01-06 16:06:22 UTC
This fails in one particular situation:

1. Search for items while not logged in.
2. From the search results page, select multiple titles, at least one (but not all) of which you already have on hold.
3. Click the "place hold" button to place multiple holds at once.
4. Log in when prompted.

This process will redirect you to opac-user.pl with the "holds were not placed" message without giving you the opportunity to place a hold on the titles which you didn't have on hold.

I think we could be clearer than "One or more holds were not placed due to existing holds." "Due to existing holds" could mean "because of duplicates," or it could mean "because you have too many existing holds."

Maybe:

"One or more holds were not placed because you already have a hold on that title."
Comment 15 Kyle M Hall 2015-01-09 15:24:10 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2015-01-09 15:24:17 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2015-01-09 15:24:19 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2015-01-09 15:24:22 UTC Comment hidden (obsolete)
Comment 19 Aleisha Amohia 2015-01-21 19:48:27 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2015-01-23 16:04:08 UTC
Comment on attachment 35091 [details] [review]
Bug 5144 [2] - Duplicate holds allowed if patron clicks back button after placing hold

Review of attachment 35091 [details] [review]:
-----------------------------------------------------------------

::: C4/Reserves.pm
@@ +161,5 @@
> +        ( $biblionumber, $borrowernumber )
> +    );
> +    if ( $count > 0 ) {
> +        carp("AddReserve: borrower $borrowernumber already has $count holds for biblionumber $biblionumber");
> +        return;

This will be in conflict with bug 7710.
Comment 21 Kyle M Hall 2015-01-28 14:46:35 UTC
(In reply to Jonathan Druart from comment #20)
> Comment on attachment 35091 [details] [review] [review]
> Bug 5144 [2] - Duplicate holds allowed if patron clicks back button after
> placing hold
> 
> Review of attachment 35091 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/Reserves.pm
> @@ +161,5 @@
> > +        ( $biblionumber, $borrowernumber )
> > +    );
> > +    if ( $count > 0 ) {
> > +        carp("AddReserve: borrower $borrowernumber already has $count holds for biblionumber $biblionumber");
> > +        return;
> 
> This will be in conflict with bug 7710.

Then the race is on! I'll have to add a followup to whichever bug is not pushed to master first.
Comment 22 Katrin Fischer 2015-02-08 19:52:58 UTC
I fail to replicate the issue in the bootstrap OPAC. I am not able to place another hold, but there is no good feedback - the button just won't work.
Investigating further.
Comment 23 Katrin Fischer 2015-02-08 19:59:50 UTC
I fail to see a message on the bootstrap OPAC with the patches as well.

Test done:
- Place an item level hold on a record
- Get redirected to patron account
- Use back button of the browser
- Hit 'place hold' button again - it's not functional, nothing happens

Kyle, can you please take a look?
Comment 24 Kyle M Hall 2015-07-07 15:38:12 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2015-07-07 15:39:58 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2015-08-21 10:42:49 UTC
Be care, AddReserve prototype changed by bug 9809 (contrainst param removed).
Comment 27 Kyle M Hall 2015-09-14 15:15:17 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2015-09-14 15:17:51 UTC
Patch updated!

(In reply to Jonathan Druart from comment #26)
> Be care, AddReserve prototype changed by bug 9809 (contrainst param removed).
Comment 29 Nicole C. Engard 2015-09-22 16:17:14 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2015-09-23 11:47:02 UTC
I cannot reproduce this issue, when I click on the "Place hold" button, the form is not resubmitted.
Comment 31 Jonathan Druart 2015-09-23 11:52:12 UTC
Ok, got it using Chromium.

But it makes the QA script fail:
 FAIL   valid
   "my" variable $fee masks earlier declaration in same scope
Comment 32 Kyle M Hall 2015-09-24 15:22:24 UTC Comment hidden (obsolete)
Comment 33 Jonathan Druart 2015-10-01 15:36:47 UTC
Kyle, I don't understand how the changes you introduced in the tests for AddReserve highlight your changes in the subroutine.

It would be better to first submit a patch to fix the issue then to clean some stuffs (indentation, replacement with Koha::Objects, etc.)
Comment 34 Kyle M Hall 2015-10-05 11:44:43 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2015-10-05 11:53:35 UTC Comment hidden (obsolete)
Comment 36 Kyle M Hall 2015-10-05 11:53:44 UTC Comment hidden (obsolete)
Comment 37 Kyle M Hall 2015-10-05 11:54:43 UTC
The updates to the unit tests in the first patch do not actually test the new functionality. Those changes were necessary to keep the tests from failing not that the patron hold limit of one per record is enforced by AddReserve. I've attached a new follow up that adds a unit test to specifically test this.

I have reworked the patches to split the unit tests into a separate patch for easier testing.

(In reply to Jonathan Druart from comment #33)
> Kyle, I don't understand how the changes you introduced in the tests for
> AddReserve highlight your changes in the subroutine.
> 
> It would be better to first submit a patch to fix the issue then to clean
> some stuffs (indentation, replacement with Koha::Objects, etc.)
Comment 38 Kyle M Hall 2015-10-05 12:12:21 UTC Comment hidden (obsolete)
Comment 39 Kyle M Hall 2015-10-05 12:12:31 UTC Comment hidden (obsolete)
Comment 40 Kyle M Hall 2015-10-05 13:06:01 UTC Comment hidden (obsolete)
Comment 41 Kyle M Hall 2015-10-05 13:06:08 UTC Comment hidden (obsolete)
Comment 42 Jonathan Druart 2015-10-06 11:30:38 UTC
Kyle, now that bug 5371 has been pushed, I cannot longer reproduce the issue.
Should we push these changes anyway?
Comment 43 Kyle M Hall 2015-10-06 11:53:16 UTC
(In reply to Jonathan Druart from comment #42)
> Kyle, now that bug 5371 has been pushed, I cannot longer reproduce the issue.
> Should we push these changes anyway?

Yes, I believe these changes should be pushed anyway. This fixes the bug at a deeper level than bug 5371 and there is a definite possibility that future changes could cause a new regression. This fix should stop all future regressions of this behavior and has a unit test to prove it.
Comment 44 Jonathan Druart 2015-10-06 12:59:33 UTC
Created attachment 43164 [details] [review]
Bug 5144 - Add unit test

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 45 Jonathan Druart 2015-10-06 12:59:43 UTC
Created attachment 43165 [details] [review]
Bug 5144 - Duplicate holds allowed if patron clicks back button after placing hold

Koha is currently not engineered to handle multiple holds per record.
Until such time that is does, we should not allow them to be created.

Test Plan:
1) Apply this patch
2) Log in to the opac
3) Place a hold
4) Hit the back button on your browser
5) Place the hold again
6) Note the new message

Signed-off-by: David Kuhn <kuhn@monterey.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 46 Jonathan Druart 2015-10-06 12:59:46 UTC
Created attachment 43166 [details] [review]
Bug 5144: Add test on carp message

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 47 Jonathan Druart 2015-10-06 12:59:51 UTC
Created attachment 43167 [details] [review]
Bug 5144: Don't display the failed_holds param in the url if not needed

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 48 Tomás Cohen Arazi 2015-10-06 14:03:12 UTC
Patches pushed to master.

Thanks Kyle and Jonathan!