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.

Kyle, could you please use Koha::Calendar instead of C4::Calendar?

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.

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.

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.

Looking at this now.

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?

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?

(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.

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.

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

And I confirm the tests pass on master.

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.

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

% 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 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.

> 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!

(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.

(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!

Kyle, comment 28 is not fixed.

> % 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?

(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.

(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.

(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!

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.

(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.

Created attachment 43979 [details]
Test Database SQL

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.

(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

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 :(

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!

(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?

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.

(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?

(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.

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?

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.

Kyle,
There is a small conflict to fix.
Do you mind if we mark this patch dependent on bug 15391?

(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!

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.

(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!

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

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>

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>

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.

Pushed to Master - Should be in the May 2016 release!  Thanks!

*** Bug 12095 has been marked as a duplicate of this bug. ***

*** Bug 8562 has been marked as a duplicate of this bug. ***