Bug 24531 - OAI-PMH set mappings only consider first field with a given tag
Summary: OAI-PMH set mappings only consider first field with a given tag
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Magnus Enger
QA Contact: Josef Moravec
URL:
Keywords:
: 12731 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-01-29 14:27 UTC by Magnus Enger
Modified: 2020-03-05 20:38 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 24531 - Test for OAI-PMH sets and repeated fields (2.75 KB, patch)
2020-01-30 10:33 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 24531 - Fix OAI-PMH sets for records with repeated fields (1.54 KB, patch)
2020-01-30 10:33 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 24531 - Test for OAI-PMH sets and repeated fields (2.79 KB, patch)
2020-01-31 09:58 UTC, David Nind
Details | Diff | Splinter Review
Bug 24531 - Fix OAI-PMH sets for records with repeated fields (1.59 KB, patch)
2020-01-31 09:58 UTC, David Nind
Details | Diff | Splinter Review
Bug 24531 - Test for OAI-PMH sets and repeated fields (3.29 KB, patch)
2020-02-14 07:49 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 24531 - Fix OAI-PMH sets for records with repeated fields (1.82 KB, patch)
2020-02-14 07:49 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 24531 - Test for OAI-PMH sets and repeated fields (3.34 KB, patch)
2020-02-14 07:52 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 24531 - Fix OAI-PMH sets for records with repeated fields (1.86 KB, patch)
2020-02-14 07:52 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 24531: Test for OAI-PMH sets and repeated fields (3.40 KB, patch)
2020-02-14 08:54 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 24531: Fix OAI-PMH sets for records with repeated fields (1.92 KB, patch)
2020-02-14 08:54 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Magnus Enger 2020-01-29 14:27:23 UTC

    
Comment 1 Magnus Enger 2020-01-29 14:38:46 UTC
A mapping to say that a record should be included in a OAI-PMH set if it has the value "PDF" in field 347$b can be defined as:

Field = 347
Subfield = b
Operator = equals
Value = PDF

In C4::OAI::Sets::_evalRule, this is the code that picks out the relevant field and subfield: 

 544     my @subfield_values = $record->subfield($field, $subfield);

But this will only return the value from the first field with tag $field, it does not really work in array context. The code works, if your 347$b = PDF occurs as the first 347 field, but not if it occurs in a subsequent field 347. 

So the code needs to be expanded into something like this:

my @subfield_values;
my @fields = $record->field($field);
foreach my $field ( @fields ) {
    if ( my $subfield_value = $field->subfield($subfield) ) {
        push @subfield_values, $subfield_value;
    }
}
Comment 2 Magnus Enger 2020-01-30 10:33:27 UTC Comment hidden (obsolete)
Comment 3 Magnus Enger 2020-01-30 10:33:30 UTC Comment hidden (obsolete)
Comment 4 David Nind 2020-01-31 09:58:44 UTC Comment hidden (obsolete)
Comment 5 David Nind 2020-01-31 09:58:47 UTC Comment hidden (obsolete)
Comment 6 Josef Moravec 2020-02-05 20:54:52 UTC
What if there is repeated subfield with same code in one field? Then only first one is taken into account. I do think we want to count on all of them...
Comment 7 Magnus Enger 2020-02-06 08:01:31 UTC
(In reply to Josef Moravec from comment #6)
> What if there is repeated subfield with same code in one field? Then only
> first one is taken into account. I do think we want to count on all of
> them...

Gah, how did I miss that? I'll update the patch. Thanks!
Comment 8 Magnus Enger 2020-02-14 07:49:19 UTC Comment hidden (obsolete)
Comment 9 Magnus Enger 2020-02-14 07:49:22 UTC Comment hidden (obsolete)
Comment 10 Magnus Enger 2020-02-14 07:52:23 UTC
Created attachment 98896 [details] [review]
Bug 24531 - Test for OAI-PMH sets and repeated fields

This patch adds two tests related to OAI-PMH sets. It creates a
dummy record with a repeated field, with different values. It then
tests if the record is caught by mappings that match the value in
the first field, and then if it is caught by mappings that match
the second field.

Signed-off-by: David Nind <david@davidnind.com>
Comment 11 Magnus Enger 2020-02-14 07:52:26 UTC
Created attachment 98897 [details] [review]
Bug 24531 - Fix OAI-PMH sets for records with repeated fields

Currently, an OAI-PMH set mapping will only match if the value it
is looking for occurs in the first instance of a repeated field.

To test:
- Apply the first patch with two new tests
- Run something like this:
  sudo koha-shell -c "prove -v t/db_dependent/OAI/Sets.t" kohadev
- Verify that the last test fails
- Apply this secind patch
- Rerun the test file above
- Verify that all tests now pass

Signed-off-by: David Nind <david@davidnind.com>
Comment 12 Magnus Enger 2020-02-14 07:54:21 UTC
(In reply to Josef Moravec from comment #6)
> What if there is repeated subfield with same code in one field? Then only
> first one is taken into account. I do think we want to count on all of
> them...

I have updated the tests and the code to account for repeated subfields. I reset the status to SO, please reset to NSO if necessary.
Comment 13 Josef Moravec 2020-02-14 08:54:26 UTC
Created attachment 98906 [details] [review]
Bug 24531: Test for OAI-PMH sets and repeated fields

This patch adds two tests related to OAI-PMH sets. It creates a
dummy record with a repeated field, with different values. It then
tests if the record is caught by mappings that match the value in
the first field, and then if it is caught by mappings that match
the second field.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 14 Josef Moravec 2020-02-14 08:54:30 UTC
Created attachment 98907 [details] [review]
Bug 24531: Fix OAI-PMH sets for records with repeated fields

Currently, an OAI-PMH set mapping will only match if the value it
is looking for occurs in the first instance of a repeated field.

To test:
- Apply the first patch with two new tests
- Run something like this:
  sudo koha-shell -c "prove -v t/db_dependent/OAI/Sets.t" kohadev
- Verify that the last test fails
- Apply this secind patch
- Rerun the test file above
- Verify that all tests now pass

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 15 Josef Moravec 2020-02-14 08:54:54 UTC
(In reply to Magnus Enger from comment #12)
> (In reply to Josef Moravec from comment #6)
> > What if there is repeated subfield with same code in one field? Then only
> > first one is taken into account. I do think we want to count on all of
> > them...
> 
> I have updated the tests and the code to account for repeated subfields. I
> reset the status to SO, please reset to NSO if necessary.

Thanks Magnus, PQA now
Comment 16 Martin Renvoize 2020-02-14 12:02:20 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 17 Joy Nelson 2020-03-05 01:09:01 UTC
does not apply to 19.11.x
please rebase if needed
Comment 18 Josef Moravec 2020-03-05 08:55:07 UTC
(In reply to Joy Nelson from comment #17)
> does not apply to 19.11.x
> please rebase if needed

This patchset fixes the _evaRule sub which was added in Bug 21520. Looking into code I think that in 19.11.x series this bug exist also, but the patch need to be rework because of refactoring made in Bug 21520.
Comment 19 Tomás Cohen Arazi 2020-03-05 20:38:05 UTC
*** Bug 12731 has been marked as a duplicate of this bug. ***