Bug 5144 - Duplicate holds allowed if patron clicks back button after placing hold
Summary: Duplicate holds allowed if patron clicks back button after placing hold
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low normal
Assignee: Kyle M Hall
QA Contact: Bugs List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-17 19:14 UTC by Liz Rea
Modified: 2016-06-21 21:40 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 5144 [1] - Unit test (3.30 KB, patch)
2014-06-04 14:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5144 [2] - Duplicate holds allowed if patron clicks back button after placing hold (1.70 KB, patch)
2014-06-04 14:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5144 [2] - Duplicate holds allowed if patron clicks back button after placing hold (1.63 KB, patch)
2014-06-04 14:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5144 [1] - Unit test (3.29 KB, patch)
2015-01-06 14:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5144 [2] - Duplicate holds allowed if patron clicks back button after placing hold (1.39 KB, patch)
2015-01-06 14:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5144 [3] - Add message for patron (4.87 KB, patch)
2015-01-06 14:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5144 [1] - Unit test (3.29 KB, patch)
2015-01-06 15:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5144 [2] - Duplicate holds allowed if patron clicks back button after placing hold (1.39 KB, patch)
2015-01-06 15:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5144 [3] - Add message for patron (4.89 KB, patch)
2015-01-06 15:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5144 [1] - Unit test (3.29 KB, patch)
2015-01-09 15:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5144 [2] - Duplicate holds allowed if patron clicks back button after placing hold (1.39 KB, patch)
2015-01-09 15:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5144 [3] - Add message for patron (4.89 KB, patch)
2015-01-09 15:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5144 [4] [QA Followup] - Fix message error and language used (1.77 KB, patch)
2015-01-09 15:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5144 [4] [QA Followup] - Fix message error and language used (1.82 KB, patch)
2015-01-21 19:48 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 5144 - Duplicate holds allowed if patron clicks back button after placing hold (13.24 KB, patch)
2015-07-07 15:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5144 - Duplicate holds allowed if patron clicks back button after placing hold (13.24 KB, patch)
2015-07-07 15:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5144 - Duplicate holds allowed if patron clicks back button after placing hold (9.74 KB, patch)
2015-09-14 15:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5144 - Duplicate holds allowed if patron clicks back button after placing hold (9.79 KB, patch)
2015-09-22 16:17 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 5144 [QA Followup] - Remove duplicated line of code (670 bytes, patch)
2015-09-24 15:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5144 [QA Followup] - Add unit test (1.24 KB, patch)
2015-10-05 11:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5144 [QA Followup] - Add unit test (3.29 KB, patch)
2015-10-05 11:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5144 - Duplicate holds allowed if patron clicks back button after placing hold (7.49 KB, patch)
2015-10-05 11:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5144 - Add unit test (3.28 KB, patch)
2015-10-05 12:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5144 - Duplicate holds allowed if patron clicks back button after placing hold (7.49 KB, patch)
2015-10-05 12:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5144 - Add unit test (4.92 KB, patch)
2015-10-05 13:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5144 - Duplicate holds allowed if patron clicks back button after placing hold (7.49 KB, patch)
2015-10-05 13:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5144 - Add unit test (4.99 KB, patch)
2015-10-06 12:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5144 - Duplicate holds allowed if patron clicks back button after placing hold (7.56 KB, patch)
2015-10-06 12:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5144: Add test on carp message (1.38 KB, patch)
2015-10-06 12:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5144: Don't display the failed_holds param in the url if not needed (903 bytes, patch)
2015-10-06 12:59 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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
Created attachment 28657 [details] [review]
Bug 5144 [1] - Unit test
Comment 4 Kyle M Hall 2014-06-04 14:52:06 UTC
Created attachment 28658 [details] [review]
Bug 5144 [2] - 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 the first patch in this series
2) Run t/db_dependent/Reserves.t
3) Note the failure of test 36
4) Apply the second patch in this series
5) Re-run Reserves.t
6) Note the success of test 36
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
Created attachment 28659 [details] [review]
Bug 5144 [2] - 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 the first patch in this series
2) Run t/db_dependent/Reserves.t
3) Note the failure of test 36
4) Apply the second patch in this series
5) Re-run Reserves.t
6) Note the success of test 36
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
Created attachment 34983 [details] [review]
Bug 5144 [1] - Unit test
Comment 9 Kyle M Hall 2015-01-06 14:52:25 UTC
Created attachment 34984 [details] [review]
Bug 5144 [2] - 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
Comment 10 Kyle M Hall 2015-01-06 14:52:28 UTC
Created attachment 34985 [details] [review]
Bug 5144 [3] - Add message for patron
Comment 11 Kyle M Hall 2015-01-06 15:30:31 UTC
Created attachment 34986 [details] [review]
Bug 5144 [1] - Unit test
Comment 12 Kyle M Hall 2015-01-06 15:30:37 UTC
Created attachment 34987 [details] [review]
Bug 5144 [2] - 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
Comment 13 Kyle M Hall 2015-01-06 15:30:40 UTC
Created attachment 34988 [details] [review]
Bug 5144 [3] - Add message for patron
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
Created attachment 35090 [details] [review]
Bug 5144 [1] - Unit test
Comment 16 Kyle M Hall 2015-01-09 15:24:17 UTC
Created attachment 35091 [details] [review]
Bug 5144 [2] - 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
Comment 17 Kyle M Hall 2015-01-09 15:24:19 UTC
Created attachment 35092 [details] [review]
Bug 5144 [3] - Add message for patron
Comment 18 Kyle M Hall 2015-01-09 15:24:22 UTC
Created attachment 35093 [details] [review]
Bug 5144 [4] [QA Followup] - Fix message error and language used
Comment 19 Aleisha Amohia 2015-01-21 19:48:27 UTC
Created attachment 35451 [details] [review]
Bug 5144 [4] [QA Followup] - Fix message error and language used

Signed-off-by: Aleisha <aleishaamohia@hotmail.com>
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
Created attachment 40823 [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
Comment 25 Kyle M Hall 2015-07-07 15:39:58 UTC
Created attachment 40824 [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
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
Created attachment 42530 [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
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
Created attachment 42792 [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>
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
Created attachment 42848 [details] [review]
Bug 5144 [QA Followup] - Remove duplicated line of code
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
Created attachment 43118 [details] [review]
Bug 5144 [QA Followup] - Add unit test
Comment 35 Kyle M Hall 2015-10-05 11:53:35 UTC
Created attachment 43119 [details] [review]
Bug 5144 [QA Followup] - Add unit test
Comment 36 Kyle M Hall 2015-10-05 11:53:44 UTC
Created attachment 43120 [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>
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
Created attachment 43121 [details] [review]
Bug 5144 - Add unit test
Comment 39 Kyle M Hall 2015-10-05 12:12:31 UTC
Created attachment 43122 [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>
Comment 40 Kyle M Hall 2015-10-05 13:06:01 UTC
Created attachment 43125 [details] [review]
Bug 5144 - Add unit test
Comment 41 Kyle M Hall 2015-10-05 13:06:08 UTC
Created attachment 43126 [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>
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!