Bug 8361

Summary: Issuing rule if no rule is defined
Product: Koha Reporter: Paul Poulain <paul.poulain>
Component: CirculationAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact:
Severity: major    
Priority: P5 - low CC: cbrannon, claire.hernandez, gmcharlt, jonathan.druart, julian.maurice, kyle.m.hall, kyle, mtj, mtompset, nick, ztajoli
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15757
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
In the previous versions if no circulation rule was defined, Koha always allowed to check out. Now, with this development, Koha refuses check-out if no rule is found.
Version(s) released in:
Circulation function:
Bug Depends on:    
Bug Blocks: 8362    
Attachments: Bug 8361: Do not allow checkouts if no rules are defined
[SIGNED-OFF] Bug 8361: Do not allow checkouts if no rules are defined
Bug 8361: Do not allow checkouts if no rules are defined
Bug 8361 (QA Followup) Add warnings
Bug 8361 (QA Followup) Add warnings
Bug 8361 (QA Followup) Add warnings

Description Paul Poulain 2012-07-05 13:12:03 UTC
If we reach a case where no issuing rule is defined, Koha always grant issuing. Koha should refuse any check-out if no rule is found

(validating the issuing is silent, while rejecting the check-out will clearly show that something is missing in your configuration)
Comment 1 Jonathan Druart 2015-03-31 13:49:03 UTC
Is this still an issue?
Why should we refuse checkouts if no rule is defined?
Comment 2 Christopher Brannon 2016-04-06 14:15:12 UTC
(In reply to Jonathan Druart from comment #1)
> Is this still an issue?
> Why should we refuse checkouts if no rule is defined?

Because that is expected behavior.  If I don't put gas in the car, I don't expect it to still drive.  There are basic components that are just required.

I guess we could make Koha ask questions when certain things are missing, like when the item is due.  If that were the only piece missing, that would be fine.  But if there are no circ rules defined, chances are there is a lot more missing in setup.
Comment 3 Jonathan Druart 2016-04-06 14:24:57 UTC
(In reply to Christopher Brannon from comment #2)
> (In reply to Jonathan Druart from comment #1)
> > Is this still an issue?
> > Why should we refuse checkouts if no rule is defined?
> 
> Because that is expected behavior.  If I don't put gas in the car, I don't
> expect it to still drive.  There are basic components that are just required.

Yes exactly, so do not expect Koha to behave correctly if you did not configure it correctly :)
Comment 4 Jonathan Druart 2016-11-22 11:25:23 UTC
Created attachment 57704 [details] [review]
Bug 8361: Do not allow checkouts if no rules are defined

We should require a circulation rule to allow checkouts and reject them
if no rules are defined.

Test plan:
- Delete all issuing rules
- Check an item out
=> Without this patch the checkout is allowed
=> With this patch applied it is rejected
Comment 5 Mark Tompsett 2016-11-22 22:23:34 UTC
Created attachment 57717 [details] [review]
[SIGNED-OFF] Bug 8361: Do not allow checkouts if no rules are defined

We should require a circulation rule to allow checkouts and reject them
if no rules are defined.

Test plan:
- Delete all issuing rules
- Check an item out
=> Without this patch the checkout is allowed
=> With this patch applied it is rejected

NOTE: Nicely fails when test is unchanged too.
Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 6 Mark Tompsett 2016-11-22 22:28:39 UTC
Perhaps add an about.tt/about.pl check for issuing rules being defined?
Comment 7 Jonathan Druart 2016-11-23 07:56:36 UTC
(In reply to M. Tompsett from comment #6)
> Perhaps add an about.tt/about.pl check for issuing rules being defined?

I'd even suggest to bump up a warning on the main page.
Comment 8 Kyle M Hall (khall) 2016-12-23 13:21:17 UTC
Shouldn't the circulation.tt template be updated with a NO_RULE_DEFINED error case?
Comment 9 Jonathan Druart 2016-12-27 08:48:04 UTC
(In reply to Kyle M Hall from comment #8)
> Shouldn't the circulation.tt template be updated with a NO_RULE_DEFINED
> error case?

The message says "This patron can't check out this item per library circulation policy"
That sounds ok to me.
We could add a check somewhere else (about page).
Comment 10 Nick Clemens (kidclamp) 2017-01-13 18:11:20 UTC
Created attachment 58966 [details] [review]
Bug 8361: Do not allow checkouts if no rules are defined

We should require a circulation rule to allow checkouts and reject them
if no rules are defined.

Test plan:
- Delete all issuing rules
- Check an item out
=> Without this patch the checkout is allowed
=> With this patch applied it is rejected

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 11 Nick Clemens (kidclamp) 2017-01-13 18:11:26 UTC
Created attachment 58967 [details] [review]
Bug 8361 (QA Followup) Add warnings

- Added message to mainpage.pl if no rules defined
 - Added message to circulation.tt to warn if rule undefined for
patron/itemtype combination

To test:
1 - Remove all rules
2 - Note that there is a warning on mainpage
3 - Add one rule
4 - Checkout to patron an itemtype that is outside of rule
defined above
5 - Note explanation that no rule is deifned
Comment 12 Nick Clemens (kidclamp) 2017-01-13 18:12:49 UTC
Jonathan, your patches work, passing QA

I agree with Kyle though, let's give a reason if we know it.

Can you take a look at my followup?

-Nick
Comment 13 Jonathan Druart 2017-01-16 11:19:15 UTC
Nick,
Bug 17855 is going to add a circ rule during the installer process.
So I think it will no longer be necessary to check the existence of a circ rule from the mainpage.
Eventually we could add a check on the about page.
Comment 14 Nick Clemens (kidclamp) 2017-01-17 11:49:15 UTC
Created attachment 59086 [details] [review]
Bug 8361 (QA Followup) Add warnings

- Added message to circulation.tt to warn if rule undefined for
patron/itemtype combination

To test:
1 - Remove all circ rules
2 - Add one rule
3 - Checkout to patron an itemtype that is outside of rule
defined above
4 - Note explanation that no rule is defined
Comment 15 Nick Clemens (kidclamp) 2017-01-17 12:02:12 UTC
Created attachment 59087 [details] [review]
Bug 8361 (QA Followup) Add warnings

- Added message to circulation.tt to warn if rule undefined for
patron/itemtype combination

To test:
1 - Remove all circ rules
2 - Add one rule
3 - Checkout to patron an itemtype that is outside of rule
defined above
4 - Note explanation that no rule is defined
Comment 16 Kyle M Hall (khall) 2017-01-20 13:41:17 UTC
Pushed to master for 17.05, thanks Jonathan, Nick!
Comment 17 Katrin Fischer 2017-01-22 19:39:53 UTC
Contains strings - will check later if this is a candidate for 16.11.03.
Comment 18 Katrin Fischer 2017-02-07 06:54:29 UTC
These patches have been pushed to 16.11.x and will be in 16.11.04.
Comment 19 Julian Maurice 2017-02-09 08:49:18 UTC
Pushed to 3.22.x for 3.22.17
Comment 20 Mason James 2017-02-22 23:00:25 UTC
Pushed to 16.05.x, for 16.05.10 release