Bug 12747 - Add configurable extra column in Z3950 search result
Summary: Add configurable extra column in Z3950 search result
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Charles Farmer
QA Contact:
URL:
Whiteboard:
Keywords:
: 11874 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-08-11 16:41 UTC by simith.doliveira
Modified: 2018-11-12 01:04 UTC (History)
24 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
This allows to display MARC fields and subfields from the record in an extra column on the Z30.50 result list. The content of the column can be configured via the AdditionalFieldsInZ3950ResultSearch system preference.


Attachments
Bug 12747 Add extra column in Z3950 search result (7.39 KB, patch)
2014-08-11 18:43 UTC, simith.doliveira
Details | Diff | Splinter Review
Fix table cell (2.74 KB, patch)
2015-03-11 19:51 UTC, simith.doliveira
Details | Diff | Splinter Review
Bug 12747 Add extra column in Z3950 search result (9.89 KB, patch)
2015-04-14 19:21 UTC, Blou
Details | Diff | Splinter Review
[Signed-off] Bug 12747 Add extra column in Z3950 search result (10.13 KB, patch)
2015-04-15 09:30 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12747 - Add extra column in Z3950 search result (2.81 KB, patch)
2015-04-20 21:27 UTC, Charles Farmer
Details | Diff | Splinter Review
[Signed-off] Bug 12747 - Add extra column in Z3950 search result (3.02 KB, patch)
2015-04-21 08:56 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12747 - Add extra column in Z3950 search result (3.72 KB, patch)
2015-04-21 12:54 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 12747 - Add extra column in Z3950 search result (4.02 KB, patch)
2015-04-21 15:57 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 12747 - Add extra column in Z3950 search (9.50 KB, patch)
2015-04-23 17:31 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 12747 - Add extra column in Z3950 search result (3.72 KB, patch)
2015-04-23 17:31 UTC, Charles Farmer
Details | Diff | Splinter Review
[Signed-off] Bug 12747 - Add extra column in Z3950 search (9.70 KB, patch)
2015-04-23 19:18 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 13606: DBRev 3.19.00.029 (1.51 KB, patch)
2015-04-23 19:23 UTC, Marc Véron
Details | Diff | Splinter Review
[Signed-off] Bug 12747 - Add extra column in Z3950 search (9.72 KB, patch)
2015-04-23 19:29 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12747 - Add extra column in Z3950 search result (3.90 KB, patch)
2015-04-23 19:31 UTC, Marc Véron
Details | Diff | Splinter Review
Bug12747 - Follow-up to QA (4.88 KB, patch)
2015-08-03 14:43 UTC, Rémi Mayrand-Provencher
Details | Diff | Splinter Review
Bug 12747 - Add extra column in Z3950 search (9.68 KB, patch)
2015-08-03 15:01 UTC, Rémi Mayrand-Provencher
Details | Diff | Splinter Review
Bug 12747 - Add extra column in Z3950 search result (3.90 KB, patch)
2015-08-03 15:01 UTC, Rémi Mayrand-Provencher
Details | Diff | Splinter Review
Bug12747 - Follow-up to QA (4.88 KB, patch)
2015-08-03 15:01 UTC, Rémi Mayrand-Provencher
Details | Diff | Splinter Review
Bug 12747 - Add extra column in Z3950 search (9.82 KB, patch)
2016-12-23 22:30 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 12747 - Add extra column in Z3950 search result (4.01 KB, patch)
2016-12-23 22:30 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug12747 - Follow-up to QA (4.89 KB, patch)
2016-12-23 22:30 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug12747 - Small fix for add this field correctly in cataloging module (2.06 KB, patch)
2017-03-09 15:39 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 12747 - Add extra column in Z3950 search (10.03 KB, patch)
2017-08-29 15:08 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 12747 - Add extra column in Z3950 search result (4.01 KB, patch)
2017-08-29 15:08 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug12747 - Follow-up to QA (4.89 KB, patch)
2017-08-29 15:08 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug12747 - Small fix for add this field correctly in cataloging module (2.06 KB, patch)
2017-08-29 15:09 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 12747 - Corrections on QA feedback (8.99 KB, patch)
2017-09-22 15:38 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 12747 - Fix : INSERT IGNORE INTO in atomicupdate (1.29 KB, patch)
2017-09-22 15:40 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 12747 - Add extra column in Z3950 search results (13.01 KB, patch)
2017-10-17 14:11 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 12747 - Missing use in tests (739 bytes, patch)
2017-10-17 14:16 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 12747 - Switch date field on marc flavour (1.21 KB, patch)
2017-11-22 21:38 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 12747 - Add extra column in Z3950 search (14.95 KB, patch)
2018-03-12 15:27 UTC, Maksim Sen
Details | Diff | Splinter Review
Bug 12747 - Missing use in tests (704 bytes, patch)
2018-03-14 19:34 UTC, Roch D'Amour
Details | Diff | Splinter Review
Bug 12747 - Switch date field on marc flavour (1.18 KB, patch)
2018-03-14 19:35 UTC, Roch D'Amour
Details | Diff | Splinter Review
Bug 12747 - Add extra column in Z3950 search (14.99 KB, patch)
2018-03-14 19:36 UTC, Roch D'Amour
Details | Diff | Splinter Review
Bug 12747 - Missing use in tests (854 bytes, patch)
2018-03-15 16:37 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 12747 - Switch date field on marc flavour (1.33 KB, patch)
2018-03-15 16:38 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 12747 - Add extra column in Z3950 search (15.14 KB, patch)
2018-03-15 16:38 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 12747 - fixed typo in uses in tests (687 bytes, patch)
2018-04-10 13:58 UTC, Roch D'Amour
Details | Diff | Splinter Review
Bug 12747: (follow-up) Removed unwanted tab character and fixed typo in tests (1.52 KB, patch)
2018-04-10 15:38 UTC, Roch D'Amour
Details | Diff | Splinter Review
Bug 12747: Missing use in tests (854 bytes, patch)
2018-04-11 19:28 UTC, Roch D'Amour
Details | Diff | Splinter Review
Bug 12747: Switch date field on marc flavour (1.33 KB, patch)
2018-04-11 19:31 UTC, Roch D'Amour
Details | Diff | Splinter Review
Bug 12747: Adjustment to Z39.05 search template (3.18 KB, patch)
2018-04-11 19:33 UTC, Roch D'Amour
Details | Diff | Splinter Review
Bug 12747: (follow-up) Removed unwanted tab character and fixed typo in tests (1.52 KB, patch)
2018-04-11 19:43 UTC, Roch D'Amour
Details | Diff | Splinter Review
Bug 12747: Adjustment to Z39.05 search template (3.18 KB, patch)
2018-04-11 19:44 UTC, Roch D'Amour
Details | Diff | Splinter Review
Bug 12747 - Add extra column in Z3950 search (14.96 KB, patch)
2018-04-24 08:09 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 12747 - Missing use in tests (847 bytes, patch)
2018-04-24 08:09 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 12747: Switch date field on marc flavour (1.33 KB, patch)
2018-04-24 08:10 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 12747: (follow-up) Removed unwanted tab character and fixed typo in tests (1.67 KB, patch)
2018-04-24 08:10 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 12747: Adjustment to Z39.05 search template (3.33 KB, patch)
2018-04-24 08:10 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 12747: Add extra column in Z3950 search (15.12 KB, patch)
2018-05-04 19:49 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 12747: Add extra column in Z3950 search (14.98 KB, patch)
2018-08-12 15:56 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 12747: Add extra column in Z3950 search (13.97 KB, patch)
2018-08-29 15:44 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 12747: Add extra column in Z3950 search (13.97 KB, patch)
2018-08-29 15:45 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 12747: (QA follow-up) Treat 010 according to marcflavour (1.26 KB, patch)
2018-08-29 15:46 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 12747: Add extra column in Z3950 search (14.12 KB, patch)
2018-09-14 18:39 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 12747: (QA follow-up) Treat 010 according to marcflavour (1.47 KB, patch)
2018-09-14 18:39 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 12747: Add extra column in Z3950 search (14.17 KB, patch)
2018-09-24 20:58 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 12747: (QA follow-up) Treat 010 according to marcflavour (1.52 KB, patch)
2018-09-24 20:58 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 12747: (QA follow-up) Rephrase system preference description (1.33 KB, patch)
2018-09-24 20:58 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description simith.doliveira 2014-08-11 16:41:07 UTC

    
Comment 1 simith.doliveira 2014-08-11 18:43:05 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2014-08-13 18:45:56 UTC
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 %]
Comment 3 Jonathan Druart 2015-02-26 14:35:11 UTC
This needs an answer.
Comment 4 simith.doliveira 2015-03-11 19:51:30 UTC Comment hidden (obsolete)
Comment 5 simith.doliveira 2015-03-11 19:52:06 UTC
Table cell fixed 

Owen #2 "why there is no corresponding change to cataloguing/z3950_search.pl?
- Our sponsor (CCSR) did not ask for it.
Comment 6 Blou 2015-04-14 19:18:39 UTC
*** Bug 11874 has been marked as a duplicate of this bug. ***
Comment 7 Blou 2015-04-14 19:21:40 UTC Comment hidden (obsolete)
Comment 8 Blou 2015-04-14 19:25:51 UTC
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 :)
Comment 9 Marc Véron 2015-04-15 09:30:02 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2015-04-17 12:56:41 UTC
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 )
Comment 11 Charles Farmer 2015-04-20 21:27:45 UTC Comment hidden (obsolete)
Comment 12 Marc Véron 2015-04-21 08:56:37 UTC Comment hidden (obsolete)
Comment 13 Charles Farmer 2015-04-21 12:54:33 UTC Comment hidden (obsolete)
Comment 14 Charles Farmer 2015-04-21 13:03:53 UTC
(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.
Comment 15 Marc Véron 2015-04-21 14:55:40 UTC
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
Comment 16 Charles Farmer 2015-04-21 15:57:42 UTC Comment hidden (obsolete)
Comment 17 Marc Véron 2015-04-22 19:49:50 UTC
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
Comment 18 Charles Farmer 2015-04-23 17:31:21 UTC Comment hidden (obsolete)
Comment 19 Charles Farmer 2015-04-23 17:31:37 UTC Comment hidden (obsolete)
Comment 20 Charles Farmer 2015-04-23 17:37:28 UTC
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.
Comment 21 Marc Véron 2015-04-23 19:18:33 UTC Comment hidden (obsolete)
Comment 22 Marc Véron 2015-04-23 19:23:06 UTC Comment hidden (obsolete)
Comment 23 Marc Véron 2015-04-23 19:24:23 UTC
Comment on attachment 38457 [details] [review]
Bug 13606: DBRev 3.19.00.029

Sorry, mistaked again...
Comment 24 Marc Véron 2015-04-23 19:29:53 UTC Comment hidden (obsolete)
Comment 25 Marc Véron 2015-04-23 19:31:31 UTC Comment hidden (obsolete)
Comment 26 Marc Véron 2015-04-23 19:33:52 UTC
Note: I forgot to add [Signed-off] to title of 2nd patch.
Comment 27 Jonathan Druart 2015-06-24 11:36:55 UTC
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.
Comment 28 Rémi Mayrand-Provencher 2015-08-03 14:43:14 UTC Comment hidden (obsolete)
Comment 29 Rémi Mayrand-Provencher 2015-08-03 15:01:02 UTC Comment hidden (obsolete)
Comment 30 Rémi Mayrand-Provencher 2015-08-03 15:01:06 UTC Comment hidden (obsolete)
Comment 31 Rémi Mayrand-Provencher 2015-08-03 15:01:10 UTC Comment hidden (obsolete)
Comment 32 Jerwyn 2015-09-03 14:39:57 UTC
If were not mistaken, nothing happen with the patch.

Kind regards,
Comment 33 Frédéric Demians 2015-11-10 12:19:28 UTC
- 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.
Comment 34 Jonathan Druart 2015-11-10 12:25:14 UTC
(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)
Comment 35 I'm just a bot 2016-03-06 21:58:15 UTC
.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.
Comment 36 Charles Farmer 2016-12-23 22:30:00 UTC Comment hidden (obsolete)
Comment 37 Charles Farmer 2016-12-23 22:30:07 UTC Comment hidden (obsolete)
Comment 38 Charles Farmer 2016-12-23 22:30:14 UTC Comment hidden (obsolete)
Comment 39 Gracie 2017-01-19 03:28:30 UTC
The patch was successful for the acquisitions but not for the cataloging
Comment 40 Bouzid Fergani 2017-03-09 15:39:56 UTC Comment hidden (obsolete)
Comment 41 Jesse Maseto 2017-07-06 15:24:11 UTC Comment hidden (obsolete)
Comment 42 Philippe Audet-Fortin 2017-08-29 15:08:16 UTC Comment hidden (obsolete)
Comment 43 Philippe Audet-Fortin 2017-08-29 15:08:44 UTC Comment hidden (obsolete)
Comment 44 Philippe Audet-Fortin 2017-08-29 15:08:57 UTC Comment hidden (obsolete)
Comment 45 Philippe Audet-Fortin 2017-08-29 15:09:14 UTC Comment hidden (obsolete)
Comment 46 Philippe Audet-Fortin 2017-08-29 15:11:35 UTC Comment hidden (obsolete)
Comment 47 Hugo Agud 2017-08-31 10:27:20 UTC
I have applied the patch to kohadevbox and proceed with test plan and It works according the expected
Comment 48 Marcel de Rooy 2017-09-01 08:20:42 UTC
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??
Comment 49 Blou 2017-09-01 10:42:13 UTC
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.  :)
Comment 50 David Bourgault 2017-09-22 15:38:32 UTC
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
Comment 51 David Bourgault 2017-09-22 15:40:03 UTC Comment hidden (obsolete)
Comment 52 M. Tompsett 2017-10-16 14:54:41 UTC
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 53 M. Tompsett 2017-10-16 15:12:50 UTC
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 54 M. Tompsett 2017-10-16 15:14:15 UTC
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 55 M. Tompsett 2017-10-16 15:16:32 UTC
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 56 M. Tompsett 2017-10-16 15:21:36 UTC
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.
Comment 57 M. Tompsett 2017-10-16 15:54:26 UTC
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.
Comment 58 M. Tompsett 2017-10-16 23:00:27 UTC
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.
Comment 59 David Bourgault 2017-10-17 14:11:05 UTC Comment hidden (obsolete)
Comment 60 David Bourgault 2017-10-17 14:16:12 UTC
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.
Comment 61 M. Tompsett 2017-10-17 14:22:10 UTC
Oh the code is so much prettier. Thank you. I'll sign off after lunch, after retest.
Comment 62 M. Tompsett 2017-10-17 19:16:32 UTC
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.
Comment 63 David Bourgault 2017-11-22 21:38:29 UTC Comment hidden (obsolete)
Comment 64 Maksim Sen 2018-03-12 15:27:16 UTC Comment hidden (obsolete)
Comment 65 Hugo Agud 2018-03-13 13:46:40 UTC
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
Comment 66 Roch D'Amour 2018-03-14 19:34:30 UTC Comment hidden (obsolete)
Comment 67 Roch D'Amour 2018-03-14 19:35:23 UTC Comment hidden (obsolete)
Comment 68 Roch D'Amour 2018-03-14 19:36:26 UTC Comment hidden (obsolete)
Comment 69 Séverine Queune 2018-03-15 16:37:56 UTC Comment hidden (obsolete)
Comment 70 Séverine Queune 2018-03-15 16:38:04 UTC Comment hidden (obsolete)
Comment 71 Séverine Queune 2018-03-15 16:38:12 UTC Comment hidden (obsolete)
Comment 72 Séverine Queune 2018-03-15 16:40:26 UTC
Tested with Unimarc DB, worked as described on both basket and cataloguing.
Thanks for this enhancement !
Comment 73 Katrin Fischer 2018-04-10 06:09:32 UTC
 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!
Comment 74 Roch D'Amour 2018-04-10 13:58:30 UTC Comment hidden (obsolete)
Comment 75 Jonathan Druart 2018-04-10 14:55:28 UTC
(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.
Comment 76 Roch D'Amour 2018-04-10 15:19:20 UTC
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.
Comment 77 Roch D'Amour 2018-04-10 15:38:30 UTC Comment hidden (obsolete)
Comment 78 Katrin Fischer 2018-04-11 06:19:09 UTC
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.
Comment 79 Katrin Fischer 2018-04-11 06:21:21 UTC
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
Comment 80 Roch D'Amour 2018-04-11 19:28:02 UTC Comment hidden (obsolete)
Comment 81 Roch D'Amour 2018-04-11 19:31:06 UTC Comment hidden (obsolete)
Comment 82 Roch D'Amour 2018-04-11 19:33:34 UTC Comment hidden (obsolete)
Comment 83 Roch D'Amour 2018-04-11 19:43:27 UTC Comment hidden (obsolete)
Comment 84 Roch D'Amour 2018-04-11 19:44:11 UTC Comment hidden (obsolete)
Comment 85 Séverine Queune 2018-04-24 08:09:53 UTC
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>
Comment 86 Séverine Queune 2018-04-24 08:09:59 UTC
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>
Comment 87 Séverine Queune 2018-04-24 08:10:05 UTC
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>
Comment 88 Séverine Queune 2018-04-24 08:10:21 UTC
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>
Comment 89 Séverine Queune 2018-04-24 08:10:26 UTC
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>
Comment 90 Séverine Queune 2018-04-24 08:14:57 UTC
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.
Comment 91 Katrin Fischer 2018-04-25 05:56:48 UTC
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.
Comment 92 David Bourgault 2018-05-04 19:49:20 UTC
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
Comment 93 David Bourgault 2018-05-04 19:49:39 UTC
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 94 Katrin Fischer 2018-08-12 15:51:58 UTC
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
Comment 95 Katrin Fischer 2018-08-12 15:56:18 UTC
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>
Comment 96 Katrin Fischer 2018-08-29 06:15:13 UTC
Can you please take a look at comment#94? Sorry for leaving this unfinished, will try to return asap when fixed.
Comment 97 Katrin Fischer 2018-08-29 06:17:51 UTC
Note: signed off this one, so won't be able to QA - unless someone else gives it another sign-off then.
Comment 98 Charles Farmer 2018-08-29 15:44:45 UTC Comment hidden (obsolete)
Comment 99 Charles Farmer 2018-08-29 15:45:09 UTC
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>
Comment 100 Charles Farmer 2018-08-29 15:46:08 UTC
Created attachment 78272 [details] [review]
Bug 12747: (QA follow-up) Treat 010 according to marcflavour
Comment 101 Charles Farmer 2018-08-29 15:50:18 UTC
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
Comment 102 sandboxes@biblibre.com 2018-09-14 18:38:15 UTC
Patch tested with a sandbox, by Séverine QUEUNE <severine.queune@bulac.fr>
Comment 103 sandboxes@biblibre.com 2018-09-14 18:39:05 UTC
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>
Comment 104 sandboxes@biblibre.com 2018-09-14 18:39:12 UTC
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>
Comment 105 Katrin Fischer 2018-09-24 20:58:05 UTC
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>
Comment 106 Katrin Fischer 2018-09-24 20:58:13 UTC
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>
Comment 107 Katrin Fischer 2018-09-24 20:58:21 UTC
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.
Comment 108 Nick Clemens 2018-10-01 16:31:37 UTC
Awesome work all!

Pushed to master for 18.11
Comment 109 Martin Renvoize 2018-10-05 15:40:27 UTC
Enhancement, will not be backported to 18.05.x series.