Bug 34620

Summary: Writeoff causes 500 error if RequirePaymentType is on
Product: Koha Reporter: Lucas Gass (lukeg) <lucas>
Component: Fines and feesAssignee: Lucas Gass (lukeg) <lucas>
Status: RESOLVED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: major    
Priority: P5 - low CC: david, fridolin.somers, kyle, m.de.rooy, martin.renvoize
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This fixes writing off a charge when the RequirePaymentType system preference is set to required. The write-off now completes successfully without generating an error page (Patrons > [patron account] > Accounting > Make a payment > Write off an individual charge).
Version(s) released in:
23.11.00,23.05.04,22.11.10
Circulation function:
Bug Depends on: 33176    
Bug Blocks:    
Attachments: Bug 34620: Do not throw expection if payment type is writeoff
Bug 34620: Do not throw expection if payment type is writeoff
Bug 34620: Do not throw expection if payment type is writeoff

Description Lucas Gass (lukeg) 2023-08-25 15:47:59 UTC
To recreate:

1. Turn on RequirePaymentType 
2. Create a manual invoice and then attempt to write it off.
3. 500 error
4. Turn of RequirePaymentType, no error.
Comment 1 Lucas Gass (lukeg) 2023-08-25 16:05:55 UTC
Created attachment 154835 [details] [review]
Bug 34620: Do not throw expection if payment type is writeoff

To test:
1. Turn on RequirePaymentType
2. Create a manual invoice and then attempt to write it off.
3. 500 error
4. Turn of RequirePaymentType, no error.
5. Apply patch, restart_all
6. Try step 2 again, you should not get an error
7. prove t/db_dependent/Koha/Account.t
8. Make sure tests pass
Comment 2 David Nind 2023-08-25 19:45:25 UTC
Created attachment 154850 [details] [review]
Bug 34620: Do not throw expection if payment type is writeoff

To test:
1. Turn on RequirePaymentType
2. Create a manual invoice and then attempt to write it off.
3. 500 error
4. Turn of RequirePaymentType, no error.
5. Apply patch, restart_all
6. Try step 2 again, you should not get an error
7. prove t/db_dependent/Koha/Account.t
8. Make sure tests pass

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 Marcel de Rooy 2023-09-01 09:31:01 UTC
Looking here
Comment 4 Marcel de Rooy 2023-09-01 09:36:29 UTC
Hmm. Are we solving this problem at the right spot?
Yes, we can prevent an exception by putting an if around it.
But if we require payment type, why not fill this column correctly when we add the writeoff?

Not sure what is the exact difference here between type and payment type?
Comment 5 Lucas Gass (lukeg) 2023-09-01 13:39:41 UTC
(In reply to Marcel de Rooy from comment #4)
> Hmm. Are we solving this problem at the right spot?
> Yes, we can prevent an exception by putting an if around it.
> But if we require payment type, why not fill this column correctly when we
> add the writeoff?

payment_type = 'WRITEOFF' ?
Comment 6 Kyle M Hall (khall) 2023-09-01 14:47:11 UTC
(In reply to Marcel de Rooy from comment #4)
> Hmm. Are we solving this problem at the right spot?
> Yes, we can prevent an exception by putting an if around it.
> But if we require payment type, why not fill this column correctly when we
> add the writeoff?
> 
> Not sure what is the exact difference here between type and payment type?

To Koha, writeoffs are not a type of payment. Both are types of credits ( along with forgiveness ). It's as simple as that. That is why we have (credit) type and payment type. To change that would require alterations to fundamental assumptions about accounting in Koha that I don't think we need to get into to fix this bug ;)
Comment 7 Martin Renvoize (ashimema) 2023-09-02 06:38:50 UTC
Initially I didn't like having the two types, though they come historically from accountlines type prior to the clarification of credit_type and debit_type.  I now appreciate the clarity of having two fields.  I wonder if we aught to have tied the require feature to the credit types rather than at a system level though.. that's a feature I think I'd get behind.
Comment 8 Marcel de Rooy 2023-09-04 09:26:28 UTC
Created attachment 155192 [details] [review]
Bug 34620: Do not throw expection if payment type is writeoff

To test:
1. Turn on RequirePaymentType
2. Create a manual invoice and then attempt to write it off.
3. 500 error
4. Turn of RequirePaymentType, no error.
5. Apply patch, restart_all
6. Try step 2 again, you should not get an error
7. prove t/db_dependent/Koha/Account.t
8. Make sure tests pass

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Tomás Cohen Arazi (tcohen) 2023-09-05 17:38:01 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 10 Fridolin Somers 2023-09-08 06:43:11 UTC
Pushed to 23.05.x for 23.05.04
Comment 11 Matt Blenkinsop 2023-09-15 09:33:48 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x