Bug 29271 - Cash register report not displaying or exporting correctly
Summary: Cash register report not displaying or exporting correctly
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 28264
Blocks: 30704
  Show dependency treegraph
 
Reported: 2021-10-19 13:02 UTC by Nick Clemens
Modified: 2022-05-16 04:12 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.05,20.11.19


Attachments
Bug 29271: Fix cash register report results (7.67 KB, patch)
2021-10-19 13:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29271: Fix cash register report results (7.73 KB, patch)
2021-10-19 14:12 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 29271: (QA follow-up) Add missing filter (1.04 KB, patch)
2021-10-19 21:11 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29271: (QA follow-up) Fetch and send descriptions to template (5.05 KB, patch)
2021-10-20 14:16 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29271: Fix cash register report results (7.83 KB, patch)
2021-10-20 20:58 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29271: (QA follow-up) Add missing filter (1.10 KB, patch)
2021-10-20 20:58 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29271: (QA follow-up) Fetch and send descriptions to template (5.13 KB, patch)
2021-10-20 20:58 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29271: (follow-up) Remove changes to accounts.inc (1.19 KB, patch)
2021-10-21 10:03 UTC, Nick Clemens
Details | Diff | Splinter Review
[20.11] Bug 29271: Fix cash register report results (8.02 KB, patch)
2022-04-07 16:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
[20.11] Bug 29271: (QA follow-up) Add missing filter (1.10 KB, patch)
2022-04-07 16:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
[20.11] Bug 29271: (QA follow-up) Fetch and send descriptions to template (5.62 KB, patch)
2022-04-07 16:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
[20.11] Bug 29271: (follow-up) Remove changes to accounts.inc (1.20 KB, patch)
2022-04-07 16:29 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-10-19 13:02:47 UTC
To recreate:
1 - Add some debits and credits to accounts
2 - Browse to Reports->Cash register
3 - Select transaction type 'All transactions'
4 - Output to screen
5 - Run report
6 - Note transaction types column is empty
7 - Output to a file
8 - Run report
9 - Open file and note:
      amount column is out of place
      extra line before total
Comment 1 Nick Clemens 2021-10-19 13:06:33 UTC
Created attachment 126498 [details] [review]
Bug 29271: Fix cash register report results

This patch changes accounts.inc to use the credit/debit type codes directly
rather than needlessly fetching the credit_type/debit_type object jsut to get
the code. This allows the BLOCK to work on non-object lines

We also only pass a debit or credit type to csv (accountline cannot be both)

empty-line.inc is also removed in favor of correcting the markup

one more column added to total line

To test:
1 - Add some debits and credits to accounts
2 - Browse to Reports->Cash register
3 - Select transaction type 'All transactions'
4 - Output to screen
5 - Run report
6 - Note transaction types column is empty
7 - Output to a file
8 - Run report
9 - Open file and note:
      amount column is out of place
      extra line before total
10 - Apply patch
11 - Repeat and note display and export are correct
Comment 2 Nick Clemens 2021-10-19 13:07:40 UTC
Note, this affects stables too, but they made need patches for their own version as the issues are slightly different (in 20.05 at least)
Comment 3 Andrew Fuerste-Henry 2021-10-19 14:12:59 UTC
Created attachment 126505 [details] [review]
Bug 29271: Fix cash register report results

This patch changes accounts.inc to use the credit/debit type codes directly
rather than needlessly fetching the credit_type/debit_type object jsut to get
the code. This allows the BLOCK to work on non-object lines

We also only pass a debit or credit type to csv (accountline cannot be both)

empty-line.inc is also removed in favor of correcting the markup

one more column added to total line

To test:
1 - Add some debits and credits to accounts
2 - Browse to Reports->Cash register
3 - Select transaction type 'All transactions'
4 - Output to screen
5 - Run report
6 - Note transaction types column is empty
7 - Output to a file
8 - Run report
9 - Open file and note:
      amount column is out of place
      extra line before total
10 - Apply patch
11 - Repeat and note display and export are correct

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 4 Katrin Fischer 2021-10-19 21:11:34 UTC
Created attachment 126548 [details] [review]
Bug 29271: (QA follow-up) Add missing filter
Comment 5 Katrin Fischer 2021-10-19 21:12:26 UTC
This is definitely a bug fix and improvement. I notice that the table displays the descriptions of the transaction types, but the export only lists the codes. Could we switch the export to descriptions too please?
Comment 6 Nick Clemens 2021-10-20 14:16:16 UTC
Created attachment 126590 [details] [review]
Bug 29271: (QA follow-up) Fetch and send descriptions to template
Comment 7 Nick Clemens 2021-10-20 14:17:22 UTC
(In reply to Katrin Fischer from comment #5)
> This is definitely a bug fix and improvement. I notice that the table
> displays the descriptions of the transaction types, but the export only
> lists the codes. Could we switch the export to descriptions too please?

Done, I was wrong before, we use the objects for the case where the type is not a system defined one. This report should eventually be updated to use object, for now I just fetch the description during the SQL call for simplicity/backportability
Comment 8 Katrin Fischer 2021-10-20 20:58:07 UTC
Created attachment 126611 [details] [review]
Bug 29271: Fix cash register report results

This patch changes accounts.inc to use the credit/debit type codes directly
rather than needlessly fetching the credit_type/debit_type object jsut to get
the code. This allows the BLOCK to work on non-object lines

We also only pass a debit or credit type to csv (accountline cannot be both)

empty-line.inc is also removed in favor of correcting the markup

one more column added to total line

To test:
1 - Add some debits and credits to accounts
2 - Browse to Reports->Cash register
3 - Select transaction type 'All transactions'
4 - Output to screen
5 - Run report
6 - Note transaction types column is empty
7 - Output to a file
8 - Run report
9 - Open file and note:
      amount column is out of place
      extra line before total
10 - Apply patch
11 - Repeat and note display and export are correct

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 9 Katrin Fischer 2021-10-20 20:58:11 UTC
Created attachment 126612 [details] [review]
Bug 29271: (QA follow-up) Add missing filter

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Katrin Fischer 2021-10-20 20:58:15 UTC
Created attachment 126613 [details] [review]
Bug 29271: (QA follow-up) Fetch and send descriptions to template

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Jonathan Druart 2021-10-21 08:14:54 UTC
This code must be moved to a module..
Comment 12 Jonathan Druart 2021-10-21 08:33:21 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 13 Jonathan Druart 2021-10-21 09:52:16 UTC
Caused an unexpected failure, please fix ASAP:

t/db_dependent/Letters/TemplateToolkit.t .. 25/28 
    #   Failed test 'Include used in notice'
    #   at t/db_dependent/Letters/TemplateToolkit.t line 1120.
    #          got: '    <span><span> (Cancelled)</span>    </span>'
    #     expected: '    <span>Payment<span> (Cancelled)</span>    </span>'
    # Looks like you failed 1 test of 1.
Comment 14 Nick Clemens 2021-10-21 10:03:51 UTC
Created attachment 126655 [details] [review]
Bug 29271: (follow-up) Remove changes to accounts.inc
Comment 15 Nick Clemens 2021-10-21 10:04:29 UTC
(In reply to Jonathan Druart from comment #13)
> Caused an unexpected failure, please fix ASAP:
> 
> t/db_dependent/Letters/TemplateToolkit.t .. 25/28 
>     #   Failed test 'Include used in notice'
>     #   at t/db_dependent/Letters/TemplateToolkit.t line 1120.
>     #          got: '    <span><span> (Cancelled)</span>    </span>'
>     #     expected: '    <span>Payment<span> (Cancelled)</span>    </span>'
>     # Looks like you failed 1 test of 1.

I missed a variable when undoing my changes
Comment 16 Jonathan Druart 2021-10-21 10:27:06 UTC
(In reply to Nick Clemens from comment #14)
> Created attachment 126655 [details] [review] [review]
> Bug 29271: (follow-up) Remove changes to accounts.inc

Pushed to master, thanks!
Comment 17 Kyle M Hall 2021-10-22 15:27:55 UTC
Pushed to 21.05.x for 21.05.05
Comment 18 Fridolin Somers 2021-10-23 02:10:30 UTC
Conflics on 20.11.x, mainly because of Bug 16486
Comment 19 Katrin Fischer 2022-04-07 16:28:55 UTC
Created attachment 133084 [details] [review]
[20.11] Bug 29271: Fix cash register report results

This patch changes accounts.inc to use the credit/debit type codes directly
rather than needlessly fetching the credit_type/debit_type object jsut to get
the code. This allows the BLOCK to work on non-object lines

We also only pass a debit or credit type to csv (accountline cannot be both)

empty-line.inc is also removed in favor of correcting the markup

one more column added to total line

To test:
1 - Add some debits and credits to accounts
2 - Browse to Reports->Cash register
3 - Select transaction type 'All transactions'
4 - Output to screen
5 - Run report
6 - Note transaction types column is empty
7 - Output to a file
8 - Run report
9 - Open file and note:
      amount column is out of place
      extra line before total
10 - Apply patch
11 - Repeat and note display and export are correct

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 20 Katrin Fischer 2022-04-07 16:29:00 UTC
Created attachment 133085 [details] [review]
[20.11] Bug 29271: (QA follow-up) Add missing filter

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Katrin Fischer 2022-04-07 16:29:05 UTC
Created attachment 133086 [details] [review]
[20.11] Bug 29271: (QA follow-up) Fetch and send descriptions to template

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 22 Katrin Fischer 2022-04-07 16:29:09 UTC
Created attachment 133087 [details] [review]
[20.11] Bug 29271: (follow-up) Remove changes to accounts.inc
Comment 23 Katrin Fischer 2022-04-07 16:29:40 UTC
Tried my hand at a rebase, with it things seem to be ok in the latest 20.11.17.
Comment 24 Victor Grousset/tuxayo 2022-04-07 18:58:21 UTC
Thanks Katrin.
Question also in regard other branches: Is that expected that the patches add spaces?

Before patch:
Manager name;Patron cardnumber;Patron name;Transaction library;Transaction date;Transaction type;Notes;Amount;Title;Barcode;Item type
 koha;eda;Eda Eda;Centerville;2022-04-07T00:00:00;;LOST;;1.00;;;
 koha;eda;Eda Eda;Centerville;2022-04-07T00:00:00;;LOST;;3.00;;;
 koha;eda;Eda Eda;Centerville;2022-04-07T00:00:00;CREDIT;;;-343.00;;;
 koha;eda;Eda Eda;Centerville;2022-04-07T00:00:00;;LOST;;112.00;;;
TOTAL
;;;;;;-227.00


After patch:
Manager name;Patron cardnumber;Patron name;Transaction library;Transaction date;Transaction type;Notes;Amount;Title;Barcode;Item type
     koha;eda;Eda Eda;Centerville;2022-04-07T00:00:00;Lost item;;1.00;;;
     koha;eda;Eda Eda;Centerville;2022-04-07T00:00:00;Lost item;;3.00;;;
     koha;eda;Eda Eda;Centerville;2022-04-07T00:00:00;Credit;;-343.00;;;
     koha;eda;Eda Eda;Centerville;2022-04-07T00:00:00;Lost item;;112.00;;;
TOTAL;;;;;;;-227.00
Comment 25 Katrin Fischer 2022-04-07 19:59:49 UTC
I didn't touch that file so I'd say no. Could some '-' in the variables be missing?
Comment 26 Victor Grousset/tuxayo 2022-04-09 18:40:15 UTC
> Could some '-' in the variables be missing?

Is that about template toolkit whitespace chomping?
http://www.template-toolkit.org/docs/manual/Syntax.html#section_Chomping_Whitespace

Anyway, is that an issue? Can that cause issues to consume the export?
Comment 27 Victor Grousset/tuxayo 2022-05-06 00:24:32 UTC
> Anyway, is that an issue?

Assuming it's not.

Backported: Pushed to 20.11.x branch for 20.11.19
Comment 28 Katrin Fischer 2022-05-06 08:09:22 UTC
(In reply to Victor Grousset/tuxayo from comment #26)
> > Could some '-' in the variables be missing?
> 
> Is that about template toolkit whitespace chomping?
> http://www.template-toolkit.org/docs/manual/Syntax.
> html#section_Chomping_Whitespace
> 
> Anyway, is that an issue? Can that cause issues to consume the export?

Hi Victor, sorry for not getting back to you about this earlier. I assume since this also happens on the other stable branches, it would be best to fix it on a separate bug. Thx for pushing!
Comment 29 wainuiwitikapark 2022-05-16 04:12:05 UTC
Does this need to be backported to 19.11.x?