Bug 18321 - One more checkouts possible than allowed by rules
Summary: One more checkouts possible than allowed by rules
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 16272
Blocks:
  Show dependency treegraph
 
Reported: 2017-03-23 14:52 UTC by Marc Véron
Modified: 2017-11-02 16:28 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Screenshot circulation rules (39.74 KB, image/png)
2017-03-23 19:34 UTC, Marc Véron
Details
Screenshots to reproduce error (199.37 KB, application/pdf)
2017-03-27 16:15 UTC, Marc Véron
Details
Bug 18321: Add tests (2.98 KB, patch)
2017-03-31 17:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18321: Correctly calculate switch_onsite_checkout (1.73 KB, patch)
2017-03-31 17:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18321: Add tests (3.11 KB, patch)
2017-04-02 17:15 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18321: Correctly calculate switch_onsite_checkout (1.90 KB, patch)
2017-04-02 17:19 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18321: Add tests (3.17 KB, patch)
2017-04-12 10:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18321: Correctly calculate switch_onsite_checkout (1.95 KB, patch)
2017-04-12 10:58 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Véron 2017-03-23 14:52:26 UTC
It is possible to have one more checkouts than allowed by rules.

To recreate:

- Delete all rules in 'Circulation rules and fines'.
- Set a Default limit by patron category, e.g. Student 2 current checkouts , 2 on-site-chekouts.
- Checkout to a student until the limit is reached (in this case: 2)
- Try a 3rd checkout; it is possible (should not)
- Try a 4th checkout: No longer possible. 
  Message says: "Too many checked out. 3 checked out, only 2 are allowed."

Expected: Message should trigger with 3rd checkout, saying:
"Too many checked out. 2 checked out, only 2 are allowed."
Comment 1 Marc Véron 2017-03-23 15:26:20 UTC
Suppose it happens in comparisons in C4/Circulation.pm
Comment 2 Jonathan Druart 2017-03-23 18:53:29 UTC
I do not recreate, I get the warning on the 3rd checkouts. Are you checking out an on-site checkouts? If so what is the value of ConsiderOnSiteCheckoutsAsNormalCheckouts?

How do you create the circ rule? I just filled "Current checkouts allowed" and "Current on-site checkouts allowed", let other values blank.
Comment 3 Marc Véron 2017-03-23 19:34:14 UTC
Created attachment 61588 [details]
Screenshot circulation rules

(In reply to Jonathan Druart from comment #2)
> I do not recreate, I get the warning on the 3rd checkouts. Are you checking
> out an on-site checkouts? If so what is the value of
> ConsiderOnSiteCheckoutsAsNormalCheckouts?
> 
> How do you create the circ rule? I just filled "Current checkouts allowed"
> and "Current on-site checkouts allowed", let other values blank.

- I tested with normal checkouts.
- Just one circulation rule for students as described in my initial comment.
- ConsiderOnSiteCheckoutsAsNormalCheckouts is: Consider
Comment 4 Marc Véron 2017-03-23 19:42:21 UTC
With ConsiderOnSiteCheckoutsAsNormalCheckouts set to 'Don't consider' it is not possible to checkout 3 items, try to checkout the 3rd item, I get the message:
"Too many checked out. 2 checked out, only 2 are allowed." => As expected

With ConsiderOnSiteCheckoutsAsNormalCheckouts set to 'Consider' I can check out one more item than allowed as described in initial comment.
Comment 5 Jonathan Druart 2017-03-27 15:20:54 UTC
Would it be possible to get a screencast, or a sandbox set up with your settings?
I still do not recreate.
Comment 6 Marc Véron 2017-03-27 16:15:46 UTC
Created attachment 61646 [details]
Screenshots to reproduce error

Re-testing:

- Set ConsiderOnSiteCheckoutsAsNormalCheckouts to 'Consider'
- Delete all rules in 'Circulation rules and fines'.
- Set a Default limit by patron category, e.g. Student with 2 current checkouts, 2 on-site-chekouts.
- Normal (not On-Site) checkout to a student until the limit is reached (in this case: 2)
- Try a 3rd checkout; it is possible (should not)
- Try a 4th checkout: No longer possible. 
  Message says: "Too many checked out. 3 checked out, only 2 are allowed."

- Switch ConsiderOnSiteCheckoutsAsNormalCheckouts to 'Do not consider'
- Check all items in
- Repeat steps to reproduce
- Checkout is blocked when trying to do 3rd checkout (as expected)
Comment 7 Jonathan Druart 2017-03-28 12:15:04 UTC
Sorry but still works for me. Did you test on a sandbox?
Comment 8 Marc Véron 2017-03-28 13:33:45 UTC
(In reply to Jonathan Druart from comment #7)
> Sorry but still works for me. Did you test on a sandbox?

No.
Anything not clear with my screenshots / description (Comment #6)?
Comment 9 Jonathan Druart 2017-03-28 14:36:03 UTC
Everything is clear, but works for me :)
Comment 10 Marc Véron 2017-03-28 15:24:18 UTC
Hmm, thinking...

The strange thing is that in my tests the behaviour depends on the status of syspref 'ConsiderOnSiteCheckoutsAsNormalCheckouts'

With 'Do not consider' it works as expected, with 'Consider' I can check out one more than allowed (normal checkouts, not onsite checkouts).

What are your settings for onsite sysprefs?
(OnSiteCheckouts, OnSiteCheckoutsForce, SwitchOnSiteCheckouts, ConsiderOnSiteCheckoutsAsNormalCheckouts)
Comment 11 Marc Véron 2017-03-28 20:42:51 UTC
To reproduce, set following sysprefs:
- ConsiderOnSiteCheckoutsAsNormalCheckouts: Consider
- SwitchOnSiteCheckouts: Switch

That triggers a $delta with value 1 to be added to $max_checkouts_allowed in C4/Circulation.pm line 485, allowing one more checkout.

I could not figure out what this $delta stands for. It has a value of 1 if SwitchOnSiteCheckouts is set to 'Switch', otherwise it is 0.
Comment 12 Jonathan Druart 2017-03-31 16:56:13 UTC
Caused by bug 16272.
Comment 13 Jonathan Druart 2017-03-31 17:50:51 UTC
Created attachment 61759 [details] [review]
Bug 18321: Add tests

Clean a bit existing tests by adding default values and add a test to
highlight the issue.
Comment 14 Jonathan Druart 2017-03-31 17:50:57 UTC
Created attachment 61760 [details] [review]
Bug 18321: Correctly calculate switch_onsite_checkout

Silly error here, the $switch_onsite_checkout is not correctly
calculated, it should not only be set to the value of the syspref, but
meet all the conditions.

Test plan:
Set a circ rule with number of on-site checkout = 2 and number of
checkouts = 2.
Switch on both ConsiderOnSiteCheckoutsAsNormalCheckouts and SwitchOnSiteCheckouts

Do a regular checkout
Do another regular checkout
At this point you have reach the maximum number of checkouts allowed.
Not try another one.
=> Without this patch you will not get the warning
=> With this patch you will see it
Comment 15 Marc Véron 2017-04-02 17:15:38 UTC
Created attachment 61770 [details] [review]
Bug 18321: Add tests

Clean a bit existing tests by adding default values and add a test to
highlight the issue.

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 16 Marc Véron 2017-04-02 17:19:09 UTC
Created attachment 61771 [details] [review]
Bug 18321: Correctly calculate switch_onsite_checkout

Silly error here, the $switch_onsite_checkout is not correctly
calculated, it should not only be set to the value of the syspref, but
meet all the conditions.

Test plan:
Set a circ rule with number of on-site checkout = 2 and number of
checkouts = 2.
Switch on both ConsiderOnSiteCheckoutsAsNormalCheckouts and SwitchOnSiteCheckouts

Do a regular checkout
Do another regular checkout
At this point you have reach the maximum number of checkouts allowed.
Not try another one.
=> Without this patch you will not get the warning
=> With this patch you will see it

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 17 Marc Véron 2017-04-02 17:22:04 UTC
Jonathan, this Bug was marked as assigned but the patches work, so I assume that you forgot to set to NSO.
Switching to NSO and then to Signed off
Comment 18 Jonathan Druart 2017-04-03 12:55:08 UTC
(In reply to Marc Véron from comment #17)
> Jonathan, this Bug was marked as assigned but the patches work, so I assume
> that you forgot to set to NSO.
> Switching to NSO and then to Signed off

Yes indeed, I forgot to change the status.
Comment 19 Nick Clemens 2017-04-12 10:58:18 UTC
Created attachment 62076 [details] [review]
Bug 18321: Add tests

Clean a bit existing tests by adding default values and add a test to
highlight the issue.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 20 Nick Clemens 2017-04-12 10:58:24 UTC
Created attachment 62077 [details] [review]
Bug 18321: Correctly calculate switch_onsite_checkout

Silly error here, the $switch_onsite_checkout is not correctly
calculated, it should not only be set to the value of the syspref, but
meet all the conditions.

Test plan:
Set a circ rule with number of on-site checkout = 2 and number of
checkouts = 2.
Switch on both ConsiderOnSiteCheckoutsAsNormalCheckouts and SwitchOnSiteCheckouts

Do a regular checkout
Do another regular checkout
At this point you have reach the maximum number of checkouts allowed.
Not try another one.
=> Without this patch you will not get the warning
=> With this patch you will see it

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 21 Marc Véron 2017-04-13 18:47:35 UTC
kyle@bywatersolutions.com 2017-04-13 12:28:01 UTC CC kyle@bywatersolutions.com
Status 	Passed QA Patch doesn't apply 

Re-tested on current master 16.12.00.022, patch applies fine here.
Comment 22 Jonathan Druart 2017-04-17 12:29:06 UTC
(In reply to Marc Véron from comment #21)
> kyle@bywatersolutions.com 2017-04-13 12:28:01 UTC CC
> kyle@bywatersolutions.com
> Status 	Passed QA Patch doesn't apply 
> 
> Re-tested on current master 16.12.00.022, patch applies fine here.

Same here, please retest.
Comment 23 Marc Véron 2017-04-17 15:40:33 UTC
Re-tested on current master, 16.12.00.024
Before applying, head is 36fd404 Bug 17835: [QA Follow-up] Fix opac-search.pl

git bz apply 18321

Both patches apply.
Comment 24 Kyle M Hall 2017-04-17 17:59:13 UTC
Pushed to master for 17.05, thanks Jonathan!
Comment 25 Katrin Fischer 2017-04-21 22:09:36 UTC
These patches have been pushed to 16.11.x and will be in 16.11.07.