Bug 30975

Summary: Framework plugins do not work on cloned fields/subfields
Product: Koha Reporter: Julian Maurice <julian.maurice>
Component: CatalogingAssignee: Julian Maurice <julian.maurice>
Status: Failed QA --- QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: caroline.cyr-la-rose, david, jonathan.druart+koha, kyle, m.de.rooy
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=3544
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 29155    
Bug Blocks:    
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

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...