Bug 17726 - TestBuilder's refactoring removed default values
Summary: TestBuilder's refactoring removed default values
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 17727 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-12-06 09:50 UTC by Jonathan Druart
Modified: 2017-02-22 23:05 UTC (History)
4 users (show)

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


Attachments
Bug 17726: TestBuilder - Add default values (2.75 KB, patch)
2016-12-13 11:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17726: TestBuilder - Add default values (2.84 KB, patch)
2016-12-13 13:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17726: [QA Follow-up] Add test descriptions (1.16 KB, patch)
2016-12-13 13:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17726: biblioitems.marc has been moved to biblio_metadata.metadata (755 bytes, patch)
2017-01-20 09:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17726: biblioitems.marc has been removed (738 bytes, patch)
2017-01-20 11:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17726: biblioitems.marc has been removed (836 bytes, patch)
2017-01-20 12:12 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 Jonathan Druart 2016-12-06 09:50:23 UTC
Reading the commit message of bug 16155, it was expected to remove the default values for given fields.
But actually I think we should reintroduce it.
Comment 1 Jonathan Druart 2016-12-06 09:57:47 UTC
See bug 17727 for a use case.
Comment 2 Marcel de Rooy 2016-12-06 12:59:24 UTC
The way is was hardcoded in the past, was not particularly attractive; removal made the code simpler. You can always pass a value via the value parameter. In this specific case of marcxml field, we should probably call a routine that ensures the consistency between the marcxml and related fields (outside the namespace of TestBuilder).

That said, I would not mind reintroducing a (better) default..
Comment 3 Jonathan Druart 2016-12-06 13:17:44 UTC
I am a bit worried about adding this default value to all the tests creating an item (and track the future ones not to forget to pass the default value).
Comment 4 Marcel de Rooy 2016-12-06 13:53:27 UTC
(In reply to Jonathan Druart from comment #3)
> I am a bit worried about adding this default value to all the tests creating
> an item (and track the future ones not to forget to pass the default value).

Yeah.
Maybe we should not call _gen_text for a "xml field" but create some 'random' xml that fits better..
Comment 5 Jonathan Druart 2016-12-13 11:55:52 UTC
Created attachment 58152 [details] [review]
Bug 17726: TestBuilder - Add default values

The items.more_subfields_xml is set to random data (generated by
TestBuilder), and so GetMarcBiblio does not manage to embed items (if
needed).

The error is:
  :1: parser error : Start tag expected, '<' not found

More precisely it explodes in
C4::Items::_parse_unlinked_item_subfields_from_xml when
MARC::Record->new_from_xml is called with an invalid xml

This patch adds a default values mechanism to TestBuilder to avoid
modifying all the existing calls.

Test plan:
Set SearchEngine to ElasticSearch
prove t/db_dependent/Circulation.pl
should return green with this patch
Comment 6 Jonathan Druart 2016-12-13 11:56:11 UTC
*** Bug 17727 has been marked as a duplicate of this bug. ***
Comment 7 Marcel de Rooy 2016-12-13 13:14:16 UTC
Created attachment 58155 [details] [review]
Bug 17726: TestBuilder - Add default values

The items.more_subfields_xml is set to random data (generated by
TestBuilder), and so GetMarcBiblio does not manage to embed items (if
needed).

The error is:
  :1: parser error : Start tag expected, '<' not found

More precisely it explodes in
C4::Items::_parse_unlinked_item_subfields_from_xml when
MARC::Record->new_from_xml is called with an invalid xml

This patch adds a default values mechanism to TestBuilder to avoid
modifying all the existing calls.

Test plan:
Set SearchEngine to ElasticSearch
prove t/db_dependent/Circulation.pl
should return green with this patch

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2016-12-13 13:14:20 UTC
Created attachment 58156 [details] [review]
Bug 17726: [QA Follow-up] Add test descriptions

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2017-01-20 08:05:55 UTC
Joubu: Please adjust in view of marcxml not being in Biblioitem any more
Comment 10 Jonathan Druart 2017-01-20 09:18:00 UTC
Created attachment 59300 [details] [review]
Bug 17726: biblioitems.marc has been moved to biblio_metadata.metadata
Comment 11 Marcel de Rooy 2017-01-20 10:05:21 UTC
(In reply to Jonathan Druart from comment #10)
> Created attachment 59300 [details] [review] [review]
> Bug 17726: biblioitems.marc has been moved to biblio_metadata.metadata

Thanks. But we still have a problem, Houston:
# Subtest: Build all sources
    1..1
DBD::mysql::st execute failed: Column 'metadata' cannot be null [for Statement "INSERT INTO `biblio_metadata` ( `biblionumber`, `format`, `marcflavour`, `metadata`) VALUES ( ?, ?, ?, ? )" with ParamValues: 0='5051', 1='wjOZm7m', 2='r8zqCCTHTdruku', 3=undef] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1832.

We have a constraint here..
Comment 12 Jonathan Druart 2017-01-20 11:28:11 UTC
Created attachment 59341 [details] [review]
Bug 17726: biblioitems.marc has been removed

We will certainly have to do something with the biblio_metadata.metadata
field later.
Comment 13 Jonathan Druart 2017-01-20 11:29:06 UTC
Not sure that will be enough, but I do not see a quick solution for now. Tests seem to pass now.
Comment 14 Marcel de Rooy 2017-01-20 12:12:11 UTC
Created attachment 59353 [details] [review]
Bug 17726: biblioitems.marc has been removed

We will certainly have to do something with the biblio_metadata.metadata
field later.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2017-01-20 12:15:40 UTC
Taking the liberty to pass qa for this one now. No problems detected with this change in the t/db_dependent tests. Does not interfere with the interface, only TestBuilder and tests; not used outside t/db_dependent.
Comment 16 Kyle M Hall 2017-01-20 13:42:55 UTC
Pushed to master for 17.05, thanks Jonathan, Marcel!
Comment 17 Katrin Fischer 2017-01-22 19:45:26 UTC
I am in doubt about the last patch here - description says marc (true for 16.11) but patch says marcxml (untrue for 16.11).

Can someone clarify if this should be included in the next 16.11.x release?
Comment 18 Marcel de Rooy 2017-01-23 07:35:18 UTC
(In reply to Katrin Fischer from comment #17)
> I am in doubt about the last patch here - description says marc (true for
> 16.11) but patch says marcxml (untrue for 16.11).
> 
> Can someone clarify if this should be included in the next 16.11.x release?

The last patch is not for 16.11.
If you have a failing Circulation test in 16.11, the first two might help you solve it.
Comment 19 Katrin Fischer 2017-01-23 09:00:06 UTC
We got a happy Jenkins for 16.11.x and locally the tests pass for me as well - so there seems no need atm.
Comment 20 Jonathan Druart 2017-01-23 09:50:05 UTC
They certainly fail if SearchEngine == ElasticSearch.
Comment 21 Katrin Fischer 2017-02-07 06:59:26 UTC
First 2 patches pushed to 16.11.x, they will be in 16.11.04.
Comment 22 Mason James 2017-02-22 23:05:50 UTC
(In reply to Katrin Fischer from comment #21)
> First 2 patches pushed to 16.11.x, they will be in 16.11.04.

First 2 patches pushed to 16.05.x, for 16.05.10 release