Bug 30975

Summary: Use event delegation for framework plugins to avoid using private jQuery method _data
Product: Koha Reporter: Julian Maurice <julian.maurice>
Component: CatalogingAssignee: Julian Maurice <julian.maurice>
Status: Failed QA --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: bibliothek, caroline.cyr-la-rose, david, dcook, domm, jonathan.druart, kyle, lmstrand, m.de.rooy, martin.renvoize, matt.blenkinsop, paliotti, patrick.robitaille, verolencinas, victor, ztajoli
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=3544
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33744
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20397
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This introduces a breaking change for users that have custom framework plugins: the first parameter of javascript functions is now always an Event object. Before that it was possible to receive a string containing an HTML ID. This ID is available in `event.data.id` (assuming the first parameter is named `event`)
Version(s) released in:
Bug Depends on: 32853, 32856, 29155, 32812, 32813, 32814, 32815, 32816, 32817, 32818, 32819, 32820, 32821, 32822, 32823, 32824, 32825, 32826, 32827, 32828, 32829, 32830, 32831, 32832, 32833, 32834, 32835, 32836, 32837, 32838, 32839, 32840, 32841, 32842, 32843, 32844, 32845, 32846, 32847, 32848, 32849, 32850, 32851, 32852, 32854, 32855, 32857, 32858, 32859, 32860, 32861, 32862, 32863, 32864, 32865, 32866, 32867, 32868, 32869, 32870, 32871, 32872, 32873, 32874, 32875, 32876    
Bug Blocks: 32877, 32569    
Attachments: Bug 30975: Fix framework plugins not working on cloned fields/subfields
Bug 30975: Fix framework plugins not working on cloned fields/subfields
[22.05] Bug 30975: Fix framework plugins on cloned fields/subfields
Bug 30975: Fix framework plugins on cloned fields/subfields
Bug 30975: Fix framework plugins not working on cloned fields/subfields
Bug 30975: Call preventDefault only for click events
Bug 30975: Fix framework plugins not working on cloned fields/subfields

Description Julian Maurice 2022-06-16 09:15:11 UTC
Since bug 29155 (Upgrade jquery version to 3.6.0) framework plugins do not work on a cloned field/subfield.
That is because we rely on jQuery's `.data('events')` to clone event handlers to the cloned elements, and that was removed in jQuery 1.8.0 (I did not confirm but it's possible it continued to work after that thanks to jQuery Migrate)
Comment 1 Julian Maurice 2022-06-16 09:16:43 UTC
Created attachment 136135 [details] [review]
Bug 30975: Fix framework plugins not working on cloned fields/subfields

Since bug 29155 (Upgrade jquery version to 3.6.0) framework plugins do
not work on a cloned field/subfield.
That is because we rely on jQuery's `.data('events')` to clone event
handlers to the cloned elements, and that was removed in jQuery 1.8.0
(I did not confirm but it's possible it continued to work after that
thanks to jQuery Migrate)

It is apparently still possible to access these event handlers by using
`jQuery._data(element, "events")` but here's what jQuery 1.8.0 release
notes says about it:
"this is not a supported public interface; the actual data structures
may change incompatibly from version to version."
So we should not rely on it.

Instead, what this patch does is use event delegation [1].
Events are bound to a parent container, so when elements are added
dynamically inside that container, we don't need to re-attach event
handlers manually

This patch also comes with a bit of cleanup, and introduce "breaking
changes" (they are breaking changes only if you happen to have custom
framework plugins):
1) Only 'click', 'focus' and 'blur' events are listened to now. They are
   the only ones we use, and the only ones we actually need.
   Events removed: change, mouseover, mouseout, mousedown, mouseup,
   mousemove, keydown, keypress, keyup.
   If you ever need one of those, you can still add event listeners in
   the 'focus' event handler, and remove them in the 'blur' event
   handler
2) Event handlers now takes a single parameter that is an Event object
   It just makes the code a lot less complicated. All framework plugins
   have been updated
3) Event handlers do not pollute the global scope anymore

[1] https://learn.jquery.com/events/event-delegation/

Test plan:
- Go to every page that has a MARC editor and verify that plugins still
  work. This includes addbiblio.pl, additem.pl, authorities.pl,
  neworderempty.pl, orderreceive.pl
- Test plugins that use 'focus' event (for instance barcode.pl), 'blur'
  event (callnumber.pl) and 'click' event (almost all the others)
- Test that plugins work on cloned fields/subfields
Comment 2 Caroline Cyr La Rose 2022-06-30 12:38:16 UTC
Hi Julian,

I would appreciate a more detailed test plan. barcode.pl is linked to an item field for me, so I can't clone it. All the other plugins for specific fields (e.g. 000, 005, 008) are on non-repeatable fields too. 

Do we have to modify the framework in order to test this?

Thanks,

Caroline
Comment 3 Julian Maurice 2022-07-11 07:00:46 UTC
> Do we have to modify the framework in order to test this?

Yes, but I'd say it's no big deal if you cannot do these tests on a plugin if that plugin is always used in a non-repeatable field/subfield.
Testing that the plugin still works in its single field should be enough.
Comment 4 Julian Maurice 2022-07-29 13:50:13 UTC
Created attachment 138318 [details] [review]
Bug 30975: Fix framework plugins not working on cloned fields/subfields

Since bug 29155 (Upgrade jquery version to 3.6.0) framework plugins do
not work on a cloned field/subfield.
That is because we rely on jQuery's `.data('events')` to clone event
handlers to the cloned elements, and that was removed in jQuery 1.8.0
(I did not confirm but it's possible it continued to work after that
thanks to jQuery Migrate)

It is apparently still possible to access these event handlers by using
`jQuery._data(element, "events")` but here's what jQuery 1.8.0 release
notes says about it:
"this is not a supported public interface; the actual data structures
may change incompatibly from version to version."
So we should not rely on it.

Instead, what this patch does is use event delegation [1].
Events are bound to a parent container, so when elements are added
dynamically inside that container, we don't need to re-attach event
handlers manually

This patch also comes with a bit of cleanup, and introduce "breaking
changes" (they are breaking changes only if you happen to have custom
framework plugins):
1) Only 'click', 'focus' and 'blur' events are listened to now. They are
   the only ones we use, and the only ones we actually need.
   Events removed: change, mouseover, mouseout, mousedown, mouseup,
   mousemove, keydown, keypress, keyup.
   If you ever need one of those, you can still add event listeners in
   the 'focus' event handler, and remove them in the 'blur' event
   handler
2) Event handlers now takes a single parameter that is an Event object
   It just makes the code a lot less complicated. All framework plugins
   have been updated
3) Event handlers do not pollute the global scope anymore

[1] https://learn.jquery.com/events/event-delegation/

Test plan:
- Go to every page that has a MARC editor and verify that plugins still
  work. This includes addbiblio.pl, additem.pl, authorities.pl,
  neworderempty.pl, orderreceive.pl
- Test plugins that use 'focus' event (for instance barcode.pl), 'blur'
  event (callnumber.pl) and 'click' event (almost all the others)
- Test that plugins work on cloned fields/subfields
Comment 5 David Nind 2022-09-16 23:16:46 UTC
Testing notes (using KTD and a MARC21 instance).

Replicating the issue for cloned fields/subfields not working *
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

1. For the BKS framework, edit the subfields for 260
2. For 260$b add the marc21_field_260b.pl plugin
3. Edit an existing record's 260$b:
   . Remove what is in the field
   . Start typing the name of a publisher that exists (requires first three letters), for example: O'R (for O'Reilly) or Apr (for Apress)
   . Select the full name
4. Clone the subfield
5. If you repeat step 3, note that the autocomplete doesn't work
6. Repeat/duplicate the 260 tag and follow steps 3-5

After the patch is applied, flush_memcached, restart_all, clear browser cache, etc., step 5 should now work.

I found this Monday Minutes from ByWater Solutions very helpful for coming up with an example of a subfield that could be cloned to replicate the issue - Monday Minutes: Publisher Drop-down in Cataloging (26 July 2021):
https://bywatersolutions.com/education/monday-minutes-publisher-drop-down-in-cataloging

Testing pages that have the MARC editor
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Run the tests as indicated (labeled with letters in the next section) to check the various pages where the MARC editor is used.

Except as noted, these should work as expected before and after the patch is applied.

Add a new catalog record (addbiblio.pl):
1. Cataloging > New record (select Nooks, Booklets, Workbooks from the drop-down list)
2. Run test D
3. Replicate the issue for cloned fields/subfields

Notes:
- Cloned subfields don't work before the patch is applied

Add a new item to a record (additem.pl)
1. Add a new item to an existing record
2. Run tests A, B, and C
3. These should work before and after the patch is applied

Add a new authority record (authorities.pl)
1. Run test E

Notes:
- I'm not familiar enough with authority records to suggest a subfield and plugin that would replicate the cloning issue

Acquisitions - create items when placing an order (neworderempty.pl)
1. Acquisitions > click search for Vendor in the manage orders section, and select 'My Vendor'.
2. Create a new basket with 'placing an order' selected for the 'Create items when' field
3. Select Add to basket and choose 'From a new (empty) record'
4. Run tests A (barcode) and C (date fields)

Acquisitions - receiving an order (orderreceive.pl)
1. Acquisitions > click search for Vendor in the manage orders section, and select 'My Vendor'.
2. Create a new basket with 'receiving an order' selected for the 'Create items when' field
3. Close the basket
4. Select Receive shipments for the vendor
5. Enter anything you like for the invoice number and select 'Next'
6. Select 'Receive' for the basket you created
7. Run tests A (barcode) and C (date fields)

Tests for plugins that use different types of events
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

--A.-- 'focus' event plugin - barcode.pl:
1. Set the system preference autoBarcode to 'generated in the form <year>-0001, <year>-0002.
2. Add a new item for any record.
3. Click on the p - Barcode field
4. Result: it should be populated with the autoBarcode pattern

--B.-- 'blur' event plugin - callnumber.pl
1. For the itemcallnumber system preference add 082ab,092ab
2. For the BKS framework, add the callnumber.pl plugin to 952$o
3. Edit any record and add some values to 082$a, 082$b, and 082$c if it doesn't already entries for these subfields
4. Add a new item to the record
5. Result: the 'o - Full call number' field should be populated with the values from 082

--C.-- 'click' events - dateaccessioned.pl:
1. Add a new item to a record
2. Click on any of the date fields, such as d - date acquired or w - Price effective from
3. Result: date picker pop-up should appear, selecting a date should populate the field

--D.-- 'click' events - new cataloguing record:
1. Add a new catalogue record for the BKS framework
2. Click in the fields for 000, 005, and 008
3. Result: these should be populated with values

--E.-- 'click' events - new authority record:
1. Add a new Personal Name authority record
2. Click in the fields for 000, 003, 005, 008, and 0040
3. Result: these should be populated with values
Comment 6 David Nind 2022-09-16 23:21:21 UTC
The patch still applies, and most things seem to work as expected (see my testing notes in comment #5).

The only thing that didn't work for me was creating new authority records after the patch is applied.

Fields 000, 003, 005, 008, and 0040 are not automatically populated when clicking on the fields.

Also, I'm not familiar enough with authority records to suggest a subfield and plugin that would replicate the cloning issue.
Comment 7 Katrin Fischer 2022-09-19 20:23:01 UTC
I am worried by the scope of this one. The changes to every plugin disqualifies this patch for backporting, especially because of the breaking changes:

> This patch also comes with a bit of cleanup, and introduce "breaking
> changes" (they are breaking changes only if you happen to have custom
> framework plugins):
> 1) Only 'click', 'focus' and 'blur' events are listened to now. They are
>    the only ones we use, and the only ones we actually need.
>    Events removed: change, mouseover, mouseout, mousedown, mouseup,
>    mousemove, keydown, keypress, keyup.
>    If you ever need one of those, you can still add event listeners in
>    the 'focus' event handler, and remove them in the 'blur' event
>    handler
> 2) Event handlers now takes a single parameter that is an Event object
>    It just makes the code a lot less complicated. All framework plugins
>    have been updated
> 3) Event handlers do not pollute the global scope anymore

We do have those custom framework plugins and I know other libraries do as well.

Also not sure about removing events - some libraires may have used them and is there a strong reason to remove?

Could we imagine a less invasive fix to the problem, that could also be applied to stable releases?
Comment 8 Julian Maurice 2022-09-20 07:33:37 UTC
(In reply to Katrin Fischer from comment #7)
> Also not sure about removing events - some libraires may have used them and
> is there a strong reason to remove?

I removed them for performance reasons. With the method used (event delegation), some events like mouseover or mousemove would get triggered all the time. And while I was at it, I removed all unused ones. Maybe I've gone too far ?
But should we support all framework plugins potentially existing in the wild ?

> Could we imagine a less invasive fix to the problem, that could also be
> applied to stable releases?

For stable releases we can have a different fix that use the not recommended method `jQuery._data(element, "events")`. But for master I think we should not use this method.
Comment 9 Katrin Fischer 2022-09-20 07:36:59 UTC
I think this has been kind of one of the first 'plugin' infrastructures Koha had, so I think we should keep people in mind that have written their own. Breaking changes are not always avoidable, but we should provide some documentation/warning in release notes.

I'd love Marcel to have a look here, as he has worked on improving plugins in the past.

Julian - could you maybe provide the patch for the older versions soon? I'd really love to see this fixed for libraries using the plugin for 7xx linking etc.
Comment 10 Julian Maurice 2022-09-20 09:35:03 UTC
Created attachment 140763 [details] [review]
[22.05] Bug 30975: Fix framework plugins on cloned fields/subfields

This patch uses the internal method jQuery._data to retrieve and
clone all events attached to a cloned field/subfield.
It's intended for stable releases only, so they can stay compatible with
existing plugins
Comment 11 Julian Maurice 2022-09-20 09:43:47 UTC
Here's the patch for 22.05 (which is the only release affected by this I believe).
Much simpler, but please note that it does not include all fixes made to individual plugins in the main patch. Some plugins will "work" but will update the wrong field (ex: unimarc_field_115a.pl)
Comment 12 Marcel de Rooy 2022-09-20 11:11:32 UTC
(In reply to Katrin Fischer from comment #9)
> 
> I'd love Marcel to have a look here, as he has worked on improving plugins
> in the past.

I had a short glance already, but the 92K scared me away ;)
Comment 13 David Nind 2022-09-20 22:07:13 UTC
Created attachment 140810 [details] [review]
Bug 30975: Fix framework plugins on cloned fields/subfields

This patch uses the internal method jQuery._data to retrieve and
clone all events attached to a cloned field/subfield.
It's intended for stable releases only, so they can stay compatible with
existing plugins

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 David Nind 2022-09-20 22:11:26 UTC
I've signed off the 22.05 patch for a MARC21 installation. Have left status as Failed QA - see comment #5 (for master, there is an issue with authority records).

This is how I tested - hope it is sufficient:
1. For the BKS framework, edit the subfields for 260.
2. For 260$b add the marc21_field_260b.pl plugin.
3. Edit an existing record's 260$b:
   . Remove what is in the field
   . Start typing the name of a publisher that exists (requires first three letters), for example: O'R (for O'Reilly) or Apr (for Apress)
   . Select the full name
4. Clone the subfield.
5. Start typing the name of a publisher that exists (requires first three letters), for example: O'R (for O'Reilly) or Apr (for Apress) and note that the autocomplete doesn't work for the cloned subfield.
6. Apply the 22.05 patch, flush_memcached, restart_all, clear browser cache, etc.
7. Repeat steps 4-5 - autocomplete should now work for the cloned subfield.
Comment 15 David Nind 2022-09-20 22:13:09 UTC
Also, I seemed to have mucked up the 22.05 attachments, and I'm not sure how to fix...
Comment 16 Julian Maurice 2023-02-03 14:28:01 UTC
Created attachment 146146 [details] [review]
Bug 30975: Fix framework plugins not working on cloned fields/subfields

Since bug 29155 (Upgrade jquery version to 3.6.0) framework plugins do
not work on a cloned field/subfield.
That is because we rely on jQuery's `.data('events')` to clone event
handlers to the cloned elements, and that was removed in jQuery 1.8.0
(I did not confirm but it's possible it continued to work after that
thanks to jQuery Migrate)

It is apparently still possible to access these event handlers by using
`jQuery._data(element, "events")` but here's what jQuery 1.8.0 release
notes says about it:
"this is not a supported public interface; the actual data structures
may change incompatibly from version to version."
So we should not rely on it.

Instead, what this patch does is use event delegation [1].
Events are bound to a parent container, so when elements are added
dynamically inside that container, we don't need to re-attach event
handlers manually

This patch also comes with a bit of cleanup, and introduce "breaking
changes" (they are breaking changes only if you happen to have custom
framework plugins):
1) 'mouseover', 'mousemove', 'keypress' events are no longer listened to
   'mouseover' and 'mousemove' are not used and would trigger too much
   events.
   'keypress' is also not used, and is deprecated
2) Event handlers now takes a single parameter that is an Event object
   It just makes the code a lot less complicated.
3) Event handlers do not pollute the global scope anymore

[1] https://learn.jquery.com/events/event-delegation/

Test plan:
- Go to every page that has a MARC editor and verify that plugins still
  work. This includes addbiblio.pl, additem.pl, authorities.pl,
  neworderempty.pl, orderreceive.pl
- Test plugins that use 'focus' event (for instance barcode.pl), 'blur'
  event (callnumber.pl) and 'click' event (almost all the others)
- Test that plugins work on cloned fields/subfields
Comment 17 Julian Maurice 2023-02-03 14:32:15 UTC
I split the patch into smaller ones (one patch per plugin, see "Depends on").
That should be easier to test and push.
Leaving this one as BLOCKED until all other patches are pushed
Comment 18 Julian Maurice 2023-02-03 14:34:28 UTC
I also reintroduced some events ('change', 'mousedown', 'mouseup', 'keydown', 'keyup') just in case someone uses a custom plugin that use one of those events.
Comment 19 Katrin Fischer 2023-02-04 18:02:47 UTC
(In reply to Julian Maurice from comment #18)
> I also reintroduced some events ('change', 'mousedown', 'mouseup',
> 'keydown', 'keyup') just in case someone uses a custom plugin that use one
> of those events.

Thx Julian!
Comment 20 David Nind 2023-02-07 19:58:10 UTC
Does cataloguing/value_builder/EXAMPLE.pl need updating to reflect the changes made in the refactoring?
Comment 21 Julian Maurice 2023-02-17 10:40:23 UTC
Created attachment 146817 [details] [review]
Bug 30975: Call preventDefault only for click events
Comment 22 David Nind 2023-02-19 21:44:41 UTC
I think I may have made a mistake in testing the bugs I have signed off so far.

I had not been testing cloning subfields, until I came to bug 32830 - unimarc_field_116.pl. (Up until then I hadn't been making subfields repeatable).

Steps to reproduce:
1. Edit a record.
2. Add a value for subfield 116$a.
3. Clone the subfield.
4. Click the tag editor:
   ==> Result: cursor jumps to the top of the page, the tag editor to select the options for the subfield is not displayed.

I'll hold off testing any more....
Comment 23 Julian Maurice 2023-02-20 07:46:47 UTC
(In reply to David Nind from comment #22)
> I think I may have made a mistake in testing the bugs I have signed off so
> far.
> 
> I had not been testing cloning subfields, until I came to bug 32830 -
> unimarc_field_116.pl. (Up until then I hadn't been making subfields
> repeatable).

I think you are doing fine (and thank you for testing by the way). Plugins on cloned subfields are not supposed to be fixed until all patches, including bug 30975's patches, are pushed.
In all "Depends on" bugs, framework plugins should work the same before and after the patch. So as long as the bug on cloned subfield also exists before applying the patch (which should be the case), it's OK
Comment 24 Victor Grousset/tuxayo 2023-03-05 03:56:01 UTC
I tried to test a patch and forgot to check if it had pending dependencies and ended up applying over 9000 dependent bugs and ended up with this one not applying.

> Bug 30975: Fix framework plugins not working on cloned fields/subfields

Anyway it's not ready until the dependencies are dealt with. Since it's status is BLOCKED, I'm not changing it and letting the info here for futur. (or in case you want gradual rebases instead of a big one)
Comment 25 David Nind 2023-04-17 06:55:57 UTC
There are 8 UNIMARC cataloguing plugins/value builders that I haven't been able to figure out what they do, and how to set them up and use them:

- unimarc_field_124.pl (bug 32850)
- unimarc_field_125.pl (bug 32853)
- unimarc_field_126.pl (bug 32856)
- unimarc_field_210c_bis.pl (bug 32868)
- unimarc_field_210c.pl (bug 32869)
- unimarc_field_225a_bis.pl (bug 32870)
- unimarc_field_225a.pl (bug 32871)
- unimarc_field_686a.pl (bug 32873)

I've posted a message to the main Koha mailing list:
https://lists.katipo.co.nz/pipermail/koha/2023-April/059297.html
Comment 26 Katrin Fischer 2023-04-23 12:31:44 UTC
I managed to test and PQA on all but:

> - unimarc_field_125.pl (bug 32853)
> - unimarc_field_126.pl (bug 32856)

These also work in theory, but don't appear to make sense. Some insight would be good.

I am missing some plugins in the list - especially almos all marc21* ones. Do we have a way to check which plugins still need updating?
Comment 27 David Nind 2023-04-24 00:13:59 UTC
Just for information, here is a list of all the cataloging plugins/value builders in the folder and the related bug number:

barcode_manual.pl - bug 32812
barcode.pl - bug 32813
callnumber-KU.pl - bug 32814
callnumber.pl - bug 32815
cn_browser.pl - bug 32816
dateaccessioned.pl - bug 32817
EXAMPLE.pl
marc21_field_005.pl - bug 32818
marc21_field_006.pl
marc21_field_007.pl
marc21_field_008_authorities.pl
marc21_field_008_classifications.pl
marc21_field_008.pl
marc21_field_245h.pl
marc21_field_260b.pl
marc21_leader_authorities.pl
marc21_leader.pl
marc21_linking_section.pl
marc21_orgcode.pl
stocknumberam123.pl - bug 32819
stocknumberAV.pl - bug 32820
stocknumber.pl - bug 32821
unimarc_field_009_ppn.pl
unimarc_field_010.pl - bug 32822
unimarc_field_100_authorities.pl - bug 32823
unimarc_field_100.pl - bug 32824
unimarc_field_105.pl - bug 32825
unimarc_field_106.pl - bug 32826
unimarc_field_110.pl - bug 32827
unimarc_field_115a.pl - bug 32828
unimarc_field_115b.pl - bug 32829
unimarc_field_116.pl - bug 32830
unimarc_field_117.pl - bug 32831
unimarc_field_120.pl - bug 32832
unimarc_field_121a.pl - bug 32833
unimarc_field_121b.pl - bug 32834
unimarc_field_122.pl - bug 32835
unimarc_field_123a.pl - bug 32836
unimarc_field_123d.pl - bug 32837
unimarc_field_123e.pl - bug 32838
unimarc_field_123f.pl - bug 32839
unimarc_field_123g.pl - bug 32840
unimarc_field_123i.pl - bug 32841
unimarc_field_123j.pl - bug 32842
unimarc_field_124a.pl - bug 32843
unimarc_field_124b.pl - bug 32844
unimarc_field_124c.pl - bug 32845
unimarc_field_124d.pl - bug 32846
unimarc_field_124e.pl - bug 32847
unimarc_field_124f.pl - bug 32848
unimarc_field_124g.pl - bug 32849
unimarc_field_124.pl - bug 32850
unimarc_field_125a.pl - bug 32851
unimarc_field_125b.pl - bug 32852
unimarc_field_125.pl - bug 32853
unimarc_field_126a.pl - bug 32854
unimarc_field_126b.pl - bug 32855
unimarc_field_126.pl - bug 32856
unimarc_field_127.pl - bug 32857
unimarc_field_128a.pl - bug 32858
unimarc_field_128b.pl - bug 32859
unimarc_field_128c.pl - bug 32860
unimarc_field_130.pl - bug 32861
unimarc_field_135a.pl - bug 32862
unimarc_field_140.pl - bug 32863
unimarc_field_141.pl - bug 32864
unimarc_field_146a.pl - bug 32865
unimarc_field_146b.pl
unimarc_field_146c.pl
unimarc_field_146d.pl
unimarc_field_146e.pl
unimarc_field_146f.pl
unimarc_field_146h.pl - bug 32866
unimarc_field_146i.pl - bug 32867
unimarc_field_181-2.pl
unimarc_field_181a.pl
unimarc_field_181b.pl
unimarc_field_181c.pl
unimarc_field_182-2.pl
unimarc_field_182a.pl
unimarc_field_182c.pl
unimarc_field_183-2.pl
unimarc_field_183a.pl
unimarc_field_210c_bis.pl - bug 32868
unimarc_field_210c.pl - bug 32869
unimarc_field_225a_bis.pl - bug 32870
unimarc_field_225a.pl - bug 32871
unimarc_field_283-2.pl
unimarc_field_283a.pl
unimarc_field_325h.pl
unimarc_field_325j.pl
unimarc_field_4XX.pl - bug 32872
unimarc_field_686a.pl - bug 32873
unimarc_field_700-4.pl - bug 32874
unimarc_leader_authorities.pl - bug 32875
unimarc_leader.pl - bug 32876
upload.pl - bug 32877
Comment 28 Lari Strand 2023-05-03 07:46:11 UTC
We use our custom value builder plugins for adding various keywords from an online vocabulary using dynamically created select2 elements. These patches fixed the issue with plugins not initialising on cloned fields. I needed to change the code so the plugin gets initialised with a click-function, formerly mouseover in the input field.
 
"Events removed: change, mouseover, mouseout, mousedown, mouseup,
   mousemove, keydown, keypress, keyup.
If you ever need one of those, you can still add event listeners in
   the 'focus' event handler, and remove them in the 'blur' event
   handler"

How would I bring mouseover back as a valid function in code? What kind of performance issues do you mean by "'mouseover' and 'mousemove' are not used and would trigger too much events."?

I also received a report that the patch breaks field/subfield cloning on Firefox 78.12.0esr. Apparently the field does not get cloned and the page goes to the top. They've tried to clear the browser cache. Does this make any sense?
Comment 29 Lari Strand 2023-05-03 08:00:03 UTC
(In reply to Lari Strand from comment #28)
> We use our custom value builder plugins for adding various keywords from an
> online vocabulary using dynamically created select2 elements. These patches
> fixed the issue with plugins not initialising on cloned fields. I needed to
> change the code so the plugin gets initialised with a click-function,
> formerly mouseover in the input field.
>  
> "Events removed: change, mouseover, mouseout, mousedown, mouseup,
>    mousemove, keydown, keypress, keyup.
> If you ever need one of those, you can still add event listeners in
>    the 'focus' event handler, and remove them in the 'blur' event
>    handler"
> 
> How would I bring mouseover back as a valid function in code? What kind of
> performance issues do you mean by "'mouseover' and 'mousemove' are not used
> and would trigger too much events."?
> 
> I also received a report that the patch breaks field/subfield cloning on
> Firefox 78.12.0esr. Apparently the field does not get cloned and the page
> goes to the top. They've tried to clear the browser cache. Does this make
> any sense?

Also reportedly for fixed fields (000, 006, 007 and 008) Koha's own editors stopped working with this older ESR version of Firefox. Newer browser versions and for example switching to Edge everything was ok.
Comment 30 Julian Maurice 2023-05-03 08:20:47 UTC
> How would I bring mouseover back as a valid function in code?
Can you post a link to your plugin ?

> What kind of performance issues do you mean by "'mouseover' and 'mousemove'
> are not used and would trigger too much events."?
I admit I did not measure it, but with the new method used here (event delegation), the browser would have to trigger tens of mouse events every time the mouse move inside the editor, which I assumed would introduce perfomance issues.
Comment 31 Lari Strand 2023-05-04 06:15:55 UTC
(In reply to Julian Maurice from comment #30)
> > How would I bring mouseover back as a valid function in code?
> Can you post a link to your plugin ?
> 
Here:
https://github.com/KohaSuomi/Koha-22x/commit/ee4464e7f428d14d9110b417878c43ddcbe73385
Should work out of the box. This one's been attached to the 650a field (modifies it's accompanying fields/tags). You can also mail me directly if you can help out with building the mouseover version. TY!
Comment 32 Lari Strand 2023-05-04 09:44:55 UTC
These patches seems to break the functionality in barcode generator plugin barcode.pl. I removed the patches and it started generating barcodes again.
Comment 33 Lari Strand 2023-05-04 12:01:31 UTC
(In reply to Lari Strand from comment #32)
> These patches seems to break the functionality in barcode generator plugin
> barcode.pl. I removed the patches and it started generating barcodes again.

This problem has been addressed in https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32813
Comment 34 Julian Maurice 2023-05-04 12:24:39 UTC
(In reply to Lari Strand from comment #33)
> (In reply to Lari Strand from comment #32)
> > These patches seems to break the functionality in barcode generator plugin
> > barcode.pl. I removed the patches and it started generating barcodes again.
> 
> This problem has been addressed in
> https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32813

Do not try to test this patch without all the other patches applied. They are marked as dependencies for a good reason ;) (and the status is "BLOCKED" for this reason as well)
Comment 35 Lari Strand 2023-05-05 07:05:30 UTC
(In reply to Julian Maurice from comment #34)
> (In reply to Lari Strand from comment #33)
> > (In reply to Lari Strand from comment #32)
> > > These patches seems to break the functionality in barcode generator plugin
> > > barcode.pl. I removed the patches and it started generating barcodes again.
> > 
> > This problem has been addressed in
> > https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32813
> 
> Do not try to test this patch without all the other patches applied. They
> are marked as dependencies for a good reason ;) (and the status is "BLOCKED"
> for this reason as well)

Good point :). Just noticed some patches marked dependencies are mostly code refactorings and do not change how some plugins work with/without the patch but that's fine by me.
Comment 36 Marcel de Rooy 2023-05-16 11:14:37 UTC
*** Bug 33744 has been marked as a duplicate of this bug. ***
Comment 37 Thomas Klausner 2023-05-29 12:01:40 UTC
Hi!

Just a quick note that we (Steiermärkische Landesbibliothek) have some custom value builders on repeatable fields (that now work again because we used the 'jQuery._data' hack). But as soon as this Bug is resolved (or maybe even a bit earlier...) we will test if the new code still works with our value builders (or figure out how we'll need to change / adapt them.

Greetings,
domm
Comment 38 Katrin Fischer 2023-06-18 12:34:31 UTC
Where are we with this?
Comment 39 Julian Maurice 2023-06-19 06:30:14 UTC
Looks like there is only two bugs left : bug 32853 and bug 32856. Both blocked because they are apparently not correct regarding UNIMARC. As they are only "refactoring" patches, can we open new bug reports for these problems in order to not block bug 30975 ?
Comment 40 Katrin Fischer 2023-06-20 06:07:49 UTC
(In reply to Julian Maurice from comment #39)
> Looks like there is only two bugs left : bug 32853 and bug 32856. Both
> blocked because they are apparently not correct regarding UNIMARC. As they
> are only "refactoring" patches, can we open new bug reports for these
> problems in order to not block bug 30975 ?

I think we couldn't test them properly for the confusion, but in theory it should all work. I am not a UNIMARC user, so can't tell how heavily used they are. 

Also can't test if we updated all plugins. David's list on comment 27 shows some missing, for example marc21_linking_section that also works on repeated fields (7xx).
Comment 41 Julian Maurice 2023-06-20 06:55:20 UTC
(In reply to Katrin Fischer from comment #40)
> Also can't test if we updated all plugins. David's list on comment 27 shows
> some missing, for example marc21_linking_section that also works on repeated
> fields (7xx).

marc21_linking_section does not need to be updated since the JS function already uses its event parameter.

A quick grep (git grep '\<function\>' cataloguing/value_builder/ | grep -v '(ev') shows that unimarc_field_125 and unimarc_field_126 are the only ones that do not use their event parameter.
Comment 42 Phil Ringnalda 2023-07-07 00:17:02 UTC
*** Bug 34221 has been marked as a duplicate of this bug. ***
Comment 43 David Cook 2023-08-18 03:33:11 UTC
Looks like it's still a problem in master for marc21_field_007.pl and marc21_field_006.pl at the very least.
Comment 44 David Nind 2023-08-25 08:36:15 UTC
The patches no longer apply:

Apply? [(y)es, (n)o, (i)nteractive] Y
Applying: Bug 30975: Fix framework plugins on cloned fields/subfields
Applying: Bug 30975: Fix framework plugins not working on cloned fields/subfields
Using index info to reconstruct a base tree...
M	C4/Items.pm
M	authorities/authorities.pl
M	koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers.inc
M	koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/acqui/orderreceive.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/authorities/authorities.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt
M	koha-tmpl/intranet-tmpl/prog/js/additem.js
M	koha-tmpl/intranet-tmpl/prog/js/cataloging.js
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/js/cataloging.js
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/js/cataloging.js
Auto-merging koha-tmpl/intranet-tmpl/prog/js/additem.js
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/authorities/authorities.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/authorities/authorities.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/orderreceive.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/orderreceive.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers.inc
Auto-merging authorities/authorities.pl
Auto-merging C4/Items.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 30975: Fix framework plugins not working on cloned fields/subfields
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-30975-Fix-framework-plugins-not-working-on-clo-2hwhjubx.patch
Comment 45 David Nind 2023-08-25 10:11:39 UTC
My comment on whether to go ahead with this big as it is (with the two Failed QA plugins/value builders), or progress Bug 33744 as an "interim" measure until they (and any other issues) are resolved.

With the amount of work that has gone into this bug, I think it should go ahead - hopefully the remaining issues can be resolved after the fact.

Current status (as I see it):

- Comment #27: List of all plugins/value builders and the related bug numbers. It is not clear to me why some don't need updating, but I assume it is because no change is required, and they are not affected. It would be good to get confirmation of this.

- EXAMPLE.pl may need updating to reflect the changes from this bug/the refactoring.

- Two dependencies are Failed QA:
  . Bug 32856 - Fix cataloguing/value_builder/unimarc_field_126.pl
  . Bug 32853 - Fix cataloguing/value_builder/unimarc_field_125.pl

Other things:

- This may (or may not) affect those who have developed their own custom plugins. How do we give advance warning of this and make the transition as seamless as possible? I guess this depends a little on whether it is back ported - 23.11 is three months away, it may be some time before those using their own plugins upgrade.

- Some of the later comments are a bit over my head, so I have no idea what they mean...

- It has been a great effort from everyone involved to get it this far (with all the dependencies!), it would be a shame for this to stall when it looks so close (to me anyway)!
Comment 46 Julian Maurice 2023-08-25 11:00:19 UTC
(In reply to David Nind from comment #45)
> - Comment #27: List of all plugins/value builders and the related bug
> numbers. It is not clear to me why some don't need updating, but I assume it
> is because no change is required, and they are not affected. It would be
> good to get confirmation of this.
Your assumption is correct. Though I haven't checked if other plugins have been created/modified recently

> - EXAMPLE.pl may need updating to reflect the changes from this bug/the
> refactoring.
EXAMPLE.pl correctly uses event.data.id so it does not need to be updated.

> - Two dependencies are Failed QA:
>   . Bug 32856 - Fix cataloguing/value_builder/unimarc_field_126.pl
>   . Bug 32853 - Fix cataloguing/value_builder/unimarc_field_125.pl
I think they should not be in this state as the issue discussed in not caused by the patches (see bug 32853 comment 6)

> Other things:
> 
> - This may (or may not) affect those who have developed their own custom
> plugins. How do we give advance warning of this and make the transition as
> seamless as possible? I guess this depends a little on whether it is back
> ported - 23.11 is three months away, it may be some time before those using
> their own plugins upgrade.
I think it's preferable to backport bug 33744 instead of this one as 33744 does not break compatibility.
As for the warning, there is one in the "release notes" field. It should be enough, right ?

Regarding the DNA status, I intend to rebase the patches once all dependencies pushed, not before.
Comment 47 David Nind 2023-08-28 19:44:32 UTC
Thanks Julian!

Thanks for the responses - I support/agree with all your comments.

I've signed off bug 33744, so hopefully that works through the QA process reasonably quickly.

I'm happy to test/sign off anything remaining to be done as it comes up for sign off - feel free to include me in any of the bugs (if I'm not on them already).

David
Comment 48 Julian Maurice 2023-10-30 13:39:00 UTC
Comment on attachment 140810 [details] [review]
Bug 30975: Fix framework plugins on cloned fields/subfields

Patch obsoleted as it was pushed in bug 33744
Comment 49 Julian Maurice 2023-10-30 13:39:53 UTC
Created attachment 158064 [details] [review]
Bug 30975: Fix framework plugins not working on cloned fields/subfields

Since bug 29155 (Upgrade jquery version to 3.6.0) framework plugins do
not work on a cloned field/subfield.
That is because we rely on jQuery's `.data('events')` to clone event
handlers to the cloned elements, and that was removed in jQuery 1.8.0
(I did not confirm but it's possible it continued to work after that
thanks to jQuery Migrate)

It is apparently still possible to access these event handlers by using
`jQuery._data(element, "events")` but here's what jQuery 1.8.0 release
notes says about it:
"this is not a supported public interface; the actual data structures
may change incompatibly from version to version."
So we should not rely on it.

Instead, what this patch does is use event delegation [1].
Events are bound to a parent container, so when elements are added
dynamically inside that container, we don't need to re-attach event
handlers manually

This patch also comes with a bit of cleanup, and introduce "breaking
changes" (they are breaking changes only if you happen to have custom
framework plugins):
1) 'mouseover', 'mousemove', 'keypress' events are no longer listened to
   'mouseover' and 'mousemove' are not used and would trigger too much
   events.
   'keypress' is also not used, and is deprecated
2) Event handlers now takes a single parameter that is an Event object
   It just makes the code a lot less complicated.
3) Event handlers do not pollute the global scope anymore

[1] https://learn.jquery.com/events/event-delegation/

Test plan:
- Go to every page that has a MARC editor and verify that plugins still
  work. This includes addbiblio.pl, additem.pl, authorities.pl,
  neworderempty.pl, orderreceive.pl
- Test plugins that use 'focus' event (for instance barcode.pl), 'blur'
  event (callnumber.pl) and 'click' event (almost all the others)
- Test that plugins work on cloned fields/subfields
Comment 50 Julian Maurice 2023-10-30 13:40:53 UTC
Patch rebased on master. All dependencies have been pushed. It's ready for testing
Comment 51 Katrin Fischer 2023-10-30 23:11:08 UTC
I found a pre-existing bug in authority and cataloguing editor while testing, but all seemed to still work as before with the patch set applied:
Bug 35194 - Javascript error when a repeatable field is repeated

In the acquisition module I found a problem:
* Activate  UseACQFrameworkForBiblioRecords 
* Crate a new basket, create an order from 'new'
* Open the Leader plugin - works
* Apply patch, restart_all etc.
* Leader plugin no longer opens :(
Comment 52 Marcel de Rooy 2023-11-09 09:06:53 UTC
> Events removed: change, mouseover, mouseout, mousedown, mouseup,
>    mousemove, keydown, keypress, keyup.

I do want to keep the Change event here.

On a few open reports I will still submit a plugin that uses the Change event to check ISBN, ISSN or callnumber for validity and existence.
See 34817 and 34860. I am using variants of those already in production (22.11).
I can imagine that others might wanna use Change too.

The other events are not really important here, I guess. Although I do not know if they may be used in the wild.. Does your argument about performance (too much events) have more background or supporting evidence?
Comment 53 Marcel de Rooy 2023-11-09 09:08:18 UTC
Btw can we change the title of this report now? And move to enh?

The clone bug is actually resolved. But this is a good enhancement(!) to simplify and modernize the plugin code.
Comment 54 Julian Maurice 2023-11-09 09:15:36 UTC
(In reply to Marcel de Rooy from comment #52)
> I do want to keep the Change event here.

It's not removed. See the commit message of the latest patches. Only 'mouseover', 'mousemove', 'keypress' are removed.
Comment 55 Julian Maurice 2023-11-09 09:19:49 UTC
(In reply to Marcel de Rooy from comment #53)
> Btw can we change the title of this report now? And move to enh?
Done
Comment 56 Pablo López Liotti 2023-11-16 18:11:25 UTC
Hi, i need use the callnumber.pl to auto-complete 952$o with 080$a and 092$a marc fields to make a full item call number.

The ITEMCALLNUMBER syspref holds '080a,092a' value.
The 952$o subfield in BKS framework have plugin->callmunber.pl value.
I make a biblio record with 080$a = '025' and 092$a = 'T 345' values.

Executing additems.pl only it shows '025' in the form.

Any ideas?

Thks
Pablo
Comment 57 Katrin Fischer 2023-11-16 21:54:24 UTC
(In reply to Pablo López Liotti from comment #56)
> Hi, i need use the callnumber.pl to auto-complete 952$o with 080$a and 092$a
> marc fields to make a full item call number.
> 
> The ITEMCALLNUMBER syspref holds '080a,092a' value.
> The 952$o subfield in BKS framework have plugin->callmunber.pl value.
> I make a biblio record with 080$a = '025' and 092$a = 'T 345' values.
> 
> Executing additems.pl only it shows '025' in the form.
> 
> Any ideas?
> 
> Thks
> Pablo

Hi Pablo, this bug is not related to your problem. When in doubt: ask on the mailing list or in the IRC chat first :) But: as stated in the system preference description, only the first field listed with a value will be used. So the preference is behaving as expected. You can currently not combine contents from different fields. You could file a new and separate enhancement request bug for this.
Comment 58 Pablo López Liotti 2023-11-16 22:47:23 UTC
(In reply to Katrin Fischer from comment #57)
> (In reply to Pablo López Liotti from comment #56)
> > Hi, i need use the callnumber.pl to auto-complete 952$o with 080$a and 092$a
> > marc fields to make a full item call number.
> > 
> > The ITEMCALLNUMBER syspref holds '080a,092a' value.
> > The 952$o subfield in BKS framework have plugin->callmunber.pl value.
> > I make a biblio record with 080$a = '025' and 092$a = 'T 345' values.
> > 
> > Executing additems.pl only it shows '025' in the form.
> > 
> > Any ideas?
> > 
> > Thks
> > Pablo
> 
> Hi Pablo, this bug is not related to your problem. When in doubt: ask on the
> mailing list or in the IRC chat first :) But: as stated in the system
> preference description, only the first field listed with a value will be
> used. So the preference is behaving as expected. You can currently not
> combine contents from different fields. You could file a new and separate
> enhancement request bug for this.

Thks Katrin, yes, I continued testing many times and understand that take only a field. 
For the call number as we make (with 2 fields), don´t be useful. 
Maybe works for us if we put UDC in 080$a and complete call number (UDC + Cutter for topographic) in 092$a. It´s a repetition but by the moment...
The other way is complete the item form in 952$o subfield.
As I understand many libraries (in Latin Américas at least) use that combination . 
This enhancemente will be most useful for many people. 

Thks again, cheers
Comment 59 Victor Grousset/tuxayo 2023-11-22 00:40:10 UTC
Hi :)

(In reply to Pablo López Liotti from comment #58)
> This enhancemente will be most useful for many people. 

Here is the place to open a new ticket: https://bugs.koha-community.org/bugzilla3/enter_bug.cgi

Here is documentation about opening tickets, mostly centered on proper bugs (stuff broken) but still has some info about enhancements/new feature requests:
https://wiki.koha-community.org/wiki/Bug_Reporting_Guidelines
Comment 60 Pablo López Liotti 2023-11-23 16:20:11 UTC
(In reply to Victor Grousset/tuxayo from comment #59)
> Hi :)
> 
> (In reply to Pablo López Liotti from comment #58)
> > This enhancemente will be most useful for many people. 
> 
> Here is the place to open a new ticket:
> https://bugs.koha-community.org/bugzilla3/enter_bug.cgi
> 
> Here is documentation about opening tickets, mostly centered on proper bugs
> (stuff broken) but still has some info about enhancements/new feature
> requests:
> https://wiki.koha-community.org/wiki/Bug_Reporting_Guidelines

Thks so much Victor.
Comment 61 Martin Renvoize 2024-01-16 15:10:48 UTC
I'm confused.. what's the current state of this.. what needs doing to get it over the finish line?
Comment 62 Marcel de Rooy 2024-01-16 15:12:14 UTC
It needs a rebase from Julian. And my final QA
Comment 63 Julian Maurice 2024-01-16 15:23:18 UTC
The bug found by Katrin in comment 51 needs to be fixed too