Description
Julian Maurice
2022-06-16 09:15:11 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 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 > 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.
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 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 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. 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?
(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. 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. 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 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) (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 ;) 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> 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. Also, I seemed to have mucked up the 22.05 attachments, and I'm not sure how to fix... 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 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 I also reintroduced some events ('change', 'mousedown', 'mouseup', 'keydown', 'keyup') just in case someone uses a custom plugin that use one of those events. (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! Does cataloguing/value_builder/EXAMPLE.pl need updating to reflect the changes made in the refactoring? Created attachment 146817 [details] [review] Bug 30975: Call preventDefault only for click events 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.... (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 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)
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 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?
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 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? (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. > 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. (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! These patches seems to break the functionality in barcode generator plugin barcode.pl. I removed the patches and it started generating barcodes again. (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 (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) (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. *** Bug 33744 has been marked as a duplicate of this bug. *** 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 Where are we with this? 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 ? (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). (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. *** Bug 34221 has been marked as a duplicate of this bug. *** Looks like it's still a problem in master for marc21_field_007.pl and marc21_field_006.pl at the very least. 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 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)! (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. 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 on attachment 140810 [details] [review] Bug 30975: Fix framework plugins on cloned fields/subfields Patch obsoleted as it was pushed in bug 33744 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 Patch rebased on master. All dependencies have been pushed. It's ready for testing 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 :( > 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?
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. (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. (In reply to Marcel de Rooy from comment #53) > Btw can we change the title of this report now? And move to enh? Done 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 (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. (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 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 (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. I'm confused.. what's the current state of this.. what needs doing to get it over the finish line? It needs a rebase from Julian. And my final QA The bug found by Katrin in comment 51 needs to be fixed too Created attachment 170499 [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 Created attachment 170500 [details] [review] Bug 30975: Fix framework plugins on acqui/neworderempty.pl Test plan: - Activate UseACQFrameworkForBiblioRecords - Make sure you have an ACQ cataloguing framework that uses at least one plugin - Create a new basket, create an order from 'new' - Verify that framework plugins work Patch rebased + fix for comment 51 Back to NSO Great to see this one rebased, thank Julian. Hopefully Marcel will find a moment soon to give it a final QA stamp. Created attachment 171513 [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 Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 171514 [details] [review] Bug 30975: Fix framework plugins on acqui/neworderempty.pl Test plan: - Activate UseACQFrameworkForBiblioRecords - Make sure you have an ACQ cataloguing framework that uses at least one plugin - Create a new basket, create an order from 'new' - Verify that framework plugins work Rebase to fix a minor conflict Looking here now. A few quick observations: 856$u Upload does not respond anymore Had some issues with dateaccessioned and cloning it in the normal editor (unusual of course). (In reply to Marcel de Rooy from comment #72) > A few quick observations: > > 856$u Upload does not respond anymore > > Had some issues with dateaccessioned and cloning it in the normal editor > (unusual of course). Ping Julian Any more needed here? Created attachment 175432 [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 Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 175433 [details] [review] Bug 30975: Fix framework plugins on acqui/neworderempty.pl Test plan: - Activate UseACQFrameworkForBiblioRecords - Make sure you have an ACQ cataloguing framework that uses at least one plugin - Create a new basket, create an order from 'new' - Verify that framework plugins work Created attachment 175434 [details] [review] Bug 30975: Fix upload plugin The upload plugin button has no 'buttonDot' class (unlike other plugins buttons) so its click event were not listened to. This patch fix the CSS selector in order to listen on click events for all plugins buttons Created attachment 175435 [details] [review] Bug 30975: Use event delegation for framework plugins This is to avoid using private jQuery method _data. 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. 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 Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 175436 [details] [review] Bug 30975: Fix framework plugins on acqui/neworderempty.pl Test plan: - Activate UseACQFrameworkForBiblioRecords - Make sure you have an ACQ cataloguing framework that uses at least one plugin - Create a new basket, create an order from 'new' - Verify that framework plugins work Created attachment 175437 [details] [review] Bug 30975: Fix upload plugin The upload plugin button has no 'buttonDot' class (unlike other plugins buttons) so its click event were not listened to. This patch fix the CSS selector in order to listen on click events for all plugins buttons Patches rebased + fix added for upload plugin + reworded the first commit message(In reply to Martin Renvoize (ashimema) from comment #74) > Any more needed here? It only needs a signoff now ;) (In reply to Julian Maurice from comment #0) > Since bug 29155 (Upgrade jquery version to 3.6.0) framework plugins do not > work on a cloned field/subfield. > [...] but it's possible it continued to work after that thanks to > jQuery Migrate) Is there something to test that will be fixed by these patches? Or is this strictly a refactoring/cleanup? It seems framework plugins did not work at some point but then they worked? Or is there still a scenario where the jquery upgrade caused a regression? On main. I found a field that had a plugin and both the original and a clone worked. Removing jquery migrate prevents the editor from loading so I couldn't go further this way. (In reply to Victor Grousset/tuxayo from comment #82) > Is there something to test that will be fixed by these patches? Or is this > strictly a refactoring/cleanup? At the beginning this was about a bug but the bugfix was moved to bug 33744. Now only the refactoring remains. (In reply to Julian Maurice from comment #83) > (In reply to Victor Grousset/tuxayo from comment #82) > > Is there something to test that will be fixed by these patches? Or is this > > strictly a refactoring/cleanup? > At the beginning this was about a bug but the bugfix was moved to bug 33744. > Now only the refactoring remains. Looking here again. We really should get this in now.. Created attachment 177612 [details] [review] Bug 30975: Use event delegation for framework plugins This is to avoid using private jQuery method _data. 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. 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 Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 177613 [details] [review] Bug 30975: Fix framework plugins on acqui/neworderempty.pl Test plan: - Activate UseACQFrameworkForBiblioRecords - Make sure you have an ACQ cataloguing framework that uses at least one plugin - Create a new basket, create an order from 'new' - Verify that framework plugins work Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 177614 [details] [review] Bug 30975: Fix upload plugin The upload plugin button has no 'buttonDot' class (unlike other plugins buttons) so its click event were not listened to. This patch fix the CSS selector in order to listen on click events for all plugins buttons Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 177615 [details] [review] Bug 30975: (QA follow-up) Fix for authorities and upload plugin Authorities still needed a few tweaks for text2 (obscure name for plugins). Upload plugin does not have buttonDot 'mark'. Cloning was still an issue. Not the most elegant solution but it works. We should not have this exception for just another icon or so. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 177616 [details] [review] Bug 30975: (QA follow-up) Improve copyright and author line This fundamental improvement may well be reflected in those lines. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> This looks really good but I concentrated most on biblio (incl upload), items and authorities. I would like someone to still test acquisition (neworderempty/orderreceive) and batchmod. Victor, David or Martin: Could you add your signoff especially for the mentioned areas? With that signoff I will pass QA. We need to get this further now. It is waiting much too long.. I'll take a look shortly especially on 'acquisition (neworderempty/orderreceive) and batchmod'. Created attachment 177719 [details] [review] Bug 30975: Use event delegation for framework plugins This is to avoid using private jQuery method _data. 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. 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 Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 177720 [details] [review] Bug 30975: Fix framework plugins on acqui/neworderempty.pl Test plan: - Activate UseACQFrameworkForBiblioRecords - Make sure you have an ACQ cataloguing framework that uses at least one plugin - Create a new basket, create an order from 'new' - Verify that framework plugins work Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 177721 [details] [review] Bug 30975: Fix upload plugin The upload plugin button has no 'buttonDot' class (unlike other plugins buttons) so its click event were not listened to. This patch fix the CSS selector in order to listen on click events for all plugins buttons Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 177722 [details] [review] Bug 30975: (QA follow-up) Fix for authorities and upload plugin Authorities still needed a few tweaks for text2 (obscure name for plugins). Upload plugin does not have buttonDot 'mark'. Cloning was still an issue. Not the most elegant solution but it works. We should not have this exception for just another icon or so. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 177723 [details] [review] Bug 30975: (QA follow-up) Improve copyright and author line This fundamental improvement may well be reflected in those lines. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au> Thanks David. Using my signoff now to pass QA. |