Bug 13438 - Convert normarc framework plugins to new style (see also 10480)
Summary: Convert normarc framework plugins to new style (see also 10480)
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-12-11 11:45 UTC by Marcel de Rooy
Modified: 2017-01-03 15:39 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 13438: Convert normarc framework plugins to new style (16.46 KB, patch)
2016-04-03 14:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13438: Follow-up for removing date_entered (1.22 KB, patch)
2016-04-03 14:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13438: Fix report function in normarc_field_008 js (3.67 KB, patch)
2016-05-12 09:27 UTC, Marcel de Rooy
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:40 UTC

    
Comment 1 Marcel de Rooy 2016-04-03 14:41:27 UTC
Created attachment 49823 [details] [review]
Bug 13438: Convert normarc framework plugins to new style

Converts NORMARC plugins to new style (with builder and launcher).
See also bugs 10480 and 13437.

Note: Search.pm is not needed in these plugins. Tabs have been removed in
the same pass.

Note 2: For the 008 plugin the tag editor already seems to have some
problems (js errors on e.g. f18 for Velg materialtype, etc.) This makes
that pressing OK does not always work. Outside the scope of this report.

Test plan:
[1] Run t/db_dependent/FrameworkPlugin.t -incl
    cataloguing/value_builder/normarc*.pl. (Ignore substr and uninitialized
    warnings; these are not in the scope of this patch.)
[2] Connect the plugin. Test focus (leader,008) and click event.
    (See note 2.)
[3] Look for warnings in the log related(!) to this change.
Comment 2 Marcel de Rooy 2016-04-03 14:43:16 UTC
Oops Subroutine date_entered redefined at /usr/share/koha/masterclone/cataloguing/value_builder/normarc_field_008.pl line 29.
Comment 3 Marcel de Rooy 2016-04-03 14:48:42 UTC
Created attachment 49824 [details] [review]
Bug 13438: Follow-up for removing date_entered

Defining this sub in normarc_field_008.pl is not necessary.
Use the one from Koha::Util::FrameworkPlugin.

Test plan:
Run the 008 plugin again.
Comment 4 Mark Tompsett 2016-04-06 00:40:39 UTC
Comment on attachment 49823 [details] [review]
Bug 13438: Convert normarc framework plugins to new style

Review of attachment 49823 [details] [review]:
-----------------------------------------------------------------

::: cataloguing/value_builder/normarc_field_007.pl
@@ +88,1 @@
>  #FIXME:  There is probably a more-elegant way to do this!

You mean something like:

use Data::Dumper;
my $str='abcdef';
my $len=22-length $str;
@tmp = split //,$str;
push @tmp,(undef)x($len);
print Dumper(\@tmp);"

And some sort of my @field_numbers = map { "f$_" } (0..21);

Then use a loop to template->param it all?
Comment 5 Marcel de Rooy 2016-04-07 07:16:27 UTC
(In reply to M. Tompsett from comment #4)
> Comment on attachment 49823 [details] [review] [review]
> Bug 13438: Convert normarc framework plugins to new style
> 
> Review of attachment 49823 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: cataloguing/value_builder/normarc_field_007.pl
> @@ +88,1 @@
> >  #FIXME:  There is probably a more-elegant way to do this!

Marc,
This is existing code (also in marc21_field_007). Probably only touched here due to preceding tabs.

Fixing this is not in the scope of this report. If there is no report yet, we should open a new one.
Btw I am not sure if we can still reproduce it. (Just tried a few combinations of f5,6,7 in normarc.)
Comment 6 Magnus Enger 2016-04-21 19:16:52 UTC
I might be missing something, but when I run the test thingy I get errors:

$ sudo koha-shell -c "prove t/db_dependent/FrameworkPlugin.t -incl cataloguing/value_builder/normarc*.pl" kohadev
[sudo] password for magnus: 
Feil. Prøv igjen.
[sudo] password for magnus: 
t/db_dependent/FrameworkPlugin.t ................ 1/5 substr outside of string at /home/magnus/scripts/kohaclone/cataloguing/value_builder/marc21_field_007.pl line 73.
substr outside of string at /home/magnus/scripts/kohaclone/cataloguing/value_builder/marc21_field_007.pl line 74.
[...]
substr outside of string at /home/magnus/scripts/kohaclone/cataloguing/value_builder/marc21_field_007.pl line 92.
Use of uninitialized value $f3 in concatenation (.) or string at /home/magnus/scripts/kohaclone/cataloguing/value_builder/marc21_field_007.pl line 99.
[...]
Use of uninitialized value $f22 in concatenation (.) or string at /home/magnus/scripts/kohaclone/cataloguing/value_builder/marc21_field_007.pl line 99.
oAuth error: Unsupported Use attribute (114) authtype Bib-1
DBIx::Class::Schema::resultset(): resultset() expects a source name at /usr/share/koha/lib/Koha/Objects.pm line 237
    # Child (Test05 -- tests with build and launch for default plugins) exited without calling finalize()
t/db_dependent/FrameworkPlugin.t ................ 5/5 
#   Failed test 'Test05 -- tests with build and launch for default plugins'
#   at /usr/local/share/perl/5.18.2/Test/Builder.pm line 279.
# Looks like you failed 1 test of 5.
# Looks like your test exited with 255 just after 5.
t/db_dependent/FrameworkPlugin.t ................ Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 1/5 subtests 
Cannot detect source of '-incl'! at /usr/local/share/perl/5.18.2/TAP/Parser/IteratorFactory.pm line 261.
	TAP::Parser::IteratorFactory::detect_source('TAP::Parser::IteratorFactory=HASH(0x27879b0)', 'TAP::Parser::Source=HASH(0x2787aa0)') called at /usr/local/share/perl/5.18.2/TAP/Parser/IteratorFactory.pm line 211
	TAP::Parser::IteratorFactory::make_iterator('TAP::Parser::IteratorFactory=HASH(0x27879b0)', 'TAP::Parser::Source=HASH(0x2787aa0)') called at /usr/local/share/perl/5.18.2/TAP/Parser.pm line 470
	TAP::Parser::_initialize('TAP::Parser=HASH(0x2787ae8)', 'HASH(0x25da478)') called at /usr/local/share/perl/5.18.2/TAP/Object.pm line 55
	TAP::Object::new('TAP::Parser', 'HASH(0x25da478)') called at /usr/local/share/perl/5.18.2/TAP/Object.pm line 130
	TAP::Object::_construct('TAP::Harness=HASH(0x22f38e8)', 'TAP::Parser', 'HASH(0x25da478)') called at /usr/local/share/perl/5.18.2/TAP/Harness.pm line 852
	TAP::Harness::make_parser('TAP::Harness=HASH(0x22f38e8)', 'TAP::Parser::Scheduler::Job=HASH(0x25da658)') called at /usr/local/share/perl/5.18.2/TAP/Harness.pm line 651
	TAP::Harness::_aggregate_single('TAP::Harness=HASH(0x22f38e8)', 'TAP::Parser::Aggregator=HASH(0x23f7770)', 'TAP::Parser::Scheduler=HASH(0x25da6d0)') called at /usr/local/share/perl/5.18.2/TAP/Harness.pm line 743
	TAP::Harness::aggregate_tests('TAP::Harness=HASH(0x22f38e8)', 'TAP::Parser::Aggregator=HASH(0x23f7770)', 't/db_dependent/FrameworkPlugin.t', '-incl', 'cataloguing/value_builder/normarc_field_007.pl', 'cataloguing/value_builder/normarc_field_008.pl', 'cataloguing/value_builder/normarc_leader.pl') called at /usr/local/share/perl/5.18.2/TAP/Harness.pm line 558
	TAP::Harness::__ANON__() called at /usr/local/share/perl/5.18.2/TAP/Harness.pm line 571
	TAP::Harness::runtests('TAP::Harness=HASH(0x22f38e8)', 't/db_dependent/FrameworkPlugin.t', '-incl', 'cataloguing/value_builder/normarc_field_007.pl', 'cataloguing/value_builder/normarc_field_008.pl', 'cataloguing/value_builder/normarc_leader.pl') called at /usr/local/share/perl/5.18.2/App/Prove.pm line 546
	App::Prove::_runtests('App::Prove=HASH(0x1e852c8)', 'HASH(0x22c42f0)', 't/db_dependent/FrameworkPlugin.t', '-incl', 'cataloguing/value_builder/normarc_field_007.pl', 'cataloguing/value_builder/normarc_field_008.pl', 'cataloguing/value_builder/normarc_leader.pl') called at /usr/local/share/perl/5.18.2/App/Prove.pm line 504
	App::Prove::run('App::Prove=HASH(0x1e852c8)') called at /usr/local/bin/prove line 12

I have replaced some repeated errors with [...]

This is on a gitified package install.
Comment 7 Marcel de Rooy 2016-04-22 06:20:56 UTC
Thanks Magnus for testing.
You discovered that prove does not like extra arguments.
"Cannot detect source of '--incl'! at /usr/share/perl/5.14/TAP/Parser/IteratorFactory.pm line 263"

Prove provides a way (I learned now too):
It is possible to supply arguments to tests. To do so separate them from prove's own arguments with the arisdottle, '::'. For example prove -v t/mytest.t :: --url http://example.com

Also note that the overwhelming number of uninitialized warnings was only hidden by disabling the warnings pragma. If you would only add "use warnings" in the plugins before this patch and run prove, you will see them all. So now we see those warnings, we should do something about them.. (I already have a report opened for that.)

prove t/db_dependent/FrameworkPlugin.t :: --incl cataloguing/value_builder/normarc_*.pl
t/db_dependent/FrameworkPlugin.t .. 1/5
[LOTs of uninitialized warnings]
t/db_dependent/FrameworkPlugin.t .. ok
All tests successful.
Files=1, Tests=5,  2 wallclock secs ( 0.03 usr  0.01 sys +  2.08 cusr  0.10 csys =  2.22 CPU)
Result: PASS
Comment 8 Bernardo Gonzalez Kriegel 2016-05-02 17:52:29 UTC
Hi,
testing loading no-NB frameworks.

Leader and 007 seems right, 007 gives warnings on log file.
Buy 008 does not save, only resets the value.
Comment 9 Marcel de Rooy 2016-05-03 11:54:25 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #8)
> Hi,
> testing loading no-NB frameworks.
> 
> Leader and 007 seems right, 007 gives warnings on log file.
> Buy 008 does not save, only resets the value.

Thx for testing.
I have the same problem with 008 (as noted on the commit message), but it is not related to this patch. You have the same problem before this patch.
Perhaps Magnus can clarify or open a new report for it ?
Comment 10 Bernardo Gonzalez Kriegel 2016-05-03 12:20:00 UTC
(In reply to Marcel de Rooy from comment #9)
> I have the same problem with 008 (as noted on the commit message), but it is
> not related to this patch. You have the same problem before this patch.

Not exactly the same, it fails to load current values but can save new ones.
With patch clicking Save does nothing, for me of course (i'm having doubts about my testing env)
Comment 11 Magnus Enger 2016-05-03 14:00:06 UTC
Yes, there is a problem with 008. My plan was to fix this here:
Bug 10997 - Modernize value_builders for NORMARC
but I have not found the time yet...
Comment 12 Marcel de Rooy 2016-05-12 09:27:23 UTC
Created attachment 51453 [details] [review]
Bug 13438: Fix report function in normarc_field_008 js

The report function did not deal well with undefined fields and
did not respect the length of all elements.

NOTE: This is a quick fix for something that is currently broken.
A next (larger) step would be to bring it back to the approach in
the marc21 counterpart.

Test plan:
Make sure that normarc_field_008.pl is connected to your field 008.
Click tag editor for 008, save some values and go back to tag editor.
Test different material types in the tag editor.
Comment 13 Marcel de Rooy 2017-01-03 15:38:58 UTC
Have been asking assistance for months. Abandoning this development.