Bug 12086 - Hold priorities incorrect, when waiting status was reversed
Summary: Hold priorities incorrect, when waiting status was reversed
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on: 11947
Blocks:
  Show dependency treegraph
 
Reported: 2014-04-15 09:38 UTC by Katrin Fischer
Modified: 2016-06-21 21:36 UTC (History)
7 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 12086 - Hold priorities incorrect, when waiting status was reversed (1.91 KB, patch)
2014-04-17 16:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12086 - Hold priorities incorrect, when waiting status was reversed (1.60 KB, patch)
2014-04-17 16:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12086 - Hold priorities incorrect, when waiting status was reversed (4.45 KB, patch)
2014-04-22 15:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12086 - Hold priorities incorrect, when waiting status was reversed (4.59 KB, patch)
2014-04-22 15:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12086 - Hold priorities incorrect, when waiting status was reversed (4.77 KB, patch)
2014-04-22 16:54 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 12086 [QA Followup] - Truncate old_reseres in unit test (723 bytes, patch)
2014-05-23 18:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12086 [QA Followup] - Delete reserves, old_reserves before running unit tests (808 bytes, patch)
2014-05-27 11:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12086 [QA Followup 2] - Implement Jonathan's suggestions (4.41 KB, patch)
2014-06-16 15:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12086 - Hold priorities incorrect, when waiting status was reversed (4.88 KB, patch)
2014-06-23 09:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12086: qa-followup: remove warning on launching unit tests (1.06 KB, patch)
2014-06-23 09:46 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 Katrin Fischer 2014-04-15 09:38:46 UTC
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
Comment 1 Kyle M Hall 2014-04-17 15:01:29 UTC
I can't reproduce this on master.
Comment 2 Katrin Fischer 2014-04-17 15:42:19 UTC
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.
Comment 3 Katrin Fischer 2014-04-17 15:50:20 UTC
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
Comment 4 Kyle M Hall 2014-04-17 15:56:18 UTC
I was able to reproduce this issue following the new test plan!
Comment 5 Kyle M Hall 2014-04-17 16:13:19 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2014-04-17 16:14:31 UTC Comment hidden (obsolete)
Comment 7 Katrin Fischer 2014-04-17 17:31:45 UTC
/me whispers *regression test*?
Comment 8 Kyle M Hall 2014-04-22 15:19:47 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2014-04-22 15:22:27 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2014-04-22 15:23:42 UTC
(In reply to Katrin Fischer from comment #7)
> /me whispers *regression test*?

Done!
Comment 11 Owen Leonard 2014-04-22 16:54:10 UTC Comment hidden (obsolete)
Comment 12 Katrin Fischer 2014-04-26 21:59:04 UTC
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.
Comment 13 Kyle M Hall 2014-05-23 18:02:20 UTC Comment hidden (obsolete)
Comment 14 Katrin Fischer 2014-05-23 18:27:58 UTC
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?
Comment 15 Kyle M Hall 2014-05-27 11:01:14 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2014-06-02 15:21:47 UTC
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.
Comment 17 Jonathan Druart 2014-06-02 15:23:13 UTC
Kyle, could you please answer my questions please?
Comment 18 Kyle M Hall 2014-06-04 17:33:03 UTC
(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.
Comment 19 Jonathan Druart 2014-06-05 07:25:33 UTC
I think something like t/db_dependent/Reserves/RevertWaitingStatus.t is more clear.

What about others questions? :)
Comment 20 Katrin Fischer 2014-06-09 10:36:52 UTC
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.
Comment 21 Kyle M Hall 2014-06-16 15:22:49 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2014-06-23 09:46:17 UTC
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.
Comment 23 Jonathan Druart 2014-06-23 09:46:28 UTC
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>
Comment 24 Tomás Cohen Arazi 2014-06-23 18:09:55 UTC
Patch pushed to master.

Thanks Kyle!
Comment 25 Galen Charlton 2014-06-25 22:31:18 UTC
Pushed to 3.16.x for inclusion in 3.16.1.
Comment 26 Fridolin Somers 2014-12-15 16:35:04 UTC
Pushed to 3.14.x, will be in 3.14.12.