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.
Created attachment 13799 [details] [review] Bug 9180 : Use default overdue rules when specific rules are not defined for a branch Test plan : - define default overdue rules - define overdue rules for one branch - find a user with late documents in another branch Without the patch, if you run misc/cronjobs/overdue_notices.pl -n -v you can see that only branch with specific rules is processed. The user does not get any notice. With the patch all branches are processes, those without specific rules using default rules. A notice is generated for the user.
Also tested that without default rules, only branch with specific rules is processed. And without any rules at all, nothing is processed.
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.
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.
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.
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 '' ?
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.
See bug 3393 and all the duplicates linked with it.
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
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.
(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
Created attachment 22574 [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.
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?
(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.
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.
(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.
Created attachment 25947 [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>
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)?
Created attachment 27427 [details] [review] Bug 9180: Use selectcol instead of selectrow The selectrow_* method only returns the *first* row of data!
(In reply to Katrin Fischer from comment #18) Oops, sorry Katrin, noob error. Fixed in a followup.
This one is major, please test.
I cannot QA this patch, still major...
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.
Created attachment 29548 [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>
(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).
QA: Taking a look
Changing complexity: Changing a routine in a module (Overdues) is generally not considered trivial.
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>
Patch pushed to master. Thanks Jonathan!
Can this be backported to 3.16.x ?