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.
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
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)
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)
(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.
> 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) )
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
CJ, can you test with my alternate patch instead?
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.
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?
(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.
> 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.
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.
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
Created attachment 172509 [details] [review] Bug 38072: Ensure the printing tab closes
Comment on attachment 172483 [details] [review] Bug 38072: [alternate] Fix regression in modal print I tried to clarify the role of the 2nd patch.
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 ^^"
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.
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.
> 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.