Bug 38072 - Regression with modalPrint
Summary: Regression with modalPrint
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-10-03 12:41 UTC by Martin Renvoize (ashimema)
Modified: 2024-10-31 13:00 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 38072: Fix regression in modal_printer (1.72 KB, patch)
2024-10-03 14:44 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38072: [alternate] Fix regression in modal print (1.74 KB, patch)
2024-10-07 15:51 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 38072: Fix regression in modal_printer (1.78 KB, patch)
2024-10-08 06:08 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 38072: Ensure the printing tab closes (1.31 KB, patch)
2024-10-08 06:08 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize (ashimema) 2024-10-03 12:41:06 UTC
We have a few instanced of modalPrint in Koha and they appear to have all ceased to function.

returns (bundle modal) and cash registers (cashup summary) are both affected.
Comment 1 Martin Renvoize (ashimema) 2024-10-03 14:44:25 UTC
Created attachment 172360 [details] [review]
Bug 38072: Fix regression in modal_printer

This patch updates the JS for the modal printer to make it more
reliable.

I found that in recent versions of chrome the print dialog failed to
open and instead you were just faced with a flash of white before the
new tab autoclosed and no print takes place.

Test plan
1) Enable cash registers and point of sale preferences
2) Add a debit type and mark it as 'Can be sold'
3) Add a cash register
4) Use 'Point of sale' to sell one of the above item for cash against
   the register you just created.
5) Navigate to the register and 'cashup'
6) Click the 'summary' link to display a modal
7) User the 'Print' option in the modal and confirm you face a print
   dialog and can print
Comment 2 CJ Lynce 2024-10-04 00:22:12 UTC
Prior to applying the patch (on main), when clicking "Print" on the modal, a new tab is opened with the print data, along with the print dialog. Successfully printing or cancelling closes the print data tab, and returns you to the screen with modal. (Tested on Chrome, Edge, Firefox, and Brave)

After applying the patch, when click "Print" on the modal, a new tab is opened with the print data and print dialog (similar to before) but successfully printing, or cancelling, DOES NOT close the new tab in Chrome, Edge, or Brave (Chromium based.) It DOES close the new tab on Firefox.

Is this intended?

(Firefox v 131.0; Chrome 129.0.6668.90; Edge 129.0.2792.65; Brave 1.70.123 based on Chromium 129.0.6668.89)
Comment 3 Martin Renvoize (ashimema) 2024-10-07 14:35:08 UTC
That's interesting.. for me prior to the patch chrome would flash open the tab and immediately close it without triggering the print dialogue.

After the patch the correct behaviour I'm seeing is exactly what you're seeing prior to the patch.  i.e. opens tab, opens print dialog, on close or print closes tab.

I'm on Chrome 129.0.6668.89 (Official Build) (64-bit)
Comment 4 Lucas Gass (lukeg) 2024-10-07 15:13:25 UTC
(In reply to CJ Lynce from comment #2)
> Prior to applying the patch (on main), when clicking "Print" on the modal, a
> new tab is opened with the print data, along with the print dialog.
> Successfully printing or cancelling closes the print data tab, and returns
> you to the screen with modal. (Tested on Chrome, Edge, Firefox, and Brave)
> 
> After applying the patch, when click "Print" on the modal, a new tab is
> opened with the print data and print dialog (similar to before) but
> successfully printing, or cancelling, DOES NOT close the new tab in Chrome,
> Edge, or Brave (Chromium based.) It DOES close the new tab on Firefox.
> 
> Is this intended?
> 
> (Firefox v 131.0; Chrome 129.0.6668.90; Edge 129.0.2792.65; Brave 1.70.123
> based on Chromium 129.0.6668.89)

With the patch applied I experience the same thing as CJ when using Chrome ( Version 129.0.6668.58 (Official Build) (64-bit) )

In Firefox it is working as intended.
Comment 5 Lucas Gass (lukeg) 2024-10-07 15:18:04 UTC
> With the patch applied I experience the same thing as CJ when using Chrome (
> Version 129.0.6668.58 (Official Build) (64-bit) )
> 
> In Firefox it is working as intended.

Sorry, not enough morning coffee. I had this backwards:

-Works in Chrome ( Version 129.0.6668.58 (Official Build) (64-bit) )
-Does not close the tab in Firefox ( 131.0 (64-bit) )
Comment 6 Lucas Gass (lukeg) 2024-10-07 15:51:36 UTC
Created attachment 172483 [details] [review]
Bug 38072: [alternate] Fix regression in modal print

Test plan
1) Enable cash registers and point of sale preferences
2) Add a debit type and mark it as 'Can be sold'
3) Add a cash register
4) Use 'Point of sale' to sell one of the above item for cash against
   the register you just created.
5) Navigate to the register and 'cashup'
6) Click the 'summary' link to display a modal
7) User the 'Print' option in the modal and confirm you face a print
   dialog and can print
Comment 7 Lucas Gass (lukeg) 2024-10-07 15:52:22 UTC
CJ, can you test with my alternate patch instead?
Comment 8 CJ Lynce 2024-10-07 16:13:44 UTC
Lucas, everything works as expected with your alternate patch, in all browsers (print dialog appears, temp tab closes when finished).

FYI- I could only apply your alt patch on top of Martin's, not on stock main.
Comment 9 Victor Grousset/tuxayo 2024-10-07 20:23:57 UTC
I can't reproduce the issue on Chromium 129.0.6668.70 :(
Regardless of having my adblocker enabled or not.
The print tab as expected is closed when printing or canceling the browser print menu. (Same in Firefox)

Anyway, we could move on with CJ's testing.

But:

> I could only apply your alt patch on top of Martin's, not on stock main.

Indeed, the alternate patch diff shows that it's one top of the changes of the 1st patch.

Is the alternate patch a follow-up to complete the 1st one?
Comment 10 Lucas Gass (lukeg) 2024-10-07 20:37:35 UTC
(In reply to Victor Grousset/tuxayo from comment #9)
> I can't reproduce the issue on Chromium 129.0.6668.70 :(
> Regardless of having my adblocker enabled or not.
> The print tab as expected is closed when printing or canceling the browser
> print menu. (Same in Firefox)
> 
> Anyway, we could move on with CJ's testing.
> 
> But:
> 
> > I could only apply your alt patch on top of Martin's, not on stock main.
> 
> Indeed, the alternate patch diff shows that it's one top of the changes of
> the 1st patch.
> 
> Is the alternate patch a follow-up to complete the 1st one?

Yes, you'll need to apply both.
Comment 11 Victor Grousset/tuxayo 2024-10-08 00:02:08 UTC
> I can't reproduce the issue on Chromium 129.0.6668.70 :(
> [...]
> The print tab as expected is closed when printing
> or canceling the browser print menu. (Same in Firefox)

At least I see no change in behavior with the patches.
Comment 12 David Nind 2024-10-08 00:18:54 UTC
I can't replicate the issue - everything works the same for me both before and after the patches.

Tested on Ubuntu 22.04 - Firefox (Normal 131.0, Developer Edition 132.0b4), Chromium (129.0.6668.89), and Google Chrome (129.0.6668.89).

Testing notes:
1. Enable the cash registers and point of sale system preferences:
   - EnablePointOfSale: Enable
   - UseCashRegisters: Use
2. Add a debit type and mark it as 'Can be sold' (Administration > Accounting > Debit types> New debit type).
3. Add a cash register (Administration > Accounting > Cash registers > New cash register).
4. Use the point of sale module to sell one of the items created for cash against the register you just created (Point of sale > click 'Add' for the item you created > Collect payment > enter an amount for 'Amount tendered').
5. Navigate to the register and 'cashup' (Point of sale > cash management > Cash summary for Centerville > Record cashup action, then 'Confirm').
6. Click the 'summary' link to display a modal (in the 'Last cashup' column for the cash register).
7. For the Koha cashup summary dialog box, click 'Print'. A standard device/system print dialog box should appear.
Comment 13 Victor Grousset/tuxayo 2024-10-08 06:08:37 UTC
Created attachment 172508 [details] [review]
Bug 38072: Fix regression in modal_printer

This patch updates the JS for the modal printer to make it more
reliable.

I found that in recent versions of chrome the print dialog failed to
open and instead you were just faced with a flash of white before the
new tab autoclosed and no print takes place.

Test plan
1) Enable cash registers and point of sale preferences
2) Add a debit type and mark it as 'Can be sold'
3) Add a cash register
4) Use 'Point of sale' to sell one of the above item for cash against
   the register you just created.
5) Navigate to the register and 'cashup'
6) Click the 'summary' link to display a modal
7) Use the 'Print' option in the modal and confirm you face a print
   dialog and can print
8) The print tab should be closed and you should be back to Koha
Comment 14 Victor Grousset/tuxayo 2024-10-08 06:08:40 UTC
Created attachment 172509 [details] [review]
Bug 38072: Ensure the printing tab closes
Comment 15 Victor Grousset/tuxayo 2024-10-08 06:12:36 UTC
Comment on attachment 172483 [details] [review]
Bug 38072: [alternate] Fix regression in modal print

I tried to clarify the role of the 2nd patch.
Comment 16 Victor Grousset/tuxayo 2024-10-08 06:19:06 UTC
And I realized even CJ their testing didn't got the main issue of this ticket.

Any more details Martin of how you get the print dialog to fail?
That's now 4 people who didn't got the completely broken print dialog with the current test plan. There must be something else needed to throw things off ^^"
Comment 17 Martin Renvoize (ashimema) 2024-10-08 14:08:57 UTC
I have no idea how to replicate it.. for me it happens consistently with and without any extensions installed.

It's odd.. browser hell.. I just assumed it was happening for anyone else using the same Chrome version, but that doesn't even appear to be the case.
Comment 18 Victor Grousset/tuxayo 2024-10-09 21:26:22 UTC
If you know about someone else having the issue and not having it anymore when the patch is there, then we can go with that. (testing on a sandbox with and without the patch)

Since it seems safe to say the patch doesn't introduce a regression, we could do that for these hard to reproduce issues.
Comment 19 Victor Grousset/tuxayo 2024-10-24 04:00:20 UTC
> If you know about someone else having the issue and not having it anymore when the patch is there, then we can go with that.

So needing one report of having seen the fix work. Whether locally or in production without knowing what are the exact conditions there that we are missing here to reproduce. At least being certain that the fix can fix :)

An alternative is to treat this as a refactor and go with it now. And cross fingers that it will fix it for people having the bug.