Bug 12803 - Add ability to skip closed libraries when generating the holds queue
Summary: Add ability to skip closed libraries when generating the holds queue
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
: 8562 12095 (view as bug list)
Depends on: 12801 15391
Blocks: 26510
  Show dependency treegraph
 
Reported: 2014-08-21 15:19 UTC by Kyle M Hall
Modified: 2020-09-28 08:31 UTC (History)
12 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 12803 - Add ability to skip closed libraries when generating the holds queue (8.06 KB, patch)
2014-08-21 15:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 - Enable feature when using Transport Cost Matrix (5.16 KB, patch)
2014-08-21 16:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 - Enable feature when using Transport Cost Matrix (5.15 KB, patch)
2014-08-21 16:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 - Enable feature when using Transport Cost Matrix (5.45 KB, patch)
2014-08-21 16:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 - Enable feature when using Transport Cost Matrix (5.37 KB, patch)
2014-08-21 16:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 - Add ability to skip closed libraries when generating the holds queue (8.11 KB, patch)
2014-09-16 13:29 UTC, Melissa Lefebvre
Details | Diff | Splinter Review
Bug 12803 - Enable feature when using Transport Cost Matrix (5.42 KB, patch)
2014-09-16 13:30 UTC, Melissa Lefebvre
Details | Diff | Splinter Review
Bug 12803 [QA Followup] - Use Koha::Calendar (1.94 KB, patch)
2014-10-07 16:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803: tidy up some tests (2.05 KB, patch)
2014-10-11 20:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12803 - Add ability to skip closed libraries when generating the holds queue (10.64 KB, patch)
2015-10-16 13:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 [QA Followup] - Allow holiday caching to be disabled for testing purposes (1.65 KB, patch)
2015-10-16 13:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 - Add ability to skip closed libraries when generating the holds queue (10.58 KB, patch)
2015-10-16 13:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 [QA Followup] - Allow holiday caching to be disabled for testing purposes (1.65 KB, patch)
2015-10-16 13:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 [QA Followup] - Remove use of C4::Dates (673 bytes, patch)
2015-10-19 18:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 [QA Followup] - Fix koha-qa.pl errors (2.86 KB, patch)
2015-10-22 18:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 [QA Followup] - Fix unit tests (2.00 KB, patch)
2015-10-23 13:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Test Database SQL (1.16 MB, application/x-gzip)
2015-10-26 10:57 UTC, Kyle M Hall
Details
Bug 12803 - Add ability to skip closed libraries when generating the holds queue (10.56 KB, patch)
2015-12-10 14:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 [QA Followup] - Allow holiday caching to be disabled for testing purposes (1.66 KB, patch)
2015-12-10 14:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 [QA Followup] - Remove use of C4::Dates (655 bytes, patch)
2015-12-10 14:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 [QA Followup] - Fix koha-qa.pl errors (2.86 KB, patch)
2015-12-10 14:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 [QA Followup] - Fix unit tests (2.00 KB, patch)
2015-12-10 14:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 [QA Followup] - Update unit tests due to changes in master (1.09 KB, patch)
2015-12-10 14:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 [QA Followup] - Update unit tests due to changes in master (1.09 KB, patch)
2015-12-10 14:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 [QA Followup] - Fix to stop failing unit tests (852 bytes, patch)
2015-12-10 14:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 [QA Followup] - Remove duplicate 'use' lines (813 bytes, patch)
2015-12-14 11:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 [QA Followup] - Remove NO_CACHE (2.26 KB, patch)
2015-12-14 12:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 [QA Followup] - Remove NO_CACHE (2.48 KB, patch)
2015-12-14 13:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 - Add ability to skip closed libraries when generating the holds queue (10.44 KB, patch)
2016-01-19 14:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 [QA Followup] - Allow holiday caching to be disabled for testing purposes (1.65 KB, patch)
2016-01-19 14:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 [QA Followup] - Remove use of C4::Dates (649 bytes, patch)
2016-01-19 14:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 [QA Followup] - Fix koha-qa.pl errors (2.82 KB, patch)
2016-01-19 14:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 [QA Followup] - Update unit tests due to changes in master (1.08 KB, patch)
2016-01-19 14:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 [QA Followup] - Fix to stop failing unit tests (846 bytes, patch)
2016-01-19 14:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 [QA Followup] - Remove duplicate 'use' lines (807 bytes, patch)
2016-01-19 14:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 [QA Followup] - Remove NO_CACHE (2.47 KB, patch)
2016-01-19 14:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12803 - Add ability to skip closed libraries when generating the holds queue (10.50 KB, patch)
2016-01-25 10:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12803 [QA Followup] - Allow holiday caching to be disabled for testing purposes (5.36 KB, patch)
2016-01-25 10:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12803 - Add ability to skip closed libraries when generating the holds queue (10.54 KB, patch)
2016-03-03 04:10 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 12803 [QA Followup] - Allow holiday caching to be disabled for testing purposes (5.38 KB, patch)
2016-03-03 04:10 UTC, Mark Tompsett
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2014-08-21 15:19:40 UTC
The holds queue is typically generated many times a day in order to select items to fill holds. Often these items are to be sent to a different library. However, if the library whose item is picked to fill a hold is closed, that hold will remain unfilled even if there are other open libraries who own that item. It would be helpful if we could skip closed libraries for the purpose of selecting items to fill holds.
Comment 1 Kyle M Hall 2014-08-21 15:26:50 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2014-08-21 16:32:26 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2014-08-21 16:34:12 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2014-08-21 16:36:53 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2014-08-21 16:39:29 UTC Comment hidden (obsolete)
Comment 6 Jason Robb 2014-09-15 20:55:21 UTC
We've tested this and it is working as described.

If a library is closed on their calendar, a holds queue isn't generated. If multiple libraries own an item, the hold goes to the open library's queue over the closed library's.
Comment 7 Melissa Lefebvre 2014-09-16 13:29:48 UTC Comment hidden (obsolete)
Comment 8 Melissa Lefebvre 2014-09-16 13:30:27 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2014-09-26 09:51:06 UTC
Kyle, could you please use Koha::Calendar instead of C4::Calendar?
Comment 10 Kyle M Hall 2014-10-07 16:32:26 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2014-10-07 16:33:29 UTC
Good catch Jonathan! Note this bug now depends on bug 12801 since it makes use of Koha::Calendar which has a caching bug that would cause this feature to work incorrectly.
Comment 12 Jonathan Druart 2014-10-11 20:11:46 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2014-10-11 20:14:03 UTC
Before this patch, load_branches_to_pull_from return undef if StaticHoldsQueueWeight was empty, now RandomizeHoldsQueueWeight and HoldsQueueSkipClosed are take into account.
Kyle, could you please confirm it is an error?
I am not comfortable enough to fix it in a followup.
Comment 14 Kyle M Hall 2014-10-30 15:26:40 UTC
This is actually a bug fix. If you read the preference, it reads that RandomizeHoldsQueueWeight should work even if StaticHoldsQueueWeight is empty. However, in the original code this was not true.

This change otherwise has no effect on the behavior of the feature. Previously, if load_branches_to_pull returned undef, GetItemsAvailableToFillHoldRequestsForBib would use all branches. This is still true, it just makes more logical sense now.

If you would like me to revert the RandomizeHoldsQueueWeight behavior so the bug can be fixed separately, I'd be more than happy to do so.


(In reply to Jonathan Druart from comment #13)
> Before this patch, load_branches_to_pull_from return undef if
> StaticHoldsQueueWeight was empty, now RandomizeHoldsQueueWeight and
> HoldsQueueSkipClosed are take into account.
> Kyle, could you please confirm it is an error?
> I am not comfortable enough to fix it in a followup.
Comment 15 Katrin Fischer 2014-10-30 19:57:55 UTC
Looking at this now.
Comment 16 Katrin Fischer 2014-10-30 20:03:23 UTC
I am sorry, but t/db_dependent/HoldsQueue.t is failing in my installation:

t/db_dependent/HoldsQueue.t ................. 2/23 
#   Failed test 'Holds not filled with items from closed libraries'
#   at t/db_dependent/HoldsQueue.t line 310.
#          got: '2'
#     expected: '1'

#   Failed test 'Holds not filled with items from closed libraries'
#   at t/db_dependent/HoldsQueue.t line 327.
#          got: '3'
#     expected: '2'
# Looks like you failed 2 tests of 23.
t/db_dependent/HoldsQueue.t ................. Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/23 subtests 

Could this conflict with the calendar changes for hourly?
Comment 17 Kyle M Hall 2014-12-22 13:12:43 UTC
I cannot reproduce this error. Are you using Koha's sample data? I'm not sure these unit tests are reliable with any other data.

(In reply to Katrin Fischer from comment #16)
> I am sorry, but t/db_dependent/HoldsQueue.t is failing in my installation:
> 
> t/db_dependent/HoldsQueue.t ................. 2/23 
> #   Failed test 'Holds not filled with items from closed libraries'
> #   at t/db_dependent/HoldsQueue.t line 310.
> #          got: '2'
> #     expected: '1'
> 
> #   Failed test 'Holds not filled with items from closed libraries'
> #   at t/db_dependent/HoldsQueue.t line 327.
> #          got: '3'
> #     expected: '2'
> # Looks like you failed 2 tests of 23.
> t/db_dependent/HoldsQueue.t ................. Dubious, test returned 2
> (wstat 512, 0x200)
> Failed 2/23 subtests 
> 
> Could this conflict with the calendar changes for hourly?
Comment 18 Jonathan Druart 2015-01-16 09:55:51 UTC
(In reply to Katrin Fischer from comment #16)
> I am sorry, but t/db_dependent/HoldsQueue.t is failing in my installation:

I got the same error, but when I executed the updatedb entry, the tests passed.
Comment 19 Katrin Fischer 2015-01-19 07:09:28 UTC
Hi Kyle and Jonathan, 

I retested and made sure I ran the database update.

ok 20 - Holds queue filling 2nd correct hold for default holds policy 'from home library'
not ok 21 - Holds not filled with items from closed libraries
#   Failed test 'Holds not filled with items from closed libraries'
#   at t/db_dependent/HoldsQueue.t line 310.
#          got: '2'
#     expected: '1'
ok 22 - Holds queue filling correct number for holds for default holds policy 'from any library'
not ok 23 - Holds not filled with items from closed libraries
#   Failed test 'Holds not filled with items from closed libraries'
#   at t/db_dependent/HoldsQueue.t line 327.
#          got: '3'
#     expected: '2'
# Looks like you failed 2 tests of 23.

Without the patch and before the database update all the tests pass nicely on my db.
Comment 20 Jonathan Druart 2015-02-12 10:30:27 UTC
Tried again, and got the error.

MariaDB [koha_ut]> select count(*) from systempreferences where variable="HoldsQueueSkipClosed";
+----------+
| count(*) |
+----------+
|        1 |
+----------+

% git diff
diff --git a/t/db_dependent/HoldsQueue.t b/t/db_dependent/HoldsQueue.t
index a73eac1..b67a2e2 100755
--- a/t/db_dependent/HoldsQueue.t
+++ b/t/db_dependent/HoldsQueue.t
@@ -307,6 +307,7 @@ C4::Calendar->new( branchcode => 'MPL' )->insert_single_holiday(
 );
 C4::HoldsQueue::CreateQueue();
 $holds_queue = $dbh->selectall_arrayref("SELECT * FROM tmp_holdsqueue", { Slice => {} });
+warn Data::Dumper::Dumper $holds_queue;
 is( scalar( @$holds_queue ), 1, "Holds not filled with items from closed libraries" );
 C4::Context->set_preference('HoldsQueueSkipClosed', 0);


% prove t/db_dependent/HoldsQueue.t
t/db_dependent/HoldsQueue.t .. 18/23 $VAR1 = [
          { 
            'itemcallnumber' => undef,
            'reservedate' => '2015-02-12',
            'borrowernumber' => '70',
            'phone' => undef,
            'surname' => 'Surname',
            'holdingbranch' => 'CPL',
            'pickbranch' => 'CPL',
            'title' => 'Test Holds Queue XXX',
            'item_level_request' => '0',
            'notes' => undef,
            'biblionumber' => '14',
            'firstname' => 'Firstname',
            'cardnumber' => 'CARDNUMBER1',
            'itemnumber' => '127',
            'barcode' => '350699408070'
          },
          { 
            'reservedate' => '2015-02-12',
            'itemcallnumber' => undef,
            'borrowernumber' => '71',
            'surname' => 'Surname',
            'phone' => undef,
            'pickbranch' => 'CPL',
            'holdingbranch' => 'FFL',
            'item_level_request' => '0',
            'title' => 'Test Holds Queue XXX',
            'notes' => undef,
            'firstname' => 'Firstname',
            'biblionumber' => '14',
            'cardnumber' => 'CARDNUMBER2',
            'itemnumber' => '128',
            'barcode' => '350699408071'
          }
        ];

#   Failed test 'Holds not filled with items from closed libraries'
#   at t/db_dependent/HoldsQueue.t line 311.
#          got: '2'
#     expected: '1'

#   Failed test 'Holds not filled with items from closed libraries'
#   at t/db_dependent/HoldsQueue.t line 328.
#          got: '3'
#     expected: '2'
# Looks like you failed 2 tests of 23.
t/db_dependent/HoldsQueue.t .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/23 subtests 

Test Summary Report
-------------------
t/db_dependent/HoldsQueue.t (Wstat: 512 Tests: 23 Failed: 2)
  Failed tests:  21, 23
  Non-zero exit status: 2
Files=1, Tests=23,  3 wallclock secs ( 0.02 usr  0.00 sys +  1.96 cusr  0.04 csys =  2.02 CPU)
Result: FAIL
Comment 21 Jonathan Druart 2015-02-12 10:31:13 UTC
And I confirm the tests pass on master.
Comment 22 Kyle M Hall 2015-02-13 16:47:21 UTC
Starting with a fresh database with all sample data.
1) t/db_dependent/HoldsQueue.t passes on master
2) t/db_dependent/HoldsQueue.t passes with bug 12803 pre-updatedatabase.pl
3) t/db_dependent/HoldsQueue.t passes with bug 12803 post-updatedatabse.pl

I have no idea where to go from here.
Comment 23 Tomás Cohen Arazi 2015-02-13 17:57:50 UTC
I have an Ubuntu 14.04 (amd64) box, all stock (I only have the koha repo, and the git ppa for the latest git).

What I did was:
- > DROP DATABASE koha_devel; CREATE DATABASE koha_devel;
- Went through the install process, all default data loaded.
- Run
  $ prove t/db_dependent/HoldsQueue.t
=> SUCCESS: all green
- Applied the patches (small rebase for updatedatabase.pl)
- Run
  $ perl installer/data/mysql/updatedatabase.pl
Upgrade to 3.19.00.XXX done (Bug 12803 - Add ability to skip closed libraries when generating the holds queue)
- Run
  $ prove t/db_dependent/HoldsQueue.t
=> FAIL:

t/db_dependent/HoldsQueue.t .. 23/23 
#   Failed test 'Holds not filled with items from closed libraries'
#   at t/db_dependent/HoldsQueue.t line 327.
#          got: '1'
#     expected: '2'
# Looks like you failed 1 test of 23.
t/db_dependent/HoldsQueue.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/23 subtests 

Test Summary Report
-------------------
t/db_dependent/HoldsQueue.t (Wstat: 256 Tests: 23 Failed: 1)
  Failed test:  23
  Non-zero exit status: 1
Files=1, Tests=23,  2 wallclock secs ( 0.02 usr  0.00 sys +  1.49 cusr  0.06 csys =  1.57 CPU)
Result: FAIL
Comment 24 Kyle M Hall 2015-10-16 13:54:37 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2015-10-16 13:54:46 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2015-10-16 13:59:32 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall 2015-10-16 13:59:41 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2015-10-19 10:46:50 UTC
% prove t/db_dependent/HoldsQueue.t
t/db_dependent/HoldsQueue.t .. ok     
All tests successful.
Files=1, Tests=22,  3 wallclock secs ( 0.04 usr  0.00 sys +  1.90 cusr  0.02 csys =  1.96 CPU)
Result: PASS
% git bz apply 12803
Bug 12803 - Add ability to skip closed libraries when generating the holds queue

43511 - Bug 12803 - Add ability to skip closed libraries when generating the holds queue
43512 - Bug 12803 [QA Followup] - Allow holiday caching to be disabled for testing purposes

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 12803 - Add ability to skip closed libraries when generating the holds queue
Applying: Bug 12803 [QA Followup] - Allow holiday caching to be disabled for testing purposes
% qa -c 2 -v 2
testing 2 commit(s) (applied to 390233a 'Bug 14966: Table header broken at det')

Processing files before patches
|========================>| 4 / 4 (100.00%)
Processing files after patches
|========================>| 4 / 4 (100.00%)

 OK     Koha/Calendar.pm
   OK     critic
   OK     forbidden patterns
   OK     pod
   OK     spelling
   OK     valid

 FAIL   t/db_dependent/HoldsQueue.t
   OK     critic
   FAIL   forbidden patterns
                forbidden pattern: trailing space char (line 322)
   OK     pod
   OK     spelling
   OK     valid

 OK     C4/HoldsQueue.pm
   OK     critic
   OK     forbidden patterns
   OK     pod
   OK     spelling
   OK     valid

 FAIL   installer/data/mysql/sysprefs.sql
   FAIL   sysprefs_order
                Not blocker: Sysprefs HoldsQueueSkipClosed is bad placed (see bug 10610)

% prove t/db_dependent/HoldsQueue.t
t/db_dependent/HoldsQueue.t .. 13/24 
#   Failed test 'Holds not filled with items from closed libraries'
#   at t/db_dependent/HoldsQueue.t line 327.
#          got: '1'
#     expected: '2'
# Looks like you failed 1 test of 24.
t/db_dependent/HoldsQueue.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/24 subtests 

Test Summary Report
-------------------
t/db_dependent/HoldsQueue.t (Wstat: 256 Tests: 24 Failed: 1)
  Failed test:  23
  Non-zero exit status: 1
Files=1, Tests=24,  3 wallclock secs ( 0.04 usr  0.00 sys +  1.76 cusr  0.06 csys =  1.86 CPU)
Result: FAIL
Comment 29 Jonathan Druart 2015-10-19 10:47:31 UTC
Comment on attachment 43511 [details] [review]
Bug 12803 - Add ability to skip closed libraries when generating the holds queue

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

::: C4/HoldsQueue.pm
@@ +29,4 @@
>  use C4::Circulation;
>  use C4::Members;
>  use C4::Biblio;
> +use C4::Dates qw/format_date/;

Don't use C4::Dates, we are going to kick it out.
Comment 30 Kyle M Hall 2015-10-19 13:14:35 UTC
> Don't use C4::Dates, we are going to kick it out.

Can you please point me to the method I should be using? I don't see a replacement in Koha::Calendar!
Comment 31 Jonathan Druart 2015-10-19 13:41:50 UTC
(In reply to Kyle M Hall from comment #30)
> > Don't use C4::Dates, we are going to kick it out.
> 
> Can you please point me to the method I should be using? I don't see a
> replacement in Koha::Calendar!

I was talking about C4::Dates, you should use Koha::DateUtils instead.
Comment 32 Kyle M Hall 2015-10-19 18:29:41 UTC
(In reply to Jonathan Druart from comment #31)
> (In reply to Kyle M Hall from comment #30)
> > > Don't use C4::Dates, we are going to kick it out.
> > 
> > Can you please point me to the method I should be using? I don't see a
> > replacement in Koha::Calendar!
> 
> I was talking about C4::Dates, you should use Koha::DateUtils instead.

Ah, I see now. I was looking at the wrong code!
Comment 33 Kyle M Hall 2015-10-19 18:32:35 UTC Comment hidden (obsolete)
Comment 34 Jonathan Druart 2015-10-21 13:43:40 UTC
Kyle, comment 28 is not fixed.
Comment 35 Kyle M Hall 2015-10-22 18:41:10 UTC Comment hidden (obsolete)
Comment 36 Kyle M Hall 2015-10-22 18:42:32 UTC
> % prove t/db_dependent/HoldsQueue.t
> t/db_dependent/HoldsQueue.t .. 13/24 
> #   Failed test 'Holds not filled with items from closed libraries'
> #   at t/db_dependent/HoldsQueue.t line 327.
> #          got: '1'
> #     expected: '2'
> # Looks like you failed 1 test of 24.
> t/db_dependent/HoldsQueue.t .. Dubious, test returned 1 (wstat 256, 0x100)
> Failed 1/24 subtests 
> 
> Test Summary Report
> -------------------
> t/db_dependent/HoldsQueue.t (Wstat: 256 Tests: 24 Failed: 1)
>   Failed test:  23
>   Non-zero exit status: 1
> Files=1, Tests=24,  3 wallclock secs ( 0.04 usr  0.00 sys +  1.76 cusr  0.06
> csys =  1.86 CPU)
> Result: FAIL

So far I have been completely unable to reproduce this failure using my test database or a fresh Koha db with all the sample data. Have you tested it with a fresh sample data database?
Comment 37 Jonathan Druart 2015-10-23 07:51:36 UTC
(In reply to Kyle M Hall from comment #36)
> So far I have been completely unable to reproduce this failure using my test
> database or a fresh Koha db with all the sample data. Have you tested it
> with a fresh sample data database?

Everything passes with a fresh install, but then add a biblio and the tests won't pass.
Comment 38 Kyle M Hall 2015-10-23 13:54:35 UTC Comment hidden (obsolete)
Comment 39 Jonathan Druart 2015-10-23 15:03:31 UTC
(In reply to Kyle M Hall from comment #38)
> Created attachment 43833 [details] [review] [review]
> Bug 12803 [QA Followup] - Fix unit tests

This is a bit radical and I would like to know why it fails.
I don't understand why adding 1 biblio to the DB makes your tests failing.
Comment 40 Kyle M Hall 2015-10-23 17:36:42 UTC
(In reply to Jonathan Druart from comment #39)
> (In reply to Kyle M Hall from comment #38)
> > Created attachment 43833 [details] [review] [review] [review]
> > Bug 12803 [QA Followup] - Fix unit tests
> 
> This is a bit radical and I would like to know why it fails.
> I don't understand why adding 1 biblio to the DB makes your tests failing.

I wish I could help you find out why, but I can't seem to reproduce your error! I tried with 1 bib and 0, 1 and 2 items. There must be something else going on in your database . If you can send me a copy of your test database I'll give it a shot!
Comment 41 Katrin Fischer 2015-10-25 15:00:15 UTC
Not much help here I am afraid, but adding to the weirdness.
Tests passed the second time I ran them, then they kept failing constantly. Additional weirdness: it's not always the same tests failing:

Sometimes: perl t/db_dependent/HoldsQueue.t

...
not ok 23 - Holds not filled with items from closed libraries
#   Failed test 'Holds not filled with items from closed libraries'
#   at t/db_dependent/HoldsQueue.t line 337.
#          got: '1'
#     expected: '2'


Other times: perl t/db_dependent/HoldsQueue.t
...
not ok 21 - Holds not filled with items from closed libraries
#   Failed test 'Holds not filled with items from closed libraries'
#   at t/db_dependent/HoldsQueue.t line 320.
#          got: '2'
#     expected: '1'
ok 22 - Holds queue filling correct number for holds for default holds policy 'from any library'
not ok 23 - Holds not filled with items from closed libraries
#   Failed test 'Holds not filled with items from closed libraries'
#   at t/db_dependent/HoldsQueue.t line 337.
#          got: '3'
#     expected: '2'
ok 24 - Bug 14297 - Holds Queue building ignoring holds where pickup & home branch don't match and item is not from least cost branch
# Looks like you failed 2 tests of 24.
Comment 42 Jonathan Druart 2015-10-26 08:31:30 UTC
(In reply to Kyle M Hall from comment #40)
> (In reply to Jonathan Druart from comment #39)
> > (In reply to Kyle M Hall from comment #38)
> > > Created attachment 43833 [details] [review] [review] [review] [review]
> > > Bug 12803 [QA Followup] - Fix unit tests
> > 
> > This is a bit radical and I would like to know why it fails.
> > I don't understand why adding 1 biblio to the DB makes your tests failing.
> 
> I wish I could help you find out why, but I can't seem to reproduce your
> error! I tried with 1 bib and 0, 1 and 2 items. There must be something else
> going on in your database . If you can send me a copy of your test database
> I'll give it a shot!

I used the DB generated by bug 10337.
Comment 43 Kyle M Hall 2015-10-26 10:57:46 UTC
Created attachment 43979 [details]
Test Database SQL
Comment 44 Kyle M Hall 2015-10-26 10:59:23 UTC
Please try running the unit tests against the attached Test Database SQL file. I've run the unit test hundreds of times with this database and got no errors. I am very interested to see if you get any errors using the same database.
Comment 45 Jonathan Druart 2015-10-26 11:39:38 UTC
(In reply to Kyle M Hall from comment #44)
> Please try running the unit tests against the attached Test Database SQL
> file. I've run the unit test hundreds of times with this database and got no
> errors. I am very interested to see if you get any errors using the same
> database.

I confirm that it fails here.

I am using mysql  Ver 15.1 Distrib 10.0.19-MariaDB, for debian-linux-gnu (i686) using readline 5.2
Comment 46 Katrin Fischer 2015-10-26 11:58:38 UTC
I was using Ubuntu with MySQL - using my 'normal' db with quite a bit of test data by now. Not sure we can find the 'common' thing for those who fail and does who fail not easily :(
Comment 47 Kyle M Hall 2015-12-09 17:39:09 UTC
I'm not sure what I can possibly do to get this Passed QA. I get no unit test failures. You aren't getting them with my "Fix unit tests" followup ( iirc ). So there is not really anything I can do on my what. Jonthan, can you please investigate this since you are getting the failures as well? Thanks!
Comment 48 Jonathan Druart 2015-12-10 09:52:51 UTC
(In reply to Kyle M Hall from comment #47)
> I'm not sure what I can possibly do to get this Passed QA. I get no unit
> test failures. You aren't getting them with my "Fix unit tests" followup (
> iirc ). So there is not really anything I can do on my what. Jonthan, can
> you please investigate this since you are getting the failures as well?
> Thanks!

If the tests fail using the same DB as you, it's not related to the patches or the DB. Alone I cannot do anything.
Are you using the same version of mysql as me?
Comment 49 Kyle M Hall 2015-12-10 14:33:31 UTC Comment hidden (obsolete)
Comment 50 Kyle M Hall 2015-12-10 14:33:39 UTC Comment hidden (obsolete)
Comment 51 Kyle M Hall 2015-12-10 14:33:42 UTC Comment hidden (obsolete)
Comment 52 Kyle M Hall 2015-12-10 14:33:45 UTC Comment hidden (obsolete)
Comment 53 Kyle M Hall 2015-12-10 14:33:48 UTC Comment hidden (obsolete)
Comment 54 Kyle M Hall 2015-12-10 14:33:51 UTC Comment hidden (obsolete)
Comment 55 Kyle M Hall 2015-12-10 14:47:07 UTC Comment hidden (obsolete)
Comment 56 Kyle M Hall 2015-12-10 14:47:15 UTC Comment hidden (obsolete)
Comment 57 Kyle M Hall 2015-12-10 14:50:17 UTC
Jonathan, Katrin, can you please try this new patch set? I think the last patch fixes the reason your tests are failing! I've removed the other 'fix' patch so you should be able to tell.
Comment 58 Jonathan Druart 2015-12-11 08:55:28 UTC
(In reply to Kyle M Hall from comment #57)
> Jonathan, Katrin, can you please try this new patch set? I think the last
> patch fixes the reason your tests are failing! I've removed the other 'fix'
> patch so you should be able to tell.

The tests now pass but... I have apply the change of the last patch to my old branch and they don't pass. Did you change something else?
Comment 59 Kyle M Hall 2015-12-11 15:10:08 UTC
(In reply to Jonathan Druart from comment #58)
> (In reply to Kyle M Hall from comment #57)
> > Jonathan, Katrin, can you please try this new patch set? I think the last
> > patch fixes the reason your tests are failing! I've removed the other 'fix'
> > patch so you should be able to tell.
> 
> The tests now pass but... I have apply the change of the last patch to my
> old branch and they don't pass. Did you change something else?

Yes, the second to last patch "Update unit tests due to changes in master" also makes some changes to HoldsQueue.t do to the fact that the tests no longer rely on the 'MPL' branchcode but instead generate random branches using TestBuilder.
Comment 60 Jonathan Druart 2015-12-14 09:54:37 UTC
1/ Please double check when you fix conflict on rebasing.
You are duplicating lines.
See use statements in t/db_dependent/HoldsQueue.t

2/ I think we could avoid the NO_CACHE env variable, which is not really meaningful and is only used once, for tests.
It seems more suitable to clear the cache from the tests, don't you think?
Comment 61 Kyle M Hall 2015-12-14 11:57:41 UTC Comment hidden (obsolete)
Comment 62 Kyle M Hall 2015-12-14 12:57:10 UTC Comment hidden (obsolete)
Comment 63 Kyle M Hall 2015-12-14 12:59:59 UTC
Fixed! 

For reasons I cannot fathom, if I call is_holiday on the branch in question before building the queue, Koha::Calendar picks up the holiday and uses it later. If is_holiday is *not* called, later on when HoldQueue.pm using is_holiday it will not see the holiday in the database. I cannot explain this but it is clearly some kind of bad cache/database/unit test interaction that is far outside the scope of this bug.
Comment 64 Kyle M Hall 2015-12-14 13:03:01 UTC Comment hidden (obsolete)
Comment 65 Jonathan Druart 2016-01-14 13:36:37 UTC
Kyle,
There is a small conflict to fix.
Do you mind if we mark this patch dependent on bug 15391?
Comment 66 Kyle M Hall 2016-01-19 14:55:39 UTC Comment hidden (obsolete)
Comment 67 Kyle M Hall 2016-01-19 14:55:47 UTC Comment hidden (obsolete)
Comment 68 Kyle M Hall 2016-01-19 14:55:50 UTC Comment hidden (obsolete)
Comment 69 Kyle M Hall 2016-01-19 14:55:53 UTC Comment hidden (obsolete)
Comment 70 Kyle M Hall 2016-01-19 14:55:56 UTC Comment hidden (obsolete)
Comment 71 Kyle M Hall 2016-01-19 14:56:00 UTC Comment hidden (obsolete)
Comment 72 Kyle M Hall 2016-01-19 14:56:03 UTC Comment hidden (obsolete)
Comment 73 Kyle M Hall 2016-01-19 14:56:06 UTC Comment hidden (obsolete)
Comment 74 Kyle M Hall 2016-01-19 15:01:24 UTC
(In reply to Jonathan Druart from comment #65)
> Kyle,
> There is a small conflict to fix.
> Do you mind if we mark this patch dependent on bug 15391?

Fixed!

I'm a bit reticent to make this dependent on bug 15391. Bug 15391 has no patches yet, indeed we haven't even truly nailed down the issue. Additionally, there is no real interaction / interdependence between the tests added here and the issues on bug 15391. My fear is that bug 15391 will take a long time to solve, which will hold up this patch set that's already been in process for years now. That being said, I'll leave the call up to you!
Comment 75 Jonathan Druart 2016-01-25 10:53:35 UTC Comment hidden (obsolete)
Comment 76 Jonathan Druart 2016-01-25 10:53:42 UTC Comment hidden (obsolete)
Comment 77 Jonathan Druart 2016-01-25 10:55:05 UTC
Kyle,
To easy the readability, I have squashed the qa follow-ups, especially because of the NOCACHE param, which has been added and finally removed.
Comment 78 Kyle M Hall 2016-01-28 19:18:17 UTC
(In reply to Jonathan Druart from comment #77)
> Kyle,
> To easy the readability, I have squashed the qa follow-ups, especially
> because of the NOCACHE param, which has been added and finally removed.

That's always fine by me!
Comment 79 Brendan Gallagher 2016-03-02 22:54:54 UTC
Didn't apply cleanly - I know because of the moving target that master is.  Please rebase.  Once it's done - leave me an @later in #koha and I'll jump right on it.  Thank you so much for the time and bother of rebasing.

Brendan
Comment 80 Mark Tompsett 2016-03-03 04:10:27 UTC
Created attachment 48574 [details] [review]
Bug 12803 - Add ability to skip closed libraries when generating the holds queue

The holds queue is typically generated many times a day in order to
select items to fill holds. Often these items are to be sent to a
different library. However, if the library whose item is picked to fill
a hold is closed, that hold will remain unfilled even if there are other
open libraries who own that item. It would be helpful if we could skip
closed libraries for the purpose of selecting items to fill holds.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Create a record with two items on it, one at Branch A, and one at
   Branch B
4) Place a hold for pickup at Branch C
5) Generate the holds queue
6) Note which branch's item is selected for the hold
7) Enable the new system preference HoldsQueueSkipClosed
8) Add today as a holiday for that branch noted in step 6
9) Regenerate the holds queue
10) View the holds queue, notice the item selected is not from
    the closed branch!
11) prove t/db_dependent/HoldsQueue.t

Signed-off by: Jason Robb <jrobb@sekls.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 81 Mark Tompsett 2016-03-03 04:10:33 UTC
Created attachment 48575 [details] [review]
Bug 12803 [QA Followup] - Allow holiday caching to be disabled for testing purposes

Bug 12803 [QA Followup] - Remove use of C4::Dates

C4::Dates was being included, but not used in the code!

Bug 12803 [QA Followup] - Fix koha-qa.pl errors

Bug 12803 [QA Followup] - Update unit tests due to changes in master

Bug 12803 [QA Followup] - Fix to stop failing unit tests

Bug 12803 [QA Followup] - Remove duplicate 'use' lines

Bug 12803 [QA Followup] - Remove NO_CACHE

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 82 Mark Tompsett 2016-03-03 04:17:39 UTC
I'm running through patch does not apply queue.
Only HoldsQueue.t file changed in terms of use statements.
Test file proves fine. Everything else identical.
Comment 83 Brendan Gallagher 2016-03-03 20:28:42 UTC
Pushed to Master - Should be in the May 2016 release!  Thanks!
Comment 84 Katrin Fischer 2017-08-27 10:29:54 UTC
*** Bug 12095 has been marked as a duplicate of this bug. ***
Comment 85 Phil Ringnalda 2019-05-08 17:51:26 UTC
*** Bug 8562 has been marked as a duplicate of this bug. ***