I can generate a discharge for a patron with fines - shouldn't the patron have no checkouts and no fines?
Generating discharge to a patron having unpaid fine is a serious problem.
*** Bug 25986 has been marked as a duplicate of this bug. ***
I have not tested but this fix should be as simple as: diff --git a/Koha/Patron/Discharge.pm b/Koha/Patron/Discharge.pm index 76e04e8533a..b9ebc7653af 100644 --- a/Koha/Patron/Discharge.pm +++ b/Koha/Patron/Discharge.pm @@ -39,7 +39,12 @@ sub can_be_discharged { return unless $patron; my $has_pending_checkouts = $patron->checkouts->count; - return $has_pending_checkouts ? 0 : 1; + return 0 if $has_pending_checkouts; + + my $has_debt = $patron->account->outstanding_debits->total_outstanding + return 0 if $has_debt; + + return 1; } sub is_discharged {
Still valid in master (22.12.00.048)
Please take some time to consider Jonathan's solution and rebase/fix this (serious) issue! It seems that original implementers of this feature believed that having a fine is not a blocking reason for issuing a discharge, but this concept should be reconsidered! Also, at the same time, additional lines will be required in discharge.tt to handle inability to issue a discharge because of pending fines.
Maybe this can use : $patron->account_balance > 0 ?
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14250#c4 -> this is part of the solution but I need to check further what code calls can_be_discharged. For example, in discharge.tt: 54 [% UNLESS can_be_discharged %] 55 <p>Cannot edit discharge: the patron has checked out items.</p>
Created attachment 170136 [details] [review] Bug 14250: Patron with fines cannot be discharged anymore TEST PLAN: 1 - In the intranet add fines to a client and ensure they have no checkout 2 - Set syspref useDischarge to 'Allow' 3 - On client profile click on "Discharge", see it is possible 4 - On the opac, go to the tab "Ask for discharge", see it is possible 5 - Apply patch 6 - Click on the button "Ask for discharge" -> it will not be possible and you will be propperly messaged 7 - Return to the tab "Ask for a discharge", there will be no button anymore 8 - On the intranet page, there will be no button anymore 9 - Check out a book 10 - Repeat 7 and 8 and see the message include issue with checkout 11 - Remove fine and repeat 7 & 8
It seems to work as expected (without the ability to generate discharge), but the reason messages are not complete: when there are checkouts and fines in borrowers account only one reason is listed (variously: checkouts in STAFF, fines in OPAC). QA script: WARN Koha/Patron/Discharge.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 47, now: 48) WARN members/discharge.pl WARN tidiness The file is less tidy than before (bad/messy lines before: 30, now: 32) WARN opac/opac-discharge.pl WARN tidiness The file is less tidy than before (bad/messy lines before: 30, now: 35) WARN t/db_dependent/Patron/Borrower_Discharge.t WARN tidiness The file is less tidy than before (bad/messy lines before: 59, now: 63)
Created attachment 170228 [details] [review] Bug 14250: Patron with fines cannot be discharged anymore TEST PLAN: 1 - In the intranet add fines to a client and ensure they have no checkout 2 - Set syspref useDischarge to 'Allow' 3 - On client profile click on "Discharge", see it is possible 4 - On the opac, go to the tab "Ask for discharge", see it is possible 5 - Apply patch 6 - Click on the button "Ask for discharge" -> it will not be possible and you will be propperly messaged 7 - Return to the tab "Ask for a discharge", there will be no button anymore 8 - On the intranet page, there will be no button anymore 9 - Check out a book 10 - Repeat 7 and 8 and see the message include issue with checkout 11 - Remove fine and repeat 7 & 8
Thanks for update. It's almost done -- one more thing: when patron has no checkout but has fine (p. 8) there are two messages visible in STAFF: "They have unpaid fines" and "Patron cannot be discharged for an unknown reason, please contact a librarian for more information". But should be only first one. In OPAC it is OK. Is it because of [% IF patron.checkouts.count == 0 and (patron.account.outstanding_debits.total_outstanding > 0) %] in koha-tmpl/intranet-tmpl/prog/en/modules/members/discharge.tt?
Created attachment 170581 [details] [review] Bug 14250: Patron with fines cannot be discharged anymore TEST PLAN: 1 - In the intranet add fines to a client and ensure they have no checkout 2 - Set syspref useDischarge to 'Allow' 3 - On client profile click on "Discharge", see it is possible 4 - On the opac, go to the tab "Ask for discharge", see it is possible 5 - Apply patch 6 - Click on the button "Ask for discharge" -> it will not be possible and you will be propperly messaged 7 - Return to the tab "Ask for a discharge", there will be no button anymore 8 - On the intranet page, there will be no button anymore 9 - Check out a book 10 - Repeat 7 and 8 and see the message include issue with checkout 11 - Remove fine and repeat 7 & 8
Created attachment 170607 [details] [review] Bug 14250: Patron with fines cannot be discharged anymore TEST PLAN: 1 - In the intranet add fines to a client and ensure they have no checkout 2 - Set syspref useDischarge to 'Allow' 3 - On client profile click on "Discharge", see it is possible 4 - On the opac, go to the tab "Ask for discharge", see it is possible 5 - Apply patch 6 - Click on the button "Ask for discharge" -> it will not be possible and you will be propperly messaged 7 - Return to the tab "Ask for a discharge", there will be no button anymore 8 - On the intranet page, there will be no button anymore 9 - Check out a book 10 - Repeat 7 and 8 and see the message include issue with checkout 11 - Remove fine and repeat 7 & 8 Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Created attachment 170662 [details] [review] Bug 14250: Patron with fines cannot be discharged anymore TEST PLAN: 1 - In the intranet add fines to a client and ensure they have no checkout 2 - Set syspref useDischarge to 'Allow' 3 - On client profile click on "Discharge", see it is possible 4 - On the opac, go to the tab "Ask for discharge", see it is possible 5 - Apply patch 6 - Click on the button "Ask for discharge" -> it will not be possible and you will be propperly messaged 7 - Return to the tab "Ask for a discharge", there will be no button anymore 8 - On the intranet page, there will be no button anymore 9 - Check out a book 10 - Repeat 7 and 8 and see the message include issue with checkout 11 - Remove fine and repeat 7 & 8 Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 170663 [details] [review] Bug 14250: (QA follow-up) Deduplicate checks Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 170664 [details] [review] Bug 14250: (QA follow-up) Tidy code
Please rebase: Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 14250: Patron with fines cannot be discharged anymore Using index info to reconstruct a base tree... M koha-tmpl/intranet-tmpl/prog/en/modules/members/discharge.tt M koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-discharge.tt M t/db_dependent/Patron/Borrower_Discharge.t Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Patron/Borrower_Discharge.t Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-discharge.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/discharge.tt Applying: Bug 14250: (QA follow-up) Deduplicate checks Applying: Bug 14250: (QA follow-up) Tidy code error: sha1 information is lacking or useless (t/db_dependent/Patron/Borrower_Discharge.t). error: could not build fake ancestor
I was just been about to say the same (that the patch no longer applies). 8-) I was testing last week, and hadn't finished before it was signed off. I had some suggested text changes - I am happy to add a follow-up once the patches apply again. This is so that it makes more sense if there are multiple reasons for not discharging. Suggested text changes for the OPAC =================================== You cannot be discharged because: . You have unpaid charges of X. Please pay your charges before reapplying. . You have X item(s) checked out. Please return your checked out items before reapplying. . You cannot be discharged for an unknown reason. Please contact library staff for more information. Suggested text changes for the staff interface ============================================== A discharge cannot be generated for this patron because: . The patron has unpaid charges of X. . The patron has X item(s) checked out. . The patron cannot be discharged for an unknown reason. Please review the patron's account to determine why. (Not sure about this one.) The patron has holds. These will be cancelled if the discharge is generated. Testing notes ============= 1. In the staff interface, set up these scenarios for some patrons: - Patron A: check out an item and add a manual fee (I used Henry Acevedo) - Patron B: add a manual fee (I used Mary Burton) - Patron C: check out an item (I used Lisa Charles) - Patron D: add a manual fee (I used Bobbie Middleton) 2. Edit the patrons so that they have a password and that you can log in to their accounts in the OPAC. 3. Set the useDischarge system preference to 'Allow'. 4. Attempt to request a discharge for the patrons (OPAC > You account > Ask for a discharge): - Patron A (Manual fee and a checked out item) (Henry): Message is "You cannot be discharged, you have checked out items. Please return items before asking for a discharge." - Patron B (Manual fee) (Mary): "Ask for a discharge" button is available and a discharge can be requested. Message "Your discharge request has been sent. Your discharge will be available on this page within a few days." - Patron C (A checked out item (Lisa)): Message is "You cannot be discharged, you have checked out items. Please return items before asking for a discharge." 5. Apply the patches and restart everything (restart_all). 6. Attempt to request a discharge for the patrons from their OPAC accounts (OPAC > You account > Ask for a discharge): - Patron A (Manual fee and a checked out item)(Henry): Message is now You cannot be discharged for following reasons: . You have 1 item(s) checked out. Please return them before reapplying. . They have unpaid fines totaling 5.00. Please pay your fines before reapplying. - Patron C (A checked out item) (Lisa): Message is now You cannot be discharged for following reasons: . You have 1 item(s) checked out. Please return them before reapplying. - Patron D (A manual fee)(Bobbie): You cannot be discharged for following reasons: . They have unpaid fines totaling 5.00. Please pay your fines before reapplying. 7. Attempt to discharge the patrons from the staff interface, from their patron account: - Patron A (Manual fee and a checked out item)(Henry): Message is now Cannot edit discharge for following reasons: . They have 1 item(s) checked out . They have unpaid fines totaling 5.00 - Patron C (A checked out item) (Lisa): Message is now Cannot edit discharge for following reasons: . They have 1 item(s) checked out - Patron D (A manual fee)(Bobbie): Cannot edit discharge for following reasons: . They have unpaid fines totaling 5.00 OPAC discharge text =================== I've created bug 37729 for the discharge text on the OPAC "Ask for a discharge" page, suggesting that this is changed to an HTML customization - so that the text can be customised and translated. Current hard coded text ----------------------- Discharge What is a discharge? This document certifies that you have returned all borrowed items. It is sometimes asked during a file transfer from a school to another. The discharge is sent by us to your school. You will also find it available on your reader account. Warning: This request is only valid if you are in good standing with the library. Once the application is made, you can not borrow library materials.
Created attachment 170705 [details] [review] Bug 14250: Patron with fines cannot be discharged anymore TEST PLAN: 1 - In the intranet add fines to a client and ensure they have no checkout 2 - Set syspref useDischarge to 'Allow' 3 - On client profile click on "Discharge", see it is possible 4 - On the opac, go to the tab "Ask for discharge", see it is possible 5 - Apply patch 6 - Click on the button "Ask for discharge" -> it will not be possible and you will be propperly messaged 7 - Return to the tab "Ask for a discharge", there will be no button anymore 8 - On the intranet page, there will be no button anymore 9 - Check out a book 10 - Repeat 7 and 8 and see the message include issue with checkout 11 - Remove fine and repeat 7 & 8 Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 170706 [details] [review] Bug 14250: (QA follow-up) Deduplicate checks Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 170707 [details] [review] Bug 14250: (QA follow-up) Tidy code
(In reply to David Nind from comment #19) > I was just been about to say the same (that the patch no longer applies). 8-) > > I was testing last week, and hadn't finished before it was signed off. > > I had some suggested text changes - I am happy to add a follow-up once the > patches apply again. This is so that it makes more sense if there are > multiple reasons for not discharging. > > Suggested text changes for the OPAC > =================================== > > You cannot be discharged because: > . You have unpaid charges of X. Please pay your charges before reapplying. > . You have X item(s) checked out. Please return your checked out items > before reapplying. > . You cannot be discharged for an unknown reason. Please contact library > staff for more information. > > Suggested text changes for the staff interface > ============================================== > > A discharge cannot be generated for this patron because: > . The patron has unpaid charges of X. > . The patron has X item(s) checked out. > . The patron cannot be discharged for an unknown reason. Please review the > patron's account to determine why. (Not sure about this one.) > > The patron has holds. These will be cancelled if the discharge is generated. > > Testing notes > ============= > > 1. In the staff interface, set up these scenarios for some patrons: > - Patron A: check out an item and add a manual fee (I used Henry Acevedo) > - Patron B: add a manual fee (I used Mary Burton) > - Patron C: check out an item (I used Lisa Charles) > - Patron D: add a manual fee (I used Bobbie Middleton) > 2. Edit the patrons so that they have a password and that you can log in to > their accounts in the OPAC. > 3. Set the useDischarge system preference to 'Allow'. > 4. Attempt to request a discharge for the patrons (OPAC > You account > Ask > for a discharge): > - Patron A (Manual fee and a checked out item) (Henry): Message is "You > cannot be discharged, you have checked out items. Please return items before > asking for a discharge." > - Patron B (Manual fee) (Mary): "Ask for a discharge" button is available > and a discharge can be requested. Message "Your discharge request has been > sent. Your discharge will be available on this page within a few days." > - Patron C (A checked out item (Lisa)): Message is "You cannot be > discharged, you have checked out items. Please return items before asking > for a discharge." > 5. Apply the patches and restart everything (restart_all). > 6. Attempt to request a discharge for the patrons from their OPAC accounts > (OPAC > You account > Ask for a discharge): > > - Patron A (Manual fee and a checked out item)(Henry): Message is now > You cannot be discharged for following reasons: > . You have 1 item(s) checked out. Please return them before > reapplying. > . They have unpaid fines totaling 5.00. Please pay your fines before > reapplying. > > - Patron C (A checked out item) (Lisa): Message is now > You cannot be discharged for following reasons: > . You have 1 item(s) checked out. Please return them before > reapplying. > > - Patron D (A manual fee)(Bobbie): > You cannot be discharged for following reasons: > . They have unpaid fines totaling 5.00. Please pay your fines before > reapplying. > > 7. Attempt to discharge the patrons from the staff interface, from their > patron account: > > - Patron A (Manual fee and a checked out item)(Henry): Message is now > Cannot edit discharge for following reasons: > . They have 1 item(s) checked out > . They have unpaid fines totaling 5.00 > > - Patron C (A checked out item) (Lisa): Message is now > Cannot edit discharge for following reasons: > . They have 1 item(s) checked out > > - Patron D (A manual fee)(Bobbie): > Cannot edit discharge for following reasons: > . They have unpaid fines totaling 5.00 > > OPAC discharge text > =================== > > I've created bug 37729 for the discharge text on the OPAC "Ask for a > discharge" page, suggesting that this is changed to an HTML customization - > so that the text can be customised and translated. > > Current hard coded text > ----------------------- > > Discharge > > What is a discharge? > > This document certifies that you have returned all borrowed items. It is > sometimes asked during a file transfer from a school to another. The > discharge is sent by us to your school. You will also find it available on > your reader account. > > Warning: This request is only valid if you are in good standing with the > library. Once the application is made, you can not borrow library materials. Those sound like good changes. Will you add a follow-up for those?
Created attachment 170708 [details] [review] Bug 14250: (follow-up) Update messages Update the text for the reasons the patron cannot be discharged.
Have added a follow-up patch to update the messages.
I think this will always show the note about checkouts: - [% ELSIF has_issues %] - <p>You cannot be discharged, you have checked out items. Please return items before asking for a discharge.</p> + [% ELSIF failure %] + <p>You cannot be discharged because you have checked out items. Please return your checked out items before asking for a discharge.</p> + <ul> + [% IF has_debt %] + <li>You have unpaid charges. Please pay your charges before reapplying.</li> + [% END %] + [% IF has_issues %] + <li>You have checked out items. Please return your checked out items before reapplying.</li> + [% END %] + [% IF !has_issue and !has_debt %] + <li>You cannot be discharged for an unknown reason. Please contact library staff for more information.</li> + [% END %] + </ul> I believe we should remove/rephrase the <p> line?
Why do we have almost the same errors/messages appear twice in the OPAC template? koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-discharge.tt [% ELSIF failure %] ... [% UNLESS can_be_discharged %] ... I like that this makes the code cleaner looking and I also like to see this fixed. Please have a look at this and my previous comment for the template changes.
I wondered myself. I thought it was odd that the messages were repeated in the code, and I couldn't get the text from (I think) the earlier set of messages to display in the OPAC. (See c#19 - step 6 - which has the different scenarios and the messages displayed, before my follow-up patch to change the text.)
(In reply to Katrin Fischer from comment #27) > Why do we have almost the same errors/messages appear twice in the OPAC > template? > koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-discharge.tt > > [% ELSIF failure %] > ... > > [% UNLESS can_be_discharged %] > ... > > I like that this makes the code cleaner looking and I also like to see this > fixed. Please have a look at this and my previous comment for the template > changes. I also found it was weird while developing. The business of the function is quite complex and if I'm not mistaken, the lonely way to go through the second one is to load the page for discharge while being allowed to discharge, then disallow the user to get discharged.
(In reply to Katrin Fischer from comment #26) > I think this will always show the note about checkouts: > > - [% ELSIF has_issues %] > - <p>You cannot be discharged, you have checked out > items. Please return items before asking for a discharge.</p> > + [% ELSIF failure %] > + <p>You cannot be discharged because you have > checked out items. Please return your checked out items before asking for a > discharge.</p> > + <ul> > + [% IF has_debt %] > + <li>You have unpaid charges. Please pay your > charges before reapplying.</li> > + [% END %] > + [% IF has_issues %] > + <li>You have checked out items. Please return > your checked out items before reapplying.</li> > + [% END %] > + [% IF !has_issue and !has_debt %] > + <li>You cannot be discharged for an unknown > reason. Please contact library staff for more information.</li> > + [% END %] > + </ul> > > I believe we should remove/rephrase the <p> line? If I understand well, these sentences have been rephrased here (https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=170708&action=diff), right ?
As the both text blocks appear to be added by this patch set - could we reuse the same text block for both, defined in one spot?
(In reply to Katrin Fischer from comment #31) > As the both text blocks appear to be added by this patch set - could we > reuse the same text block for both, defined in one spot? Hum.. Yes I tried to focus on the change needed. I think the page needs a cleanup out from this scope. The business of the function was quite messy, with multiple variables containing the same information but for different purposes (for example has_issues and has_checkout do not have the same content according to the fact the user can or cannot be discharged). In a first while, we should probably use two different texts, one referring to the fact that the user cannot ask for a discharge and one referring to the fact that user's state has changed between the load of the page and the click on the discharge button (but tbh I don't know if anyone will see it). In a second while, I'll submit a separate patch to de-duplicate variables and clean the function itself. I think this will be easier to track the changes so. Does this solution looks fine for you ?
Created attachment 171025 [details] [review] Bug 14250: (follow-up) Refacto the algorithm for discharge messages TEST PLAN: 1 - In the intranet add fines to a client and ensure they have no checkout 2 - Set syspref useDischarge to 'Allow' 3 - On client profile click on "Discharge", see it is possible 4 - On the opac, go to the tab "Ask for discharge", see it is possible 5 - Apply patch 6 - Click on the button "Ask for discharge" -> it will not be possible and you will be propperly messaged 7 - Return to the tab "Ask for a discharge", there will be no button anymore 8 - On the intranet page, there will be no button anymore 9 - Check out a book 10 - Repeat 7 and 8 and see the message include issue with checkout 11 - Remove fine and repeat 7 & 8 Note: removing the useless has_checkout and has_debt
Created attachment 171026 [details] [review] Bug 14250: (follow-up) Refacto discharge messages display The script that determines if one should display messages to the user, informing him whether they are allowed or not to ask for a discharge is quite hard to understand. For sake of readability, transforming the unless to an if since the error behaviour was in the unless and the success in the else. Changing to success behaviour in the if and failure in the else.
Created attachment 171271 [details] [review] Bug 14250: Patron with fines cannot be discharged anymore TEST PLAN: 1 - In the intranet add fines to a client and ensure they have no checkout 2 - Set syspref useDischarge to 'Allow' 3 - On client profile click on "Discharge", see it is possible 4 - On the opac, go to the tab "Ask for discharge", see it is possible 5 - Apply patch 6 - Click on the button "Ask for discharge" -> it will not be possible and you will be propperly messaged 7 - Return to the tab "Ask for a discharge", there will be no button anymore 8 - On the intranet page, there will be no button anymore 9 - Check out a book 10 - Repeat 7 and 8 and see the message include issue with checkout 11 - Remove fine and repeat 7 & 8 Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Sukhmandeep Benipal <sukhmandeep.benipal@inLibro.com>
Created attachment 171272 [details] [review] Bug 14250: (QA follow-up) Deduplicate checks Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Sukhmandeep Benipal <sukhmandeep.benipal@inLibro.com>
Created attachment 171273 [details] [review] Bug 14250: (QA follow-up) Tidy code Signed-off-by: Sukhmandeep Benipal <sukhmandeep.benipal@inLibro.com>
Created attachment 171274 [details] [review] Bug 14250: (follow-up) Update messages Update the text for the reasons the patron cannot be discharged. Signed-off-by: Sukhmandeep Benipal <sukhmandeep.benipal@inLibro.com>
Created attachment 171275 [details] [review] Bug 14250: (follow-up) Refacto the algorithm for discharge messages TEST PLAN: 1 - In the intranet add fines to a client and ensure they have no checkout 2 - Set syspref useDischarge to 'Allow' 3 - On client profile click on "Discharge", see it is possible 4 - On the opac, go to the tab "Ask for discharge", see it is possible 5 - Apply patch 6 - Click on the button "Ask for discharge" -> it will not be possible and you will be propperly messaged 7 - Return to the tab "Ask for a discharge", there will be no button anymore 8 - On the intranet page, there will be no button anymore 9 - Check out a book 10 - Repeat 7 and 8 and see the message include issue with checkout 11 - Remove fine and repeat 7 & 8 Note: removing the useless has_checkout and has_debt Signed-off-by: Sukhmandeep Benipal <sukhmandeep.benipal@inLibro.com>
Created attachment 171276 [details] [review] Bug 14250: (follow-up) Refacto discharge messages display The script that determines if one should display messages to the user, informing him whether they are allowed or not to ask for a discharge is quite hard to understand. For sake of readability, transforming the unless to an if since the error behaviour was in the unless and the success in the else. Changing to success behaviour in the if and failure in the else. Signed-off-by: Sukhmandeep Benipal <sukhmandeep.benipal@inLibro.com>