Tested on top of patch for 11947! 1) Prepare for headache. 2) Check out an item from any record. 3) Place 3 holds, for patrons a, b, c on the same record. 4) Return the checked out item - it is now waiting for a, b = 1, c = 2 5) Check out the book to b(!), revert waiting status as offered by the GUI You will end up with: a = 1, c = 1 in the holds list, but in the database it's kind of more correct: a = 1, c = 3
I can't reproduce this on master.
Gah. I am sure it was there - but what is the missing piece? I reproduced the behaviour twice - one of my holds was item level, I remember that much. I will see if I can still reproduce it.
Ok, repeating tests, adding some information to the test plan: 1) Test record has 1 single item, checked out to patron X 2) Place 3 holds for patrons A, B and C, all title level hold this time A, B, C, item branches and staff branch are the same. 3) Return item, confirm hold 4) Confirm item is now waiting for patron A Priorities are: A = Waiting, B = 1, C = 2 5) Open patron account of user B, checkout book Koha asks: Item X has been waiting for patron A... Revert waiting status Confirm. 6) Check priorities: Hold list shows: A = 1, C = 1 Database says: A = 1, C = 3
I was able to reproduce this issue following the new test plan!
Created attachment 27262 [details] [review] Bug 12086 - Hold priorities incorrect, when waiting status was reversed 1) Test record has 1 single item, checked out to patron X 2) Place 3 holds for patrons A, B and C, all title level hold this time A, B, C, item branches and staff branch are the same. 3) Return item, confirm hold 4) Confirm item is now waiting for patron A Priorities are: A = Waiting, B = 1, C = 2 5) Open patron account of user B, checkout book Koha asks: Item X has been waiting for patron A... Revert waiting status Confirm. 6) Check priorities: Hold list shows: A = 1, C = 1 Database says: A = 1, C = 3 7) Apply this patch 8) Repeat steps 1-6 9) Note the priorities are correct
Created attachment 27263 [details] [review] Bug 12086 - Hold priorities incorrect, when waiting status was reversed 1) Test record has 1 single item, checked out to patron X 2) Place 3 holds for patrons A, B and C, all title level hold this time A, B, C, item branches and staff branch are the same. 3) Return item, confirm hold 4) Confirm item is now waiting for patron A Priorities are: A = Waiting, B = 1, C = 2 5) Open patron account of user B, checkout book Koha asks: Item X has been waiting for patron A... Revert waiting status Confirm. 6) Check priorities: Hold list shows: A = 1, C = 1 Database says: A = 1, C = 3 7) Apply this patch 8) Repeat steps 1-6 9) Note the priorities are correct
/me whispers *regression test*?
Created attachment 27406 [details] [review] Bug 12086 - Hold priorities incorrect, when waiting status was reversed 1) Test record has 1 single item, checked out to patron X 2) Place 3 holds for patrons A, B and C, all title level hold this time A, B, C, item branches and staff branch are the same. 3) Return item, confirm hold 4) Confirm item is now waiting for patron A Priorities are: A = Waiting, B = 1, C = 2 5) Open patron account of user B, checkout book Koha asks: Item X has been waiting for patron A... Revert waiting status Confirm. 6) Check priorities: Hold list shows: A = 1, C = 1 Database says: A = 1, C = 3 7) Apply this patch 8) Repeat steps 1-6 9) Note the priorities are correct
Created attachment 27407 [details] [review] Bug 12086 - Hold priorities incorrect, when waiting status was reversed 1) Test record has 1 single item, checked out to patron X 2) Place 3 holds for patrons A, B and C, all title level hold this time A, B, C, item branches and staff branch are the same. 3) Return item, confirm hold 4) Confirm item is now waiting for patron A Priorities are: A = Waiting, B = 1, C = 2 5) Open patron account of user B, checkout book Koha asks: Item X has been waiting for patron A... Revert waiting status Confirm. 6) Check priorities: Hold list shows: A = 1, C = 1 Database says: A = 1, C = 3 7) Apply this patch 8) Repeat steps 1-6 9) Note the priorities are correct
(In reply to Katrin Fischer from comment #7) > /me whispers *regression test*? Done!
Created attachment 27428 [details] [review] [SIGNED-OFF] Bug 12086 - Hold priorities incorrect, when waiting status was reversed 1) Test record has 1 single item, checked out to patron X 2) Place 3 holds for patrons A, B and C, all title level hold this time A, B, C, item branches and staff branch are the same. 3) Return item, confirm hold 4) Confirm item is now waiting for patron A Priorities are: A = Waiting, B = 1, C = 2 5) Open patron account of user B, checkout book Koha asks: Item X has been waiting for patron A... Revert waiting status Confirm. 6) Check priorities: Hold list shows: A = 1, C = 1 Database says: A = 1, C = 3 7) Apply this patch 8) Repeat steps 1-6 9) Note the priorities are correct Signed-off-by: Owen Leonard <oleonard@myacpl.org> Test plan correctly predicts the error and the correction made by the patch.
The new tests fail :( bumblebee:~/kohaclone (106-12086-holdreversed) $ perl t/db_dependent/Holds_12086.t 1..3 Subroutine C4::Context::userenv redefined at t/db_dependent/Holds_12086.t line 29. # Creating biblio instance for testing. # Creating item instance for testing. DBD::mysql::st execute failed: Duplicate entry '2' for key 'PRIMARY' at /home/katrin/kohaclone/C4/Reserves.pm line 1194. DBD::mysql::st execute failed: Duplicate entry '2' for key 'PRIMARY' at /home/katrin/kohaclone/C4/Reserves.pm line 1194. # Looks like your test exited with 255 before it could output anything.
Created attachment 28471 [details] [review] Bug 12086 [QA Followup] - Truncate old_reseres in unit test
Hi Kyle, I think we should not use truncate in the unit tests as it appears to do an implicit commit in MySQL... which we don't really want here. Could you please rewrite it?
Created attachment 28482 [details] [review] Bug 12086 [QA Followup] - Delete reserves, old_reserves before running unit tests
Comment on attachment 27428 [details] [review] [SIGNED-OFF] Bug 12086 - Hold priorities incorrect, when waiting status was reversed Review of attachment 27428 [details] [review]: ----------------------------------------------------------------- Why don't you use t/db_dependent/Holds.t? ::: t/db_dependent/Holds_12086.t @@ +1,2 @@ > +#!/usr/bin/perl > + Are you sure we want 1 file per bug? This is a new way to do. Maybe should you sent an email to the list to get other opinions. @@ +15,5 @@ > + > +BEGIN { > + use FindBin; > + use lib $FindBin::Bin; > +} Why do you use FindBin? @@ +40,5 @@ > +my ($bibnum, $title, $bibitemnum) = create_helper_biblio(); > + > +# Helper item for that biblio. > +diag("Creating item instance for testing."); > +my $item_barcode = '99999999999999999999'; Hum, yes but why not "my_barcode"? :) @@ +48,5 @@ > +my @borrowernumbers; > +foreach (1..$borrowers_count) { > + my $borrowernumber = AddMember( > + firstname => 'my firstname', > + surname => 'my surname ' . $_, Please don't use the variable loop $_ to avoid futures errors.
Kyle, could you please answer my questions please?
(In reply to Jonathan Druart from comment #17) > Kyle, could you please answer my questions please? The answer is a blatantly stole a different unit test as the basis for this one. I chose to create a new unit test file because adding it to the existing holds unit test proved to be difficult. This way, the test can start with no assumptions about what has taken place previously, and does not need to care about what takes place in the future.
I think something like t/db_dependent/Reserves/RevertWaitingStatus.t is more clear. What about others questions? :)
Kyle, could you take another look at comments 16 and 19? I like Jonathan's suggestion on renaming the file to make the purpose more clear.
Created attachment 28873 [details] [review] Bug 12086 [QA Followup 2] - Implement Jonathan's suggestions
Created attachment 29162 [details] [review] Bug 12086 - Hold priorities incorrect, when waiting status was reversed 1) Test record has 1 single item, checked out to patron X 2) Place 3 holds for patrons A, B and C, all title level hold this time A, B, C, item branches and staff branch are the same. 3) Return item, confirm hold 4) Confirm item is now waiting for patron A Priorities are: A = Waiting, B = 1, C = 2 5) Open patron account of user B, checkout book Koha asks: Item X has been waiting for patron A... Revert waiting status Confirm. 6) Check priorities: Hold list shows: A = 1, C = 1 Database says: A = 1, C = 3 7) Apply this patch 8) Repeat steps 1-6 9) Note the priorities are correct Signed-off-by: Owen Leonard <oleonard@myacpl.org> Test plan correctly predicts the error and the correction made by the patch. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Amended patch: I squashed the 3 patches for a better readability. A file was created in the first patch and deleted later.
Created attachment 29163 [details] [review] Bug 12086: qa-followup: remove warning on launching unit tests Without this patch, the following warning appears: t/db_dependent/Holds/RevertWaitingStatus.t .. Subroutine C4::Context::userenv redefined at t/db_dependent/Holds/RevertWaitingStatus.t line 25. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Patch pushed to master. Thanks Kyle!
Pushed to 3.16.x for inclusion in 3.16.1.
Pushed to 3.14.x, will be in 3.14.12.