Bug 27779

Summary: Cashup summary 'refunds' should denote what the refund was actioned against
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: Staff interfaceAssignee: Martin Renvoize <martin.renvoize>
Status: Needs documenting --- QA Contact: Katrin Fischer <katrin.fischer>
Severity: enhancement    
Priority: P5 - low CC: gmcharlt, lucas, rcoert, sally.healey, tomascohen, victor
Version: masterKeywords: release-notes-needed
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00
Bug Depends on: 26272, 26273, 31121    
Bug Blocks: 13985    
Attachments: Bug 27779: Group payouts by type of debit they're applied against
Bug 27779: Update 'REFUND' description in the database
Bug 27779: Group payouts by type of debit they're applied against
Bug 27779: Update 'REFUND' description in the database
Bug 27779: Group payouts by type of debit they're applied against
Bug 27779: Update 'REFUND' description in the database
Bug 27779: New atomic update syntax
Bug 27779: (QA follow-up) Fix translation issue with 'against'
Bug 27779: Group payouts by type of debit they're applied against
Bug 27779: Update 'REFUND' description in the database
Bug 27779: New atomic update syntax
Bug 27779: (QA follow-up) Fix translation issue with 'against'
Bug 27779: (QA follow-up) Fix database update
Bug 27779: Group payouts by type of debit they're applied against
Bug 27779: Update 'REFUND' description in the database
Bug 27779: New atomic update syntax
Bug 27779: (QA follow-up) Fix translation issue with 'against'
Bug 27779: (QA follow-up) Fix database update
Bug 27779: Group payouts by type of debit they're applied against
Bug 27779: Update 'REFUND' description in the database
Bug 27779: New atomic update syntax
Bug 27779: (QA follow-up) Fix translation issue with 'against'
Bug 27779: (QA follow-up) Fix database update
Bug 27779: (QA follow-up) Better translatability
Bug 27779: (QA follow-up) Better translatability
Bug 27779: (QA follow-up) Fix unit test

Description Martin Renvoize 2021-02-24 16:39:53 UTC
Currently, we group 'Refunds' together in one group. It would be preferable to group them by the type of transaction they're refunding.
Comment 1 Martin Renvoize 2021-05-20 07:50:02 UTC
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.
Comment 2 Martin Renvoize 2021-05-20 07:50:06 UTC
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'
Comment 3 Martin Renvoize 2021-05-20 07:50:43 UTC
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.
Comment 4 Martin Renvoize 2021-05-20 07:50:49 UTC
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'
Comment 5 Tomás Cohen Arazi 2021-11-11 18:32:33 UTC
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>
Comment 6 Tomás Cohen Arazi 2021-11-11 18:32:37 UTC
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>
Comment 7 Tomás Cohen Arazi 2021-11-11 18:32:40 UTC
Created attachment 127554 [details] [review]
Bug 27779: New atomic update syntax

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Katrin Fischer 2022-01-31 21:01:50 UTC
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 {
Comment 9 Katrin Fischer 2022-01-31 21:08:21 UTC
Created attachment 130024 [details] [review]
Bug 27779: (QA follow-up) Fix translation issue with 'against'
Comment 10 Katrin Fischer 2022-01-31 21:13:50 UTC
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.
Comment 11 Katrin Fischer 2022-01-31 21:51:01 UTC
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?
Comment 12 Katrin Fischer 2022-01-31 22:19:51 UTC
Realized my follow-up is a little wrong too - translatable strings should always be in double quotes "" because of some languages using '.
Comment 13 Martin Renvoize 2022-06-24 13:32:21 UTC Comment hidden (obsolete)
Comment 14 Martin Renvoize 2022-06-24 13:32:25 UTC Comment hidden (obsolete)
Comment 15 Martin Renvoize 2022-06-24 13:32:29 UTC Comment hidden (obsolete)
Comment 16 Martin Renvoize 2022-06-24 13:32:33 UTC Comment hidden (obsolete)
Comment 17 Martin Renvoize 2022-06-24 13:32:37 UTC
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>
Comment 18 Martin Renvoize 2022-06-24 13:34:04 UTC
Rebased, follow-up added to correct issue with DB update and corrected and signed off Katrins follow-up.

Setting back to signed off.
Comment 19 Martin Renvoize 2022-06-24 13:36:08 UTC
(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.
Comment 20 Martin Renvoize 2022-06-24 13:37:27 UTC
(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
Comment 21 Katrin Fischer 2022-07-08 09:40:35 UTC
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>
Comment 22 Katrin Fischer 2022-07-08 09:40:39 UTC
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>
Comment 23 Katrin Fischer 2022-07-08 09:40:43 UTC
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>
Comment 24 Katrin Fischer 2022-07-08 09:40:47 UTC
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>
Comment 25 Katrin Fischer 2022-07-08 09:40:51 UTC
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>
Comment 26 Tomás Cohen Arazi 2022-07-22 18:03:52 UTC
Please rebase.
Comment 27 Martin Renvoize 2022-08-08 14:18:40 UTC
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>
Comment 28 Martin Renvoize 2022-08-08 14:18:45 UTC
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>
Comment 29 Martin Renvoize 2022-08-08 14:18:49 UTC
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>
Comment 30 Martin Renvoize 2022-08-08 14:18:53 UTC
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>
Comment 31 Martin Renvoize 2022-08-08 14:18:57 UTC
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>
Comment 32 Martin Renvoize 2022-08-08 14:23:15 UTC
Rebased and fixed up the dependency tree in Bugzilla to reflect the order things were pushed.
Comment 33 Tomás Cohen Arazi 2022-08-08 16:51:09 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 34 Tomás Cohen Arazi 2022-08-08 18:42:00 UTC
Created attachment 138849 [details] [review]
Bug 27779: (QA follow-up) Better translatability

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 35 Tomás Cohen Arazi 2022-08-08 18:55:13 UTC
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.
Comment 36 Victor Grousset/tuxayo 2022-08-08 20:22:40 UTC
__x() usage looks correct 👍️

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
Comment 37 Martin Renvoize 2022-08-09 08:05:35 UTC
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>
Comment 38 Martin Renvoize 2022-08-09 08:13:24 UTC
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!
Comment 39 Martin Renvoize 2022-08-09 08:28:32 UTC
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.
Comment 40 Martin Renvoize 2022-08-09 08:29:30 UTC
Patch attached to fix the second failing test.. I can't seem to replicate the failure of the first one however?
Comment 41 Tomás Cohen Arazi 2022-08-09 10:53:03 UTC
Follow-up pushed to master.

Thanks!
Comment 42 Lucas Gass 2022-09-29 17:25:16 UTC
Enhancement will not be backported to 22.05.x series