Bug 29987 - Manual credits are not recorded for a register
Summary: Manual credits are not recorded for a register
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 13985
  Show dependency treegraph
 
Reported: 2022-01-31 21:44 UTC by Katrin Fischer
Modified: 2022-09-27 08:45 UTC (History)
5 users (show)

See Also:
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:


Attachments
Bug 29987: Add register support to manual credits (4.76 KB, patch)
2022-06-29 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29987: Add register support to manual credits (4.81 KB, patch)
2022-06-29 23:12 UTC, David Nind
Details | Diff | Splinter Review
Bug 29987: Display credit without offsets in transactions (3.84 KB, patch)
2022-08-24 10:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29987: (follow-up) Fix missing payment type (2.92 KB, patch)
2022-08-24 10:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29987: Display credit without offsets in transactions (3.84 KB, patch)
2022-08-24 10:25 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.