Bug 14201 - Remove unused code or template from some MARC21 leader plugins
Summary: Remove unused code or template from some MARC21 leader plugins
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-14 10:19 UTC by Marcel de Rooy
Modified: 2017-09-14 19:03 UTC (History)
5 users (show)

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


Attachments
Bug 14201: Plugin marc21_leader_book is useless (17.33 KB, patch)
2015-07-14 16:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14201: We can do without plugin marc21_leader_computerfile (15.96 KB, patch)
2015-07-14 16:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14201: Yes, goodbye to the third plugin marc21_leader_video too (4.55 KB, patch)
2015-07-14 16:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14201: Plugin marc21_leader_book is useless (17.32 KB, patch)
2015-07-14 16:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14201: We can do without plugin marc21_leader_computerfile (15.96 KB, patch)
2015-07-14 16:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14201: Yes, goodbye to the third plugin marc21_leader_video too (4.55 KB, patch)
2015-07-14 16:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED-OFF]Bug 14201: Plugin marc21_leader_book is useless (17.35 KB, patch)
2015-09-03 21:32 UTC, Héctor Eduardo Castro Avalos
Details | Diff | Splinter Review
[SIGNED-OFF]Bug 14201: We can do without plugin marc21_leader_computerfile (15.99 KB, patch)
2015-09-03 21:32 UTC, Héctor Eduardo Castro Avalos
Details | Diff | Splinter Review
[SIGNED-OFF]Bug 14201: Yes, goodbye to the third plugin marc21_leader_video too (4.58 KB, patch)
2015-09-03 21:33 UTC, Héctor Eduardo Castro Avalos
Details | Diff | Splinter Review
Bug 14201: Plugin marc21_leader_book is useless (17.41 KB, patch)
2015-09-17 14:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14201: We can do without plugin marc21_leader_computerfile (16.04 KB, patch)
2015-09-17 14:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14201: Yes, goodbye to the third plugin marc21_leader_video too (4.64 KB, patch)
2015-09-17 14:18 UTC, Jonathan Druart
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 2015-05-14 10:19:53 UTC

    
Comment 1 Marcel de Rooy 2015-05-14 10:23:38 UTC
Some of the marc21 leader plugins refer to leader.pl and include unused code. Have to check also if attached template is used at all.

Pertains to:
marc21_leader_book.pl
marc21_leader_computerfile.pl
marc21_leader_video.pl
Comment 2 Marcel de Rooy 2015-07-14 16:19:52 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2015-07-14 16:35:13 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2015-07-14 16:50:45 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2015-07-14 16:58:05 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2015-07-14 16:58:09 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2015-07-14 16:58:12 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2015-07-14 16:58:39 UTC
Just a small change in the first commit message. accuracy++
Comment 9 Héctor Eduardo Castro Avalos 2015-09-03 21:23:47 UTC
Hi Marcel you're right, the three perl scripts/templates do nothing or do the same thing, but the magic of having too many plugins perhaps is for filling different data in the frameworks. i.e., books only uses "06 - Type of record: a - Language material" but computer files uses "m - Computer file" and videos uses "g - Projected medium"

With this you save time for experimented catalogers. The data in the leader is predefined and she or he doesn't need to fill at all. Just by the fact to choose the proper framework is enough.

Maybe you can do a trick in your plugin to catch the framework code and redefine the leader for the type of resource/material.

Regards
Comment 10 Héctor Eduardo Castro Avalos 2015-09-03 21:32:37 UTC Comment hidden (obsolete)
Comment 11 Héctor Eduardo Castro Avalos 2015-09-03 21:32:52 UTC Comment hidden (obsolete)
Comment 12 Héctor Eduardo Castro Avalos 2015-09-03 21:33:06 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2015-09-14 11:50:14 UTC
(In reply to Héctor Eduardo Castro Avalos from comment #9)
> Hi Marcel you're right, the three perl scripts/templates do nothing or do
> the same thing, but the magic of having too many plugins perhaps is for
> filling different data in the frameworks. i.e., books only uses "06 - Type
> of record: a - Language material" but computer files uses "m - Computer
> file" and videos uses "g - Projected medium"
> 
> With this you save time for experimented catalogers. The data in the leader
> is predefined and she or he doesn't need to fill at all. Just by the fact to
> choose the proper framework is enough.
> 
> Maybe you can do a trick in your plugin to catch the framework code and
> redefine the leader for the type of resource/material.

Yes, you are right that this would be handy. And it probably was the idea when these files came up. But it did not work.
Note that this report does not change behavior.
So I am opening a new report to collect some ideas how to improve the default leader. Since we have the framework code already, we could pass that one to the plugin. When we add a preference, we could add leaders there per framework code. I would not favor hardcoding BKS BOOKS etc. in the plugin..

See bug 14815.
Comment 14 Jonathan Druart 2015-09-15 08:41:14 UTC
Are we sure they are useless?
Maybe there is a bug (the plugin_name parameter could be wrong) in them.

The differences are, for marc21_leader_computerfile vs marc21_leader:
  my $f2023 = substr( $result, 20, 4 );
  "f2023"   => $f2023,
These 2 lines exist in marc21_leader, no in marc21_leader_computerfile.

Could we ask for a functional point of view, to know if we should remove them or fix them?
Comment 15 Marcel de Rooy 2015-09-17 12:58:08 UTC
(In reply to Jonathan Druart from comment #14)
> Are we sure they are useless?
> Maybe there is a bug (the plugin_name parameter could be wrong) in them.
> 
> The differences are, for marc21_leader_computerfile vs marc21_leader:
>   my $f2023 = substr( $result, 20, 4 );
>   "f2023"   => $f2023,
> These 2 lines exist in marc21_leader, no in marc21_leader_computerfile.
> 
> Could we ask for a functional point of view, to know if we should remove
> them or fix them?

See also my commit messages. I checked it too. The computerfile is out of date.
The difference above is not really interesting. The positions 20-23 are hardcoded 4500 (see template too) and should not be changed at all.
Comment 16 Marcel de Rooy 2015-09-17 12:59:32 UTC
btw the leader template refers to pos 20-24 in the text
this should be 20-23 (see loc)
Comment 17 Jonathan Druart 2015-09-17 14:18:18 UTC
Created attachment 42675 [details] [review]
Bug 14201: Plugin marc21_leader_book is useless

A closer look at this plugin revealed the following:
[1] The leader_book builder redirects to leader.pl; the launcher code with
    leader_book.tt is dead.
[2] A diff of the two templates showed that most changes are minimal and
    irrelevant. Some updates to leader have not been incorporated into
    the unused leader_book.
[3] Leader_book does not use Focus while leader does.

This patch removes all traces of leader_book :)
It includes a tiny db revision for those brave people using it. The only
behavior change for them is the focus change.
Another small change: we do not need Search.pm in marc21_leader.

Test plan:
[1] Run the db revision. If you had some field attached to leader_book,
    it should be gone.
[2] Check if the leader plugin still works as expected.
[3] Run the FrameworkPlugin unit test. Ignore warnings from field_007.
[4] Git grep on marc21_leader_book. You will only find references to
    PO files in misc/translator.

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 18 Jonathan Druart 2015-09-17 14:18:23 UTC
Created attachment 42676 [details] [review]
Bug 14201: We can do without plugin marc21_leader_computerfile

A closer look at this plugin revealed the same things as the previous
patch for leader_book.
[1] Template leader_computerfile is not used.
[2] The unused template is not up-to-date either.
[3] Leader_computerfile does not use Focus while leader does.

This patch removes all traces of leader_computerfile.
It includes a tiny db revision for those brave people using it. The only
behavior change for them is the focus change.

Test plan:
[1] Run the db revision.
[2] Check if the leader plugin still works as expected.
[3] Git grep on marc21_leader_computerfile. Ignore PO files.

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 19 Jonathan Druart 2015-09-17 14:18:26 UTC
Created attachment 42677 [details] [review]
Bug 14201: Yes, goodbye to the third plugin marc21_leader_video too

A closer look at this plugin revealed the same things as earlier:
[1] Since the click redirects to leader, the launcher is not used.
    In this case no template file existed.
[2] Leader_video does not use Focus while leader does.

This patch removes 'all' traces of leader_video.
It includes a tiny db revision for those brave people using it. The only
behavior change for them is the focus change.

Test plan:
[1] Run the db revision.
[2] Check if the leader plugin still works as expected.
[3] Git grep on marc21_leader_video. Ignore atomicupdate. No PO files :)

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 20 Tomás Cohen Arazi 2015-09-28 17:55:34 UTC
Patch pushed to master.

Thanks Marcel!