Bug 11247 - Fix calls of TransformHtmlToXml
Summary: Fix calls of TransformHtmlToXml
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Srdjan Jankovic
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-14 05:05 UTC by Srdjan Jankovic
Modified: 2016-06-21 21:38 UTC (History)
6 users (show)

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


Attachments
bug_11247: Removed unused params from TransformHtmlToXml() (13.93 KB, patch)
2013-11-14 06:19 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11247: Removed unused params from TransformHtmlToXml() (13.92 KB, patch)
2013-11-14 06:23 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_11247: Removed unused params from TransformHtmlToXml() (13.86 KB, patch)
2013-11-14 23:27 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 11247: Removed unused params from TransformHtmlToXml() (13.93 KB, patch)
2015-08-25 12:55 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 11247: Fix calls of TransformHtmlToXml (4.21 KB, patch)
2015-08-26 12:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11247: Add a simple unit test for TransformHtmlToXml (1.55 KB, patch)
2015-08-26 12:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11247: Add a simple unit test for TransformHtmlToXml (1.73 KB, patch)
2015-08-28 15:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11247: Fix calls of TransformHtmlToXml (4.23 KB, patch)
2015-09-01 08:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11247: Add a simple unit test for TransformHtmlToXml (1.75 KB, patch)
2015-09-01 08:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11247: Improve tests (4.20 KB, patch)
2015-09-02 14:07 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 Srdjan Jankovic 2013-11-14 05:05:36 UTC
Indicator and indicator_tag are placed in revers order when making TransformHtmlToXml() call. It cannot be spotted, because indicators are empty anyway. It generates warnings in the log file.
Comment 1 Srdjan Jankovic 2013-11-14 06:19:45 UTC Comment hidden (obsolete)
Comment 2 Srdjan Jankovic 2013-11-14 06:23:49 UTC Comment hidden (obsolete)
Comment 3 Mark Tompsett 2013-11-14 14:06:12 UTC
use Data::Dumper; die Dumper($itemhash{$item});
I don't think so. Sorry.
Comment 4 Srdjan Jankovic 2013-11-14 23:27:17 UTC Comment hidden (obsolete)
Comment 5 Srdjan Jankovic 2013-11-14 23:29:18 UTC
Omg, what a blunder...
Comment 6 Marcel de Rooy 2014-02-12 13:43:47 UTC
Just some question from glancing through code:

How do the following remaining statements interact? 
A call somewhere like:
    my $xml = TransformHtmlToXml(\@tags,\@subfields,\@values, 'ITEM');
with the first lines from the routine:
    my ( $tags, $subfields, $values, $auth_type ) = @_;
    my $xml = MARC::File::XML::header('UTF-8');
    $xml .= "<record>\n";
    $auth_type = C4::Context->preference('marcflavour') unless $auth_type;
    MARC::File::XML->default_record_format($auth_type);
Note that MARC::File::XML->default_record_format does not expect ITEM but only:
Valid formats are MARC21, USMARC, UNIMARC and UNIMARCAUTH.
You could argue that this is outside the scope of this report. But you are removing parameters and not just touching a few lines here of the routine? 

It seems that this routine is only used in items now. The pod refers to unimarc authority records too. Perhaps it is not used anymore. I have the impression that your changes are somewhat rigorous and could cut off future use in a broader sense (biblio or auth instead of item only)? Please note that I do not oppose to remove them, I am just triggering a discussion..

A more elaborated test plan and even some unit tests now for the TransformHtmlToXml routine could further help to get this patch further in the process.

Thanks for your attention.
Comment 7 Mark Tompsett 2014-03-21 03:57:44 UTC
Clearly parameters may come and go. Perhaps swapping to a hashref for the parameters might be an idea?
Comment 8 Mark Tompsett 2014-03-21 04:09:42 UTC
It would seem that bug 6875 introduced function(s) [find_value] similar to what you are chopping out. I don't think chopping it out is the correct solution.

I was playing with editing MARC and using the Tag Editor for some subfields, and the indicators are always blanked. This may be in part because this function isn't used like it should be. Just some related thoughts.
Comment 9 Mark Tompsett 2015-08-25 12:55:36 UTC Comment hidden (obsolete)
Comment 10 Mark Tompsett 2015-08-25 19:08:04 UTC
I have put this to 'In Discussion', since srdjan@catalyst.net.nz hasn't responded to the comments made in comments #6, #7, and #8.
Comment 11 Srdjan Jankovic 2015-08-25 23:32:45 UTC
(In reply to M. Tompsett from comment #10)
> I have put this to 'In Discussion', since srdjan@catalyst.net.nz hasn't
> responded to the comments made in comments #6, #7, and #8.

Sorry, I did not know what to do with it. My intention was to correct something that I found obviusly wrong when doing some other work. I did not want to go into changing the way things work.
I'm happy for this to be dropped, but the fact stays that the code as it is now is wrong. It works just because it is being called in particular way.
Comment 12 Mark Tompsett 2015-08-26 01:15:49 UTC
I'm not sure indicators are working properly in all contexts, but I don't have the time nor energy to test. *sigh*
Comment 13 Marcel de Rooy 2015-08-26 12:52:35 UTC
(In reply to Srdjan Jankovic from comment #11)
> Sorry, I did not know what to do with it. My intention was to correct
> something that I found obviusly wrong when doing some other work. I did not
> want to go into changing the way things work.
> I'm happy for this to be dropped, but the fact stays that the code as it is
> now is wrong. It works just because it is being called in particular way.

I amended your path in order to not lose everything including your remarks about wrong calls and a param not being used. (Adding a comment)
I switched the params at three places where they were in reverse order.
Added a trivial unit test.

Since the indicator param works fine, I favored leaving it here.
Comment 14 Marcel de Rooy 2015-08-26 12:53:26 UTC Comment hidden (obsolete)
Comment 15 Marcel de Rooy 2015-08-26 12:53:29 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2015-08-28 13:58:47 UTC
% sudo service mysql stop

% prove t/Biblio_TransformHtmlToXml.t
t/Biblio_TransformHtmlToXml.t .. DBI connect('dbname=koha_ut;host=localhost;port=3306','koha_',...) failed: Can't connect to local MySQL server through socket '/var/run/mysqld/mysqld.sock' (2) at /usr/share/perl5/DBIx/Connector.pm line 32.


We already have a t/Biblio/TransformKohaToMarc.t, it makes much more sense to put this new file in the t/Biblio dir, don't you think?
Comment 17 Marcel de Rooy 2015-08-28 15:25:42 UTC
(In reply to Jonathan Druart from comment #16)
> % sudo service mysql stop
> 
> % prove t/Biblio_TransformHtmlToXml.t
> t/Biblio_TransformHtmlToXml.t .. DBI
> connect('dbname=koha_ut;host=localhost;port=3306','koha_',...) failed: Can't
> connect to local MySQL server through socket '/var/run/mysqld/mysqld.sock'
> (2) at /usr/share/perl5/DBIx/Connector.pm line 32.

Oops, forgot that it checks a preference..
 
> We already have a t/Biblio/TransformKohaToMarc.t, it makes much more sense
> to put this new file in the t/Biblio dir, don't you think?

Will move it to t/db_dependent somewhere..
Comment 18 Marcel de Rooy 2015-08-28 15:30:35 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2015-09-01 08:49:48 UTC
Created attachment 42137 [details] [review]
Bug 11247: Fix calls of TransformHtmlToXml

The ind_tag of TransformHtmlToXml is unused.
Some calls to this function incorrectly revert indicator and ind_tag (which
is not a problem when both are empty..)

Patch of Srdjan Jankovic, amended and signed off by Marcel de Rooy.

The following calls are fixed:
call in acqui/addorder.pl: switched indicator with ind_tag
call in acqui/addorderiso2709.pl replaced too
acqui/finishreceive.pl replaced too

These calls are fine:
two calls in cataloguing/additem.pl are fine
call in serials/serials-edit.pl is fine
call in tools/batchMod.pl is fine

The folllow-up patch adds a simple unit test.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
With AcqCreateItem=='placing an order', tested if adding an order still
worked (covered both addorder.pl and addorderiso2709.pl).

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 20 Jonathan Druart 2015-09-01 08:49:54 UTC
Created attachment 42138 [details] [review]
Bug 11247: Add a simple unit test for TransformHtmlToXml

The unit test is db dependent (as Jonathan correctly remarked),
since the sub checks a preference.

Test plan:
Run t/db_dependent/Biblio/TransformHtmlToXml.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 21 Jonathan Druart 2015-09-02 14:07:56 UTC
Created attachment 42226 [details] [review]
Bug 11247: Improve tests

This patch makes the tests non dependent on the DB and test the 3 marc
flavours.
Comment 22 Tomás Cohen Arazi 2015-09-02 17:47:20 UTC
Patches pushed to master.

Thanks guys!
Comment 23 Marcel de Rooy 2015-09-03 07:42:51 UTC
(In reply to Jonathan Druart from comment #21)
> This patch makes the tests non dependent on the DB and test the 3 marc
> flavours.
Thx for doing so.
Comment 24 Chris Cormack 2015-09-22 01:33:41 UTC
Pushed to 3.20.x will be in 3.20.4
Comment 25 Liz Rea 2015-09-22 02:23:38 UTC
pushed to 3.18.x will be in 3.18.11.