Bug 14217 - Add a DOM syntax for specifying conditions on indexes
Summary: Add a DOM syntax for specifying conditions on indexes
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 14453
Blocks: 14277 15555
  Show dependency treegraph
 
Reported: 2015-05-19 03:16 UTC by Tomás Cohen Arazi
Modified: 2017-06-14 22:10 UTC (History)
6 users (show)

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


Attachments
Sample MARCXML record (3.61 KB, application/xml)
2015-07-08 17:19 UTC, Tomás Cohen Arazi
Details
Bug 14217: Add 'condition' attribute for DOM index definition (8.31 KB, patch)
2015-07-08 17:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14217: Add 'condition' attribute for DOM index definition (8.38 KB, patch)
2015-08-27 18:25 UTC, Barton Chittenden
Details | Diff | Splinter Review
Bug 14217: (QA followup) avoid repeating code (7.13 KB, patch)
2015-09-21 15:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14217: (QA followup) avoid repeating code (7.20 KB, patch)
2015-09-21 16:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14217: Add 'condition' attribute for DOM index definition (7.59 KB, patch)
2015-09-21 16:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14217: Add 'condition' attribute for DOM index definition (7.66 KB, patch)
2015-09-21 16:31 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2015-05-19 03:16:52 UTC
It would be great to have a way on the XML syntax to specify conditions for fields to get indexed.

For example: we could want to index the 100$a subfield on "Author" only if the 1st indicator is not 'z'.

The current syntax doesn't allow us to specify that.
Comment 1 Tomás Cohen Arazi 2015-07-08 17:19:41 UTC
Created attachment 40841 [details]
Sample MARCXML record

Sample record containing a 100$a field, with ind2=7 for testing purposes
Comment 2 Tomás Cohen Arazi 2015-07-08 17:20:24 UTC Comment hidden (obsolete)
Comment 3 Barton Chittenden 2015-08-27 15:43:48 UTC
(In reply to Tomás Cohen Arazi from comment #2)

> To test:
> - Verify that the shipped XSLT files are current regarding the shipped index
> definitions:
>   $ for i in marc21 normarc unimarc; do
>         xsltproc etc/zebradb/xsl/xsl/koha-indexdefs-to-zebra.xsl \
>               etc/zebradb/marc_defs/$i/biblios/biblio-koha-indexdefs.xml \
>               > etc/zebradb/marc_defs/$i/biblios/biblio-zebra-indexdefs.xsl
>     done

Small change in test plan:

    etc/zebradb/xsl/xsl/koha-indexdefs-to-zebra.xsl

should be

    etc/zebradb/xsl/koha-indexdefs-to-zebra.xsl

so the first step in the test plan becomes.

- Verify that the shipped XSLT files are current regarding the shipped index
definitions:
  $ for i in marc21 normarc unimarc; do
        xsltproc etc/zebradb/xsl/koha-indexdefs-to-zebra.xsl \
              etc/zebradb/marc_defs/$i/biblios/biblio-koha-indexdefs.xml \ 
              > etc/zebradb/marc_defs/$i/biblios/biblio-zebra-indexdefs.xsl
    done
Comment 4 Barton Chittenden 2015-08-27 18:25:08 UTC Comment hidden (obsolete)
Comment 5 Barton Chittenden 2015-08-27 18:31:32 UTC
I also tested the case where the indicator does *not* match, just for completeness.
Comment 6 Marcel de Rooy 2015-08-28 06:58:09 UTC
This is certainly an interesting development!

But it might need some further (broader) discussion before getting in.
In the first place: Why do we really need it? Can you elaborate/illustrate?
If we add conditions, what kind of conditions do we want? Syntax? Etc. A broader design may be easier to maintain..
How do comparable systems implement this?
If we add this for Zebra, what about other search engines?
Comment 7 Barton Chittenden 2015-08-28 12:32:06 UTC
(In reply to Marcel de Rooy from comment #6)
> This is certainly an interesting development!
> 
> But it might need some further (broader) discussion before getting in.
> In the first place: Why do we really need it? Can you elaborate/illustrate?
> If we add conditions, what kind of conditions do we want? Syntax? Etc. A
> broader design may be easier to maintain..
> How do comparable systems implement this?
> If we add this for Zebra, what about other search engines?

Marcel,

The functionality definitely *is* needed -- see, for instance bug 14277, where the meaning of 521$a varies drastically based on the value of ind1.

The 'condition=' syntax feels like overkill to me -- before work with the inicators, I assumed that the arguments to the <index_subfields> xml tag were essentially limits that were ANDed together... I guess they're not, but I think that we could pull a 'convention over configuration' fast one and pretend that they are: 'tag' and 'subfield' are required attributes, specifying the marc field and subfield, alll other attributes are limits -- if not present, all matching tags/subfields will be used.

...or we could go with the 'condition=' syntax, and make it explicit. I'm fine either way, as long as I can limit by indicator. :-)
Comment 8 Tomás Cohen Arazi 2015-08-28 12:48:59 UTC
(In reply to Marcel de Rooy from comment #6)
> This is certainly an interesting development!
> 
> But it might need some further (broader) discussion before getting in.
> In the first place: Why do we really need it? Can you elaborate/illustrate?
> If we add conditions, what kind of conditions do we want? Syntax? Etc. A
> broader design may be easier to maintain..
> How do comparable systems implement this?

There are some situations where you need to add conditions as Barton told you. Possible conditions (the ones I had in mind are conditions on the indicator values, as I stated on the commit message). As Barton wrote, if you want to send a field/subfield to a different index depending on the value of the indicators, you can do it with this.
If you look at it, we already do it for authority records, but with a more complex syntax. Look at authority-koha-indexdefs.xml:

  <kohaidx:index_heading_conditional tag="450" test="substring(marc:subfield[@code='w']/text(), 2, 1)" subfields="abvxyz" subdivisions="vxyz">
    <kohaidx:target_index>Previous-heading-see-from:p</kohaidx:target_index>
  </kohaidx:index_heading_conditional>

the whole syntax is similar to the one i implement, but you need to add a new tag name if you want conditional indexing, while i just overload the current syntax, which is really convenient.

> If we add this for Zebra, what about other search engines?

Each search engine provides means to specify what/how to index. I guess stuff like ES are more flexible than Zebra, for sure.
Comment 9 Jonathan Druart 2015-09-03 10:31:17 UTC
I haven't tested yet, but looking at the code I am wondering if there is not a way to add a flag 'index_it!' instead of c/p the 2 foreach marc::subfield loops?
Comment 10 Tomás Cohen Arazi 2015-09-03 10:48:55 UTC
(In reply to Jonathan Druart from comment #9)
> I haven't tested yet, but looking at the code I am wondering if there is not
> a way to add a flag 'index_it!' instead of c/p the 2 foreach marc::subfield
> loops?

Maybe elaborate a bit more? Remember the current syntax lets you choose what subfields to include for indexing, so you need to loop.
Anyway, I just hooked the extra 'condition' attribute and then test for it to skip if condition is not met.

If you propose a major XSLT refactoring it should probably belong to it's own bug
Comment 11 Jonathan Druart 2015-09-03 11:01:35 UTC
If I am not wrong, the algorithm is:

if conditions exist:
  if the condition is satisfied:
    do a big loop on subfield
else:
  do the same big loop on subfield

You have c/p the "big loop"

I suggest to change with something like

need_to_loop = 1
if conditions exist:
  need_to_loop = 0
  if the condition is satisfied:
    need_to_loop = 1

if need_to_loop:
  do the same big loop on subfield

Just to avoid the code duplication.
Comment 12 Tomás Cohen Arazi 2015-09-21 15:49:51 UTC Comment hidden (obsolete)
Comment 13 Tomás Cohen Arazi 2015-09-21 16:04:17 UTC Comment hidden (obsolete)
Comment 14 Tomás Cohen Arazi 2015-09-21 16:08:32 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2015-09-21 16:31:32 UTC
Created attachment 42741 [details] [review]
Bug 14217: Add 'condition' attribute for DOM index definition

This patch introduces an extension to the current syntax for DOM index definition.
Specifically, it extends the 'index_subfields' tag to allow adding a 'condition'
attribute that is used as a condition ofr applying the specified index.

This (exotic) example is self-explanatory:

The previous syntax (which is keeped by this patch) took this snippet from biblio-koha-indexdefs.xml

  <index_subfields tag="100" subfields="acbd">
    <target_index>Encuadernador:w</target_index>
  </index_subfields>

and generated an XSLT snippet in the DOM indexing XSLT that looks like this:

    <xslo:for-each select="marc:subfield">
      <xslo:if test="contains('acbd', @code)">
        <z:index name="Encuadernador:w">
          <xslo:value-of select="."/>
        </z:index>
      </xslo:if>
    </xslo:for-each>

This patch introduces this syntax change (note the 'condition' attribute:

  <index_subfields tag="100" subfields="acbd" condition="@ind2='7'">
    <target_index>Encuadernador:w</target_index>
  </index_subfields>

which yields to this XSLT snippet in the DOM indexing XSLT:

    <xslo:if test="@ind2='7'">
      <xslo:for-each select="marc:subfield">
        <xslo:if test="contains('acbd', @code)">
          <z:index name="Encuadernador:w">
            <xslo:value-of select="."/>
          </z:index>
        </xslo:if>
      </xslo:for-each>
    </xslo:if>

To test:
- Verify that the shipped XSLT files are current regarding the shipped index definitions:
  $ for i in marc21 normarc unimarc; do
        xsltproc etc/zebradb/xsl/koha-indexdefs-to-zebra.xsl \
              etc/zebradb/marc_defs/$i/biblios/biblio-koha-indexdefs.xml \
              > etc/zebradb/marc_defs/$i/biblios/biblio-zebra-indexdefs.xsl
    done
  $ git status
(repeat for authorities, skip normarc which doesn't have authorities)
- Apply the patch
- Re-run the previous commands
=> SUCCESS: no changes
- Add a condition to an index_subfields tag (for example, condition="@ind2='7'" in the Author's index
- Regenerate the specific XSLT
=> SUCCESS: doing a diff shows the only change is the code has been wrapped inside an xslo:if using the condition for the test
- Apply the generated xsl to a MARCXML record that has a field matching the condition like this:
  $ xsltproc .../biblio-zebra-indexdefs.xsl sample_record.xml
=> SUCCESS: There's an index on the result, containing the configured field/subfields, that matches the criteria.
- Sign off and feel really happy :-D

Note: the attached sample record includes a 100 field, with ind2=7 and $a=Tomasito

Edit: This patch was squashed once I figured it got too complex and Jonathan required a followup
to avoid code duplication.

This avoids code duplication, with the same results.

Sponsored-by: Orex Digital
Signed-off-by: Barton Chittenden <barton@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 16 David Cook 2015-09-21 23:09:49 UTC
Ooooh. I really like this. It might be useful for "Private" notes as well.

Example:
http://www.loc.gov/marc/bibliographic/bd541.html

First Indicator
Privacy
# - No information provided 
0 - Private 
1 - Not private 

Of course, in this case, it means it wouldn't be indexed for both the OPAC and the Staff Client... but I suppose that's a choice that could be made at the library level when tweaking their indexing.
Comment 17 Tomás Cohen Arazi 2015-09-25 14:58:35 UTC
Patch pushed to master.