Bug 13437 - Convert marc21 framework plugins to new style
Summary: Convert marc21 framework plugins to new style
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-12-11 11:45 UTC by Marcel de Rooy
Modified: 2016-12-05 21:23 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 13437: Perltidy marc21_linking_section (22.58 KB, patch)
2015-05-12 15:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13437: Preliminary changes for marc21 plugins field 008 (3.93 KB, patch)
2015-05-12 15:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13437: Perltidy some plugin files before conversion (46.80 KB, patch)
2015-05-13 15:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13437: Preliminary changes for marc21 plugins field 008 (3.93 KB, patch)
2015-05-13 15:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13437: Trivial edits on marc21 plugins before conversion (8.99 KB, patch)
2015-05-13 15:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13437: Conversion of marc21 cataloguing plugins to new style (33.88 KB, patch)
2015-05-14 10:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13437: Replace javascript function parameter in builder (18.07 KB, patch)
2015-05-14 13:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13437: Preliminary changes for marc21 plugins field 008 (6.35 KB, patch)
2015-05-14 17:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13437: Trivial edits on marc21 plugins before conversion (8.89 KB, patch)
2015-05-14 17:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13437: Conversion of marc21 cataloguing plugins to new style (33.83 KB, patch)
2015-05-14 17:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13437: Replace javascript function parameter in builder (18.07 KB, patch)
2015-05-14 17:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13437: Perltidy some plugin files before conversion (46.88 KB, patch)
2015-05-21 18:55 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13437: Preliminary changes for marc21 plugins field 008 (6.42 KB, patch)
2015-05-21 18:55 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13437: Trivial edits on marc21 plugins before conversion (9.01 KB, patch)
2015-05-21 18:55 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13437: Conversion of marc21 cataloguing plugins to new style (33.91 KB, patch)
2015-05-21 18:55 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13437: Replace javascript function parameter in builder (18.03 KB, patch)
2015-05-21 18:55 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[PASSED QA] Bug 13437: Perltidy some plugin files before conversion (46.93 KB, patch)
2015-06-06 18:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13437: Preliminary changes for marc21 plugins field 008 (6.47 KB, patch)
2015-06-06 18:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13437: Trivial edits on marc21 plugins before conversion (9.11 KB, patch)
2015-06-06 18:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13437: Conversion of marc21 cataloguing plugins to new style (33.96 KB, patch)
2015-06-06 18:22 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13437: Replace javascript function parameter in builder (18.16 KB, patch)
2015-06-06 18:22 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2014-12-11 11:45:08 UTC

    
Comment 1 Marcel de Rooy 2015-05-12 15:34:17 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2015-05-12 15:57:23 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2015-05-12 16:15:42 UTC
Still more to come..Please wait
Comment 4 Marcel de Rooy 2015-05-13 15:03:57 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2015-05-13 15:04:00 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2015-05-13 15:04:04 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2015-05-13 15:04:48 UTC
Two more coming
Comment 8 Marcel de Rooy 2015-05-14 10:11:38 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2015-05-14 13:57:00 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2015-05-14 14:08:12 UTC
Ready for signoff.
This patch set can go on its own. The see also reports may bring some later refinements, but are not needed right now.
Comment 11 Marcel de Rooy 2015-05-14 17:38:16 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2015-05-14 17:38:21 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2015-05-14 17:38:25 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2015-05-14 17:38:28 UTC Comment hidden (obsolete)
Comment 15 Marcel de Rooy 2015-05-14 17:39:14 UTC
Second patch slightly adjusted.
Comment 16 Bernardo Gonzalez Kriegel 2015-05-21 18:55:12 UTC Comment hidden (obsolete)
Comment 17 Bernardo Gonzalez Kriegel 2015-05-21 18:55:19 UTC Comment hidden (obsolete)
Comment 18 Bernardo Gonzalez Kriegel 2015-05-21 18:55:25 UTC Comment hidden (obsolete)
Comment 19 Bernardo Gonzalez Kriegel 2015-05-21 18:55:33 UTC Comment hidden (obsolete)
Comment 20 Bernardo Gonzalez Kriegel 2015-05-21 18:55:40 UTC Comment hidden (obsolete)
Comment 21 Bernardo Gonzalez Kriegel 2015-05-21 18:57:11 UTC
Great work!
Comment 22 Katrin Fischer 2015-06-06 18:22:08 UTC
Created attachment 39949 [details] [review]
[PASSED QA] Bug 13437: Perltidy some plugin files before conversion

Preliminary work before converting to new plugin style. Several files still
contained tabs or had strange indentation.

Perltidied: marc21_linking_section.pl
Perltidied: marc21_field_007.pl
Perltidied: marc21_leader_authorities.pl
Perltidied: marc21_leader.pl
Perltidied: marc21_leader_book.pl
Perltidied: marc21_leader_computerfile.pl
Perltidied: marc21_leader_video.pl

Test plan:
Run perltidy -pro=xt/perltidyrc on marc21_linking_section and compare.
Try another one too :)

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Minor differences on comparison :)
No errors

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 23 Katrin Fischer 2015-06-06 18:22:12 UTC
Created attachment 39950 [details] [review]
[PASSED QA] Bug 13437: Preliminary changes for marc21 plugins field 008

Preliminary work on marc21_field_008.pl and 008_authorities.pl.
Moving $dateentered en $defaultval to lower scope level.
date_entered is a new sub (in a module); sub Field008 has been reduced to
one line.
Added a trivial unit test for date_entered.
Note: the format used in date_entered could be added in DateUtils, moving
this logic to a better place.

Test plan:
Use both plugins in the marc21 editor (biblios/authorities).
Run the adjusted unit test.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Both plugins works, no errors

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 24 Katrin Fischer 2015-06-06 18:22:15 UTC
Created attachment 39951 [details] [review]
[PASSED QA] Bug 13437: Trivial edits on marc21 plugins before conversion

marc21_field_003.pl: copy-pasta comment about date removed
marc21_field_005.pl: commented use removed
marc21_field_006.pl: old/irrelevant pod lines
marc21_field_007.pl: old/irrelevant pod lines
marc21_field_008.pl: old/irrelevant pod lines, move sub par line twice
marc21_field_008_authorities.pl: whitespace, old/irrelevant pod lines
marc21_field_040c.pl: two commented lines removed
marc21_field_040d.pl: whitespace, commented lines, old/irrelevant pod
marc21_field_245h.pl: whitespace

marc21_linking_section.pl: relocated some comment lines, and replaced a new
CGI object by the one passed in via the plugin launcher (agreed, this may
not be so trivial as the other changes)

Test plan:
These (trivial) changes are hard to test.
Pick a few plugins and verify that behavior is not changed in the marc
editor.
For the brave: Try marc21_linking_section.pl.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Tried marc21_linking_section.pl :)
(ling plugin to 773$9, create new record, search for parent and check values inserted)
No errors

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 25 Katrin Fischer 2015-06-06 18:22:19 UTC
Created attachment 39952 [details] [review]
[PASSED QA] Bug 13437: Conversion of marc21 cataloguing plugins to new style

This patch converts marc21 plugins to new style by making the following
modifications:

[1] Replace use strict with use Modern::Perl. This implies that we now
re-enabled warnings. There are no redefine warnings anymore, but note that
we need to silence some warnings from individual plugins that were covered
by disabling the warnings pragma until now. Silencing these individual
warnings is outside the scope of this report.
[2] Sub plugin_javascript is replaced by an anonymous subroutine $builder.
[3] The parameters of $builder are combined in a params hashref. In most
cases we only need $params->{id} for the function name.
[4] Javascript function Clicxxx is renamed to Clickxxx.
[5] The builder does no longer return function_name.
[6] Sub plugin is replaced by subroutine $launcher.
[7] The parameters of $launcher are combined in a params hashref. We only
use $params->{cgi}. Mostly we save that to $input. One exception: $query.
[8] The plugins returns a hash with $builder and/or $launcher.

Test plan:
[1] Run t/db_dependent/FrameworkPlugin.t -incl cataloguing/value_builder/
marc21*.pl. This should catch compile errors and general problems when
building or launching these plugins.
NOTE: You will see several initialize warnings from individual plugins that
were hidden until now by disabling warnings. This is fine; we will be
able to address these warnings now on new reports.
[2] Check behavior of several plugins in the marc editor.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Checked all marc21 plugins. Attached unused plugins to some field.
Some plugins (unused by default) may need some further attention, but
also outside the scope of this report.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
New warnigs, but all seems to work.
No errors.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 26 Katrin Fischer 2015-06-06 18:22:22 UTC
Created attachment 39953 [details] [review]
[PASSED QA] Bug 13437: Replace javascript function parameter in builder

The builder contains js functions with a parameter like subfield_managed
or i or something similar. This parameter contains the html id of the
field corresponding with the plugin.
With the functionality of Koha::FrameworkPlugin in place, we can eliminate
one js function call and get the same id via the event passed in. Note
that this actually makes the function a 'real' event handler.

Also note that in many cases this parameter was not used but the id
was borrowed from a perl variable like $params->{id}. If the field is not
cloned, this is not a problem. But some fields can be cloned and should
not use the static perl value but should get it from the event.

Test plan:
Look for js errors when loading the marc editor.
Since the Focus or Click event code has been touched for most marc21
plugins, move your cursor into the field or click on the tag editor
button. Verify that the focus event updates the correct field or the
click event correctly launches the plugin AND the value comes back into
the right field.
Bonus: Attach a plugin with popup (like leader) to 040$d. Clone this field.
Verify that the two launch buttons operate on the correct value.
(This resolves a current bug.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Checked all 15 plugins.
Plugin marc21_linking_section seems to work, but I could not get it to pass
back something useful into my field. (Same without this patch.)
Tested the clone button with leader on 040d.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
All seems to work, no errors

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 27 Tomás Cohen Arazi 2015-06-10 19:04:57 UTC
Patches pushed to master.

Thanks Marcel!