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.
Created attachment 22923 [details] [review] bug_11247: Removed unused params from TransformHtmlToXml() $indicator and $ind_tag should not be TransformHtmlToXml() params. TransformHtmlToXml() is only used for items: - $indicator is always empty - $ind_tag was not even used in the function To test: Check that integrity of following functions is preserved: * Acquisition - ordering and receiving, needs setting AcqCreateItem to placing and receiving an order * Cataloging - create/modify an item * Batcih modify in Tools * Serials receiving
Created attachment 22924 [details] [review] bug_11247: Removed unused params from TransformHtmlToXml() $indicator and $ind_tag should not be TransformHtmlToXml() params. TransformHtmlToXml() is only used for items: - $indicator is always empty - $ind_tag was not even used in the function To test: Check that integrity of following functions is preserved: * Acquisition - ordering and receiving, needs setting AcqCreateItem to placing and receiving an order * Cataloging - create/modify an item * Batcih modify in Tools * Serials receiving
use Data::Dumper; die Dumper($itemhash{$item}); I don't think so. Sorry.
Created attachment 22953 [details] [review] bug_11247: Removed unused params from TransformHtmlToXml() $indicator and $ind_tag should not be TransformHtmlToXml() params. TransformHtmlToXml() is only used for items: - $indicator is always empty - $ind_tag was not even used in the function To test: Check that integrity of following functions is preserved: * Acquisition - ordering and receiving, needs setting AcqCreateItem to placing and receiving an order * Cataloging - create/modify an item * Batcih modify in Tools * Serials receiving
Omg, what a blunder...
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.
Clearly parameters may come and go. Perhaps swapping to a hashref for the parameters might be an idea?
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.
Created attachment 41911 [details] [review] Bug 11247: Removed unused params from TransformHtmlToXml() $indicator and $ind_tag should not be TransformHtmlToXml() params. TransformHtmlToXml() is only used for items: - $indicator is always empty - $ind_tag was not even used in the function To test: Check that integrity of following functions is preserved: * Acquisition - ordering and receiving, needs setting AcqCreateItem to placing and receiving an order * Cataloging - create/modify an item * Batcih modify in Tools * Serials receiving
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.
(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.
I'm not sure indicators are working properly in all contexts, but I don't have the time nor energy to test. *sigh*
(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.
Created attachment 41971 [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).
Created attachment 41972 [details] [review] Bug 11247: Add a simple unit test for TransformHtmlToXml Test plan: Run t/Biblio_TransformHtmlToXml.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
% 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?
(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..
Created attachment 42088 [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>
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>
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>
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.
Patches pushed to master. Thanks guys!
(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.
Pushed to 3.20.x will be in 3.20.4
pushed to 3.18.x will be in 3.18.11.