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.
This bug is still valid.
This is still true in 3.14 and master.
Created attachment 28657 [details] [review] Bug 5144 [1] - Unit test
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
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!
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
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
Created attachment 34983 [details] [review] Bug 5144 [1] - Unit test
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
Created attachment 34985 [details] [review] Bug 5144 [3] - Add message for patron
Created attachment 34986 [details] [review] Bug 5144 [1] - Unit test
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
Created attachment 34988 [details] [review] Bug 5144 [3] - Add message for patron
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."
Created attachment 35090 [details] [review] Bug 5144 [1] - Unit test
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
Created attachment 35092 [details] [review] Bug 5144 [3] - Add message for patron
Created attachment 35093 [details] [review] Bug 5144 [4] [QA Followup] - Fix message error and language used
Created attachment 35451 [details] [review] Bug 5144 [4] [QA Followup] - Fix message error and language used Signed-off-by: Aleisha <aleishaamohia@hotmail.com>
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.
(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.
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.
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?
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
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
Be care, AddReserve prototype changed by bug 9809 (contrainst param removed).
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
Patch updated! (In reply to Jonathan Druart from comment #26) > Be care, AddReserve prototype changed by bug 9809 (contrainst param removed).
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>
I cannot reproduce this issue, when I click on the "Place hold" button, the form is not resubmitted.
Ok, got it using Chromium. But it makes the QA script fail: FAIL valid "my" variable $fee masks earlier declaration in same scope
Created attachment 42848 [details] [review] Bug 5144 [QA Followup] - Remove duplicated line of code
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.)
Created attachment 43118 [details] [review] Bug 5144 [QA Followup] - Add unit test
Created attachment 43119 [details] [review] Bug 5144 [QA Followup] - Add unit test
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>
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.)
Created attachment 43121 [details] [review] Bug 5144 - Add unit test
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>
Created attachment 43125 [details] [review] Bug 5144 - Add unit test
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>
Kyle, now that bug 5371 has been pushed, I cannot longer reproduce the issue. Should we push these changes anyway?
(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.
Created attachment 43164 [details] [review] Bug 5144 - Add unit test Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
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>
Created attachment 43166 [details] [review] Bug 5144: Add test on carp message Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
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>
Patches pushed to master. Thanks Kyle and Jonathan!