Bug 31039 - Rebase issues lead to duplicate JS for cash summary modal printing
Summary: Rebase issues lead to duplicate JS for cash summary modal printing
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 31038
Blocks: 31121
  Show dependency treegraph
 
Reported: 2022-06-24 11:22 UTC by Martin Renvoize
Modified: 2023-06-08 22:26 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes a duplicate print dialogue box appearing when printing the cashup summary for cash registers - ins some circumstances when cancelling the print dialogue, it reappeared instead of closing.
Version(s) released in:
22.11.00, 22.05.05, 21.11.12


Attachments
Bug 31039: Remove duplicate modal printing JS (1.70 KB, patch)
2022-06-24 12:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31039: Remove duplicate modal printing JS (1.75 KB, patch)
2022-06-24 16:58 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 31039: (follow-up) Wrap jQuery in $(document).ready() (4.88 KB, patch)
2022-06-24 16:58 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 31039: (follow-up) Wrap jQuery in $(document).ready() (4.94 KB, patch)
2022-07-02 22:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 31039: Remove duplicate modal printing JS (1.84 KB, patch)
2022-07-08 07:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31039: (follow-up) Wrap jQuery in $(document).ready() (5.03 KB, patch)
2022-07-08 07:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31039: Remove duplicate modal printing JS (1.87 KB, patch)
2022-07-11 14:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31039: (follow-up) Wrap jQuery in $(document).ready() (5.17 KB, patch)
2022-07-11 14:05 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2022-06-24 11:22:38 UTC
We have duplicate JS for the cashup summary modals and this leads to double print dialogues.
Comment 1 Martin Renvoize 2022-06-24 12:13:05 UTC
Created attachment 136488 [details] [review]
Bug 31039: Remove duplicate modal printing JS

This patch removes the erroneos duplication of the modal printing js.

Test plan
1) Create some transactions and perform a cashup.
2) Open the cashup summary modal
3) Click 'Print'
4) Cancel the print dialogue
5) Note that the dialogue re-appears
6) Apply the patch
7) Repeat and note the dialogue closes first time now.
Comment 2 David Nind 2022-06-24 13:54:52 UTC
I attempted to test, but I am not seeing this problem:
1. Print
2. Opens print dialogue
3. Cancel
4. Goes back to cashup summary modal

Tested using Ubuntu 18.04 with Firefox and Google Chrome.
Comment 3 Martin Renvoize 2022-06-24 14:23:11 UTC
Weird.. it was consistent for Firefox and Chrome for me.. even got Owen to test it on IRC before I reported the bug and he agreed.

Hmm.. wonder what's going on...
Comment 4 David Nind 2022-06-24 14:42:21 UTC
If it's any consolation, I didn't notice anything different after applying the patch.

I use Firefox Developer Edition for testing (with the history/caching deleted each time it starts up).

Maybe it only shows up on other operating systems?
Comment 5 Owen Leonard 2022-06-24 16:58:48 UTC
Created attachment 136532 [details] [review]
Bug 31039: Remove duplicate modal printing JS

This patch removes the erroneos duplication of the modal printing js.

Test plan
1) Create some transactions and perform a cashup.
2) Open the cashup summary modal
3) Click 'Print'
4) Cancel the print dialogue
5) Note that the dialogue re-appears
6) Apply the patch
7) Repeat and note the dialogue closes first time now.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 6 Owen Leonard 2022-06-24 16:58:52 UTC
Created attachment 136533 [details] [review]
Bug 31039: (follow-up) Wrap jQuery in $(document).ready()

cashup_modal.js consists only of jQuery code, so the whole thing should
be contained in a $(document).ready() function. This may or may not be
contributing to the behavior this bug is trying to fix.

Please note that this patch contains whitespace changes, so diff
accordingly.
Comment 7 Katrin Fischer 2022-07-02 22:45:25 UTC
Created attachment 137046 [details] [review]
Bug 31039: (follow-up) Wrap jQuery in $(document).ready()

cashup_modal.js consists only of jQuery code, so the whole thing should
be contained in a $(document).ready() function. This may or may not be
contributing to the behavior this bug is trying to fix.

Please note that this patch contains whitespace changes, so diff
accordingly.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 8 Katrin Fischer 2022-07-02 22:46:55 UTC
I couldn't replicate the doubled up printing dialogues, but could also not find any regression and the changes look good.
Comment 9 Marcel de Rooy 2022-07-08 06:59:50 UTC
Argument "" isn't numeric in multiplication (*) at /usr/share/koha/koha-tmpl/intranet-tmpl/prog/en/modules/pos/register.tt line 86.
Argument "" isn't numeric in multiplication (*) at /usr/share/koha/koha-tmpl/intranet-tmpl/prog/en/modules/pos/register.tt line 86.
Argument "" isn't numeric in multiplication (*) at /usr/share/koha/koha-tmpl/intranet-tmpl/prog/en/modules/pos/register.tt line 87.
Argument "" isn't numeric in multiplication (*) at /usr/share/koha/koha-tmpl/intranet-tmpl/prog/en/modules/pos/register.tt line 87.
Argument "" isn't numeric in multiplication (*) at /usr/share/koha/koha-tmpl/intranet-tmpl/prog/en/modules/pos/register.tt line 88.
Argument "" isn't numeric in multiplication (*) at /usr/share/koha/koha-tmpl/intranet-tmpl/prog/en/modules/pos/register.tt line 158.
Argument "" isn't numeric in multiplication (*) at /usr/share/koha/koha-tmpl/intranet-tmpl/prog/en/modules/pos/register.tt line 288.
Comment 10 Marcel de Rooy 2022-07-08 07:04:14 UTC
Created attachment 137367 [details] [review]
Bug 31039: Remove duplicate modal printing JS

This patch removes the erroneos duplication of the modal printing js.

Test plan
1) Create some transactions and perform a cashup.
2) Open the cashup summary modal
3) Click 'Print'
4) Cancel the print dialogue
5) Note that the dialogue re-appears
6) Apply the patch
7) Repeat and note the dialogue closes first time now.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2022-07-08 07:04:18 UTC
Created attachment 137368 [details] [review]
Bug 31039: (follow-up) Wrap jQuery in $(document).ready()

cashup_modal.js consists only of jQuery code, so the whole thing should
be contained in a $(document).ready() function. This may or may not be
contributing to the behavior this bug is trying to fix.

Please note that this patch contains whitespace changes, so diff
accordingly.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Tomás Cohen Arazi 2022-07-08 18:52:05 UTC
Sorry, please rebase
Comment 13 Owen Leonard 2022-07-08 18:58:50 UTC
(In reply to Marcel de Rooy from comment #9)
> Argument "" isn't numeric in multiplication (*) at
> /usr/share/koha/koha-tmpl/intranet-tmpl/prog/en/modules/pos/register.tt line
> 86.
...

What is the purpose of this comment?
Comment 14 Martin Renvoize 2022-07-08 23:27:51 UTC
I'll rebase on Monday, and take a look at the highlighted warnings at the same time.
Comment 15 Katrin Fischer 2022-07-10 21:10:23 UTC
(In reply to Martin Renvoize from comment #14)
> I'll rebase on Monday, and take a look at the highlighted warnings at the
> same time.

I filed the warnings already when looking at one of the other cash register patches: 
Bug 31125 - Fix warnings from register.tt
Comment 16 Martin Renvoize 2022-07-11 14:05:24 UTC
Created attachment 137514 [details] [review]
Bug 31039: Remove duplicate modal printing JS

This patch removes the erroneos duplication of the modal printing js.

Test plan
1) Create some transactions and perform a cashup.
2) Open the cashup summary modal
3) Click 'Print'
4) Cancel the print dialogue
5) Note that the dialogue re-appears
6) Apply the patch
7) Repeat and note the dialogue closes first time now.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Martin Renvoize 2022-07-11 14:05:37 UTC
Created attachment 137515 [details] [review]
Bug 31039: (follow-up) Wrap jQuery in $(document).ready()

cashup_modal.js consists only of jQuery code, so the whole thing should
be contained in a $(document).ready() function. This may or may not be
contributing to the behavior this bug is trying to fix.

Please note that this patch contains whitespace changes, so diff
accordingly.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2022-07-11 14:05:58 UTC
Rebased
Comment 19 Tomás Cohen Arazi 2022-07-11 21:23:55 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 20 Lucas Gass 2022-08-23 15:37:25 UTC
Backported to 22.05.x for 22.05.05
Comment 21 Arthur Suzuki 2022-08-30 09:15:45 UTC
thx pushed to 21.11.x for 21.11.12
Comment 22 Victor Grousset/tuxayo 2022-09-09 19:58:46 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.

Nothing to document, marking resolved.