Bug 35012 - Framework item plugins fire twice on Acquisition item blocks
Summary: Framework item plugins fire twice on Acquisition item blocks
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jan Kissig
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-10-09 12:43 UTC by Jan Kissig
Modified: 2023-11-16 09:37 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.06,22.11.12


Attachments
Bug_35012 (1.80 KB, patch)
2023-10-10 12:35 UTC, Jan Kissig
Details | Diff | Splinter Review
Bug_35012 (1.63 KB, patch)
2023-10-10 12:39 UTC, Jan Kissig
Details | Diff | Splinter Review
Bug 35012: Prevent Click handler firing twice (1.89 KB, patch)
2023-11-09 14:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35012: DO_NOT_PUSH - Add action to barcode's Blur event (899 bytes, patch)
2023-11-09 14:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35012: Apply change to all events (2.28 KB, patch)
2023-11-09 14:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35012: Apply change to all events (2.29 KB, patch)
2023-11-10 08:12 UTC, Jan Kissig
Details | Diff | Splinter Review
Bug 35012: DO_NOT_PUSH - Add action to barcode's Blur event (859 bytes, patch)
2023-11-10 18:54 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 35012: Prevent Click handler firing twice (1.91 KB, patch)
2023-11-10 18:54 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 35012: Apply change to all events (2.35 KB, patch)
2023-11-10 18:54 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 Jan Kissig 2023-10-09 12:43:26 UTC
When adding items on received orders (or on created orders) the value builder buttons fire twice when clicked. This could lead to f.e. inventory numbers increased by 2 or other unwanted behaviors

to rebuild this behavior:

a) build a new basket + order line using "item create on placing an order"
b) in the item creation dialog please use the webbrowsers dev tools to analyze the [...] button next to subfield d (Date acquired) and inspect the events that get fired by clicking
c) notice the Clicktag_952_subfield_d_* event gets fired twice.


This also happens using:  "item create on receiving an order" or other cataloguing plugins.
Comment 1 Jan Kissig 2023-10-10 09:32:46 UTC
It looks like additem.js calls the BindPluginEvents function which is registering the event listeners. 

Then the ready function for each plugin is also called and registers the event listeners again.
Comment 2 Jan Kissig 2023-10-10 12:35:28 UTC Comment hidden (obsolete)
Comment 3 Jan Kissig 2023-10-10 12:39:24 UTC
Created attachment 156793 [details] [review]
Bug_35012

test plan:
a) build a new basket + order line using "item create on placing an order"
b) in the item creation dialog (neworderempty.pl) please use the webbrowsers dev tools to analyze the [...] button next to subfield d (Date acquired) and inspect the events that get fired by clicking
c) notice the Clicktag_952_subfield_d_* event gets fired twice.
d) apply patch and reload page
e) there should be only 1 click handler left (besides the jquery one), which points to the Clicktag_952_subfield_* function
Comment 4 Marcel de Rooy 2023-10-10 12:41:30 UTC
Interesting patch ! Not sure if we are on the right track yet but thats just intuition.. Will be looking.
Comment 5 Julian Maurice 2023-10-10 12:47:09 UTC
Maybe bug 30975 fix this ?
Comment 6 Marcel de Rooy 2023-11-09 14:14:34 UTC
Created attachment 158718 [details] [review]
Bug 35012: Prevent Click handler firing twice

test plan:
a) build a new basket + order line using "item create on placing an order"
b) in the item creation dialog (neworderempty.pl) please use the webbrowsers dev tools to analyze the [...] button next to subfield d (Date acquired) and inspect the events that get fired by clicking
c) notice the Clicktag_952_subfield_d_* event gets fired twice.
d) apply patch and reload page
e) there should be only 1 click handler left (besides the jquery one), which points to the Clicktag_952_subfield_* function

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
See also bug 35298. You may have seen that the Focus event of dateaccessioned
did not fire. But that is related to use of Flatpickr in this plugin.
Comment 7 Marcel de Rooy 2023-11-09 14:14:37 UTC
Created attachment 158719 [details] [review]
Bug 35012: DO_NOT_PUSH - Add action to barcode's Blur event

No test plan. See next patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2023-11-09 14:14:39 UTC
Created attachment 158720 [details] [review]
Bug 35012: Apply change to all events

We should not just look at Click.

Test plan:
[1] Apply the example patch for a Blur action in barcode.pl.
[2] Check that barcode.pl is active in your framework and that
    autoBarcode is not disabled.
[3] Open the browser dev console.
[4] Tab a few times through barcode in the neworderempty form of
    Acquisition. If you leave the field, the Blur event fires.
    => Without this patch you will see two console lines (FF may
    show a little '2' icon at the right side) added. With this
    patch just one.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2023-11-09 14:16:52 UTC
(In reply to Julian Maurice from comment #5)
> Maybe bug 30975 fix this ?

Yes, definitely. I recommend to use this as a temporary fix for master and backport it to stable branches.

Hopefully, we can push 30975 soon after 23.11 release. And further fix this code.
Comment 10 Jan Kissig 2023-11-10 08:12:23 UTC
Created attachment 158782 [details] [review]
Bug 35012: Apply change to all events

We should not just look at Click.

Test plan:
[1] Apply the example patch for a Blur action in barcode.pl.
[2] Check that barcode.pl is active in your framework and that
    autoBarcode is not disabled.
[3] Open the browser dev console.
[4] Tab a few times through barcode in the neworderempty form of
    Acquisition. If you leave the field, the Blur event fires.
    => Without this patch you will see two console lines (FF may
    show a little '2' icon at the right side) added. With this
    patch just one.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Jan Kissig <jkissig@th-wildau.de>
Comment 11 Katrin Fischer 2023-11-10 18:52:23 UTC
Next time, please start the test plan with getting into the acquisition module, I was quite getting quite frustrated with the normal item form :D
Comment 12 Katrin Fischer 2023-11-10 18:54:50 UTC
Created attachment 158816 [details] [review]
Bug 35012: DO_NOT_PUSH - Add action to barcode's Blur event

No test plan. See next patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Katrin Fischer 2023-11-10 18:54:52 UTC
Created attachment 158817 [details] [review]
Bug 35012: Prevent Click handler firing twice

test plan:
a) build a new basket + order line using "item create on placing an order"
b) in the item creation dialog (neworderempty.pl) please use the webbrowsers dev tools to analyze the [...] button next to subfield d (Date acquired) and inspect the events that get fired by clicking
c) notice the Clicktag_952_subfield_d_* event gets fired twice.
d) apply patch and reload page
e) there should be only 1 click handler left (besides the jquery one), which points to the Clicktag_952_subfield_* function

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
See also bug 35298. You may have seen that the Focus event of dateaccessioned
did not fire. But that is related to use of Flatpickr in this plugin.
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Katrin Fischer 2023-11-10 18:54:55 UTC
Created attachment 158818 [details] [review]
Bug 35012: Apply change to all events

We should not just look at Click.

Test plan:
[1] Apply the example patch for a Blur action in barcode.pl.
[2] Check that barcode.pl is active in your framework and that
    autoBarcode is not disabled.
[3] Open the browser dev console.
[4] Tab a few times through barcode in the neworderempty form of
    Acquisition. If you leave the field, the Blur event fires.
    => Without this patch you will see two console lines (FF may
    show a little '2' icon at the right side) added. With this
    patch just one.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Jan Kissig <jkissig@th-wildau.de>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Katrin Fischer 2023-11-10 18:55:45 UTC
(In reply to Katrin Fischer from comment #11)
> Next time, please start the test plan with getting into the acquisition
> module, I was quite getting quite frustrated with the normal item form :D

Ok, I realize there is 2 test plans, maybe I should start the weekend :)
Comment 16 Marcel de Rooy 2023-11-13 07:01:52 UTC
(In reply to Katrin Fischer from comment #11)
> Next time, please start the test plan with getting into the acquisition
> module, I was quite getting quite frustrated with the normal item form :D

Ah yes, that would have been helpful indeed :) Thanks.
Comment 17 Tomás Cohen Arazi 2023-11-14 13:33:03 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 18 Fridolin Somers 2023-11-14 19:15:27 UTC
Pushed to 23.05.x for 23.05.06
Comment 19 Matt Blenkinsop 2023-11-16 09:37:30 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x