Bug 23355 adds a process for recording cash register cashups; however it does not expose the captured cashup data, leaving that to the end user via reports. This bug serves as a request for enhancement to add a way to display a quick summary of the transactions that have make up a cashup set.
Created attachment 108093 [details] [review] Bug 26172: Add cashup summary view modal This patch adds a cashup summary modal to the register details page. Test plan 1/ Enable Cash Registers and Point of Sale 2/ Enable some debit_types as 'Sale items' 3/ Perform a series of transactions 4/ Perform a 'Cashup' on the register 5/ Note the new '(Summary)' link next to the last cashup on the register page. 6/ Click the link and confirm the modal contains the pertinent information
Initial proof of concept patch attached.. it would be good to get some early feedback whilst I write tests for the existing code and refine the finer details.
Created attachment 108095 [details] [review] Bug 26172: Add cashup summary view modal This patch adds a cashup summary modal to the register details page. Test plan 1/ Enable Cash Registers and Point of Sale 2/ Enable some debit_types as 'Sale items' 3/ Perform a series of transactions 4/ Perform a 'Cashup' on the register 5/ Note the new '(Summary)' link next to the last cashup on the register page. 6/ Click the link and confirm the modal contains the pertinent information
Hi Martin, thanks for writing this! When I confirmed the cashup: cgi-bin/koha/pos/register.pl?op=cashup I received the following error: Template process failed: undef error - DBIx::Class::Storage::DBI::_dbh_execute(): 'koha_testing1.debit_type_code.description' isn't in GROUP BY at /kohadevbox/koha/Koha/Objects.pm line 413 at /kohadevbox/koha/C4/Templates.pm line 122
Created attachment 108523 [details] [review] Bug 26172: Add cashup summary view modal This patch adds a cashup summary modal to the register details page. Test plan 1/ Enable Cash Registers and Point of Sale 2/ Enable some debit_types as 'Sale items' 3/ Perform a series of transactions 4/ Perform a 'Cashup' on the register 5/ Note the new '(Summary)' link next to the last cashup on the register page. 6/ Click the link and confirm the modal contains the pertinent information
I've fixed the issue you highlighted now Sally, thanks for testing.
(In reply to Martin Renvoize from comment #2) > Initial proof of concept patch attached.. it would be good to get some early > feedback whilst I write tests for the existing code and refine the finer > details. This looks great, Martin - really really useful. It's in a really logical location to retrieve, and it's great that the summary contains both the register name and cashing up period. The table is really clear, and the split between the 'Total' takings and the 'Bankable' physical cash is really useful. Having had a test with it, there's two things that would be useful: 1) Could a button be added to send the contents of the modal to the printer? Highlighting it and then choosing Print Selection achieves this (obviously), but it would be great to make that easier for those librarians who are less confident with computers. 2) From the Total and the Bankable, it's possible to calculate the payments which has already been reconciled (Banked?) - but could this total also be displayed to save any math errors by the librarians? I know Martin knows our system very well but to explain for others reading this, we currently have a custom cash module and all transactions processed through the till are listed under 'Total'. This is then split into payment types - e.g. Cash, Card, Cheque. This is because we submit these differently to our accounting department, so the cash up summary looks like this: Total: 57.00 Card: 37.00 Cash: 11.00 Cheque: 9.00 With the current modal, it would be useful if we could see the payment breakdown (like above), or an 'Banked' total listed, e.g. Total: 57.00 Banked: 37.00 Bankable: 20.00 So Total is all takings, Banked is credit/debit card payments that have already been reconciled, Bankable is cash/cheque which needs to be taken from the till and deposited in the bank. But yeah, it works brilliantly.
Created attachment 108817 [details] [review] Bug 26172: Add cashup summary view modal This patch adds a cashup summary modal to the register details page. Test plan 1/ Enable Cash Registers and Point of Sale 2/ Enable some debit_types as 'Sale items' 3/ Perform a series of transactions 4/ Perform a 'Cashup' on the register 5/ Note the new '(Summary)' link next to the last cashup on the register page. 6/ Click the link and confirm the modal contains the pertinent information
Created attachment 108818 [details] [review] Bug 26172: (follow-up) Summary by payment_type This patch alters the modal so that the totals are broken down by payment type's available on the system.
(In reply to Sally from comment #7) > 1) Could a button be added to send the contents of the modal to the > printer? Highlighting it and then choosing Print Selection achieves this > (obviously), but it would be great to make that easier for those librarians > who are less confident with computers. I'm still working on this one. > > 2) From the Total and the Bankable, it's possible to calculate the payments > which has already been reconciled (Banked?) - but could this total also be > displayed to save any math errors by the librarians? > > I know Martin knows our system very well but to explain for others reading > this, we currently have a custom cash module and all transactions processed > through the till are listed under 'Total'. This is then split into payment > types - e.g. Cash, Card, Cheque. This is because we submit these > differently to our accounting department, so the cash up summary looks like > this: > > Total: 57.00 > Card: 37.00 > Cash: 11.00 > Cheque: 9.00 > > With the current modal, it would be useful if we could see the payment > breakdown (like above), or an 'Banked' total listed, e.g. > > Total: 57.00 > Banked: 37.00 > Bankable: 20.00 > > So Total is all takings, Banked is credit/debit card payments that have > already been reconciled, Bankable is cash/cheque which needs to be taken > from the till and deposited in the bank. > > But yeah, it works brilliantly. Thanks for the feedback Sally :) I can easily implement the former approach, summarising totals by their payment types, and have attached a followup patch to achieve this. The latter format of Banked/Bankable is proving more allusive as it would require some additional handling within the definition of payments types such that we could easily identify types that are 'automatically banked' vs those that require the librarian to collect from the till at the end of the day. It seems obvious on first look, but I wonder for example in the cases where libraries take foodbank donations in lue of payments for example if that would be 'automatically banked' in so much as it immediately gets moved into a shared pile of foodbank goods vs being kept under the till to be checked at the end of the day. As such I think the later approach lends itself to requiring the payment_types authorized value being merged with credit_types first.
Created attachment 108822 [details] [review] Bug 26172: (follow-up) Add print option
Created attachment 108825 [details] [review] Bug 26172: Add cashup summary view modal This patch adds a cashup summary modal to the register details page. Test plan 1/ Enable Cash Registers and Point of Sale 2/ Enable some debit_types as 'Sale items' 3/ Perform a series of transactions 4/ Perform a 'Cashup' on the register 5/ Note the new '(Summary)' link next to the last cashup on the register page. 6/ Click the link and confirm the modal contains the pertinent information
Created attachment 108826 [details] [review] Bug 26172: (follow-up) Summary by payment_type This patch alters the modal so that the totals are broken down by payment type's available on the system.
Created attachment 108827 [details] [review] Bug 26172: (follow-up) Add print option
Added Eric as a CC here as they expressed interest in the 'End of Day' functionality which this enhances.
Created attachment 111162 [details] [review] Bug 26172: Add cashup summary view modal This patch adds a cashup summary modal to the register details page. Test plan 1/ Enable Cash Registers and Point of Sale 2/ Enable some debit_types as 'Sale items' 3/ Perform a series of transactions 4/ Perform a 'Cashup' on the register 5/ Note the new '(Summary)' link next to the last cashup on the register page. 6/ Click the link and confirm the modal contains the pertinent information Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Created attachment 111163 [details] [review] Bug 26172: (follow-up) Summary by payment_type This patch alters the modal so that the totals are broken down by payment type's available on the system. Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Created attachment 111164 [details] [review] Bug 26172: (follow-up) Add print option Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
(In reply to Martin Renvoize from comment #10) > I can easily implement the former approach, summarising totals by their > payment types, and have attached a followup patch to achieve this. Fantastic, that definitely meets our requirements. :) I added a couple of payment types and had a test, and all worked well. Thanks for implementing the print button, that's really useful. It'd be great if it could just send the contents of the modal to the printer (currently it sends the rest of the page as well) - but I don't feel it's a blocker, so I've signed off.
Thanks for testing Sally, I'm really happy it's working for you. On the print front, can I ask what browser you tested with and whether you ran the CSS build option on the sandbox? The code is intended to hide the background and only print the modal content so if you weren't seeing that I may still have a bug there. I'll investigate and can work through it with whoever picks this up for QA if it's not working as expected. Thanks again
(In reply to Martin Renvoize from comment #20) > Thanks for testing Sally, I'm really happy it's working for you. > > On the print front, can I ask what browser you tested with and whether you > ran the CSS build option on the sandbox? The code is intended to hide the > background and only print the modal content so if you weren't seeing that I > may still have a bug there. I'll investigate and can work through it with > whoever picks this up for QA if it's not working as expected. > > Thanks again Argh, my mistake - I didn't run build CSS. I shouldn't have been rushing to get it tested on Friday! I've re-tested, and can confirm that it works as expected.
Created attachment 111598 [details] [review] Bug 26172: Add cashup summary view modal This patch adds a cashup summary modal to the register details page. Test plan 1/ Enable Cash Registers and Point of Sale 2/ Enable some debit_types as 'Sale items' 3/ Perform a series of transactions 4/ Perform a 'Cashup' on the register 5/ Note the new '(Summary)' link next to the last cashup on the register page. 6/ Click the link and confirm the modal contains the pertinent information Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Created attachment 111599 [details] [review] Bug 26172: (follow-up) Summary by payment_type This patch alters the modal so that the totals are broken down by payment type's available on the system. Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Created attachment 111600 [details] [review] Bug 26172: (follow-up) Add print option Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Created attachment 111601 [details] [review] Bug 26172: (QA follow-up) Add missing filter
Ok, failing this for the missing unit tests, but already adding my QA stamp on the other patches. 2 things: - When doing the cash-up action from the tools page, after the cash-up the summary link for printing doesn't display. It only displays on the 'register details'. I will file a separate bug for this as a later improvement. - I think it would be pretty great to have a way to see the 'recorded' cashups with summary links for each to allow libraries to get more of an overview of cashups, the amounts taken - this would be helpful when something has gone wrong but also to see if processes could be improved. I'll link both to this one as see alsos.
Created attachment 111629 [details] [review] Bug 26172: Add cashup summary view modal This patch adds a cashup summary modal to the register details page. Test plan 1/ Enable Cash Registers and Point of Sale 2/ Enable some debit_types as 'Sale items' 3/ Perform a series of transactions 4/ Perform a 'Cashup' on the register 5/ Note the new '(Summary)' link next to the last cashup on the register page. 6/ Click the link and confirm the modal contains the pertinent information Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Created attachment 111630 [details] [review] Bug 26172: (follow-up) Summary by payment_type This patch alters the modal so that the totals are broken down by payment type's available on the system. Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Created attachment 111631 [details] [review] Bug 26172: (follow-up) Add print option Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Created attachment 111632 [details] [review] Bug 26172: (QA follow-up) Add missing filter
Created attachment 111633 [details] [review] Bug 26172: (QA follow-up) Fix order of summary
Created attachment 111634 [details] [review] Bug 26172: (QA follow-up) Add Unit Test
Unit tests added, leaving to Katrin for the final QA Stamp :)
Created attachment 111668 [details] [review] Bug 26172: (QA follow-up) Add Unit Test Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
The new tests pass, but the QA script fails: FAIL t/db_dependent/Koha/Cash/Register/Action.t OK critic FAIL file permissions File must have the exec flag FAIL forbidden patterns forbidden pattern: Do not use compare float with precision (bug 24408). Ignore this if you know what you are doing. (line 197) forbidden pattern: Do not use compare float with precision (bug 24408). Ignore this if you know what you are doing. (line 207) Can you assure me you know what you are doing or make it go away?
Created attachment 111683 [details] [review] Bug 26172: (QA follow-up) Add Unit Test Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Grr, How did I forget to run the QA script against this! I've corrected the file permission issue. As for the float comparison I will defer to Jonathan.. really I should be doing the +0 trick somewhere, but as this is within an 'is_deeply' comparison, I'm not actually sure where I can place it.
Created attachment 111691 [details] [review] Bug 26172: Fix return value for ->register There is a ON CASCADE DELETE clause on this FK
(In reply to Jonathan Druart from comment #38) > Created attachment 111691 [details] [review] [review] > Bug 26172: Fix return value for ->register > > There is a ON DELETE CASCADE clause on this FK Martin, there is the same problem for ->manager (not coming from this patchset). It must be fixed separately. Actually I think the code is code, but the ON DELETE CASCADE should be SET NULL instead for this one.
Created attachment 111692 [details] [review] Bug 26172: Fix return value for ->register There is a ON DELETE CASCADE clause on this FK
1. Tests must cover the "$previous" case (that is never set). 2. Then the code may call something like my ($timestamp) = ->search->get_column('timestamp'); to avoid the single and _result calls.
Created attachment 111731 [details] [review] Bug 26175: (QA follow-up) Add tests for previous cashups
Created attachment 111732 [details] [review] Bug 26172: (QA follow-up) Add tests for previous cashups
Created attachment 112907 [details] [review] Bug 26172: Replace calls to _result[set]
Created attachment 112908 [details] [review] Bug 26172: Replace _result[set] calls
Created attachment 112909 [details] [review] Bug 26172: Replace _result[set] calls
(In reply to Jonathan Druart from comment #46) > Created attachment 112909 [details] [review] [review] > Bug 26172: Replace _result[set] calls Martin, do you agree with this change?
Seems reasonable to me.. I'm going to run the test with dbic_trace enabled to double check the db calls we make.. it feels like we might have introduced a couple of extra one's this way. Having said that.. I'm not sure how a correlate subquery performs compared to a pair of independent queries anyway.
So.. I've tested my hypothesis and your version does indeed result in 2 additional DB calls (the subselect being executed independently).. However, it does not change performance in any meaningful way (I benchamarked it).. So.. for consistency and to be more Koha::Object like, not having to reach inside dbic guts to much I would take your approach. Thanks for the followup :)
Pushed to master for 20.11, thanks to everybody involved!
enhancement will not be backported to 20.05.x