Bug 8422 - Fix impossible warning in circulation.pl when OverduesBlockCirc set to Block
Summary: Fix impossible warning in circulation.pl when OverduesBlockCirc set to Block
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: 3.8
Hardware: All All
: P5 - low minor (vote)
Assignee: David Cook
QA Contact: Mason James
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-12 01:23 UTC by David Cook
Modified: 2013-12-05 19:59 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 8422: Fix impossible warning in circulation.pl when OverduesBlockCirc set to Block (1.26 KB, patch)
2012-07-12 01:30 UTC, David Cook
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8422 - Fix impossible warning in circulation.pl when OverduesBlockCirc set to Block (1.29 KB, patch)
2012-07-13 16:42 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2012-07-12 01:23:57 UTC
Currently, circulation.pl states:

"Patron has x overdue item(s). Check out anyway?" when syspref OverduesBlockCirc is set to "Ask for confirmation" or "Block".

Since checkout is completely blocked, it shouldn't ask that question. Instead, it should say "Checkouts are BLOCKED because patron has overdue items".

---

On a side note, this bug reveals inconsistencies in how blocked checkouts are handled. Even when OverduesBlockCirc is set to Block, the barcode input still appears. It's only when one hits checkout that the checkout is prevented and the warning message appears.

However, when the patron surpasses the fine/charge limit specified in syspref "noissuescharge", the barcode input doesn't appear at all. Instead, a message appears that says "Cannot check out!" I think that this would be a much better behaviour when checkouts are blocked for all other reasons besides exceeding fine limits.

That said, this template is still not great. It says: "Cannot check out!
Overdues: Patron has ITEMS OVERDUE. See highlighted items below
Fees & Charges: Patron has Outstanding fees & charges of 10.00. Checkouts are BLOCKED because fine balance is OVER THE LIMIT. Make Payment"

These are the "Attention" messages. They should still appear on the right side of the screen under "Attention". The only thing that should appear in the centre is "Checkouts are BLOCKED because fine balance is OVER THE LIMIT. Make Payment".

The current template is misleading. Even though the message does say explicitly that checkouts are blocked due to the fines, it SEEMS that checkouts are blocked due to "Overdues, Fees & Charges" based on the visual layout.

Thoughts?
Comment 1 David Cook 2012-07-12 01:30:56 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2012-07-13 16:42:37 UTC
Created attachment 10829 [details] [review]
[SIGNED-OFF] Bug 8422 - Fix impossible warning in circulation.pl when OverduesBlockCirc set to Block

Previously, the message Patron has x overdue item(s). Checkout anyway? was shown when syspref OverduesBlockCirc was set to Ask for confirmation or Block.

This patch changes the message for Block to: Checkouts are BLOCKED because patron has overdues.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 3 Mason James 2012-07-17 05:03:09 UTC
(In reply to comment #2)
> Created attachment 10829 [details] [review]
> [SIGNED-OFF] Bug 8422 - Fix impossible warning in circulation.pl when
> OverduesBlockCirc set to Block
> 
> Previously, the message Patron has x overdue item(s). Checkout anyway? was
> shown when syspref OverduesBlockCirc was set to Ask for confirmation or
> Block.
> 
> This patch changes the message for Block to: Checkouts are BLOCKED because
> patron has overdues.
> 
> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

passing QA...

trivial patch passes QA tests OK
Comment 4 Paul Poulain 2012-07-17 13:37:07 UTC
> Thoughts?

David, my thoughts are that the Circulation would not deserve a complete rewritte... Many things are crappy/ years of code piled, you've found one.

About your patch = it's pushed. Note that you could have kept the [% USERBLOCKEDOVERDUE %] information:
Checkouts are BLOCKED because patron has [% USERBLOCKEDOVERDUE %] overdue items

Is there reason not to do it that way ?
Comment 5 David Cook 2012-07-18 00:24:55 UTC
(In reply to comment #4)
> > Thoughts?
> 
> David, my thoughts are that the Circulation would not deserve a complete
> rewritte... Many things are crappy/ years of code piled, you've found one.
> 
> About your patch = it's pushed. Note that you could have kept the [%
> USERBLOCKEDOVERDUE %] information:
> Checkouts are BLOCKED because patron has [% USERBLOCKEDOVERDUE %] overdue
> items
> 
> Is there reason not to do it that way ?

Point well received, Paul. I'm not sure that the template would need a complete re-write, but I figured it was worth flagging and maybe I will come back and fix that up someday. I don't have the programming skill (yet) to work on some of the deeper issues, but I figure that I can help iron out the interface.

As for my patch, I thought about keeping the [%
> USERBLOCKEDOVERDUE %] information, but I thought that it was potentially misleading. The patron isn't blocked because of the number of overdue items. They're blocked because they have overdue items. I figured that my patch used more precise language.
Comment 6 Chris Cormack 2012-07-21 08:43:37 UTC
Pushed to 3.8.x will be in 3.8.4