Bug 9180 - Default rules are not always used for overdues
Summary: Default rules are not always used for overdues
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-30 13:35 UTC by Sophie MEYNIEUX
Modified: 2015-12-03 22:11 UTC (History)
9 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 9180 : Use default overdue rules when specific rules are not defined for a branch (1.51 KB, patch)
2012-11-30 14:41 UTC, Sophie MEYNIEUX
Details | Diff | Splinter Review
Bug 9180: All branches should be returned if a default rule exists (3.76 KB, patch)
2013-10-30 09:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9180: All branches should be returned if a default rule exists (3.80 KB, patch)
2014-03-07 17:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9180: Use selectcol instead of selectrow (2.38 KB, patch)
2014-04-22 16:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9180: All branches should be returned if a default rule exists (4.91 KB, patch)
2014-07-07 14:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9180: All branches should be returned if a default rule exists (5.00 KB, patch)
2014-07-31 12:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Sophie MEYNIEUX 2012-11-30 13:35:30 UTC
Default overdue rules are defined for a lybrary with several branches
Overdue rules are also defined for one specific branch

A user is late to return a book from a branch without overdue rules.
But default rules should apply. 
No notification is sent.
Comment 1 Sophie MEYNIEUX 2012-11-30 14:41:03 UTC Comment hidden (obsolete)
Comment 2 Sophie MEYNIEUX 2012-11-30 14:42:58 UTC
Also tested that without default rules, only branch with specific rules is processed.

And without any rules at all, nothing is processed.
Comment 3 Melia Meggs 2012-11-30 21:28:41 UTC
I can't seem to reproduce this bug on master.  Here's what I tried:

 - I set up default circulation rules.  
 - I set up circ rules for Centerville Branch.  
 - I found a patron from Midway Branch with overdue items.  (Midway Branch has no specific circ rules so should be using the default circ rules.)
 - I ran misc/cronjobs/overdue_notices.pl -n -v.

I found that an overdue notice was correctly generated a notice for my Midway Branch patron, as well as patrons from other branches.
Comment 4 Katrin Fischer 2012-12-01 07:14:26 UTC
Hi Melia, thank you for all your testing work!
Reading through this bug, I think Sophie meant the overdue notice triggers in tools - could you take a look at those? I think to make the overdues job use those you have to use -t as an option when running it after setting the rules up.
Comment 5 Chris Cormack 2013-01-01 22:21:51 UTC
I also can not recreate, with rules set up in overdue notices and running with -n -v and -n -v -t

I get the same output before and after the patch.
Comment 6 Sophie MEYNIEUX 2013-02-02 08:07:00 UTC
Another of our library client complains about overdue notices not generated.

They've got overduerules for all kind of patrons and specific rules only for one branch U1BUCG :

Here is the output logs I've got :
"Found 1 branch with first message enabled: 'U1BUCG'
branch U1AGCCPF passed on parameter
No active overduerules for branch  'U1AGCCPF'
No active overduerules for DEFAULT either!"

In database, default overdue rules are stored with '' as a branchcode. And the function that get all overduerules does not select thoses  (C4::Overdues::GetBranchcodesWithOverdueRules):
"SELECT DISTINCT branchcode FROM overduerules WHERE delay1 IS NOT NULL AND branchcode <> '' ORDER BY branchcode"

I don't know how it can work without the patch... Maybe your default rules got another branchcode than '' ?
Comment 7 Katrin Fischer 2013-02-02 08:19:36 UTC
We never use the default rules but always set up specific rules for each branch, because there has been problems with the default for a longer while. I am not sure if the problem is still on master or why Chris couldn't recreate - but I know there have been other bugs for this around.
Comment 8 Katrin Fischer 2013-02-02 08:27:35 UTC
See bug 3393 and all the duplicates linked with it.
Comment 9 Sophie MEYNIEUX 2013-02-04 13:23:46 UTC
bug 3393 says the patch does not apply because "it looks like" another patch has fixed the pb. But I can't find any patch that corrects it in C4/Overdues.pm history.
My patch does the same as bug 3393 and should be applied in order default rules are taken into account
Comment 10 Marcel de Rooy 2013-03-26 15:07:22 UTC
This patch got status Signed off by koha.aixmarseille@gmail.com.
Please add a signoff line and explain what/how you tested in view of the multiple comments referring to impossibilities to test it.
Thanks for your patience.
Comment 11 Jonathan Druart 2013-10-29 15:53:35 UTC
(In reply to M. de Rooy from comment #10)
> This patch got status Signed off by koha.aixmarseille@gmail.com.
> Please add a signoff line and explain what/how you tested in view of the
> multiple comments referring to impossibilities to test it.
> Thanks for your patience.

Switch back to Needs Signoff, I don't understand the "Failed QA" status
Comment 12 Jonathan Druart 2013-10-30 09:52:36 UTC Comment hidden (obsolete)
Comment 13 Katrin Fischer 2013-10-30 10:08:47 UTC
Hi Jonathan, sorry for commenting without testing - but I am not sure I understand your comment correctly. I think if a specific rule exists, we only want to use this rule, so why would returning only the specific rule be a bug?
Comment 14 Jonathan Druart 2013-10-30 10:17:22 UTC
(In reply to Katrin Fischer from comment #13)
> Hi Jonathan, sorry for commenting without testing - but I am not sure I
> understand your comment correctly. I think if a specific rule exists, we
> only want to use this rule, so why would returning only the specific rule be
> a bug?

Katrin,
The routine does not return the rules but the branchcodes.
So if a default rule exists in the DB, all branchcodes should be returned.
Comment 15 Katrin Fischer 2013-10-30 10:19:33 UTC
Hm, why not only the branchcodes for those where no specific rule exists? I should really have tested first, sorry for confusing. Overdues is quite a sensitive area for us.
Comment 16 Jonathan Druart 2014-01-15 13:00:11 UTC
(In reply to Katrin Fischer from comment #15)
> Hm, why not only the branchcodes for those where no specific rule exists? I
> should really have tested first, sorry for confusing. Overdues is quite a
> sensitive area for us.

The routine returns all branchcodes with overdue rules defined.
If you have only specific rules, all branches with at least a rule defined will be returned.
But if you have at least 1 default rule, ALL branchcodes should be returned (the default rule affect all branches).

If the routine does not return all branchcodes, the issue described by Sophie occurs.

It will only affect instance with at least a default rule defined.

This is marked as major but I think it is a critical one.
Comment 17 Kyle M Hall 2014-03-07 17:59:08 UTC Comment hidden (obsolete)
Comment 18 Katrin Fischer 2014-04-20 10:40:43 UTC
Hi Sophie,

I have tested this twice, but I think there is still a bug/regression in this:

Test user from library A has checked out
- various books from library A and B in A
- books from libary B in B

./misc/cronjobs/overdue_notices.pl -t -v

Tests: Overdue notice triggers
1) only defined for default/all libraries
  - with and without patch: 2 notices are generated.
    Both generated notices contain a complete list of overdue items
    from both branches.
2) defined for default/all libraries and library C
  - without patch: no notice is generated
  - with patch: 2 notices are generated.
3) defined for default/all libraries and library A
  - without patch: only a notice for library A is generated
  - with patch: 2 notices are generated.
4) only defined for library A
  - with and without patch: only 1 notice is generated for library A
5) defined for library A and B, no default rule
  - without patch: 2 messages are generated
  - with patch: only 1 message is generated

For this bug: 2) seems ok with the patch, but we seem to introduce a regression on 5).

In general: Should the item lists be identical for the patches? I think it would only make sense if each overdue notice contained a complementary list of items. But should they be divided by checkout library/homebranch? Should it be controlled by one of the existing preferences (CircControl or HomeOrHoldingBranch)?
Comment 19 Jonathan Druart 2014-04-22 16:26:07 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2014-04-22 16:26:27 UTC
(In reply to Katrin Fischer from comment #18)
Oops, sorry Katrin, noob error.
Fixed in a followup.
Comment 21 Jonathan Druart 2014-06-02 15:24:05 UTC
This one is major, please test.
Comment 22 Jonathan Druart 2014-06-26 14:11:45 UTC
I cannot QA this patch, still major...
Comment 23 Katrin Fischer 2014-07-06 20:52:49 UTC
Doing some first tests on this, noticing a few things:

1) The unit tests (yay!) are in a new file Overdues.pl - I think they need to be integrated into Overdues.t or the file should be renamed to be a .t file.

2) While t/db_dependent/Overdues.t passes with the tests, Ovedues.pl does not:

bumblebee:~/kohaclone (19-9180-defaultoverduerules) $ perl t/db_dependent/Overdues.pl 
1..4
not ok 1 - If a default rule exists, all branches should be returned
#   Failed test 'If a default rule exists, all branches should be returned'
#   at t/db_dependent/Overdues.pl line 26.
#     Structures begin differing at:
#          $got->[0] = 'TPL'
#     $expected->[0] = 'FRL'
not ok 2 - If a default rule exists and a specific rule exists, all branches should be returned
#   Failed test 'If a default rule exists and a specific rule exists, all branches should be returned'
#   at t/db_dependent/Overdues.pl line 36.
#     Structures begin differing at:
#          $got->[0] = 'RPL'
#     $expected->[0] = 'FRL'
ok 3 - If only a specific rule exist, only 1 branch should be returned
ok 4 - If only 2 specific rules exist, 2 branches should be returned
# Looks like you failed 2 tests of 4.
Comment 24 Jonathan Druart 2014-07-07 14:01:29 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2014-07-07 14:03:07 UTC
(In reply to Katrin Fischer from comment #23)
> Doing some first tests on this, noticing a few things:
> 
> 1) The unit tests (yay!) are in a new file Overdues.pl - I think they need
> to be integrated into Overdues.t or the file should be renamed to be a .t
> file.

I really don't understand what I did...
I squashed the patches, rename the unit tests file and added a "sort" on comparing the array (this fixes the test failures).
Comment 26 Marcel de Rooy 2014-07-31 11:30:35 UTC
QA: Taking a look
Comment 27 Marcel de Rooy 2014-07-31 11:36:30 UTC
Changing complexity: Changing a routine in a module (Overdues) is generally not considered trivial.
Comment 28 Marcel de Rooy 2014-07-31 12:00:48 UTC
Created attachment 30392 [details] [review]
Bug 9180: All branches should be returned if a default rule exists

The C4::Overdues::GetBranchcodesWithOverdueRules routine has a bug.
If a default rule *and* a specific rule exist, only the branchcode for
the specific rule is returned.

Test plan:
prove t/db_dependent/Overdues.t
and verify the unit tests are consistent.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 29 Tomás Cohen Arazi 2014-08-19 12:35:11 UTC
Patch pushed to master.

Thanks Jonathan!
Comment 30 Fridolin Somers 2015-04-02 08:50:30 UTC
Can this be backported to 3.16.x ?