Bug 12404

There is a conflict on circ/circulation.tt. Could you fix it?

Bug 10703 introduces a big regression, see bug 13190.
Has to be apply before this patch set.

For me, there is a design error in you dev. You send to TT a representation of
MARC record which is not correct. You loose the field order in the record, and
subfields order inside each field. The fields object sent to TT is a hashref of
arrayref of hashref of array. The valid representation is an arrayref (1 per
tag) of arrayref (1 per subfield) of arrayref (1 per pair letter/value).

So for example (bad example), if you have:

245 ## - TITLE STATEMENT
  a Title My first title
  b Remainder of title My remainder
  a Title My second title

With your represenation, you can't know anymore that the second $a comes after
$b. In MARC21, it's important since separators are in the subfield content
itself. Your 245 field will be represented like that:

  245 => {
    'a' => [
      'My first title',
      'My second title'
    ],
    'b' => [
      'My remainder'
    ]
  }

(In reply to Frédéric Demians from comment #13)
> For me, there is a design error in you dev. You send to TT a representation
> of
> MARC record which is not correct. You loose the field order in the record,
> and
> subfields order inside each field. The fields object sent to TT is a hashref
> of
> arrayref of hashref of array. The valid representation is an arrayref (1 per
> tag) of arrayref (1 per subfield) of arrayref (1 per pair letter/value).

It's easier to manipulate hashref than arrayref in this case.
If you want to access to the $b subfield, you don't need to process all subfields.
Could you give me an where this structure would be a problem for a CSV export?

(In reply to Jonathan Druart from comment #14)

> It's easier to manipulate hashref than arrayref in this case.  If you want to
> access to the $b subfield, you don't need to process all subfields.  Could
> you give me an where this structure would be a problem for a CSV export?

I agree. I have seen TT syntax you can get: fields.245, fields.245.a. And then
[% FOREACH %] on hashref keys... It's elegant, but it is also a trap.

In MARC21, you can have this kind of title field:

  245 14 $a The phonics basic skills workbook. 
         $n Part 1, 
         $p Reading readiness
         $h [videorecording] : 
         $b preparing for kindergarten.

which should be rendered: The phonics basic skills workbook. Part 1, Reading
readiness [videorecording] : preparing for kindergarten.

With your field representation, you loose the subfield order: a, n, p, h, and
b. There is chance a TT construction will try to put $b just after $a, which is
wrong.

(In reply to Frédéric Demians from comment #15)
> With your field representation, you loose the subfield order: a, n, p, h, and
> b. There is chance a TT construction will try to put $b just after $a, which
> is
> wrong.

I am not sure to understand the problem.
In our case (CSV), you will set something like:
===
Title=[% FOREACH field IN fields.245 %][% field.a.0 %] [% field.a.n %] [% field.a.p %] [% field.a.h %] [% field.a.b %][% END %]
===

Since it's not possible to display an entire field and you need to list the subfields you want to display, the order is not a problem.

But maybe I am missing something.

IMO this way allows us a very good flexibility (since we can use the TT methods) with a minimum of code.

> I am not sure to understand the problem.
> In our case (CSV), you will set something like:
> ===
> Title=[% FOREACH field IN fields.245 %][% field.a.0 %] [% field.a.n %] [%
> field.a.p %] [% field.a.h %] [% field.a.b %][% END %]
> ===
> 
> Since it's not possible to display an entire field and you need to list the
> subfields you want to display, the order is not a problem.

Let suppose than another valid 245 field contains (from LoC MARC21 format):

245 00 $a Love from Joy :
       $b letters from a farmer’s wife.
       $n Part III,
       $p 1987-1995, At the bungalow.

It must be displayed (and exported): Love from Joy : letters from a farmer’s
wife. Part III, 1987-1995, At the bungalow.

With you TT (minus typo): Love from Joy : Part III, 1987-1995, At the bungalow.
letters from a farmer's wife.

You can't have an unique TT for all 245 subfields combinations.

> IMO this way allows us a very good flexibility (since we can use the TT
> methods) with a minimum of code.

It still possible. Fields order isn't that important, but sufields is. You
could build an hashref of arrayref of arrayref. For a $record containing a
MARC::Record (its a variation of your code):

my $fields;
for my $field ( $record->fields ) {
    my $fieldtag = $field->tag;
    my $values;
    if ( $field->is_control_field ) {
        $values = $field->data();
    } else {
        $values->{indicator}{1} = $field->indicator(1);
        $values->{indicator}{2} = $field->indicator(2);
        for ( $field->subfields ) {
            my ($letter, $value) = @$_;
            push @{ $values->{subf} }, [ $letter => $value ];
        }
    }
    # We force the key as an integer (trick for 00X and OXX fields)
    push @{ $fields->{fields}{0+$fieldtag} }, $values;
}

And then to display the first 245 field, all subfields in order:

[% FOREACH s IN fields.245.first.subf %][% s.1 %] [% END %]

(In reply to Frédéric Demians from comment #17)
> It still possible. Fields order isn't that important, but sufields is. You
> could build an hashref of arrayref of arrayref. For a $record containing a
> MARC::Record (its a variation of your code):

I accept, of course, all kinds of followup to improve this enhancement :)

(In reply to Jonathan Druart from comment #18)

> I accept, of course, all kinds of followup to improve this enhancement :)

I don't say no :-) I've even tried.

But I'me encountering now an issue with the mix in the code between previous syntax and TT syntax. I can't have a TT directive containing a = sign. For example:

  [% IF s.0 == 'b' %]
  [% SET letter = s.0 %]

The = sign is used to split the CSV profile here:

# Separating the marcfields from the user-supplied headers
    my @csv_structures;
    foreach (@marcfieldsarray) {
        my @result = split('=', $_);
        my $content = ( @result == 2 )
            ? $result[1]
            : $result[0];

So it breaks valid TT code.

(In reply to Frédéric Demians from comment #19)
> (In reply to Jonathan Druart from comment #18)
> 
> > I accept, of course, all kinds of followup to improve this enhancement :)
> 
> I don't say no :-) I've even tried.
> 
> But I'me encountering now an issue with the mix in the code between previous
> syntax and TT syntax. I can't have a TT directive containing a = sign.

The last patch should allow you to do what you want.

(In reply to Jonathan Druart from comment #21)

> The last patch should allow you to do what you want.

It works if I have something like that (with an arrayref of subfields):

Title=[% FOREACH s IN fields.200.first.subfields %][% IF s.0 == "a" %][% s.1 %][% END %][% END %]

It doesn't work with:

[% FOREACH s IN fields.200.first.subfields %][% IF s.0 == "a" %][% s.1 %][% END %][% END %]

The editing workflow is really awkward. After saving a profile, you have to do several clicks, to return on edit the CSV profile. And worst, you haven't any feedback, syntax verification, or a visual feedback. You have to switch to Download cart to see the result. When there is no result, you can surmise you have done something wrong.

(In reply to Frédéric Demians from comment #22)
> (In reply to Jonathan Druart from comment #21)
> 
> > The last patch should allow you to do what you want.
> 
> It works if I have something like that (with an arrayref of subfields):
> 
> Title=[% FOREACH s IN fields.200.first.subfields %][% IF s.0 == "a" %][% s.1
> %][% END %][% END %]
> 
> It doesn't work with:
> 
> [% FOREACH s IN fields.200.first.subfields %][% IF s.0 == "a" %][% s.1 %][%
> END %][% END %]

Hum, yes. I have no solution for this case. It's a limitation.

> The editing workflow is really awkward. After saving a profile, you have to
> do several clicks, to return on edit the CSV profile. And worst, you haven't
> any feedback, syntax verification, or a visual feedback. You have to switch
> to Download cart to see the result. When there is no result, you can surmise
> you have done something wrong.

Yes, I totally agree...
But it is not in the scope of this enhancement.

I tested this with two CSV profils :

Without TT methods
1. Personal name=200|Entry element=210$a|300|009 -> It works

With TT methods
2. 245i1=[% fields.245.0.indicator.2 %]|245ac=[% FOREACH field IN fields.245 %] [% field.a.0 %] [% field.c.0 %] [% END %] | Subject=[% FOREACH field IN fields.650 %][% IF field.indicator.2 %][% field.a.0 %] [% END %][% END %] | Language=[% fields.008.0.substr( 28, 3 ) %] |  Title=[% IF fields.100.0.indicator.1 %][% fields.245.0.a.0 %][% END %]

I used the examples in the help section for the last one. I also works BUT I had to correct one method.

The help section says :
Display all 245$a and 245$c into the same column:
[% FOREACH field IN fields.245 %] [% field.a %] [% field.c %] [% END %] 

but it doesn't work. I think it should be corrected by :
[% FOREACH field IN fields.245 %] [% field.a.0 %] [% field.c.0 %] [% END %]
which works.

Since I'm not familiar with template toolkit, I let you confirm it's correct.

Once the documentation is corrected I'll test it again to sign it off!

(In reply to Francois Charbonnier from comment #24)

I updated the documentation in the last patch!

Patch tested with a sandbox, by scourret@gmail.com <2>

Problem with sandbox, back to needs signoff.

patches rebased, conflicts with bug 13308 fixed.

Patch works for displaying subfields in the same column. Also works for displaying the first subfield for a field if specified indicator  for field  is present. Can display a subfield if an indicator is set. 
So most of the features are working without trouble. 
But it looks like I’m unable to extract a substring from a field (typically, UNIMARC language field from 100$a – character 22-24). May someone test it ?

(In reply to Courret from comment #41)
> But it looks like I’m unable to extract a substring from a field (typically,
> UNIMARC language field from 100$a – character 22-24). May someone test it ?

Did you try something like

  [% fields.100.0.a.0.substr( 22, 3 ) %]

?

(In reply to Jonathan Druart from comment #42)
> (In reply to Courret from comment #41)
> > But it looks like I’m unable to extract a substring from a field (typically,
> > UNIMARC language field from 100$a – character 22-24). May someone test it ?
> 
> Did you try something like
> 
>   [% fields.100.0.a.0.substr( 22, 3 ) %]
> 
> ?

It works ! Seems good. For me, as far as I have tested, every feature is OK and patch can be validated.

Created attachment 35048 [details] [review]
[PASSED QA] Bug 12404: Add UT for marcrecord2csv

Verify that these unit tests pass before any changes and after applying
all patches.

Signed-off-by: Courret <scourret@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 35049 [details] [review]
[PASSED QA] Bug 12404: Add new unit tests

These unit tests reflect the changes done in next patches.

Signed-off-by: Courret <scourret@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 35050 [details] [review]
[PASSED QA] Bug 12404: Allow TT tags for csv profiles

This patch is the main patch. It contains the changes in
C4::Record::marcrecord2csv.

The goal of this development is to provide a better flexibility on
creating a CSV profile.
Currently it is not possible to:
- Concatenate specific subfields into a csv column
- Display a field/subfield using a condition
- Extract a substring of a subfield value
and a lot of other actions.

This patch allows to write Template Toolkit code and to extract only
data you want.

See the help page for more information (in next patch).

Test plan:
Create some CSV profiles (MARC, not SQL) using some TT methods.
Use the basket export and the export tool and verify the CSV file
generated is what you expected.

Signed-off-by: Courret <scourret@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 35051 [details] [review]
[PASSED QA] Bug 12404: tools/export.pl allows CSV export

Signed-off-by: Courret <scourret@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 35052 [details] [review]
[PASSED QA] Bug 12404: Add some documentation in the help page.

Signed-off-by: Courret <scourret@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 35053 [details] [review]
[PASSED QA] Bug 12404: Allow equal sign '=' in the TT directive

Signed-off-by: Courret <scourret@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 35054 [details] [review]
[PASSED QA] Bug 12404: FIX documentation to join subfield

To display all subfields, the join TT method must be used.

Signed-off-by: Courret <scourret@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Feature pushed to master.

Thanks Jonathan!

Appears sadly to be undocumented!

(In reply to Katrin Fischer from comment #59)
> Appears sadly to be undocumented!

I can be retrieved from
  commit 57b48279bdf781e74329735fc3f5068da8398ac4
  Bug 12404: Add some documentation in the help page.

I tried to use an example from the old help file, but I got:

+  <li>Display all 245$a and 245$c into the same column:
+    <p>
+    [% FOREACH field IN fields.245 %]
+        [% field.a %] [% field.c %]
+    [% END %]
+    </p>
+  </li>

Titel=[% FOREACH field IN fields.245 %][% field.a %] [% field.b %] [% field.n %] [% field.p %] [% field.c %][% END %]

ARRAY(0x5642032a0638) ARRAY(0x564202a832f0) ARRAY(0x564203388d48) ARRAY(0x5642033cc020) ARRAY(0x564203439cc0)

(In reply to Katrin Fischer from comment #61)
> I tried to use an example from the old help file, but I got:
> 
> +  <li>Display all 245$a and 245$c into the same column:
> +    <p>
> +    [% FOREACH field IN fields.245 %]
> +        [% field.a %] [% field.c %]
> +    [% END %]
> +    </p>
> +  </li>
> 
> Titel=[% FOREACH field IN fields.245 %][% field.a %] [% field.b %] [%
> field.n %] [% field.p %] [% field.c %][% END %]
> 
> ARRAY(0x5642032a0638) ARRAY(0x564202a832f0) ARRAY(0x564203388d48)
> ARRAY(0x5642033cc020) ARRAY(0x564203439cc0)

field.a contains the list of a's.
The following works:
Title=[% FOREACH field IN fields.245 %][% field.a.0 %] [% field.b.0 %] [% field.n.0 %] [% field.p.0 %] [% field.c.0 %][% END %]

(In reply to Jonathan Druart from comment #62)
> (In reply to Katrin Fischer from comment #61)
> > I tried to use an example from the old help file, but I got:
> > 
> > +  <li>Display all 245$a and 245$c into the same column:
> > +    <p>
> > +    [% FOREACH field IN fields.245 %]
> > +        [% field.a %] [% field.c %]
> > +    [% END %]
> > +    </p>
> > +  </li>
> > 
> > Titel=[% FOREACH field IN fields.245 %][% field.a %] [% field.b %] [%
> > field.n %] [% field.p %] [% field.c %][% END %]
> > 
> > ARRAY(0x5642032a0638) ARRAY(0x564202a832f0) ARRAY(0x564203388d48)
> > ARRAY(0x5642033cc020) ARRAY(0x564203439cc0)
> 
> field.a contains the list of a's.
> The following works:
> Title=[% FOREACH field IN fields.245 %][% field.a.0 %] [% field.b.0 %] [%
> field.n.0 %] [% field.p.0 %] [% field.c.0 %][% END %]

Ha, there is a follow-up commit:
  commit c33c56028fb39a464a780b2a22e95af0da73585f
  Bug 12404: FIX documentation to join subfield

The correct syntax is:
   <li>Display all 245$a and 245$c into the same column:
     <p>
     [% FOREACH field IN fields.245 %]
       [% field.a.join(' ') %] [% field.c.join(' ') %]
     [% END %]
     </p>
   </li>