Created attachment 30713 [details] [review] Bug 12747 Add extra column in Z3950 search result This patch permits to add an extra column in Z3950 search result. The system preference AdditionalFieldsInZ3950ResultSearch maps which MARC field/subfields are displayed in the column. Testing: I Apply the patch II Run updatedatabase.pl 0) Enter a field/subfield in the AdditionalFieldsInZ3950ResultSearch; 1) Create a new basket or use an existing one; 2) In -Add order to basket-, click "From an external source"; 3) Select some search targets and enter a subject heading ex. house; 4) Click Search bouton; 5) Validate "Additional fields" column with the field/subfield value. sponsored by the CCSR ( http://www.ccsr.qc.ca )
This seems to work well, but I'm curious why there is no corresponding change to cataloguing/z3950_search.pl? I don't think it works very well to have the additional data displayed in a table cell which doesn't allow text to wrap. If someone picked a subfield which contains a lot of data that could completely blow up the layout. Maybe an unordered list? [% IF additionalFields %] <td> <ul> [% FOREACH addnumberfield IN breeding_loo.addnumberfields %] [% IF ( breeding_loo.$addnumberfield ) %] <li><strong>[% addnumberfield %]:</strong>[% breeding_loo.$addnumberfield %]</li> [% END %] [% END %] </ul> </td> [% END %] Or a definition list? [% IF additionalFields %] <td> <dl> [% FOREACH addnumberfield IN breeding_loo.addnumberfields %] <dt>[% addnumberfield %]:</dt> <dd>[% breeding_loo.$addnumberfield %]</dd> [% END %] </dl> </td> [% END %]
This needs an answer.
Created attachment 36823 [details] [review] Fix table cell
Table cell fixed Owen #2 "why there is no corresponding change to cataloguing/z3950_search.pl? - Our sponsor (CCSR) did not ask for it.
*** Bug 11874 has been marked as a duplicate of this bug. ***
Created attachment 37836 [details] [review] Bug 12747 Add extra column in Z3950 search result This patch permits to add an extra column in Z3950 search result. The system preference AdditionalFieldsInZ3950ResultSearch maps which MARC field/subfields are displayed in the column. Testing: I Apply the patch II Run updatedatabase.pl ACQUISITIONS 0) Enter a field/subfield in the AdditionalFieldsInZ3950ResultSearch; 1) Create a new basket or use an existing one; 2) In -Add order to basket-, click "From an external source"; 3) Select some search targets and enter a subject heading ex. house; 4) Click Search bouton; 5) Validate "Additional fields" column with the field/subfield value. CATALOGUING 0) shares same syspref as above 1) Go to cataloguing, click New from z3950 2) fill to result in a successful search 3) validate column Addition Fields sponsored by the CCSR ( http://www.ccsr.qc.ca )
This is a redo of the two previous patches, along with the changes to make it identical in Cataloguing as in Acquisitions (see #11874). I switched the component to Cataloguing since it's the most used or the two, and that "Acquisitions" has a tendency to frighten out would-be testers :)
Created attachment 37893 [details] [review] [Signed-off] Bug 12747 Add extra column in Z3950 search result This patch permits to add an extra column in Z3950 search result. The system preference AdditionalFieldsInZ3950ResultSearch maps which MARC field/subfields are displayed in the column. Testing: I Apply the patch II Run updatedatabase.pl ACQUISITIONS 0) Enter a field/subfield in the AdditionalFieldsInZ3950ResultSearch; 1) Create a new basket or use an existing one; 2) In -Add order to basket-, click "From an external source"; 3) Select some search targets and enter a subject heading ex. house; 4) Click Search bouton; 5) Validate "Additional fields" column with the field/subfield value. CATALOGUING 0) shares same syspref as above 1) Go to cataloguing, click New from z3950 2) fill to result in a successful search 3) validate column Addition Fields sponsored by the CCSR ( http://www.ccsr.qc.ca ) Tested in both Acquisition and Cataloguing. Works as expected (and is a great feature). Signed-off-by: Marc Véron <veron@veron.ch>
I find the configuration for this to be a bit on the odd side. I'd much prefer to see it use the standard 123$a syntax rather than 123a. In addition, it does not support fields that have no subfields. Another minor issue, no other pref uses semicolons for a separator. It would be better to use a pipe (|) or a space ( which would be my preference )
Created attachment 38221 [details] [review] Bug 12747 - Add extra column in Z3950 search result This patch uses the standard field syntax (ex: 023$ab), allows writing fields without mentionning subfields and uses whitespace as a field separator instead of semicolon. sponsored by the CCSR ( http://www.ccsr.qc.ca )
Created attachment 38233 [details] [review] [Signed-off] Bug 12747 - Add extra column in Z3950 search result This patch uses the standard field syntax (ex: 023$ab), allows writing fields without mentionning subfields and uses whitespace as a field separator instead of semicolon. sponsored by the CCSR ( http://www.ccsr.qc.ca ) Tested with standard field syntax and whitespace. Works as expected. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 38240 [details] [review] Bug 12747 - Add extra column in Z3950 search result This patch uses the standard field syntax (ex: 023$ab), allows writing fields without mentionning subfields and uses whitespace as a field separator instead of semicolon. sponsored by the CCSR ( http://www.ccsr.qc.ca )
(In reply to Charles Farmer from comment #13) > Created attachment 38240 [details] [review] [review] > Bug 12747 - Add extra column in Z3950 search result > > This patch uses the standard field syntax (ex: 023$ab), allows writing > fields without mentionning subfields and uses whitespace as a field > separator instead of semicolon. > > sponsored by the CCSR ( http://www.ccsr.qc.ca ) This patch is basically the same as the one submitted yesterday, except that the cataloguing preference now has proper description of its syntax in the administration panel.
Applying: Bug 12747 - Add extra column in Z3950 search result fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 12747 - Add extra column in Z3950 search result
Created attachment 38286 [details] [review] Bug 12747 - Add extra column in Z3950 search result This patch uses the standard field syntax (ex: 023$ab), allows writing fields without mentionning subfields and uses whitespace as a field separator instead of semicolon. sponsored by the CCSR ( http://www.ccsr.qc.ca )
Charles, I'm sorry, I was not fast enough, after the newest pushes both patches again do not apply. For the first patch you can remove your changes from installer/data/mysql/updatedatabase.pland follow the new rules in http://wiki.koha-community.org/wiki/Database_updates See part updatedatabase.pl Oh, have a look at C4/Breeding.pm in the second patch, you have remaining lines from merging like: <<<<<<< HEAD ======= (....) >>>>>>> c4c12ed... Bug 12747 - Add extra column in Z3950 search result
Created attachment 38439 [details] [review] Bug 12747 - Add extra column in Z3950 search This patch permits to add an extra column in Z3950 search result. The system preference AdditionalFieldsInZ3950ResultSearch maps which MARC field/subfields are displayed in the column. Testing: I Apply the patch II Run updatedatabase.pl ACQUISITIONS 0) Enter a field/subfield in the AdditionalFieldsInZ3950ResultSearch; 1) Create a new basket or use an existing one; 2) In -Add order to basket-, click "From an external source"; 3) Select some search targets and enter a subject heading ex. house; 4) Click Search bouton; 5) Validate "Additional fields" column with the field/subfield value. CATALOGUING 0) shares same syspref as above 1) Go to cataloguing, click New from z3950 2) fill to result in a successful search 3) validate column Addition Fields sponsored by the CCSR ( http://www.ccsr.qc.ca )
Created attachment 38440 [details] [review] Bug 12747 - Add extra column in Z3950 search result This patch uses the standard field syntax (ex: 023$ab), allows writing fields without mentionning subfields and uses whitespace as a field separator instead of semicolon. sponsored by the CCSR ( http://www.ccsr.qc.ca )
These two new patches should do the trick. The code in updatedatabase.pl was moved to an atomicupdate script and the second patch was cleaned of the dangling "<<<< HEAD" stuff. Everything applies cleanly on master for me.
Created attachment 38456 [details] [review] [Signed-off] Bug 12747 - Add extra column in Z3950 search This patch permits to add an extra column in Z3950 search result. The system preference AdditionalFieldsInZ3950ResultSearch maps which MARC field/subfields are displayed in the column. Testing: I Apply the patch II Run updatedatabase.pl ACQUISITIONS 0) Enter a field/subfield in the AdditionalFieldsInZ3950ResultSearch; 1) Create a new basket or use an existing one; 2) In -Add order to basket-, click "From an external source"; 3) Select some search targets and enter a subject heading ex. house; 4) Click Search bouton; 5) Validate "Additional fields" column with the field/subfield value. CATALOGUING 0) shares same syspref as above 1) Go to cataloguing, click New from z3950 2) fill to result in a successful search 3) validate column Addition Fields sponsored by the CCSR ( http://www.ccsr.qc.ca ) Tested both patches together, works as expected. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 38457 [details] [review] Bug 13606: DBRev 3.19.00.029 Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com> Tested both patches together, works as expected. Signed-off-by: Marc Véron <veron@veron.ch>
Comment on attachment 38457 [details] [review] Bug 13606: DBRev 3.19.00.029 Sorry, mistaked again...
Created attachment 38458 [details] [review] [Signed-off] Bug 12747 - Add extra column in Z3950 search This patch permits to add an extra column in Z3950 search result. The system preference AdditionalFieldsInZ3950ResultSearch maps which MARC field/subfields are displayed in the column. Testing: I Apply the patch II Run updatedatabase.pl ACQUISITIONS 0) Enter a field/subfield in the AdditionalFieldsInZ3950ResultSearch; 1) Create a new basket or use an existing one; 2) In -Add order to basket-, click "From an external source"; 3) Select some search targets and enter a subject heading ex. house; 4) Click Search bouton; 5) Validate "Additional fields" column with the field/subfield value. CATALOGUING 0) shares same syspref as above 1) Go to cataloguing, click New from z3950 2) fill to result in a successful search 3) validate column Addition Fields sponsored by the CCSR ( http://www.ccsr.qc.ca ) Tested with both patches together, followed test plan, works as expected Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 38460 [details] [review] Bug 12747 - Add extra column in Z3950 search result This patch uses the standard field syntax (ex: 023$ab), allows writing fields without mentionning subfields and uses whitespace as a field separator instead of semicolon. sponsored by the CCSR ( http://www.ccsr.qc.ca ) Tested with both patches, works as expected. Signed-off-by: Marc Véron <veron@veron.ch>
Note: I forgot to add [Signed-off] to title of 2nd patch.
Comment on attachment 38458 [details] [review] [Signed-off] Bug 12747 - Add extra column in Z3950 search Review of attachment 38458 [details] [review]: ----------------------------------------------------------------- I am sorry to fail qa it again, but you have to provide tests for the change in the C4::Breeding module. I don't ask you to completely cover the subroutine, but at least highlight what the new pref does. ::: C4/Breeding.pm @@ +224,5 @@ > > + @servers = (); > + foreach my $id (@id) { > + push @servers, {id => $id}; > + } This has been removed by commit fb0834e8f582e9e268ecef6de891ba5dc25c6fd9 Bug 6536: Include SRU searching in Breeding.pm Are you sure this change is intended? @@ +232,5 @@ > + if ($pref_addnumber){ > + $template->param( > + additionalFields => '1', > + ); > + } I'd say you don't need to do that. I can just access to the AdditionalFieldsInZ3950ResultSearch value from the template using the Koha TT plugin.
Created attachment 41313 [details] [review] Bug12747 - Follow-up to QA Removed unnecessary code in Breeding.pm and provided a new test in Breeding.t
Created attachment 41317 [details] [review] Bug 12747 - Add extra column in Z3950 search This patch permits to add an extra column in Z3950 search result. The system preference AdditionalFieldsInZ3950ResultSearch maps which MARC field/subfields are displayed in the column. Testing: I Apply the patch II Run updatedatabase.pl ACQUISITIONS 0) Enter a field/subfield in the AdditionalFieldsInZ3950ResultSearch 1) Create a new basket or use an existing one 2) In -Add order to basket-, click "From an external source" 3) Select some search targets and enter a subject heading ex. house 4) Click Search bouton 5) Validate "Additional fields" column with the field/subfield value. CATALOGUING 0) shares same syspref as above 1) Go to cataloguing, click New from z3950 2) fill to result in a successful search 3) validate column Addition Fields sponsored by the CCSR ( http://www.ccsr.qc.ca ) Tested with both patches together, followed test plan, works as expected Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 41318 [details] [review] Bug 12747 - Add extra column in Z3950 search result This patch uses the standard field syntax (ex: 023$ab), allows writing fields without mentionning subfields and uses whitespace as a field separator instead of semicolon. sponsored by the CCSR ( http://www.ccsr.qc.ca ) Tested with both patches, works as expected. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 41319 [details] [review] Bug12747 - Follow-up to QA Removed unnecessary code in Breeding.pm and provided a new test in Breeding.t
If were not mistaken, nothing happen with the patch. Kind regards,
- The patch doesn't apply anymore on master. There is a conflict on .pref template. - I'd personally prefer a unique patch rather than 3 consecutive patches. - The UT doesn't work with a Unimarc DB. It fails because the biblio record constructed in the test has a title in 245 tag. In Unimarc, title is in 200. I don't know it is considered problematic by QAers.
(In reply to Frédéric Demians from comment #33) > - The UT doesn't work with a Unimarc DB. It fails because the biblio record > constructed in the test has a title in 245 tag. In Unimarc, title is in > 200. I > don't know it is considered problematic by QAers. No it's not. The requirement is that the tests must pass on a "fresh" MARC21 DB with all sample data. We tend to ask for the tests to create their own data (using t::lib::TestBuilder for instance)
.git/rebase-apply/patch:161: trailing whitespace. <td><a href="#" onclick="Import([% breeding_loo.breedingid %],[% breeding_loo.biblionumber %]); return false">Import</a></td> warning: 1 line adds whitespace errors. .git/rebase-apply/patch:161: trailing whitespace. <td><a href="#" onclick="Import([% breeding_loo.breedingid %],[% breeding_loo.biblionumber %]); return false">Import</a></td> error: patch failed: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/z3950_search.tt:180 error: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/z3950_search.tt: patch does not apply error: Did you hand edit your patch? It does not apply to blobs recorded in its index. at ./getter.pl line 196.
Created attachment 58424 [details] [review] Bug 12747 - Add extra column in Z3950 search This patch permits to add an extra column in Z3950 search result. The system preference AdditionalFieldsInZ3950ResultSearch maps which MARC field/subfields are displayed in the column. Testing: I Apply the patch II Run updatedatabase.pl ACQUISITIONS 0) Enter a field/subfield in the AdditionalFieldsInZ3950ResultSearch 1) Create a new basket or use an existing one 2) In -Add order to basket-, click "From an external source" 3) Select some search targets and enter a subject heading ex. house 4) Click Search bouton 5) Validate "Additional fields" column with the field/subfield value. CATALOGUING 0) shares same syspref as above 1) Go to cataloguing, click New from z3950 2) fill to result in a successful search 3) validate column Addition Fields sponsored by the CCSR ( http://www.ccsr.qc.ca ) Tested with both patches together, followed test plan, works as expected Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 58425 [details] [review] Bug 12747 - Add extra column in Z3950 search result This patch uses the standard field syntax (ex: 023$ab), allows writing fields without mentionning subfields and uses whitespace as a field separator instead of semicolon. sponsored by the CCSR ( http://www.ccsr.qc.ca ) Tested with both patches, works as expected. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 58426 [details] [review] Bug12747 - Follow-up to QA Removed unnecessary code in Breeding.pm and provided a new test in Breeding.t
The patch was successful for the acquisitions but not for the cataloging
Created attachment 60965 [details] [review] Bug12747 - Small fix for add this field correctly in cataloging module
This patch no longer applies: Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 12747 - Add extra column in Z3950 search Using index info to reconstruct a base tree... M installer/data/mysql/sysprefs.sql M koha-tmpl/intranet-tmpl/prog/en/modules/acqui/z3950_search.tt M koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/z3950_search.tt Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/z3950_search.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/z3950_search.tt Auto-merging installer/data/mysql/sysprefs.sql CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql Failed to merge in the changes. Patch failed at 0001 Bug 12747 - Add extra column in Z3950 search The copy of the patch that failed is found in: /home/vagrant/kohaclone/.git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-12747---Add-extra-column-in-Z3950-search-rZkwMW.patch
Created attachment 66586 [details] [review] Bug 12747 - Add extra column in Z3950 search This patch permits to add an extra column in Z3950 search result. The system preference AdditionalFieldsInZ3950ResultSearch maps which MARC field/subfields are displayed in the column. Testing: I Apply the patch II Run updatedatabase.pl ACQUISITIONS 0) Enter a field/subfield in the AdditionalFieldsInZ3950ResultSearch 1) Create a new basket or use an existing one 2) In -Add order to basket-, click "From an external source" 3) Select some search targets and enter a subject heading ex. house 4) Click Search bouton 5) Validate "Additional fields" column with the field/subfield value. CATALOGUING 0) shares same syspref as above 1) Go to cataloguing, click New from z3950 2) fill to result in a successful search 3) validate column Addition Fields sponsored by the CCSR ( http://www.ccsr.qc.ca ) Tested with both patches together, followed test plan, works as expected Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 66587 [details] [review] Bug 12747 - Add extra column in Z3950 search result This patch uses the standard field syntax (ex: 023$ab), allows writing fields without mentionning subfields and uses whitespace as a field separator instead of semicolon. sponsored by the CCSR ( http://www.ccsr.qc.ca ) Tested with both patches, works as expected. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 66588 [details] [review] Bug12747 - Follow-up to QA Removed unnecessary code in Breeding.pm and provided a new test in Breeding.t
Created attachment 66589 [details] [review] Bug12747 - Small fix for add this field correctly in cataloging module
The patch apply without conflict.
I have applied the patch to kohadevbox and proceed with test plan and It works according the expected
QA Comment: Thx for your proposal. Some points that need attention: Breeding.pm: @servers = (); foreach my $id (@id) { push @servers, {id => $id}; } Please explain what you are doing here. Seems unneeded or wrong. $template->param( additionalFields => '1' ) if C4::Context->preference('AdditionalFieldsInZ3950ResultSearch'); Please use Koha::Preference in the template. sub _add_rowdata Just intuitive, these additions dont look that nice to me. I think you should add more validation and put this in another subroutine, taking care that you do not overwrite the default fields. +++ b/installer/data/mysql/atomicupdate/bug_12747-additional_fields_in_Z3950_search_result.sql @@ -0,0 +1 @@ +INSERT INTO systempreferences Please INSERT IGNORE +('AddressFormat','us','','Choose format to display postal addresses', 'Choice'), +('AdditionalFieldsInZ3950ResultSearch', '', NULL, 'Determines which MARC field/subfields are displayed in -Additional field- column in the result of a search Z3950', 'Free'), Please respect the sort order in sysprefs. Note: I saw that a LOT of other new prefs are not in the right order. But you dont need to correct that here of course. Just add your own one in the right place. + [% IF additionalFields %] + <td> + <dl> + [% FOREACH addnumberfield IN breeding_loo.addnumberfields %] + <dt>[% addnumberfield %]:</dt> + <dd>[% breeding_loo.$addnumberfield %]</dd> + [% END %] + </dl> + </td> + [% END %] You add all additional fields in one column??
To the last comment: << You add all additional fields in one column?? >> Yes. And it's beautiful. I'll get someone to look at your other points as soon as possible. The one about Koha.Preference just shows the age of this feature. :)
Created attachment 67314 [details] [review] Bug 12747 - Corrections on QA feedback C4/Breeding.pm + code in _add_rowdata moved to new subroutine (_add_custom_field_rowdata) - removed useless code Templates: * template use Koha.Preference instead of passed param. Syspref.sql * corrected order - removed unrelated modification to other system preferences
Created attachment 67315 [details] [review] Bug 12747 - Fix : INSERT IGNORE INTO in atomicupdate
Comment on attachment 66586 [details] [review] Bug 12747 - Add extra column in Z3950 search Review of attachment 66586 [details] [review]: ----------------------------------------------------------------- ::: C4/Breeding.pm @@ +365,5 @@ > + my $fieldContent=''; > + if ($length > 3){ > + my $adSubfield; > + for (my $i=0; $i<$length-3; $i++){ > + $adSubfield = substr($_, 3+$i, 1); When you have nested loops, one should avoid $_ and be explicit (on line 359) to improve readability and prevent accidental breakage later.
Comment on attachment 66587 [details] [review] Bug 12747 - Add extra column in Z3950 search result Review of attachment 66587 [details] [review]: ----------------------------------------------------------------- ::: C4/Breeding.pm @@ +362,4 @@ > my @addnumberfields; > foreach (@addnumber_array) { > my $length = length($_); > my $adTag = substr($_, 0, 3); If the field is: 100$a 245$a 700$a Then why aren't you: my @fields_to_add = split / /, $pref_addnumber; And inside the the loop: foreach my $field (@fields_to_add) { my ($tag,$subtags) = split |\$|, $field; You are making an assumption that the user won't type something bad like 20$a instead of 020$a. Notice my use of $subtags, because that would be the 'abc' or whatever was set. my @subtags = split //,$subtags; # I believe this splits each character out. foreach my $subtag (@subtags) { substr() calculations are just messy and slow. @@ +385,5 @@ > + my $subfields_text = $field->as_string(); > + push(@addnumberfields, $_); > + $row->{$_} = $subfields_text; > + } > + } And then all this logic could be optimized.
Comment on attachment 66588 [details] [review] Bug12747 - Follow-up to QA Review of attachment 66588 [details] [review]: ----------------------------------------------------------------- ::: t/db_dependent/Breeding.t @@ +195,5 @@ > + > +sub test_add_rowdata { > + > + my $old_preference = C4::Context->preference("AdditionalFieldsInZ3950ResultSearch"); > + C4::Context->set_preference("AdditionalFieldsInZ3950ResultSearch",""); It would be better to mock the preference, so that failure would not affect the database.
Comment on attachment 66589 [details] [review] Bug12747 - Small fix for add this field correctly in cataloging module Review of attachment 66589 [details] [review]: ----------------------------------------------------------------- I don't see the point of this patch. If the issue was adding 000, then check length. Less parameters is always better.
Comment on attachment 67314 [details] [review] Bug 12747 - Corrections on QA feedback Review of attachment 67314 [details] [review]: ----------------------------------------------------------------- ::: C4/Breeding.pm @@ -224,5 @@ > - @servers = (); > - foreach my $id (@id) { > - push @servers, {id => $id}; > - } > - Why was this removed? Did you add it before? @@ +344,5 @@ > + > + return $row; > +} > + > +sub _add_custom_field_rowdata Nice extraction. ::: installer/data/mysql/sysprefs.sql @@ +9,2 @@ > ('AdditionalFieldsInZ3950ResultSearch', '', NULL, 'Determines which MARC field/subfields are displayed in -Additional field- column in the result of a search Z3950', 'Free'), > +('AddressFormat','us','us|de|fr','Choose format to display postal addresses', 'Choice'), Intermediate patches dealing with rebases makes this look like a mistake. Squashing is frequently a good idea where possible. ::: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/z3950_search.tt @@ +133,4 @@ > <th>Author</th> > <th>ISBN</th> > <th>LCCN</th> > + [% IF Koha.Preference('AdditionalFieldsInZ3950ResultSearch') != '' %] Length, != ''... okay, this is better.
Trying to test acquisition change... having difficulties. I don't think it is a code issue. BTW, Additional fields does look okay for a circulation Z39.50 search.
Okay, tested. It generally works. However, the test bothers me. The preference is not mocked; it is used directly. This can be bad if the test is interrupted in the middle of the test with the value changed. And the test is loud, generating noise when the prove is run.
Created attachment 68214 [details] [review] Bug 12747 - Add extra column in Z3950 search results This is a complete squash of previous patches, with additional code quality improvments following M. Tompsett's feedback. Squashing was done (as suggested) because many lines add be added and subsequently deleted, which made for confusing patches. New changes: * Major enhancement to _add_custom_rowdata(), as suggested by QA. The $_ variable is no longer used (at all), and the split() routine is used. * t/db_dependent/Breeding.t now uses the mocked preference logic to avoid sysprefs being affected. Functionality and test plan have not changed. -- This patch makes it possible to add an extra column to Z3950 search results. The system preference AdditionalFieldsInZ3950ResultSearch maps which MARC field/subfields are displayed in the column. Testing: I Apply the patch II Run updatedatabase.pl ACQUISITIONS 0) Enter a field/subfield in the AdditionalFieldsInZ3950ResultSearch 1) Create a new basket or use an existing one 2) In -Add order to basket-, click "From an external source" 3) Select some search targets and enter a subject heading ex. house 4) Click Search bouton 5) Validate "Additional fields" column with the field/subfield value. CATALOGUING 0) Shares same syspref as above 1) Go to cataloguing, click New from z3950 2) Fill to result in a successful search 3) Validate column Addition Fields Sponsored-by: CCSR (https://ccsr.qc.ca)
Created attachment 68215 [details] [review] Bug 12747 - Missing use in tests Adds missing `use t::lib::Mocks ...` to Breeding.t Tests should now pass correctly.
Oh the code is so much prettier. Thank you. I'll sign off after lunch, after retest.
Ah ha! I figured out why there is noise. Sorry my brain is slow. You have MARC21 and UNIMARC attempts in the same loop (biblioitems.publicationyear). That's going to generate noise on a MARC21 system. I have yet to test on a UNIMARC system. And sorry... fails on unimarc system. You create 245$a, which is MARC21. You need to grab the appropriate title tag and subtag for title under the current system MARC flavour, so that the test won't die. In fact, you should split the test to a unimarc and marc21 run. Sorry, but Failed QA again.
Created attachment 69297 [details] [review] Bug 12747 - Switch date field on marc flavour Amended _add_rowdata to check only for the marcflavour-appropriate date field. This makes the test pass without noise.
Created attachment 72706 [details] [review] Bug 12747 - Add extra column in Z3950 search This is a complete squash of previous patches, with additional code quality improvments following M. Tompsett's feedback. Squashing was done (as suggested) because many lines add be added and subsequently deleted, which made for confusing patches. New changes: * Major enhancement to _add_custom_rowdata(), as suggested by QA. The $_ variable is no longer used (at all), and the split() routine is used. * t/db_dependent/Breeding.t now uses the mocked preference logic to avoid sysprefs being affected. Functionality and test plan have not changed. -- This patch makes it possible to add an extra column to Z3950 search results. The system preference AdditionalFieldsInZ3950ResultSearch maps which MARC field/subfields are displayed in the column. Testing: I Apply the patch II Run updatedatabase.pl ACQUISITIONS 0) Enter a field/subfield in the AdditionalFieldsInZ3950ResultSearch 1) Create a new basket or use an existing one 2) In -Add order to basket-, click "From an external source" 3) Select some search targets and enter a subject heading ex. house 4) Click Search bouton 5) Validate "Additional fields" column with the field/subfield value. CATALOGUING 0) Shares same syspref as above 1) Go to cataloguing, click New from z3950 2) Fill to result in a successful search 3) Validate column Addition Fields Sponsored-by: CCSR (https://ccsr.qc.ca)
patch doesn't apply The sandbox you've requested is not ready. Some problems occurred applying patches from bug 12747: <h1>Something went wrong !</h1>Applying: Bug 12747 - Missing use in tests fatal: sha1 information is lacking or useless (t/db_dependent/Breeding.t). Patch failed at 0001 Bug 12747 - Missing use in tests The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run git bz apply --continue. If you would prefer to skip this patch, instead run git bz apply --skip. To restore the original branch and stop patching run git bz apply --abort. error: could not build fake ancestor Bug 12747 - Add extra column in Z3950 search result 68215 - Bug 12747 - Missing use in tests 69297 - Bug 12747 - Switch date field on marc flavour
Created attachment 72896 [details] [review] Bug 12747 - Missing use in tests Adds missing `use t::lib::Mocks ...` to Breeding.t Tests should now pass correctly.
Created attachment 72897 [details] [review] Bug 12747 - Switch date field on marc flavour Amended _add_rowdata to check only for the marcflavour-appropriate date field. This makes the test pass without noise.
Created attachment 72898 [details] [review] Bug 12747 - Add extra column in Z3950 search This is a complete squash of previous patches, with additional code quality improvments following M. Tompsett's feedback. Squashing was done (as suggested) because many lines add be added and subsequently deleted, which made for confusing patches. New changes: * Major enhancement to _add_custom_rowdata(), as suggested by QA. The $_ variable is no longer used (at all), and the split() routine is used. * t/db_dependent/Breeding.t now uses the mocked preference logic to avoid sysprefs being affected. Functionality and test plan have not changed. -- This patch makes it possible to add an extra column to Z3950 search results. The system preference AdditionalFieldsInZ3950ResultSearch maps which MARC field/subfields are displayed in the column. Testing: I Apply the patch II Run updatedatabase.pl ACQUISITIONS 0) Enter a field/subfield in the AdditionalFieldsInZ3950ResultSearch 1) Create a new basket or use an existing one 2) In -Add order to basket-, click "From an external source" 3) Select some search targets and enter a subject heading ex. house 4) Click Search bouton 5) Validate "Additional fields" column with the field/subfield value. CATALOGUING 0) Shares same syspref as above 1) Go to cataloguing, click New from z3950 2) Fill to result in a successful search 3) Validate column Addition Fields Sponsored-by: CCSR (https://ccsr.qc.ca)
Created attachment 72979 [details] [review] Bug 12747 - Missing use in tests Adds missing `use t::lib::Mocks ...` to Breeding.t Tests should now pass correctly. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 72980 [details] [review] Bug 12747 - Switch date field on marc flavour Amended _add_rowdata to check only for the marcflavour-appropriate date field. This makes the test pass without noise. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 72981 [details] [review] Bug 12747 - Add extra column in Z3950 search This is a complete squash of previous patches, with additional code quality improvments following M. Tompsett's feedback. Squashing was done (as suggested) because many lines add be added and subsequently deleted, which made for confusing patches. New changes: * Major enhancement to _add_custom_rowdata(), as suggested by QA. The $_ variable is no longer used (at all), and the split() routine is used. * t/db_dependent/Breeding.t now uses the mocked preference logic to avoid sysprefs being affected. Functionality and test plan have not changed. -- This patch makes it possible to add an extra column to Z3950 search results. The system preference AdditionalFieldsInZ3950ResultSearch maps which MARC field/subfields are displayed in the column. Testing: I Apply the patch II Run updatedatabase.pl ACQUISITIONS 0) Enter a field/subfield in the AdditionalFieldsInZ3950ResultSearch 1) Create a new basket or use an existing one 2) In -Add order to basket-, click "From an external source" 3) Select some search targets and enter a subject heading ex. house 4) Click Search bouton 5) Validate "Additional fields" column with the field/subfield value. CATALOGUING 0) Shares same syspref as above 1) Go to cataloguing, click New from z3950 2) Fill to result in a successful search 3) Validate column Addition Fields Sponsored-by: CCSR (https://ccsr.qc.ca) Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Tested with Unimarc DB, worked as described on both basket and cataloguing. Thanks for this enhancement !
FAIL koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/z3950_search.tt FAIL forbidden patterns forbidden pattern: tab char (line 110) OK git manipulation OK spelling OK tt_valid OK valid_template FAIL t/db_dependent/Breeding.t OK critic OK forbidden patterns OK git manipulation OK pod OK spelling FAIL valid Can't locate t/luib/Mocks.pm in @INC (you may need to install the t::luib::Mocks module) (@INC contains: /home/vagrant/kohaclone /home/vagrant/qa-test-tools /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.20.2 /usr/local/share/perl/5.20.2 /usr/lib/x86_64-linux-gnu/perl5/5.20 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.20 /usr/share/perl/5.20 /usr/local/lib/site_perl .) Typo in the test file? Please fix!
Created attachment 73930 [details] [review] Bug 12747 - fixed typo in uses in tests
(In reply to Roch D'Amour from comment #74) > Created attachment 73930 [details] [review] [review] > Bug 12747 - fixed typo in uses in tests Really? please read https://wiki.koha-community.org/wiki/Commit_messages#Formatting Update your QA tools repo and run the tests.
You're right, that commit was a bit rushed :/ I will clean it up a bit and fix all the red stuff from the QA tools.
Created attachment 73955 [details] [review] Bug 12747: (follow-up) Removed unwanted tab character and fixed typo in tests Removed a typo introduced while adding missing `use t::lib::Mocks ...` to Breeding.t Also removed tab character from z3950_search.tt
Please double check the Z39.05 search, MARC, ISBD, etc. have been moved into a pull down menu a whlie ago and your patch adds back the initial separate columns.
Please also fix your commit messages on the first 2 patches as pointed out by the QA tools: Bug 12747 - Add extra column in Z3950 search result
Created attachment 74035 [details] [review] Bug 12747: Missing use in tests Adds missing `use t::lib::Mocks ...` to Breeding.t Tests should now pass correctly. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74037 [details] [review] Bug 12747: Switch date field on marc flavour Amended _add_rowdata to check only for the marcflavour-appropriate date field. This makes the test pass without noise. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74038 [details] [review] Bug 12747: Adjustment to Z39.05 search template Removed some old columns that were moved in a previous patch and brought back in while rebasing. Also moved the additionnal fields column to the left of the action columns, so the pull down menu stays at the far right of the line.
Created attachment 74039 [details] [review] Bug 12747: (follow-up) Removed unwanted tab character and fixed typo in tests Removed a typo introduced while adding missing `use t::lib::Mocks ...` to Breeding.t Also removed tab character from z3950_search.tt
Created attachment 74040 [details] [review] Bug 12747: Adjustment to Z39.05 search template Removed some old columns that were moved in a previous patch and brought back in while rebasing. Also moved the additionnal fields column to the left of the action columns, so the pull down menu stays at the far right of the line.
Created attachment 74783 [details] [review] Bug 12747 - Add extra column in Z3950 search This is a complete squash of previous patches, with additional code quality improvments following M. Tompsett's feedback. Squashing was done (as suggested) because many lines add be added and subsequently deleted, which made for confusing patches. New changes: * Major enhancement to _add_custom_rowdata(), as suggested by QA. The $_ variable is no longer used (at all), and the split() routine is used. * t/db_dependent/Breeding.t now uses the mocked preference logic to avoid sysprefs being affected. Functionality and test plan have not changed. -- This patch makes it possible to add an extra column to Z3950 search results. The system preference AdditionalFieldsInZ3950ResultSearch maps which MARC field/subfields are displayed in the column. Testing: I Apply the patch II Run updatedatabase.pl ACQUISITIONS 0) Enter a field/subfield in the AdditionalFieldsInZ3950ResultSearch 1) Create a new basket or use an existing one 2) In -Add order to basket-, click "From an external source" 3) Select some search targets and enter a subject heading ex. house 4) Click Search bouton 5) Validate "Additional fields" column with the field/subfield value. CATALOGUING 0) Shares same syspref as above 1) Go to cataloguing, click New from z3950 2) Fill to result in a successful search 3) Validate column Addition Fields Sponsored-by: CCSR (https://ccsr.qc.ca) Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74784 [details] [review] Bug 12747 - Missing use in tests Adds missing `use t::lib::Mocks ...` to Breeding.t Tests should now pass correctly. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74785 [details] [review] Bug 12747: Switch date field on marc flavour Amended _add_rowdata to check only for the marcflavour-appropriate date field. This makes the test pass without noise. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74786 [details] [review] Bug 12747: (follow-up) Removed unwanted tab character and fixed typo in tests Removed a typo introduced while adding missing `use t::lib::Mocks ...` to Breeding.t Also removed tab character from z3950_search.tt Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74787 [details] [review] Bug 12747: Adjustment to Z39.05 search template Removed some old columns that were moved in a previous patch and brought back in while rebasing. Also moved the additionnal fields column to the left of the action columns, so the pull down menu stays at the far right of the line. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Feature works fine. A detail : maybe the pull down menu is too far from the selected line (1 or 2 lines down). I can't remember exactly what the first feature looked like.
Hi all, retesting this, there are a few small glitches (tested with MARC21): - the label will show empty, if the subfield you asked for doesn't exist. Example: 035$a: will show, if there is a 035 with only a $9 in the data. - it will only show the first field from the data, making this not work for repeatable fields like 700$a - only the first entry is shown. The sys pref should be reworded a little, but that's minor and can be done easily (I would have done it, but got stuck on the other items) I am not entirely persuaded by the 2 line display using <dd> <dt>, but also non blocker. Plus: Works well for control fields without subfields, Example: 001. Maybe be good to add an example to the list of fields in the sys pref description.
Created attachment 75096 [details] [review] Bug 12747: Add extra column in Z3950 search This patch makes it possible to add an extra column to Z3950 search results. The system preference AdditionalFieldsInZ3950ResultSearch decides which MARC field/subfields are displayed in the column. Testing: I Apply the patch II Run updatedatabase.pl ACQUISITIONS 0) Enter a field/subfield in the AdditionalFieldsInZ3950ResultSearch 1) Create a new basket or use an existing one 2) In -Add order to basket-, click "From an external source" 3) Select some search targets and enter a subject heading ex. house 4) Click Search bouton 5) Validate "Additional fields" column with the field/subfield value. CATALOGUING 0) Shares same syspref as above 1) Go to cataloguing, click New from z3950 2) Fill to result in a successful search 3) Validate column Addition Fields prove t/db_dependent/Breeding.t Sponsored-by: CCSR (https://ccsr.qc.ca) Current patch-complexity: Small patch
I've resquashed the patch to keep it clean. New changes: * Support repeatable tags Repeated tags will repeat the field name, e.g.: " 035$a: some value 035$a: another value " * Delimiter switched to comma from whitespace (whitespace is ignored). I did this to make it uniform with the sysprefs 'z3950NormalizeAuthor' and 'z3950AuthorAuthFields'. * Reworded syspref description slightly, added '001' to example " Display the MARC field/subfields <> in the 'Additional fields' column in the result of a search Z3950 (use comma as delimiter e.g.: "001, 082$ab, 090$ab") " * No longer shows tags missing the target subfield * Added a test for repeatable tags and syspref splitting to t/db_dependent/Breeding.t Adding multiple additional tags will result in the result table being very big (lots of whitespace). I tried using a <table> to make it more compact, but it became too hard to read (cluttered). I left it with <dl> for now. QA Tool green.
Comment on attachment 75096 [details] [review] Bug 12747: Add extra column in Z3950 search Review of attachment 75096 [details] [review]: ----------------------------------------------------------------- ::: C4/Breeding.pm @@ +371,5 @@ > + if ( not $str eq '') { > + push @content, $str; > + } > + } elsif ( $tag <= 10 ) { > + push @content, $marcfield->data(); If I looked it up correctly 010 in UNIMARC is ISBN and has subfields. Can you please double check? https://archive.ifla.org/VI/8/unimarc-concise-bibliographic-format-2008.pdf
Created attachment 77729 [details] [review] Bug 12747: Add extra column in Z3950 search This patch makes it possible to add an extra column to Z3950 search results. The system preference AdditionalFieldsInZ3950ResultSearch decides which MARC field/subfields are displayed in the column. Testing: I Apply the patch II Run updatedatabase.pl ACQUISITIONS 0) Enter a field/subfield in the AdditionalFieldsInZ3950ResultSearch 1) Create a new basket or use an existing one 2) In -Add order to basket-, click "From an external source" 3) Select some search targets and enter a subject heading ex. house 4) Click Search bouton 5) Validate "Additional fields" column with the field/subfield value. CATALOGUING 0) Shares same syspref as above 1) Go to cataloguing, click New from z3950 2) Fill to result in a successful search 3) Validate column Addition Fields prove t/db_dependent/Breeding.t Sponsored-by: CCSR (https://ccsr.qc.ca) Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Can you please take a look at comment#94? Sorry for leaving this unfinished, will try to return asap when fixed.
Note: signed off this one, so won't be able to QA - unless someone else gives it another sign-off then.
Created attachment 78270 [details] [review] Bug 12747: Add extra column in Z3950 search This patch makes it possible to add an extra column to Z3950 search results. The system preference AdditionalFieldsInZ3950ResultSearch decides which MARC field/subfields are displayed in the column. Testing: I Apply the patch II Run updatedatabase.pl ACQUISITIONS 0) Enter a field/subfield in the AdditionalFieldsInZ3950ResultSearch 1) Create a new basket or use an existing one 2) In -Add order to basket-, click "From an external source" 3) Select some search targets and enter a subject heading ex. house 4) Click Search bouton 5) Validate "Additional fields" column with the field/subfield value. CATALOGUING 0) Shares same syspref as above 1) Go to cataloguing, click New from z3950 2) Fill to result in a successful search 3) Validate column Addition Fields prove t/db_dependent/Breeding.t Sponsored-by: CCSR (https://ccsr.qc.ca) Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 78271 [details] [review] Bug 12747: Add extra column in Z3950 search This patch makes it possible to add an extra column to Z3950 search results. The system preference AdditionalFieldsInZ3950ResultSearch decides which MARC field/subfields are displayed in the column. Testing: I Apply the patch II Run updatedatabase.pl ACQUISITIONS 0) Enter a field/subfield in the AdditionalFieldsInZ3950ResultSearch 1) Create a new basket or use an existing one 2) In -Add order to basket-, click "From an external source" 3) Select some search targets and enter a subject heading ex. house 4) Click Search bouton 5) Validate "Additional fields" column with the field/subfield value. CATALOGUING 0) Shares same syspref as above 1) Go to cataloguing, click New from z3950 2) Fill to result in a successful search 3) Validate column Addition Fields prove t/db_dependent/Breeding.t Sponsored-by: CCSR (https://ccsr.qc.ca) Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 78272 [details] [review] Bug 12747: (QA follow-up) Treat 010 according to marcflavour
From the information I could gather on IFLA.org, you're completely right, the 010 field of UNIMARC is indeed used for ISBN and has subfields. I rebased the first patch on the current master, and added a QA follow-up one to deal with the 010 issue, but I did not consider the case for NORMARC in part because of Bug 18984. If this is a problem, I'll add another patch for it. Needs Signoff
Patch tested with a sandbox, by Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 78687 [details] [review] Bug 12747: Add extra column in Z3950 search This patch makes it possible to add an extra column to Z3950 search results. The system preference AdditionalFieldsInZ3950ResultSearch decides which MARC field/subfields are displayed in the column. Testing: I Apply the patch II Run updatedatabase.pl ACQUISITIONS 0) Enter a field/subfield in the AdditionalFieldsInZ3950ResultSearch 1) Create a new basket or use an existing one 2) In -Add order to basket-, click "From an external source" 3) Select some search targets and enter a subject heading ex. house 4) Click Search bouton 5) Validate "Additional fields" column with the field/subfield value. CATALOGUING 0) Shares same syspref as above 1) Go to cataloguing, click New from z3950 2) Fill to result in a successful search 3) Validate column Addition Fields prove t/db_dependent/Breeding.t Sponsored-by: CCSR (https://ccsr.qc.ca) Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 78688 [details] [review] Bug 12747: (QA follow-up) Treat 010 according to marcflavour Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 79349 [details] [review] Bug 12747: Add extra column in Z3950 search This patch makes it possible to add an extra column to Z3950 search results. The system preference AdditionalFieldsInZ3950ResultSearch decides which MARC field/subfields are displayed in the column. Testing: I Apply the patch II Run updatedatabase.pl ACQUISITIONS 0) Enter a field/subfield in the AdditionalFieldsInZ3950ResultSearch 1) Create a new basket or use an existing one 2) In -Add order to basket-, click "From an external source" 3) Select some search targets and enter a subject heading ex. house 4) Click Search bouton 5) Validate "Additional fields" column with the field/subfield value. CATALOGUING 0) Shares same syspref as above 1) Go to cataloguing, click New from z3950 2) Fill to result in a successful search 3) Validate column Addition Fields prove t/db_dependent/Breeding.t Sponsored-by: CCSR (https://ccsr.qc.ca) Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 79350 [details] [review] Bug 12747: (QA follow-up) Treat 010 according to marcflavour Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 79351 [details] [review] Bug 12747: (QA follow-up) Rephrase system preference description Just a suggestion for a little change to the pref description.
Awesome work all! Pushed to master for 18.11
Enhancement, will not be backported to 18.05.x series.