|
Description
Martin Renvoize (ashimema)
2020-08-21 14:01:52 UTC
Created attachment 115803 [details] [review] Bug 26274: Add Cashups subclass and API classes Created attachment 115804 [details] [review] Bug 26274: Allow embedding summary Is there a test plan, or some way to test? 8-) Created attachment 115991 [details] [review] Bug 26274: Add cashups api routes This patch creates a Koha::Cash::Register::Cashup(s) class pair which subclass Koha::Cash::Register::Action(s) and moves the cashup specific code into these new classes to improve code separation. We then introduce API routes based on these classes to allow fetching a list of cashups associated to a cash register and a full cashup with emeddable summary for individual cashups. Test plan 1/ Run the updated unit tests. t/db_dependent/Koha/Cash/Register/Action.t t/db_dependent/Koha/Cash/Register/Cashup.t 2/ Run the incuded api tests. t/ Created attachment 115992 [details] [review] Bug 26274: Update register.tt to use the API This patch updates the existing register details page to utilise the new api routes to gather the summary details on demand. Test plan 1/ Enable cash registers 2/ Add some transactions 3/ Perform a cashup 4/ Click 'Summary' next to the last cashup date 5/ Note the modal appears as it did prior to the patch being applied. 6/ Check the print option still works 7/ Signoff Sorry David, Once I dug more into this there was a little more to do.. the current patches now have attached test plans :) Thanks Martin! Also, the patch no longer apply! 8-( Created attachment 116136 [details] [review] Bug 26274: Add cashups api routes This patch creates a Koha::Cash::Register::Cashup(s) class pair which subclass Koha::Cash::Register::Action(s) and moves the cashup specific code into these new classes to improve code separation. We then introduce API routes based on these classes to allow fetching a list of cashups associated to a cash register and a full cashup with emeddable summary for individual cashups. Test plan 1/ Run the updated unit tests. t/db_dependent/Koha/Cash/Register/Action.t t/db_dependent/Koha/Cash/Register/Cashup.t 2/ Run the incuded api tests. t/ Created attachment 116137 [details] [review] Bug 26274: Update register.tt to use the API This patch updates the existing register details page to utilise the new api routes to gather the summary details on demand. Test plan 1/ Enable cash registers 2/ Add some transactions 3/ Perform a cashup 4/ Click 'Summary' next to the last cashup date 5/ Note the modal appears as it did prior to the patch being applied. 6/ Check the print option still works 7/ Signoff Created attachment 116142 [details] [review] Bug 26274: Add cashups api routes This patch creates a Koha::Cash::Register::Cashup(s) class pair which subclass Koha::Cash::Register::Action(s) and moves the cashup specific code into these new classes to improve code separation. We then introduce API routes based on these classes to allow fetching a list of cashups associated to a cash register and a full cashup with emeddable summary for individual cashups. Test plan 1/ Run the updated unit tests. t/db_dependent/Koha/Cash/Register/Action.t t/db_dependent/Koha/Cash/Register/Cashup.t 2/ Run the incuded api tests. t/ Signed-off-by: David Nind <david@davidnind.com> Created attachment 116143 [details] [review] Bug 26274: Update register.tt to use the API This patch updates the existing register details page to utilise the new api routes to gather the summary details on demand. Test plan 1/ Enable cash registers 2/ Add some transactions 3/ Perform a cashup 4/ Click 'Summary' next to the last cashup date 5/ Note the modal appears as it did prior to the patch being applied. 6/ Check the print option still works 7/ Signoff Signed-off-by: David Nind <david@davidnind.com> I've signed off, but just checking about this step in the test plab: 2/ Run the incuded api tests. t/ I ran prove t/, so I hope that is what was required here. Created attachment 116145 [details] [review] Bug 26274: Add cashups api routes This patch creates a Koha::Cash::Register::Cashup(s) class pair which subclass Koha::Cash::Register::Action(s) and moves the cashup specific code into these new classes to improve code separation. We then introduce API routes based on these classes to allow fetching a list of cashups associated to a cash register and a full cashup with emeddable summary for individual cashups. Test plan 1/ Run the updated unit tests. t/db_dependent/Koha/Cash/Register/Action.t t/db_dependent/Koha/Cash/Register/Cashup.t 2/ Run the incuded api tests. t/db_dependent/api/v1/cashups.t Signed-off-by: David Nind <david@davidnind.com> Created attachment 116146 [details] [review] Bug 26274: Update register.tt to use the API This patch updates the existing register details page to utilise the new api routes to gather the summary details on demand. Test plan 1/ Enable cash registers 2/ Add some transactions 3/ Perform a cashup 4/ Click 'Summary' next to the last cashup date 5/ Note the modal appears as it did prior to the patch being applied. 6/ Check the print option still works 7/ Signoff Signed-off-by: David Nind <david@davidnind.com> (In reply to David Nind from comment #12) > I've signed off, but just checking about this step in the test plab: > > 2/ Run the incuded api tests. > t/ > > I ran prove t/, so I hope that is what was required here. Oops, seems I missed writing the actual test name in that second point.. however you will have inadvertently run it by running t/ I've fixed the commit message to include it now :) Thanks David :) Created attachment 116174 [details] [review] Bug 26274: Add cashups api routes This patch creates a Koha::Cash::Register::Cashup(s) class pair which subclass Koha::Cash::Register::Action(s) and moves the cashup specific code into these new classes to improve code separation. We then introduce API routes based on these classes to allow fetching a list of cashups associated to a cash register and a full cashup with emeddable summary for individual cashups. Test plan 1/ Run the updated unit tests. t/db_dependent/Koha/Cash/Register/Action.t t/db_dependent/Koha/Cash/Register/Cashup.t 2/ Run the incuded api tests. t/db_dependent/api/v1/cashups.t Signed-off-by: David Nind <david@davidnind.com> Created attachment 116175 [details] [review] Bug 26274: Update register.tt to use the API This patch updates the existing register details page to utilise the new api routes to gather the summary details on demand. Test plan 1/ Enable cash registers 2/ Add some transactions 3/ Perform a cashup 4/ Click 'Summary' next to the last cashup date 5/ Note the modal appears as it did prior to the patch being applied. 6/ Check the print option still works 7/ Signoff Signed-off-by: David Nind <david@davidnind.com> > Oops, seems I missed writing the actual test name in that second point..
> however you will have inadvertently run it by running t/
>
> I've fixed the commit message to include it now :)
Thanks Martin!
Just to confirm that the tests for t/db_dependent/api/v1/cashups.t pass.
David
Created attachment 116324 [details] [review] Bug 26274: Add cashups api routes This patch creates a Koha::Cash::Register::Cashup(s) class pair which subclass Koha::Cash::Register::Action(s) and moves the cashup specific code into these new classes to improve code separation. We then introduce API routes based on these classes to allow fetching a list of cashups associated to a cash register and a full cashup with emeddable summary for individual cashups. Test plan 1/ Run the updated unit tests. t/db_dependent/Koha/Cash/Register/Action.t t/db_dependent/Koha/Cash/Register/Cashup.t 2/ Run the incuded api tests. t/db_dependent/api/v1/cashups.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 116325 [details] [review] Bug 26274: Update register.tt to use the API This patch updates the existing register details page to utilise the new api routes to gather the summary details on demand. Test plan 1/ Enable cash registers 2/ Add some transactions 3/ Perform a cashup 4/ Click 'Summary' next to the last cashup date 5/ Note the modal appears as it did prior to the patch being applied. 6/ Check the print option still works 7/ Signoff Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 116326 [details] [review] Bug 26274: (QA follow-up) Minor fixes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 116327 [details] [review] Bug 26274: (QA follow-up) Minor fixes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Please fill the release notes. Pushed to master for 21.05, thanks to everybody involved! There is a failing test on My8
# Failed test 'income_grouped arrayref is correct'
# at t/db_dependent/Koha/Cash/Register/Cashup.t line 223.
# Structures begin differing at:
# $got->[0]{debit_type}{description} = 'Overdue fine'
# $expected->[0]{debit_type}{description} = 'Account creation fee'
# Looks like you failed 1 test of 29.
The order is different that the one we expect, I think we should order by created_on.
Created attachment 116939 [details] [review] Bug 26274: (QA follow-up) Fix the order of summary groups The grouped income/outgoing summaries were not being explicitly ordered and it appears the default ordering for MySQL 8 is different to that of MariaDB and previous versions of MySQL and as such was causing test failures for that version. This patch adds an explicit sort on the 'description' for each debit_type and credit_type grouping. (In reply to Jonathan Druart from comment #25) > There is a failing test on My8 > > # Failed test 'income_grouped arrayref is correct' > # at t/db_dependent/Koha/Cash/Register/Cashup.t line 223. > # Structures begin differing at: > # $got->[0]{debit_type}{description} = 'Overdue fine' > # $expected->[0]{debit_type}{description} = 'Account creation fee' > # Looks like you failed 1 test of 29. > > The order is different that the one we expect, I think we should order by > created_on. Actually.. I opted for an explicit sort on the description. I think this is more useful to the end user and also it prevents us hitting up against issues with the created_on field not being part of the grouping. We could, alternatively, sort on the amounts. This is a SUM, however, and I'm not sure how nicely that plays either.. and I personally think the alpha sort is more useful to end users who may want to compare one summary to another. Patch pushed to master, thanks Martin! Enhancement not pushed to 20.11.x + value => { flags => 25**2 } # cash_management flag = 25
It must be 2**25 instead.
|