Bug 31041

Summary: Cashup summary modal printing issue
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: Staff interfaceAssignee: Lucas Gass <lucas>
Status: Pushed to oldstable --- QA Contact: Martin Renvoize <martin.renvoize>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, gmcharlt, jheltibridle, lucas, oleonard, sally.healey
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This bugfix updates the modal printing system to trigger a new page for dialogue printing. Whilst this causes a minor flash unsightly content at print preview, it significantly improves the reliability of modal printing where such dialogues appear on pages containing a lot of content or the modals themselves contain a enough content to require a scroll.
Version(s) released in:
23.11.00,23.05.06,22.11.12
Bug Depends on:    
Bug Blocks: 13985, 35295    
Attachments: Bug 31041: Prevent duplicate display of cashup summary when printing
Bug 31041: Prevent duplicate display of cashup summary when printing
Bug 31041: Prevent duplicate display of cashup summary when printing
Bug 31041: (follow-up) Clean up and generalise
Bug 31041: DO NOT PUSH - Testing Only
Bug 31041: DO NOT PUSH - Testing Only
Bug 31041: add modal_printer.js asset to registers.tt
Bug 31041: Prevent duplicate display of cashup summary when printing
Bug 31041: (follow-up) Clean up and generalise
Bug 31041: Add modal_printer.js asset to registers.tt
Bug 31041: DO NOT PUSH - Testing Only

Description Martin Renvoize 2022-06-24 13:19:40 UTC
When printing the cashup summary modal, if the content is long enough to make the modal scrollable then the print cuts off/duplicates some of the details over the second page.
Comment 1 Lucas Gass 2023-11-02 14:38:06 UTC
I was about to file a bug when I found this one. I have a similar, but not exactly the same, problem. 

We are noticing that the Cashup summary can be duplicated when the register.pl page prints longer than two pages. From a stackoverflow post:

"The problem is that the content underlying the modal determines how many pages are printed and if you are trying to print the content of the modal it will print duplicates equaling the number pages in the underlying document"

https://stackoverflow.com/questions/68206205/same-page-prints-twice-when-using-a-bootstrap-full-screen-modal


So, to recreate:
1. Have POS on.
2. Have some registers and items for sale. Make some sales to 'Cashup'. 
3. If you have enough enough lines in the table so that 'register.pl' would print on 2 or more pages, the summary will be duplicated that same number of time. 

If you are lazy like me and don't want to make that many transaction you can also add some content to the page like this:

for (let i = 0; i < 100; i++) {
  $('#register').append('<h1>TEST</h1>');
}
Comment 2 Lucas Gass 2023-11-02 19:57:18 UTC
Created attachment 158283 [details] [review]
Bug 31041: Prevent duplicate display of cashup summary when printing

To test:
1. Have POS on.
2. Have some registers and items for sale. Make some sales to 'Cashup'.
3. If you have enough enough lines in the table so that 'register.pl' would print on 2 or more pages, the summary will be duplicated that same number of time.

If you are lazy like me and don't want to make that many transaction you can also add some content to the page like this:

for (let i = 0; i < 100; i++) {
  $('#register').append('<h1>TEST</h1>');
}

4. APPLY PATCH, clear browser cahche
5. Try printing again, you should only get one cashup summary when printing.
Comment 3 Lucas Gass 2023-11-02 20:16:34 UTC
CC'ing Owen here.
Comment 4 ByWater Sandboxes 2023-11-03 15:02:19 UTC
Created attachment 158342 [details] [review]
Bug 31041: Prevent duplicate display of cashup summary when printing

To test:
1. Have POS on.
2. Have some registers and items for sale. Make some sales to 'Cashup'.
3. If you have enough enough lines in the table so that 'register.pl' would print on 2 or more pages, the summary will be duplicated that same number of time.

If you are lazy like me and don't want to make that many transaction you can also add some content to the page like this:

for (let i = 0; i < 100; i++) {
  $('#register').append('<h1>TEST</h1>');
}

4. APPLY PATCH, clear browser cahche
5. Try printing again, you should only get one cashup summary when printing.

Signed-off-by: Juliet Heltibridle <jheltibridle@rcplib.org>
Comment 5 Katrin Fischer 2023-11-05 13:13:48 UTC
I am having some trouble testing here, even with the "cheat" - can you please add some more instructions on how to test and how to apply the JS?
Comment 6 Juliet Heltibridle 2023-11-06 15:25:19 UTC
(In reply to Katrin Fischer from comment #5)
> I am having some trouble testing here, even with the "cheat" - can you
> please add some more instructions on how to test and how to apply the JS?

Katrin, are you making sure you are testing this on the register.pl page and not the registers.pl page? The registers.pl page works correctly.

To test:
1. Have POS on.
2. Have some registers and items for sale. Make some sales to 'Cashup'.
2b. Record a Cashup
2c. Go to register.pl page (transaction history page) and attempt to print the Cashup summary.
3. If you have enough enough lines in the table so that 'register.pl' would print on 2 or more pages, the summary will be duplicated that same number of time.
Comment 7 Martin Renvoize 2023-11-06 16:36:20 UTC
This causes the print formatting to change a bit.. I'm not entirely against that, but it is a change.

I need to think a bit about how this is implemented.  The 'modalprinter' patturn is in use for the bundle confirmation modal as well as this one.. so it would be good to have a standard include for the JS that can be re-used for other printable modals.

The basic premise here is.. Open the content of the modal in a new window and then print that.. as apposed the existing "hide everything that's not the modal" method that was in place prior to here.. we could certainly do some cleanup if we decide the 'open new window' approach is prefereable.

I think I tried and failed to get the 'hide content' version to work as a fix here in the past so maybe the open window is the best way.
Comment 8 Katrin Fischer 2023-11-06 19:07:26 UTC
Thx for the help, Juliet :) 

Since Martin knows this code really well (having written it), I'll leave QA to him.
Comment 9 Martin Renvoize 2023-11-07 11:38:37 UTC
Created attachment 158589 [details] [review]
Bug 31041: Prevent duplicate display of cashup summary when printing

To test:
1. Have POS on.
2. Have some registers and items for sale. Make some sales to 'Cashup'.
3. If you have enough enough lines in the table so that 'register.pl' would print on 2 or more pages, the summary will be duplicated that same number of time.

If you are lazy like me and don't want to make that many transaction you can also add some content to the page like this:

for (let i = 0; i < 100; i++) {
  $('#register').append('<h1>TEST</h1>');
}

4. APPLY PATCH, clear browser cahche
5. Try printing again, you should only get one cashup summary when printing.

Signed-off-by: Juliet Heltibridle <jheltibridle@rcplib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2023-11-07 11:38:40 UTC
Created attachment 158590 [details] [review]
Bug 31041: (follow-up) Clean up and generalise

This patch cleans out the print media css rules that were originally
added for modalprinting.  The window.open method is simpler to maintain
and will more reliably print modal content as expected.

We factor out the printer code into it's own JS asset that we can apply
to other printable modals and then use it in the two existing places
where such modals have been defined already (cashup summary and bundle
confirmation).

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2023-11-07 11:45:58 UTC
Created attachment 158591 [details] [review]
Bug 31041: DO NOT PUSH - Testing Only

This patch adds a script to allow easy population of a cashup using KTD.

To use it
1) Start up a KTD
2) Enable UseCashRegisters system preference
3) Add a cash register with the name 'TEST'
4) Run this script from the command line
5) Navigate to the register you created to look at the cashups
6) Click on the summary link and print at the bottom of the modal and
   confirm that the modal prints as expected accross multiple pages
   without duplications or data lose.
7) Back on the register page, select to view the past months 'Older
   transactions'. This will lengthen the page significantly
8) Click on the summary link again and print.. Confirm that again the
   correct number of pages is printed and contain the correct content.

This tests both scenarios.. where the background page spans multiple
pages and where the modal content requires a scroll.
Comment 12 Martin Renvoize 2023-11-07 11:48:08 UTC
Created attachment 158592 [details] [review]
Bug 31041: DO NOT PUSH - Testing Only

This patch adds a script to allow easy population of a cashup using KTD.

To use it
1) Start up a KTD
2) Enable UseCashRegisters system preference
3) Add a cash register with the name 'TEST'
4) Run this script from the command line
5) Navigate to the register you created to look at the cashups
6) Click on the summary link and print at the bottom of the modal and
   confirm that the modal prints as expected accross multiple pages
   without duplications or data lose.
7) Back on the register page, select to view the past months 'Older
   transactions'. This will lengthen the page significantly
8) Click on the summary link again and print.. Confirm that again the
   correct number of pages is printed and contain the correct content.

This tests both scenarios.. where the background page spans multiple
pages and where the modal content requires a scroll.
Comment 13 Martin Renvoize 2023-11-07 13:02:08 UTC
All working well, QA scripts happy. Passing QA
Comment 14 Lucas Gass 2023-11-07 15:29:20 UTC
Thanks Martin! This all looks pretty good. However, there is one link in the #registers table ( in the Last cashup column) where the modal will pop-up but when I click 'Print' nothing happens.
Comment 15 Lucas Gass 2023-11-07 22:33:27 UTC
Created attachment 158636 [details] [review]
Bug 31041: add modal_printer.js asset to registers.tt
Comment 16 Martin Renvoize 2023-11-08 07:07:29 UTC
Cheers for taking care of the missing inc Lucas, I fell asleep last night before getting to it. :-)
Comment 17 Martin Renvoize 2023-11-08 07:32:23 UTC
Created attachment 158644 [details] [review]
Bug 31041: Prevent duplicate display of cashup summary when printing

To test:
1. Have POS on.
2. Have some registers and items for sale. Make some sales to 'Cashup'.
3. If you have enough enough lines in the table so that 'register.pl' would print on 2 or more pages, the summary will be duplicated that same number of time.

If you are lazy like me and don't want to make that many transaction you can also add some content to the page like this:

for (let i = 0; i < 100; i++) {
  $('#register').append('<h1>TEST</h1>');
}

4. APPLY PATCH, clear browser cahche
5. Try printing again, you should only get one cashup summary when printing.

Signed-off-by: Juliet Heltibridle <jheltibridle@rcplib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2023-11-08 07:32:26 UTC
Created attachment 158645 [details] [review]
Bug 31041: (follow-up) Clean up and generalise

This patch cleans out the print media css rules that were originally
added for modalprinting.  The window.open method is simpler to maintain
and will more reliably print modal content as expected.

We factor out the printer code into it's own JS asset that we can apply
to other printable modals and then use it in the two existing places
where such modals have been defined already (cashup summary and bundle
confirmation).

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 19 Martin Renvoize 2023-11-08 07:32:29 UTC
Created attachment 158646 [details] [review]
Bug 31041: Add modal_printer.js asset to registers.tt

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2023-11-08 07:32:31 UTC
Created attachment 158647 [details] [review]
Bug 31041: DO NOT PUSH - Testing Only

This patch adds a script to allow easy population of a cashup using KTD.

To use it
1) Start up a KTD
2) Enable UseCashRegisters system preference
3) Add a cash register with the name 'TEST'
4) Run this script from the command line
5) Navigate to the register you created to look at the cashups
6) Click on the summary link and print at the bottom of the modal and
   confirm that the modal prints as expected accross multiple pages
   without duplications or data lose.
7) Back on the register page, select to view the past months 'Older
   transactions'. This will lengthen the page significantly
8) Click on the summary link again and print.. Confirm that again the
   correct number of pages is printed and contain the correct content.

This tests both scenarios.. where the background page spans multiple
pages and where the modal content requires a scroll.
Comment 21 Martin Renvoize 2023-11-08 07:33:30 UTC
Tested the follow-up and setting to PQA :)
Comment 22 Tomás Cohen Arazi 2023-11-08 12:59:54 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 23 Victor Grousset/tuxayo 2023-11-08 21:00:12 UTC
It breaks the check-in pop notifying of holds! :o

commit ef880e349f959686a830bda67ec5a295e3d76eb4
Bug 31041: (follow-up) Clean up and generalise

Opened bug 35295
Comment 24 Fridolin Somers 2023-11-10 08:06:18 UTC
Pushed to 23.05.x for 23.05.06
Comment 25 Katrin Fischer 2023-11-10 10:10:14 UTC
(In reply to Fridolin Somers from comment #24)
> Pushed to 23.05.x for 23.05.06

This one cause a problem - you will need bug 35295 as well!
Comment 26 Pedro Amorim 2023-11-14 14:49:47 UTC
Nice work everyone!

Pushed to 22.11.x for next release
Comment 27 Fridolin Somers 2023-11-15 07:55:15 UTC
(In reply to Katrin Fischer from comment #25)
> (In reply to Fridolin Somers from comment #24)
> > Pushed to 23.05.x for 23.05.06
> 
> This one cause a problem - you will need bug 35295 as well!

Also backported