From bug 9921, Galen: All the more reason to stick to our coding guidelines, if the additional warning messages in the log annoys somebody enough to inspire them to fix that issue with the cataloging plugins, then. :) I am submitting a proposal here. May not be perfect yet, but we loose the redefines.
Created attachment 19064 [details] [review] Bug 10480: New cataloging plugin style without redefining This reports adds the way we handle cataloging plugins. Currently, three functions are redefined: plugin_parameters, plugin_javascript and plugin. In this new proposal, communication between addbiblio and the plugin goes via a global variable. The plugin script detects that way if we are building the plugin or executing it. A example plugin is added just to illustrate the concept. Plugin marc21_leader.pl is adjusted to the new style (plus some cosmetic changes: removing tabs etc.) Test plan: Run the MARC editor and test your current plugins. If you use marc21_leader, test it again. Temporarily connect new_example_plugin to some field and test that. Check the log for warnings about redefines.
adds the way => changes the way
Created attachment 19072 [details] [review] Bug 10480: New cataloging plugin style without redefining This reports adds the way we handle cataloging plugins. Currently, three functions are redefined: plugin_parameters, plugin_javascript and plugin. In this new proposal, communication between addbiblio and the plugin goes via a global variable. The plugin script detects that way if we are building the plugin or executing it. A example plugin is added just to illustrate the concept. Plugin marc21_leader.pl is adjusted to the new style (plus some cosmetic changes: removing tabs etc.) Test plan: Run the MARC editor and test your current plugins. If you use marc21_leader, test it again. Temporarily connect new_example_plugin to some field and test that. Check the log for warnings about redefines.
Created attachment 19098 [details] [review] [SIGNED-OFF] Bug 10480: New cataloging plugin style without redefining It works fine. ;)
Created attachment 19152 [details] [review] Bug 10480: New cataloging plugin style without redefining This reports adds the way we handle cataloging plugins. Currently, three functions are redefined: plugin_parameters, plugin_javascript and plugin. In this new proposal, communication between addbiblio and the plugin goes via a global variable. The plugin script detects that way if we are building the plugin or executing it. A example plugin is added just to illustrate the concept. Plugin marc21_leader.pl is adjusted to the new style (plus some cosmetic changes: removing tabs etc.) Test plan: Run the MARC editor and test your current plugins. If you use marc21_leader, test it again. Temporarily connect new_example_plugin to some field and test that. Check the log for warnings about redefines. Signed-off-by: Nuño López Ansótegui <nunyo@masmedios.com> Still amended this patch: arrayref $building_plugins => hashref $building_plugin hash entry names: tabno and subfieldid are slightly better slightly adjusted test on passed hash values Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 19154 [details] [review] Bug 10480: New cataloging plugin style without redefining This reports changes the way we handle cataloging plugins. Currently, three functions are redefined: plugin_parameters, plugin_javascript and plugin. In this new proposal, communication between addbiblio and the plugin goes via a global variable. The plugin script detects that way if we are building the plugin or executing it. A example plugin is added just to illustrate the concept. Plugin marc21_leader.pl is adjusted to the new style (plus some cosmetic changes: removing tabs etc.) Test plan: Run the MARC editor and test your current plugins. If you use marc21_leader, test it again. Temporarily connect new_example_plugin to some field and test that. Check the log for warnings about redefines. Signed-off-by: Nuno Lopez Ansotegui <nunyo@masmedios.com> Still amended this patch: arrayref $building_plugins => hashref $building_plugin hash entry names: tabno and subfieldid are slightly better slightly adjusted test on passed hash values Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Could have a signoff again, since I amended my patch. Note that git may have some trouble with diacritics in the commit message..
I just realize that we do need to make a similar change in additem.pl (of course..) So indeed, it was not perfect yet.
Created attachment 19168 [details] [review] Bug 10480: Followup for additem.pl (no redefines for plugins) Adds the same functionality for additem as in addbiblio. Note that the new subroutine is slightly different from the addbiblio case. Test plan: Test your cataloging plugins on Add/edit items.
Created attachment 19169 [details] [review] Bug 10480: Followup for additem.pl (no redefines for plugins) Adds the same functionality for additem as in addbiblio. Note that the new subroutine is slightly different from the addbiblio case. Test plan: Test your cataloging plugins on Add/edit items. If you have no one, temporarily connect new_example_plugin.pl; it will only put 12345 in your item field.
Patch applied cleanly, go forth and signoff
Still want to proceed on this in another direction.
(In reply to M. de Rooy from comment #12) > Still want to proceed on this in another direction. This took some more time than I hoped for. But will submit them now, based on the concept of two anynomous subroutines.
Created attachment 34308 [details] [review] Bug 10480: New module and unit test for framework plugins This patch introduces the Koha::FrameworkPlugin object to Koha. This object supports the current "old-style" plugins while adding a new style based on the concept of two anynomous subroutines for building and launching. I will summarize the advantages of this new approach, justifying the additional lines of code in this patch: [1] Centralizing the code for building and launching plugins. [2] Extensive unit testing: this was not possible before. [3] Simplicity: Only define what you need in the plugin. A follow-up patch will delete 1500 lines with *empty* routines. [4] Make it possible to restore the warnings pragma for all plugins. New style plugins do no longer depend on redefinition. [5] Event binding moved from HTML attributes moved to jQuery code. This separates behavior and presentation. [6] Much more documentation, including EXAMPLE plugin in follow-up. [7] Improved error handling. [8] Usability: property noclick tells you if plugin's buttonDot is active. [9] More events supported: Change, keyboard/mouse events. See EXAMPLE. NOTE ON EXAMPLE PLUGIN: The example plugin is added in the third patch of this report. Since it is new style, it can be used only after we start using this object. It also contains an example for a keypress and mouseover event. NOTE ON ITEM PLUGINS: Old style plugins for items contain an additional parameter in the js functions for Blur, Focus and Change. This distinction has no actual use and is resolved for new plugins in the object code. When converting item plugins, this minor correction will be addressed. In the meantime old style item plugins behave as expected. TEST PLAN: Run the new test t/db_dependent/FrameworkPlugin.t At this point in time, you do not need to do anything more. Follow-up patches will incorporate the object in real-life Koha and provide additional test plans.
Created attachment 34309 [details] [review] Bug 10480: Use the framework plugin object in cataloguing This patch implements the use of Koha::FrameworkPlugin in Cataloguing, Authorities, Acquisition, Serials and Tools. The main change is architectural: see the commit message of the previous patch. No changes in behavior are expected, but the support of new events may provide additional functionality in the future. Some small bugs are resolved along the way. The change primarily focuses on the MARC and items editor in Cataloguing. But the MARC editor for Authorities and the item editor in Acquisition, Serials and Tools are touched too. This commit message gives some comments per module. NOTE FOR CATALOGUING: A new plugin without popup (or other click event code) now shows the title No popup when hovering over the tag editor image. The image alerts the user on a plugin, the title tells about its status. The noclick property allows for further style modifications in the template. Note that a follow-up patch will clean up the old style plugins too with the same effect. Some additional code in cataloging.js makes it possible to clone subfields with plugins (although only theoretically useful). The clones use the same javascript functions but event.data contains an updated id. This effectively resolves bug 13306. Note that if old plugins do not use the javascript parameter for the id but the perl variable, cloning does still operate on the wrong field (with and without this patch set). In the absence of report 12176 in master, it is not yet necessary to modify additem.tt. When it gets pushed, it should be an easy rebase. New style item plugins will no longer need an extra parameter. (The code in the FrameworkPlugin object actually takes care of that.) NOTE FOR AUTHORITIES: This patch also adds class name tag_editor to the buttonDot anchors. This effectively makes the same tag editor image appear as in Cataloguing. Futhermore it removes the button from the tab sequence if there is no click event (really effective after conversion to the new style, since the old style plugins contain empty onclicks and launchers). Both small adjustments increase consistency between auth and bib edits. NOTE FOR ACQUISITION: In Acquisition two scripts use an item editor, but in a different way. The scripts addorderiso2709 and neworderempty both rely on the routine PrepareItemrecordDisplay in C4::Items, but neworderempty creates item blocks dynamically via an ajax call to services/itemrecorddisplay.pl. In order to make the dynamic item blocks work with plugins, some code changes were needed in additem.js. (Normally the event binding is done at document ready time; now it must be done later.) At this moment the routine in Items.pm contains the html tags, and this makes changes to the following templates not necessary for now: * acqui/addorderiso2709.tt * services/itemrecorddisplay.tt Report 13397 has been opened to address moving the html to the templates. NOTE FOR SERIALS: Script serial-edit relies also on C4::Items (just as in Acquisition). This makes changes to serials/serials-edit.tt not necessary for now. NOTE FOR TOOLS: The current code in tools/batchMod.pl allows the use of plugins for batch modification of items. This patch just converts that code to use the new object. Most item plugins however may not be very useful for operating on multiple items at once. PERFORMANCE: I have benchmarked build_tabs in addbiblio to see how especially the additional processing of the javascript in the FrameworkPlugin object would impact performance. Testing default MARC21 framework with 8 plugins gave the following figures: - Old situation: 851 ms - New situation: 942 ms (+10,7%) - New situation after plugin cleanup: 881 ms (+3,4%) Note also that adding lines for event binding is compensated by removing lines for unused events. Page load should essentially be the same. TEST PLAN: Suggestion: If you also apply the next patch with the EXAMPLE plugin, you can test with a rather harmless plugin (with popup) on various places :) But your test should also include old style plugins, with[out] popups. If you want to test a new plugin without popup, rename/remove Click$id in the javascript code of the $builder definition (temporarily). [1] Test Cataloguing: - Add/Edit biblio. Try plugins with and without popup. - Add/Edit items. (EXAMPLE can be used as an item plugin with popup.) - Clone a subfield with plugin (use EXAMPLE): Verify that the plugin works on both original and clone with the respective field values. Is the value put back in the right field too? [2] Test Authorities: Edit an authority record. Try plugins with an without popup. [3] Test Acquisition: Set system preference AcqCreateItem to "placing an order". Check the item editor in the following two places: a- addorderiso2709: Open a basket, add an order from a staged file. Select a file, click Add orders, and go to tab Item information. b- neworderempty: Open a basket, add an order from a new empty record. [4] Test Serials: Check the item editor on serials-edit. Go to subscription detail. Click Receive. Choose "Click to add item". (Note that this subscription should create an item record when receiving this serial.) [5] Test Tools: Check the item editor for batch item modification. Enter a few valid barcodes and press Continue to reach the item editor.
Created attachment 34310 [details] [review] Bug 10480: EXAMPLE plugin with associated template The EXAMPLE plugin helps you to create a new style framework plugin by providing a simple working example and additional documentation for developers. Test plan: [1] Connect the EXAMPLE plugin to one or more fields. [2] Try the following events: a- Focus: If the field is empty, it should put EXAMPLE: into it. b- MouseOver: If the field is empty and you move your mouse over it, it should have the same effect as Focus. c- Change: Edit the field in the editor and tab out of it, the color of the text should toggle (randomly) between red, green and blue. d- KeyPress: If you edit the field and you type @, it should give AT. e- Click: Click on the tag editor. Change the value in the popup. If you press OK, the field should be changed. [3] Would the documentation in the perl script help you to create a plugin?
Created attachment 34311 [details] [review] Bug 10480: Remove useless routines and irrelevant pod lines This patch makes the following cleanup actions for all plugins: 1- Remove sub plugin_parameters. This function is not used. 2- Remove empty plugin or empty plugin_javascript subs. 3- Remove empty Blur, Clic or Focus routines from javascript. 4- Remove pod lines (copy-pasta) only referring to the 3 plugin subs. 5- Remove the last 1; line. It is no longer needed. Test plan: Run the unit test t/db_dependent/FrameworkPlugins.t Test some cataloguing plugins in addbiblio. Test some item plugins in additem and neworderempty. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> I also checked the syntax of all plugins with perl -c. And checked the pod (if any) with podchecker: Two files still produce a warning; the follow-up deals with them: unimarc_field_686a/_700-4.pl.
Created attachment 34312 [details] [review] Bug 10480: Follow-up for unimarc_field_686a/_700-4.pl This patch fixes the pod errors on those two files by removing the pod section that actually does not add anything useful.
I will add see also-reports to convert old style plugins to new style plugins for marc21, normarc and unimarc. This can be tested and handled separately however.
Created attachment 35187 [details] [review] Bug 10480: New module and unit test for framework plugins This patch introduces the Koha::FrameworkPlugin object to Koha. This object supports the current "old-style" plugins while adding a new style based on the concept of two anynomous subroutines for building and launching. I will summarize the advantages of this new approach, justifying the additional lines of code in this patch: [1] Centralizing the code for building and launching plugins. [2] Extensive unit testing: this was not possible before. [3] Simplicity: Only define what you need in the plugin. A follow-up patch will delete 1500 lines with *empty* routines. [4] Make it possible to restore the warnings pragma for all plugins. New style plugins do no longer depend on redefinition. [5] Event binding moved from HTML attributes moved to jQuery code. This separates behavior and presentation. [6] Much more documentation, including EXAMPLE plugin in follow-up. [7] Improved error handling. [8] Usability: property noclick tells you if plugin's buttonDot is active. [9] More events supported: Change, keyboard/mouse events. See EXAMPLE. NOTE ON EXAMPLE PLUGIN: The example plugin is added in the third patch of this report. Since it is new style, it can be used only after we start using this object. It also contains an example for a keypress and mouseover event. NOTE ON ITEM PLUGINS: Old style plugins for items contain an additional parameter in the js functions for Blur, Focus and Change. This distinction has no actual use and is resolved for new plugins in the object code. When converting item plugins, this minor correction will be addressed. In the meantime old style item plugins behave as expected. TEST PLAN: Run the new test t/db_dependent/FrameworkPlugin.t At this point in time, you do not need to do anything more. Follow-up patches will incorporate the object in real-life Koha and provide additional test plans.
Created attachment 35188 [details] [review] Bug 10480: Use the framework plugin object in cataloguing This patch implements the use of Koha::FrameworkPlugin in Cataloguing, Authorities, Acquisition, Serials and Tools. The main change is architectural: see the commit message of the previous patch. No changes in behavior are expected, but the support of new events may provide additional functionality in the future. Some small bugs are resolved along the way. The change primarily focuses on the MARC and items editor in Cataloguing. But the MARC editor for Authorities and the item editor in Acquisition, Serials and Tools are touched too. This commit message gives some comments per module. NOTE FOR CATALOGUING: A new plugin without popup (or other click event code) now shows the title No popup when hovering over the tag editor image. The image alerts the user on a plugin, the title tells about its status. The noclick property allows for further style modifications in the template. Note that a follow-up patch will clean up the old style plugins too with the same effect. Some additional code in cataloging.js makes it possible to clone subfields with plugins (although only theoretically useful). The clones use the same javascript functions but event.data contains an updated id. This effectively resolves bug 13306. Note that if old plugins do not use the javascript parameter for the id but the perl variable, cloning does still operate on the wrong field (with and without this patch set). In the absence of report 12176 in master, it is not yet necessary to modify additem.tt. When it gets pushed, it should be an easy rebase. New style item plugins will no longer need an extra parameter. (The code in the FrameworkPlugin object actually takes care of that.) NOTE FOR AUTHORITIES: This patch also adds class name tag_editor to the buttonDot anchors. This effectively makes the same tag editor image appear as in Cataloguing. Futhermore it removes the button from the tab sequence if there is no click event (really effective after conversion to the new style, since the old style plugins contain empty onclicks and launchers). Both small adjustments increase consistency between auth and bib edits. NOTE FOR ACQUISITION: In Acquisition two scripts use an item editor, but in a different way. The scripts addorderiso2709 and neworderempty both rely on the routine PrepareItemrecordDisplay in C4::Items, but neworderempty creates item blocks dynamically via an ajax call to services/itemrecorddisplay.pl. In order to make the dynamic item blocks work with plugins, some code changes were needed in additem.js. (Normally the event binding is done at document ready time; now it must be done later.) At this moment the routine in Items.pm contains the html tags, and this makes changes to the following templates not necessary for now: * acqui/addorderiso2709.tt * services/itemrecorddisplay.tt Report 13397 has been opened to address moving the html to the templates. NOTE FOR SERIALS: Script serial-edit relies also on C4::Items (just as in Acquisition). This makes changes to serials/serials-edit.tt not necessary for now. NOTE FOR TOOLS: The current code in tools/batchMod.pl allows the use of plugins for batch modification of items. This patch just converts that code to use the new object. Most item plugins however may not be very useful for operating on multiple items at once. PERFORMANCE: I have benchmarked build_tabs in addbiblio to see how especially the additional processing of the javascript in the FrameworkPlugin object would impact performance. Testing default MARC21 framework with 8 plugins gave the following figures: - Old situation: 851 ms - New situation: 942 ms (+10,7%) - New situation after plugin cleanup: 881 ms (+3,4%) Note also that adding lines for event binding is compensated by removing lines for unused events. Page load should essentially be the same. TEST PLAN: Suggestion: If you also apply the next patch with the EXAMPLE plugin, you can test with a rather harmless plugin (with popup) on various places :) But your test should also include old style plugins, with[out] popups. If you want to test a new plugin without popup, rename/remove Click$id in the javascript code of the $builder definition (temporarily). [1] Test Cataloguing: - Add/Edit biblio. Try plugins with and without popup. - Add/Edit items. (EXAMPLE can be used as an item plugin with popup.) - Clone a subfield with plugin (use EXAMPLE): Verify that the plugin works on both original and clone with the respective field values. Is the value put back in the right field too? [2] Test Authorities: Edit an authority record. Try plugins with an without popup. [3] Test Acquisition: Set system preference AcqCreateItem to "placing an order". Check the item editor in the following two places: a- addorderiso2709: Open a basket, add an order from a staged file. Select a file, click Add orders, and go to tab Item information. b- neworderempty: Open a basket, add an order from a new empty record. [4] Test Serials: Check the item editor on serials-edit. Go to subscription detail. Click Receive. Choose "Click to add item". (Note that this subscription should create an item record when receiving this serial.) [5] Test Tools: Check the item editor for batch item modification. Enter a few valid barcodes and press Continue to reach the item editor.
Created attachment 35189 [details] [review] Bug 10480: EXAMPLE plugin with associated template The EXAMPLE plugin helps you to create a new style framework plugin by providing a simple working example and additional documentation for developers. Test plan: [1] Connect the EXAMPLE plugin to one or more fields. [2] Try the following events: a- Focus: If the field is empty, it should put EXAMPLE: into it. b- MouseOver: If the field is empty and you move your mouse over it, it should have the same effect as Focus. c- Change: Edit the field in the editor and tab out of it, the color of the text should toggle (randomly) between red, green and blue. d- KeyPress: If you edit the field and you type @, it should give AT. e- Click: Click on the tag editor. Change the value in the popup. If you press OK, the field should be changed. [3] Would the documentation in the perl script help you to create a plugin?
Created attachment 35190 [details] [review] Bug 10480: Remove useless routines and irrelevant pod lines This patch makes the following cleanup actions for all plugins: 1- Remove sub plugin_parameters. This function is not used. 2- Remove empty plugin or empty plugin_javascript subs. 3- Remove empty Blur, Clic or Focus routines from javascript. 4- Remove pod lines (copy-pasta) only referring to the 3 plugin subs. 5- Remove the last 1; line. It is no longer needed. Test plan: Run the unit test t/db_dependent/FrameworkPlugins.t Test some cataloguing plugins in addbiblio. Test some item plugins in additem and neworderempty. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> I also checked the syntax of all plugins with perl -c. And checked the pod (if any) with podchecker: Two files still produce a warning; the follow-up deals with them: unimarc_field_686a/_700-4.pl.
Created attachment 35191 [details] [review] Bug 10480: Follow-up for unimarc_field_686a/_700-4.pl This patch fixes the pod errors on those two files by removing the pod section that actually does not add anything useful.
Rebased. Anyone willing to help getting this further?
Created attachment 35286 [details] [review] Bug 10480: Remove useless routines and irrelevant pod lines This patch makes the following cleanup actions for all plugins: 1- Remove sub plugin_parameters. This function is not used. 2- Remove empty plugin or empty plugin_javascript subs. 3- Remove empty Blur, Clic or Focus routines from javascript. 4- Remove pod lines (copy-pasta) only referring to the 3 plugin subs. 5- Remove the last 1; line. It is no longer needed. Test plan: Run the unit test t/db_dependent/FrameworkPlugins.t Test some cataloguing plugins in addbiblio. Test some item plugins in additem and neworderempty. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> I also checked the syntax of all plugins with perl -c. And checked the pod (if any) with podchecker: Two files still produce a warning; the follow-up deals with them: unimarc_field_686a/_700-4.pl.
Created attachment 35287 [details] [review] Bug 10480: Follow-up for unimarc_field_686a/_700-4.pl This patch fixes the pod errors on those two files by removing the pod section that actually does not add anything useful.
Created attachment 36106 [details] [review] Bug 10480: New module and unit test for framework plugins This patch introduces the Koha::FrameworkPlugin object to Koha. This object supports the current "old-style" plugins while adding a new style based on the concept of two anynomous subroutines for building and launching. I will summarize the advantages of this new approach, justifying the additional lines of code in this patch: [1] Centralizing the code for building and launching plugins. [2] Extensive unit testing: this was not possible before. [3] Simplicity: Only define what you need in the plugin. A follow-up patch will delete 1500 lines with *empty* routines. [4] Make it possible to restore the warnings pragma for all plugins. New style plugins do no longer depend on redefinition. [5] Event binding moved from HTML attributes moved to jQuery code. This separates behavior and presentation. [6] Much more documentation, including EXAMPLE plugin in follow-up. [7] Improved error handling. [8] Usability: property noclick tells you if plugin's buttonDot is active. [9] More events supported: Change, keyboard/mouse events. See EXAMPLE. NOTE ON EXAMPLE PLUGIN: The example plugin is added in the third patch of this report. Since it is new style, it can be used only after we start using this object. It also contains an example for a keypress and mouseover event. NOTE ON ITEM PLUGINS: Old style plugins for items contain an additional parameter in the js functions for Blur, Focus and Change. This distinction has no actual use and is resolved for new plugins in the object code. When converting item plugins, this minor correction will be addressed. In the meantime old style item plugins behave as expected. TEST PLAN: Run the new test t/db_dependent/FrameworkPlugin.t At this point in time, you do not need to do anything more. Follow-up patches will incorporate the object in real-life Koha and provide additional test plans. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 36107 [details] [review] Bug 10480: Use the framework plugin object in cataloguing This patch implements the use of Koha::FrameworkPlugin in Cataloguing, Authorities, Acquisition, Serials and Tools. The main change is architectural: see the commit message of the previous patch. No changes in behavior are expected, but the support of new events may provide additional functionality in the future. Some small bugs are resolved along the way. The change primarily focuses on the MARC and items editor in Cataloguing. But the MARC editor for Authorities and the item editor in Acquisition, Serials and Tools are touched too. This commit message gives some comments per module. NOTE FOR CATALOGUING: A new plugin without popup (or other click event code) now shows the title No popup when hovering over the tag editor image. The image alerts the user on a plugin, the title tells about its status. The noclick property allows for further style modifications in the template. Note that a follow-up patch will clean up the old style plugins too with the same effect. Some additional code in cataloging.js makes it possible to clone subfields with plugins (although only theoretically useful). The clones use the same javascript functions but event.data contains an updated id. This effectively resolves bug 13306. Note that if old plugins do not use the javascript parameter for the id but the perl variable, cloning does still operate on the wrong field (with and without this patch set). In the absence of report 12176 in master, it is not yet necessary to modify additem.tt. When it gets pushed, it should be an easy rebase. New style item plugins will no longer need an extra parameter. (The code in the FrameworkPlugin object actually takes care of that.) NOTE FOR AUTHORITIES: This patch also adds class name tag_editor to the buttonDot anchors. This effectively makes the same tag editor image appear as in Cataloguing. Futhermore it removes the button from the tab sequence if there is no click event (really effective after conversion to the new style, since the old style plugins contain empty onclicks and launchers). Both small adjustments increase consistency between auth and bib edits. NOTE FOR ACQUISITION: In Acquisition two scripts use an item editor, but in a different way. The scripts addorderiso2709 and neworderempty both rely on the routine PrepareItemrecordDisplay in C4::Items, but neworderempty creates item blocks dynamically via an ajax call to services/itemrecorddisplay.pl. In order to make the dynamic item blocks work with plugins, some code changes were needed in additem.js. (Normally the event binding is done at document ready time; now it must be done later.) At this moment the routine in Items.pm contains the html tags, and this makes changes to the following templates not necessary for now: * acqui/addorderiso2709.tt * services/itemrecorddisplay.tt Report 13397 has been opened to address moving the html to the templates. NOTE FOR SERIALS: Script serial-edit relies also on C4::Items (just as in Acquisition). This makes changes to serials/serials-edit.tt not necessary for now. NOTE FOR TOOLS: The current code in tools/batchMod.pl allows the use of plugins for batch modification of items. This patch just converts that code to use the new object. Most item plugins however may not be very useful for operating on multiple items at once. PERFORMANCE: I have benchmarked build_tabs in addbiblio to see how especially the additional processing of the javascript in the FrameworkPlugin object would impact performance. Testing default MARC21 framework with 8 plugins gave the following figures: - Old situation: 851 ms - New situation: 942 ms (+10,7%) - New situation after plugin cleanup: 881 ms (+3,4%) Note also that adding lines for event binding is compensated by removing lines for unused events. Page load should essentially be the same. TEST PLAN: Suggestion: If you also apply the next patch with the EXAMPLE plugin, you can test with a rather harmless plugin (with popup) on various places :) But your test should also include old style plugins, with[out] popups. If you want to test a new plugin without popup, rename/remove Click$id in the javascript code of the $builder definition (temporarily). [1] Test Cataloguing: - Add/Edit biblio. Try plugins with and without popup. - Add/Edit items. (EXAMPLE can be used as an item plugin with popup.) - Clone a subfield with plugin (use EXAMPLE): Verify that the plugin works on both original and clone with the respective field values. Is the value put back in the right field too? [2] Test Authorities: Edit an authority record. Try plugins with an without popup. [3] Test Acquisition: Set system preference AcqCreateItem to "placing an order". Check the item editor in the following two places: a- addorderiso2709: Open a basket, add an order from a staged file. Select a file, click Add orders, and go to tab Item information. b- neworderempty: Open a basket, add an order from a new empty record. [4] Test Serials: Check the item editor on serials-edit. Go to subscription detail. Click Receive. Choose "Click to add item". (Note that this subscription should create an item record when receiving this serial.) [5] Test Tools: Check the item editor for batch item modification. Enter a few valid barcodes and press Continue to reach the item editor. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 36108 [details] [review] Bug 10480: EXAMPLE plugin with associated template The EXAMPLE plugin helps you to create a new style framework plugin by providing a simple working example and additional documentation for developers. Test plan: [1] Connect the EXAMPLE plugin to one or more fields. [2] Try the following events: a- Focus: If the field is empty, it should put EXAMPLE: into it. b- MouseOver: If the field is empty and you move your mouse over it, it should have the same effect as Focus. c- Change: Edit the field in the editor and tab out of it, the color of the text should toggle (randomly) between red, green and blue. d- KeyPress: If you edit the field and you type @, it should give AT. e- Click: Click on the tag editor. Change the value in the popup. If you press OK, the field should be changed. [3] Would the documentation in the perl script help you to create a plugin? Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 36109 [details] [review] Bug 10480: Remove useless routines and irrelevant pod lines This patch makes the following cleanup actions for all plugins: 1- Remove sub plugin_parameters. This function is not used. 2- Remove empty plugin or empty plugin_javascript subs. 3- Remove empty Blur, Clic or Focus routines from javascript. 4- Remove pod lines (copy-pasta) only referring to the 3 plugin subs. 5- Remove the last 1; line. It is no longer needed. Test plan: Run the unit test t/db_dependent/FrameworkPlugins.t Test some cataloguing plugins in addbiblio. Test some item plugins in additem and neworderempty. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> I also checked the syntax of all plugins with perl -c. And checked the pod (if any) with podchecker: Two files still produce a warning; the follow-up deals with them: unimarc_field_686a/_700-4.pl. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 36110 [details] [review] Bug 10480: Follow-up for unimarc_field_686a/_700-4.pl This patch fixes the pod errors on those two files by removing the pod section that actually does not add anything useful. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Notes - Followed test plan and recommendations. I couldn't find anything wrong with it. It took me a long time to get through it - not sure if I am just slow or what - so heads up for QA (I poked at parts for a few differnt days . As I went through it I didn't really find an extra area that needed checking. So QA should be able to follow plans forward. Also read through the code and didn't find anything that was out of place for code. Thanks, Brendan
(In reply to Brendan Gallagher from comment #33) > Followed test plan and recommendations. I couldn't find anything wrong with > it. It took me a long time to get through it - not sure if I am just slow > or what - so heads up for QA (I poked at parts for a few differnt days . As > I went through it I didn't really find an extra area that needed checking. > So QA should be able to follow plans forward. Also read through the code > and didn't find anything that was out of place for code. Thanks!
I think this probably shouldn't go on to the stable branches as it would break custom plugins the library might have. As the title says "improvements" switching to "enh" - but I know it's not quite.
(In reply to Katrin Fischer from comment #35) > I think this probably shouldn't go on to the stable branches as it would > break custom plugins the library might have. Do you have any reasons to say that it would break custom plugins? Part of some complexity here is backward compatibility. BTW I agree that this is not targeted for the stable branches in the first place.
Created attachment 36953 [details] [review] Bug 10480: New module and unit test for framework plugins
Created attachment 36954 [details] [review] Bug 10480: Use the framework plugin object in cataloguing
Created attachment 36955 [details] [review] Bug 10480: EXAMPLE plugin with associated template
Created attachment 36956 [details] [review] Bug 10480: Remove useless routines and irrelevant pod lines
Created attachment 36957 [details] [review] Bug 10480: Follow-up for unimarc_field_686a/_700-4.pl
Great work Marcel! I did not find any regression and the code looks better than before. I really would like to see this one moving forward to motivate you to continue the job :) However I would like to get another pair of eyes on it, that's why I don't change the status for now. Just one question: The EXAMPLE plugin should be removed (or not pushed) or hidden in the dropdown list, don't you think?
(In reply to Jonathan Druart from comment #42) > I did not find any regression and the code looks better than before. I > really would like to see this one moving forward to motivate you to continue > the job :) > However I would like to get another pair of eyes on it, that's why I don't > change the status for now. Thanks for testing. > Just one question: The EXAMPLE plugin should be removed (or not pushed) or > hidden in the dropdown list, don't you think? It is some kind of documentation; also easy to copy and extend it etc. But we could certainly hide in the dropdown list. That would be a trivial follow-up.
Created attachment 37948 [details] [review] [PASSED QA] Bug 10480: New module and unit test for framework plugins This patch introduces the Koha::FrameworkPlugin object to Koha. This object supports the current "old-style" plugins while adding a new style based on the concept of two anynomous subroutines for building and launching. I will summarize the advantages of this new approach, justifying the additional lines of code in this patch: [1] Centralizing the code for building and launching plugins. [2] Extensive unit testing: this was not possible before. [3] Simplicity: Only define what you need in the plugin. A follow-up patch will delete 1500 lines with *empty* routines. [4] Make it possible to restore the warnings pragma for all plugins. New style plugins do no longer depend on redefinition. [5] Event binding moved from HTML attributes moved to jQuery code. This separates behavior and presentation. [6] Much more documentation, including EXAMPLE plugin in follow-up. [7] Improved error handling. [8] Usability: property noclick tells you if plugin's buttonDot is active. [9] More events supported: Change, keyboard/mouse events. See EXAMPLE. NOTE ON EXAMPLE PLUGIN: The example plugin is added in the third patch of this report. Since it is new style, it can be used only after we start using this object. It also contains an example for a keypress and mouseover event. NOTE ON ITEM PLUGINS: Old style plugins for items contain an additional parameter in the js functions for Blur, Focus and Change. This distinction has no actual use and is resolved for new plugins in the object code. When converting item plugins, this minor correction will be addressed. In the meantime old style item plugins behave as expected. TEST PLAN: Run the new test t/db_dependent/FrameworkPlugin.t At this point in time, you do not need to do anything more. Follow-up patches will incorporate the object in real-life Koha and provide additional test plans. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 37949 [details] [review] [PASSED QA] Bug 10480: New module and unit test for framework plugins This patch introduces the Koha::FrameworkPlugin object to Koha. This object supports the current "old-style" plugins while adding a new style based on the concept of two anynomous subroutines for building and launching. I will summarize the advantages of this new approach, justifying the additional lines of code in this patch: [1] Centralizing the code for building and launching plugins. [2] Extensive unit testing: this was not possible before. [3] Simplicity: Only define what you need in the plugin. A follow-up patch will delete 1500 lines with *empty* routines. [4] Make it possible to restore the warnings pragma for all plugins. New style plugins do no longer depend on redefinition. [5] Event binding moved from HTML attributes moved to jQuery code. This separates behavior and presentation. [6] Much more documentation, including EXAMPLE plugin in follow-up. [7] Improved error handling. [8] Usability: property noclick tells you if plugin's buttonDot is active. [9] More events supported: Change, keyboard/mouse events. See EXAMPLE. NOTE ON EXAMPLE PLUGIN: The example plugin is added in the third patch of this report. Since it is new style, it can be used only after we start using this object. It also contains an example for a keypress and mouseover event. NOTE ON ITEM PLUGINS: Old style plugins for items contain an additional parameter in the js functions for Blur, Focus and Change. This distinction has no actual use and is resolved for new plugins in the object code. When converting item plugins, this minor correction will be addressed. In the meantime old style item plugins behave as expected. TEST PLAN: Run the new test t/db_dependent/FrameworkPlugin.t At this point in time, you do not need to do anything more. Follow-up patches will incorporate the object in real-life Koha and provide additional test plans. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 37950 [details] [review] [PASSED QA] Bug 10480: Use the framework plugin object in cataloguing This patch implements the use of Koha::FrameworkPlugin in Cataloguing, Authorities, Acquisition, Serials and Tools. The main change is architectural: see the commit message of the previous patch. No changes in behavior are expected, but the support of new events may provide additional functionality in the future. Some small bugs are resolved along the way. The change primarily focuses on the MARC and items editor in Cataloguing. But the MARC editor for Authorities and the item editor in Acquisition, Serials and Tools are touched too. This commit message gives some comments per module. NOTE FOR CATALOGUING: A new plugin without popup (or other click event code) now shows the title No popup when hovering over the tag editor image. The image alerts the user on a plugin, the title tells about its status. The noclick property allows for further style modifications in the template. Note that a follow-up patch will clean up the old style plugins too with the same effect. Some additional code in cataloging.js makes it possible to clone subfields with plugins (although only theoretically useful). The clones use the same javascript functions but event.data contains an updated id. This effectively resolves bug 13306. Note that if old plugins do not use the javascript parameter for the id but the perl variable, cloning does still operate on the wrong field (with and without this patch set). In the absence of report 12176 in master, it is not yet necessary to modify additem.tt. When it gets pushed, it should be an easy rebase. New style item plugins will no longer need an extra parameter. (The code in the FrameworkPlugin object actually takes care of that.) NOTE FOR AUTHORITIES: This patch also adds class name tag_editor to the buttonDot anchors. This effectively makes the same tag editor image appear as in Cataloguing. Futhermore it removes the button from the tab sequence if there is no click event (really effective after conversion to the new style, since the old style plugins contain empty onclicks and launchers). Both small adjustments increase consistency between auth and bib edits. NOTE FOR ACQUISITION: In Acquisition two scripts use an item editor, but in a different way. The scripts addorderiso2709 and neworderempty both rely on the routine PrepareItemrecordDisplay in C4::Items, but neworderempty creates item blocks dynamically via an ajax call to services/itemrecorddisplay.pl. In order to make the dynamic item blocks work with plugins, some code changes were needed in additem.js. (Normally the event binding is done at document ready time; now it must be done later.) At this moment the routine in Items.pm contains the html tags, and this makes changes to the following templates not necessary for now: * acqui/addorderiso2709.tt * services/itemrecorddisplay.tt Report 13397 has been opened to address moving the html to the templates. NOTE FOR SERIALS: Script serial-edit relies also on C4::Items (just as in Acquisition). This makes changes to serials/serials-edit.tt not necessary for now. NOTE FOR TOOLS: The current code in tools/batchMod.pl allows the use of plugins for batch modification of items. This patch just converts that code to use the new object. Most item plugins however may not be very useful for operating on multiple items at once. PERFORMANCE: I have benchmarked build_tabs in addbiblio to see how especially the additional processing of the javascript in the FrameworkPlugin object would impact performance. Testing default MARC21 framework with 8 plugins gave the following figures: - Old situation: 851 ms - New situation: 942 ms (+10,7%) - New situation after plugin cleanup: 881 ms (+3,4%) Note also that adding lines for event binding is compensated by removing lines for unused events. Page load should essentially be the same. TEST PLAN: Suggestion: If you also apply the next patch with the EXAMPLE plugin, you can test with a rather harmless plugin (with popup) on various places :) But your test should also include old style plugins, with[out] popups. If you want to test a new plugin without popup, rename/remove Click$id in the javascript code of the $builder definition (temporarily). [1] Test Cataloguing: - Add/Edit biblio. Try plugins with and without popup. - Add/Edit items. (EXAMPLE can be used as an item plugin with popup.) - Clone a subfield with plugin (use EXAMPLE): Verify that the plugin works on both original and clone with the respective field values. Is the value put back in the right field too? [2] Test Authorities: Edit an authority record. Try plugins with an without popup. [3] Test Acquisition: Set system preference AcqCreateItem to "placing an order". Check the item editor in the following two places: a- addorderiso2709: Open a basket, add an order from a staged file. Select a file, click Add orders, and go to tab Item information. b- neworderempty: Open a basket, add an order from a new empty record. [4] Test Serials: Check the item editor on serials-edit. Go to subscription detail. Click Receive. Choose "Click to add item". (Note that this subscription should create an item record when receiving this serial.) [5] Test Tools: Check the item editor for batch item modification. Enter a few valid barcodes and press Continue to reach the item editor. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 37951 [details] [review] [PASSED QA] Bug 10480: EXAMPLE plugin with associated template The EXAMPLE plugin helps you to create a new style framework plugin by providing a simple working example and additional documentation for developers. Test plan: [1] Connect the EXAMPLE plugin to one or more fields. [2] Try the following events: a- Focus: If the field is empty, it should put EXAMPLE: into it. b- MouseOver: If the field is empty and you move your mouse over it, it should have the same effect as Focus. c- Change: Edit the field in the editor and tab out of it, the color of the text should toggle (randomly) between red, green and blue. d- KeyPress: If you edit the field and you type @, it should give AT. e- Click: Click on the tag editor. Change the value in the popup. If you press OK, the field should be changed. [3] Would the documentation in the perl script help you to create a plugin? Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 37952 [details] [review] [PASSED QA] Bug 10480: Remove useless routines and irrelevant pod lines This patch makes the following cleanup actions for all plugins: 1- Remove sub plugin_parameters. This function is not used. 2- Remove empty plugin or empty plugin_javascript subs. 3- Remove empty Blur, Clic or Focus routines from javascript. 4- Remove pod lines (copy-pasta) only referring to the 3 plugin subs. 5- Remove the last 1; line. It is no longer needed. Test plan: Run the unit test t/db_dependent/FrameworkPlugins.t Test some cataloguing plugins in addbiblio. Test some item plugins in additem and neworderempty. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> I also checked the syntax of all plugins with perl -c. And checked the pod (if any) with podchecker: Two files still produce a warning; the follow-up deals with them: unimarc_field_686a/_700-4.pl. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 37953 [details] [review] [PASSED QA] Bug 10480: Follow-up for unimarc_field_686a/_700-4.pl This patch fixes the pod errors on those two files by removing the pod section that actually does not add anything useful. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 37983 [details] [review] Bug 10480: (QA followup) hide the EXAMPLE.pl plugin The EXAMPLE.pl plugin remains on the source tree for documentation purposes only, and shouldn't be listed when editing both biblio and authority cataloguing frameworks. Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Enhancement pushed to master. Great work Marcel!
(In reply to Tomás Cohen Arazi from comment #51) > Enhancement pushed to master. And thanks for your follow-up.
I found a bug: 1/ Open the console 2/ Go on the new order page 3/ TypeError: events is null for(i=0; i<events.length; i++) { additem.js (ligne 176, col. 13) I suppose it happens when none of the fields use a plugin
(In reply to Jonathan Druart from comment #53) > I found a bug: > 1/ Open the console > 2/ Go on the new order page > 3/ TypeError: events is null > for(i=0; i<events.length; i++) { > additem.js (ligne 176, col. 13) > > I suppose it happens when none of the fields use a plugin Thanks for reporting. See bug 14327.