Summary: | One more checkouts possible than allowed by rules | ||
---|---|---|---|
Product: | Koha | Reporter: | Marc Véron <veron> |
Component: | Circulation | Assignee: | Jonathan Druart <jonathan.druart> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | normal | ||
Priority: | P5 - low | CC: | gmcharlt, jonathan.druart, kyle.m.hall, kyle |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | --- | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: | Circulation function: | ||
Bug Depends on: | 16272 | ||
Bug Blocks: | |||
Attachments: |
Screenshot circulation rules
Screenshots to reproduce error Bug 18321: Add tests Bug 18321: Correctly calculate switch_onsite_checkout Bug 18321: Add tests Bug 18321: Correctly calculate switch_onsite_checkout Bug 18321: Add tests Bug 18321: Correctly calculate switch_onsite_checkout |
Description
Marc Véron
2017-03-23 14:52:26 UTC
Suppose it happens in comparisons in C4/Circulation.pm 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. 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 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. Would it be possible to get a screencast, or a sandbox set up with your settings? I still do not recreate. 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)
Sorry but still works for me. Did you test on a sandbox? (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)? Everything is clear, but works for me :) 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) 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. Caused by bug 16272. 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. 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 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> 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> 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 (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. 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> 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> 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. (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. 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. Pushed to master for 17.05, thanks Jonathan! These patches have been pushed to 16.11.x and will be in 16.11.07. |