Bug 29987

Summary: Manual credits are not recorded for a register
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: Fines and feesAssignee: Martin Renvoize <martin.renvoize>
Status: RESOLVED FIXED QA Contact: Kyle M Hall <kyle>
Severity: normal    
Priority: P5 - low CC: arthur.suzuki, david, kyle, lucas, martin.renvoize, pascal.uphaus, sally.healey, victor, wainuiwitikapark
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This fixes the recording of manual credits for patrons so that these transactions are now included in the cash summary report for a library. When adding a manual credit, there are now fields for choosing the transaction type and cash register.
Version(s) released in:
22.11.00, 22.05.08, 21.11.15
Bug Depends on:    
Bug Blocks: 13985    
Attachments: Bug 29987: Add register support to manual credits
Bug 29987: Add register support to manual credits
Bug 29987: Display credit without offsets in transactions
Bug 29987: (follow-up) Fix missing payment type
Bug 29987: Display credit without offsets in transactions
Bug 29987: Add register support to manual credits
Bug 29987: (follow-up) Fix missing payment type
Bug 29987: Display credit without offsets in transactions
Bug 29987: Improve validation
Bug 29987: Add register support to manual credits
Bug 29987: (follow-up) Fix missing payment type
Bug 29987: Display credit without offsets in transactions
Bug 29987: Improve validation
Bug 29987: Add register support to manual credits
Bug 29987: (follow-up) Fix missing payment type
Bug 29987: Display credit without offsets in transactions
Bug 29987: Improve validation
Bug 29987: (QA follow-up) Remove unused cnt attribute missing a filter

Description Katrin Fischer 2022-01-31 21:44:47 UTC
What I did:

- Activated UseRegister
- Added a cash register
- Made sure it was selected for my login and showed in upper right corner
- Went to a sample patron's account (used Nicole and Henry)
- Added a manual credit using "Create manual credit"

The cash register was not recorded:

select * from accountlines;

register_id is NULL

As this would mean an unexplainable surplus of cash, I think it's a bug?
Comment 1 Martin Renvoize 2022-06-24 14:00:11 UTC
Credit is credit, it's not a case of money being handed over so we don't currently give the option to select a register on the manual credit form so it wouldn't be recorded.  In the cash management sense we're only interested in money changing hands so we only record registers for those cases.
Comment 2 Katrin Fischer 2022-06-24 21:31:04 UTC
Hm, for me a credit means money exchanging hands. A patron might make a deposit that future fines/fees may be deducted from.

Otherwise the option to pay out a credit would not make any sense...?

I tihnk at least this needs a little more discussion.
Comment 3 Katrin Fischer 2022-06-24 21:32:19 UTC
Wondering: Maybe we need the payment type pull down on the credits? That way libraries could say it was a cash or other?
Comment 4 Martin Renvoize 2022-06-29 15:57:10 UTC
We could certainly need the payment type pulldown to go with it.. 

I can take a look at adding this.
Comment 5 Martin Renvoize 2022-06-29 16:20:28 UTC
Created attachment 136743 [details] [review]
Bug 29987: Add register support to manual credits

This patch adds the register and transaction type selection options to
the manual credit page.
Comment 6 David Nind 2022-06-29 23:12:51 UTC
Created attachment 136755 [details] [review]
Bug 29987: Add register support to manual credits

This patch adds the register and transaction type selection options to
the manual credit page.

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2022-06-29 23:26:43 UTC
Testing notes (using koha-testing-docker):

1. Followed steps in the bug description to replicate the issue.

2. After applying the patch, repeated these steps to add some new transactions. There are now dropdown lists for transaction type and cash register when adding manual credits.

3. These transactions are now included in the cash summary report for the library, and a cashup can be recorded.

Notes: 

1. If the EnablePointOfSale system preference is disabled, but UseCashRegisters is enabled, then to get to the cash management area to cashup go to Tools > Cash management > Cash summary for <currentlibrary> (http://127.0.0.1:8081/cgi-bin/koha/pos/registers.pl)

2. I'm assuming that libraries would just use this going forward to manage cash - previous cash manual credits would have already been dealt with in someway  (in terms of reconciling), so there is no need to update existing transactions.
Comment 8 David Nind 2022-06-29 23:46:54 UTC
After testing this, signing off, and adding a draft release note, I noticed that:

1. In the cash summary for the library, the amounts are now included in the income (cash) total (good!).

2. However, they are not included in the bankable column (bad?).

3. In the details for the cash register, for transactions there is a total, but it says "No data available in table" (and the same for the older transactions).

4. If you do a cashup, these transactions are not included (if there are no other cash transactions, then do a cashup the message is "Please confirm that you have removed 0.00 from the cash register and left a float of 0.00.").

My query: If you receive cash (say, paying charges in advance) and create a manual credit, then should it be included in items 2-4? 

I think it should, as when you go to do the banking you want it included in that total so that everything reconciles.

However, other manual credits, for example if you write off charges, shouldn't be as you have neither added to or taken anything out of the cash register.
Comment 9 Martin Renvoize 2022-06-30 06:12:28 UTC
Thanks for reviewing David.. you've highlighted perfectly what I feared may be the case. Cashup as it stands makes some assumptions that adding cash to a register should always have an associated debt to offset.

I'll try to remind myself how the maths and queries work today to see if this is resolvable.
Comment 10 Katrin Fischer 2022-07-22 09:02:56 UTC
I can confirm David's findings in comment#8. Setting to Failed QA for now as I feel that the display on the cash register transactions and summary is not right yet.

I do think the payment type option on the credit page is the right way to go, but probably needs some more adjustments.
Comment 11 Martin Renvoize 2022-08-24 10:08:17 UTC
Created attachment 139703 [details] [review]
Bug 29987: Display credit without offsets in transactions

This patch adds a row to the transactions table for credits without
corresponding offsets
Comment 12 Martin Renvoize 2022-08-24 10:10:22 UTC
This is a somewhat horrible work around for how rowGroup works in datatables.. I'm considering some other approaches as part of converting the table to an ajax driven datatable... perhaps dropping rowGroup and instead triggering Ajax calls to populate the detailed breakdown rows.
Comment 13 Martin Renvoize 2022-08-24 10:25:06 UTC
Created attachment 139704 [details] [review]
Bug 29987: (follow-up) Fix missing payment type

The payment type include has changed since the patch was first written,
we now require a 'type' is passed to properly set the field name.

This should fix the 'bankable' issue raised.
Comment 14 Martin Renvoize 2022-08-24 10:25:10 UTC
Created attachment 139705 [details] [review]
Bug 29987: Display credit without offsets in transactions

This patch adds a row to the transactions table for credits without
corresponding offsets
Comment 15 Martin Renvoize 2022-08-24 10:32:13 UTC
(In reply to David Nind from comment #8)
> After testing this, signing off, and adding a draft release note, I noticed
> that:
> 
> 1. In the cash summary for the library, the amounts are now included in the
> income (cash) total (good!).

Correct, I believe this is as expected

> 2. However, they are not included in the bankable column (bad?).

Payment type was not being recorded correctly.. that is now fixed in the first follow-up. 'Cash' type transactions should be 'bankable' (I think I have a bug somewhere to make the types included in bankable configurable but for now it's cash only)

> 3. In the details for the cash register, for transactions there is a total,
> but it says "No data available in table" (and the same for the older
> transactions).

This is the second follow up patch.. though I've only implimented it for current transactions so far.. we can copy the logic to old transactions if we're happy it makes sense.

> 4. If you do a cashup, these transactions are not included (if there are no
> other cash transactions, then do a cashup the message is "Please confirm
> that you have removed 0.00 from the cash register and left a float of
> 0.00.").

I think this should be fixed by the first follow-up above.

> My query: If you receive cash (say, paying charges in advance) and create a
> manual credit, then should it be included in items 2-4? 
>
> I think it should, as when you go to do the banking you want it included in
> that total so that everything reconciles.
> 
> However, other manual credits, for example if you write off charges,
> shouldn't be as you have neither added to or taken anything out of the cash
> register.

You are correct.. any process by which money actually changes hands at the register should be reported.
Comment 16 Pascal Uphaus 2022-09-26 15:41:08 UTC
I applied the patches and did the testplan described in the first comment. The register_id was still NULL. Is the testplan still up-to-date?
Comment 17 Katrin Fischer 2022-09-26 19:22:58 UTC
Hi Pascal, thx for testing! Did you restart Plack/Memcached after applying the patch? Sometimes you won't see the change as Plack still has the old version of the files. On a koha-testing-docker environment there is a shortcut to do this: restart_all.
Comment 18 Victor Grousset/tuxayo 2022-09-27 08:45:44 UTC
Hi, we used a sandbox and unless something went wrong, the services should be restarted when applying a patch.

We were screen sharing and I didn't notice anything suspicious that could explain that register_id was still NULL.

We double checked on the git log of the sandbox that the 3 patches were applied.
Comment 19 Martin Renvoize 2022-09-30 14:15:14 UTC
Created attachment 141165 [details] [review]
Bug 29987: Add register support to manual credits

This patch adds the register and transaction type selection options to
the manual credit page.
Comment 20 Martin Renvoize 2022-09-30 14:15:19 UTC
Created attachment 141166 [details] [review]
Bug 29987: (follow-up) Fix missing payment type

The payment type include has changed since the patch was first written,
we now require a 'type' is passed to properly set the field name.

This should fix the 'bankable' issue raised.
Comment 21 Martin Renvoize 2022-09-30 14:15:23 UTC
Created attachment 141167 [details] [review]
Bug 29987: Display credit without offsets in transactions

This patch adds a row to the transactions table for credits without
corresponding offsets
Comment 22 Martin Renvoize 2022-09-30 14:15:46 UTC
Just a quick rebase and now testing on a sandbox.. I can't replicate any of your issues locally.
Comment 23 Martin Renvoize 2022-09-30 14:26:18 UTC
(In reply to pascal.uphaus from comment #16)
> I applied the patches and did the testplan described in the first comment.
> The register_id was still NULL. Is the testplan still up-to-date?

Did you select a register at the point of adding a manual credit.. there is a select box and you can opt to not select anything by default (There's a setting that makes the register required, but that's disabled by default and so not enabled on the sandboxes)
Comment 24 Martin Renvoize 2022-09-30 15:07:42 UTC
Created attachment 141171 [details] [review]
Bug 29987: Improve validation

This patch adds hinting to display the required state of the cash
register fields.
Comment 25 David Nind 2022-10-06 19:54:15 UTC
Created attachment 141469 [details] [review]
Bug 29987: Add register support to manual credits

This patch adds the register and transaction type selection options to
the manual credit page.

Signed-off-by: David Nind <david@davidnind.com>
Comment 26 David Nind 2022-10-06 19:54:20 UTC
Created attachment 141470 [details] [review]
Bug 29987: (follow-up) Fix missing payment type

The payment type include has changed since the patch was first written,
we now require a 'type' is passed to properly set the field name.

This should fix the 'bankable' issue raised.

Signed-off-by: David Nind <david@davidnind.com>
Comment 27 David Nind 2022-10-06 19:54:25 UTC
Created attachment 141471 [details] [review]
Bug 29987: Display credit without offsets in transactions

This patch adds a row to the transactions table for credits without
corresponding offsets

Signed-off-by: David Nind <david@davidnind.com>
Comment 28 David Nind 2022-10-06 19:54:30 UTC
Created attachment 141472 [details] [review]
Bug 29987: Improve validation

This patch adds hinting to display the required state of the cash
register fields.

Signed-off-by: David Nind <david@davidnind.com>
Comment 29 David Nind 2022-10-06 20:14:14 UTC
I've retested and signed off.

I think things are now working as expected:

- Any manual credits that involve cash are now recorded and display correctly when recording a cashup
- When creating manual credits, you can now choose the transaction type (not sure how to add transaction types, but just used the cash option)
- The cash register (register_id) is recorded correctly in the database

End result = when recording cashups for a register, the correct amount is shown for cash transactions.

Note that for the transaction history for the cash register, it displays as "No data available in table", but the total income is shown correctly.
Comment 30 Kyle M Hall 2022-10-21 10:52:35 UTC
Created attachment 142321 [details] [review]
Bug 29987: Add register support to manual credits

This patch adds the register and transaction type selection options to
the manual credit page.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 31 Kyle M Hall 2022-10-21 10:52:51 UTC
Created attachment 142322 [details] [review]
Bug 29987: (follow-up) Fix missing payment type

The payment type include has changed since the patch was first written,
we now require a 'type' is passed to properly set the field name.

This should fix the 'bankable' issue raised.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 32 Kyle M Hall 2022-10-21 10:52:55 UTC
Created attachment 142323 [details] [review]
Bug 29987: Display credit without offsets in transactions

This patch adds a row to the transactions table for credits without
corresponding offsets

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 33 Kyle M Hall 2022-10-21 10:52:59 UTC
Created attachment 142324 [details] [review]
Bug 29987: Improve validation

This patch adds hinting to display the required state of the cash
register fields.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 34 Kyle M Hall 2022-10-21 10:53:03 UTC
Created attachment 142325 [details] [review]
Bug 29987: (QA follow-up) Remove unused cnt attribute missing a filter

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 35 Tomás Cohen Arazi 2022-10-21 13:27:02 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 36 Lucas Gass 2022-12-05 21:18:00 UTC
Backported to 22.05.x for upcoming 22.05.08
Comment 37 Arthur Suzuki 2022-12-16 13:35:39 UTC
applied to 21.11 for 21.11.15
Comment 38 wainuiwitikapark 2023-03-15 00:58:31 UTC
Not backported to 21.05.x