|
Description
Katrin Fischer
2022-01-31 21:44:47 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. 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. Wondering: Maybe we need the payment type pull down on the credits? That way libraries could say it was a cash or other? We could certainly need the payment type pulldown to go with it.. I can take a look at adding this. 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. 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> 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. 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. 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. 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. 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 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. 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. 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 (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. 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? 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. 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. 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. 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. 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 Just a quick rebase and now testing on a sandbox.. I can't replicate any of your issues locally. (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) Created attachment 141171 [details] [review] Bug 29987: Improve validation This patch adds hinting to display the required state of the cash register fields. 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> 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> 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> 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> 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. 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> 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> 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> 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> 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> Pushed to master for 22.11. Nice work everyone, thanks! Backported to 22.05.x for upcoming 22.05.08 applied to 21.11 for 21.11.15 Not backported to 21.05.x |