Currently, we group 'Refunds' together in one group. It would be preferable to group them by the type of transaction they're refunding.
Created attachment 121218 [details] [review] Bug 27779: Group payouts by type of debit they're applied against This patch adds additional grouping to the cashup summary output such that payouts are additionally grouped by the types of debit they're applied against. The cashup sumary modal is adapted to expose the descriptions at the grouping level too. Test plan 1/ Add a various transactions using a cash register (Using Point of Sale, Patron Accounts with payments etc). 2/ Refund some of the debts and pick the 'cash' option for payout. (ensure you pick a variety of debit types) 3/ Add some credit to a patron account, (either refund a debt as 'credit' or add a 'manual credit') 4/ Payout the credit as 'cash' on the patron account 5/ Cashup the register 6/ Inspect the cashup summary for your cashup.
Created attachment 121219 [details] [review] Bug 27779: Update 'REFUND' description in the database This patch simplified the REFUND description from 'Refund applied to a patrons fine' to simply 'Refund'.. this allows the description to be combined with debit type descriptions simply to denote what it was applied against instead of using the generic 'applied to a patrons fine'
Created attachment 121224 [details] [review] Bug 27779: Group payouts by type of debit they're applied against This patch adds additional grouping to the cashup summary output such that payouts are additionally grouped by the types of debit they're applied against. The cashup sumary modal is adapted to expose the descriptions at the grouping level too. Test plan 1/ Add a various transactions using a cash register (Using Point of Sale, Patron Accounts with payments etc). 2/ Refund some of the debts and pick the 'cash' option for payout. (ensure you pick a variety of debit types) 3/ Add some credit to a patron account, (either refund a debt as 'credit' or add a 'manual credit') 4/ Payout the credit as 'cash' on the patron account 5/ Cashup the register 6/ Inspect the cashup summary for your cashup.
Created attachment 121225 [details] [review] Bug 27779: Update 'REFUND' description in the database This patch simplified the REFUND description from 'Refund applied to a patrons fine' to simply 'Refund'.. this allows the description to be combined with debit type descriptions simply to denote what it was applied against instead of using the generic 'applied to a patrons fine'
Created attachment 127552 [details] [review] Bug 27779: Group payouts by type of debit they're applied against This patch adds additional grouping to the cashup summary output such that payouts are additionally grouped by the types of debit they're applied against. The cashup sumary modal is adapted to expose the descriptions at the grouping level too. Test plan 1/ Add a various transactions using a cash register (Using Point of Sale, Patron Accounts with payments etc). 2/ Refund some of the debts and pick the 'cash' option for payout. (ensure you pick a variety of debit types) 3/ Add some credit to a patron account, (either refund a debt as 'credit' or add a 'manual credit') 4/ Payout the credit as 'cash' on the patron account 5/ Cashup the register 6/ Inspect the cashup summary for your cashup. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 127553 [details] [review] Bug 27779: Update 'REFUND' description in the database This patch simplified the REFUND description from 'Refund applied to a patrons fine' to simply 'Refund'.. this allows the description to be combined with debit type descriptions simply to denote what it was applied against instead of using the generic 'applied to a patrons fine' Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 127554 [details] [review] Bug 27779: New atomic update syntax Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Small translation issue here: + if ( out.credit_type_code == 'REFUND' ) { + tbody.append('<tr><td>' + out.credit_type.description + ' against ' + out.related_debit.debit_type.description + '</td><td>- ' + out.total + '</td></tr>'); + } else {
Created attachment 130024 [details] [review] Bug 27779: (QA follow-up) Fix translation issue with 'against'
Wondering about another I18N issue here: You assume in your database update that Refund has not been translated to read something different. But they will be translated if you have used a translated web installer, right? So this will throw it back to English, with no way to fix it apart from using SQL, as the GUI doesn't allow to edit the system debit and credit types. Could we negotiate making the description field editable in the GUI on a separate bug? I don't see another way to fix this translation issue nicely.
Testing this led me to filing bug 29985, bug 29986 and bug 29987. :( I am not sure if I am doing this right or wrong. All of these not being strictly related to this patch set, apart from maybe the descriptions issue. The one remaining thing now is that I feel like we should really price format this output (meaning adhering to CurrencyFormat). Any hope we could do this in a follow up?
Realized my follow-up is a little wrong too - translatable strings should always be in double quotes "" because of some languages using '.
Created attachment 136491 [details] [review] Bug 27779: Group payouts by type of debit they're applied against This patch adds additional grouping to the cashup summary output such that payouts are additionally grouped by the types of debit they're applied against. The cashup sumary modal is adapted to expose the descriptions at the grouping level too. Test plan 1/ Add a various transactions using a cash register (Using Point of Sale, Patron Accounts with payments etc). 2/ Refund some of the debts and pick the 'cash' option for payout. (ensure you pick a variety of debit types) 3/ Add some credit to a patron account, (either refund a debt as 'credit' or add a 'manual credit') 4/ Payout the credit as 'cash' on the patron account 5/ Cashup the register 6/ Inspect the cashup summary for your cashup. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 136492 [details] [review] Bug 27779: Update 'REFUND' description in the database This patch simplified the REFUND description from 'Refund applied to a patrons fine' to simply 'Refund'.. this allows the description to be combined with debit type descriptions simply to denote what it was applied against instead of using the generic 'applied to a patrons fine' Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 136493 [details] [review] Bug 27779: New atomic update syntax Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 136494 [details] [review] Bug 27779: (QA follow-up) Fix translation issue with 'against' Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 136495 [details] [review] Bug 27779: (QA follow-up) Fix database update Don't overwrite peoples translations if they've already changed the description. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Rebased, follow-up added to correct issue with DB update and corrected and signed off Katrins follow-up. Setting back to signed off.
(In reply to Katrin Fischer from comment #10) > Wondering about another I18N issue here: You assume in your database update > that Refund has not been translated to read something different. But they > will be translated if you have used a translated web installer, right? So > this will throw it back to English, with no way to fix it apart from using > SQL, as the GUI doesn't allow to edit the system debit and credit types. > > Could we negotiate making the description field editable in the GUI on a > separate bug? I don't see another way to fix this translation issue nicely. I've corrected the database update so it won't overwrite already changed descriptions. The descriptions are already editable in the GUI.. they're just debit/credit types.
(In reply to Katrin Fischer from comment #11) > The one remaining thing now is that I feel like we should really price > format this output (meaning adhering to CurrencyFormat). > > Any hope we could do this in a follow up? It's done on bug 31038
Created attachment 137386 [details] [review] Bug 27779: Group payouts by type of debit they're applied against This patch adds additional grouping to the cashup summary output such that payouts are additionally grouped by the types of debit they're applied against. The cashup sumary modal is adapted to expose the descriptions at the grouping level too. Test plan 1/ Add a various transactions using a cash register (Using Point of Sale, Patron Accounts with payments etc). 2/ Refund some of the debts and pick the 'cash' option for payout. (ensure you pick a variety of debit types) 3/ Add some credit to a patron account, (either refund a debt as 'credit' or add a 'manual credit') 4/ Payout the credit as 'cash' on the patron account 5/ Cashup the register 6/ Inspect the cashup summary for your cashup. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 137387 [details] [review] Bug 27779: Update 'REFUND' description in the database This patch simplified the REFUND description from 'Refund applied to a patrons fine' to simply 'Refund'.. this allows the description to be combined with debit type descriptions simply to denote what it was applied against instead of using the generic 'applied to a patrons fine' Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 137388 [details] [review] Bug 27779: New atomic update syntax Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 137389 [details] [review] Bug 27779: (QA follow-up) Fix translation issue with 'against' Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 137390 [details] [review] Bug 27779: (QA follow-up) Fix database update Don't overwrite peoples translations if they've already changed the description. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Please rebase.
Created attachment 138809 [details] [review] Bug 27779: Group payouts by type of debit they're applied against This patch adds additional grouping to the cashup summary output such that payouts are additionally grouped by the types of debit they're applied against. The cashup sumary modal is adapted to expose the descriptions at the grouping level too. Test plan 1/ Add a various transactions using a cash register (Using Point of Sale, Patron Accounts with payments etc). 2/ Refund some of the debts and pick the 'cash' option for payout. (ensure you pick a variety of debit types) 3/ Add some credit to a patron account, (either refund a debt as 'credit' or add a 'manual credit') 4/ Payout the credit as 'cash' on the patron account 5/ Cashup the register 6/ Inspect the cashup summary for your cashup. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 138810 [details] [review] Bug 27779: Update 'REFUND' description in the database This patch simplified the REFUND description from 'Refund applied to a patrons fine' to simply 'Refund'.. this allows the description to be combined with debit type descriptions simply to denote what it was applied against instead of using the generic 'applied to a patrons fine' Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 138811 [details] [review] Bug 27779: New atomic update syntax Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 138812 [details] [review] Bug 27779: (QA follow-up) Fix translation issue with 'against' Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 138813 [details] [review] Bug 27779: (QA follow-up) Fix database update Don't overwrite peoples translations if they've already changed the description. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Rebased and fixed up the dependency tree in Bugzilla to reflect the order things were pushed.
Pushed to master for 22.11. Nice work everyone, thanks!
Created attachment 138849 [details] [review] Bug 27779: (QA follow-up) Better translatability Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Hi, please review my follow-up patch and sign it if correct. The way the phrase was built is incorrect IMO, but I didn't find precedent uses of __x() in the codebase. Also, and important too: t/db_dependent/Koha/Cash/Register/Cashups.t .. # Failed test 'Returns only CASHUP actions' # at t/db_dependent/Koha/Cash/Register/Cashups.t line 66. # got: '5' # expected: '1' # Looks like you failed 1 test of 3. t/db_dependent/Koha/Cash/Register/Cashups.t .. 1/1 # Failed test 'search' # at t/db_dependent/Koha/Cash/Register/Cashups.t line 74. # Looks like you failed 1 test of 1. t/db_dependent/Koha/Cash/Register/Cashups.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/1 subtests t/db_dependent/Koha/Cash/Register/Cashup.t ... 1/3 # Failed test 'payout_grouped arrayref is correct' # at t/db_dependent/Koha/Cash/Register/Cashup.t line 226. # Structures begin differing at: # $got->[0]{related_debit} = HASH(0x55c08306c098) # $expected->[0]{related_debit} = Does not exist # Failed test 'payout_grouped arrayref is correct' # at t/db_dependent/Koha/Cash/Register/Cashup.t line 346. # Structures begin differing at: # $got->[0]{credit_type}{description} = 'Refund' # $expected->[0]{credit_type}{description} = 'A refund applied to a patrons fine' # Looks like you failed 2 tests of 29. t/db_dependent/Koha/Cash/Register/Cashup.t ... 3/3 # Failed test 'summary' # at t/db_dependent/Koha/Cash/Register/Cashup.t line 350. # Looks like you failed 1 test of 3.
__x() usage looks correct [U+1F44D]️ https://metacpan.org/pod/Locale::TextDomain https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15395#c131 can't say for the rest of the patch: correct building of the phrase, complete and accurate usage of format_price or escape_str
Created attachment 138883 [details] [review] Bug 27779: (QA follow-up) Better translatability Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Great improvement for translation.. think this was originally written long enough ago that I wasn't aware of nicer ways to do it for translators and I never really revisited how this actually worked here.. great spot Tomas!
Created attachment 138884 [details] [review] Bug 27779: (QA follow-up) Fix unit test I forgot to adapt the unit test for the new 'related_debit' key in the outpur from the summary method.
Patch attached to fix the second failing test.. I can't seem to replicate the failure of the first one however?
Follow-up pushed to master. Thanks!
Enhancement will not be backported to 22.05.x series