[Bug 11247] New: Wrong param order in TransformHtmlToXml() call
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 Bug ID: 11247 Summary: Wrong param order in TransformHtmlToXml() call Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Acquisitions Assignee: koha-bugs@lists.koha-community.org Reporter: srdjan@catalyst.net.nz QA Contact: testopia@bugs.koha-community.org 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |srdjan@catalyst.net.nz |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 --- Comment #1 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 22923 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22923&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl Component|Acquisitions |Cataloging Summary|Wrong param order in |Remove unused params from |TransformHtmlToXml() call |TransformHtmlToXml() -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #22923|0 |1 is obsolete| | --- Comment #2 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 22924 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22924&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA CC| |mtompset@hotmail.com --- Comment #3 from M. Tompsett <mtompset@hotmail.com> --- use Data::Dumper; die Dumper($itemhash{$item}); I don't think so. Sorry. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #22924|0 |1 is obsolete| | --- Comment #4 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 22953 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22953&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 --- Comment #5 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Omg, what a blunder... -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 --- Comment #6 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 --- Comment #7 from M. Tompsett <mtompset@hotmail.com> --- Clearly parameters may come and go. Perhaps swapping to a hashref for the parameters might be an idea? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 --- Comment #8 from M. Tompsett <mtompset@hotmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=13180 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=11493 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 --- Comment #9 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 41911 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41911&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #22953|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |In Discussion --- Comment #10 from M. Tompsett <mtompset@hotmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 --- Comment #11 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 --- Comment #12 from M. Tompsett <mtompset@hotmail.com> --- I'm not sure indicators are working properly in all contexts, but I don't have the time nor energy to test. *sigh* -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #41911|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 41971 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41971&action=edit 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). -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 --- Comment #15 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 41972 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41972&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Remove unused params from |Fix calls of |TransformHtmlToXml() |TransformHtmlToXml -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |minor -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org Status|Signed Off |Failed QA --- Comment #16 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- % 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? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 --- Comment #17 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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.. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #41972|0 |1 is obsolete| | --- Comment #18 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 42088 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42088&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #41971|0 |1 is obsolete| | Attachment #42088|0 |1 is obsolete| | --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 42137 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42137&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 --- Comment #20 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 42138 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42138&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 --- Comment #21 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 42226 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42226&action=edit Bug 11247: Improve tests This patch makes the tests non dependent on the DB and test the 3 marc flavours. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com Status|Passed QA |Pushed to Master --- Comment #22 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Patches pushed to master. Thanks guys! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 --- Comment #23 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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.
-- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable CC| |chris@bigballofwax.co.nz --- Comment #24 from Chris Cormack <chris@bigballofwax.co.nz> --- Pushed to 3.20.x will be in 3.20.4 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11247 Liz Rea <liz@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |liz@catalyst.net.nz --- Comment #25 from Liz Rea <liz@catalyst.net.nz> --- pushed to 3.18.x will be in 3.18.11. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org