Bug 12252 - OAI-PMH GetRecord result doesn't include item data
Summary: OAI-PMH GetRecord result doesn't include item data
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Robin Sheat
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 13475 (view as bug list)
Depends on: 3206 13568
Blocks: 17744
  Show dependency treegraph
 
Reported: 2014-05-14 02:35 UTC by Robin Sheat
Modified: 2016-12-08 09:12 UTC (History)
13 users (show)

See Also:
Change 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:


Attachments
Bug 12252 - OAI-PMH result includes item data (3.14 KB, patch)
2014-05-14 02:52 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 12252 - OAI-PMH result includes item data (3.24 KB, patch)
2014-05-14 03:41 UTC, David Cook
Details | Diff | Splinter Review
Bug 12252 - OAI-PMH result includes item data (3.31 KB, patch)
2014-05-14 08:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12252 - OAI-PMH items respect OpacHiddenItems (4.61 KB, patch)
2014-05-20 05:17 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 12252 - OAI-PMH items respect OpacHiddenItems (5.32 KB, patch)
2014-05-20 05:30 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 12252 - include item data in OAI-PMH ListRecords result (1.71 KB, patch)
2014-06-23 05:19 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 12252 - include item data in OAI-PMH ListRecords result (1.71 KB, patch)
2014-06-23 05:24 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 12252 - OAI-PMH result includes item data (3.42 KB, patch)
2014-11-01 15:34 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 12252 - OAI-PMH items respect OpacHiddenItems (5.54 KB, patch)
2014-11-01 15:35 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 12252 - include item data in OAI-PMH ListRecords result (1.89 KB, patch)
2014-11-01 15:36 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 12252: Add tests for EmbedItemsInMarcBiblio (3.74 KB, patch)
2015-01-02 15:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12252 - OAI-PMH result includes item data (3.48 KB, patch)
2015-01-07 14:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12252 - OAI-PMH items respect OpacHiddenItems (5.60 KB, patch)
2015-01-07 14:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12252 - include item data in OAI-PMH ListRecords result (1.95 KB, patch)
2015-01-07 14:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12252: Add tests for EmbedItemsInMarcBiblio (4.25 KB, patch)
2015-01-07 14:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12252: (qa-follow-up) Remove perlcritic error (1.22 KB, patch)
2015-01-07 14:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12252: (qa-follow-up) Remove perlcritic error (1.22 KB, patch)
2015-01-07 14:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12252 - OAI-PMH result includes item data (5.07 KB, patch)
2015-07-23 16:47 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 12252 - OAI-PMH items respect OpacHiddenItems (4.83 KB, patch)
2015-07-23 16:48 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 12252 - include item data in OAI-PMH ListRecords result (2.09 KB, patch)
2015-07-23 16:48 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 12252: Add tests for EmbedItemsInMarcBiblio (4.25 KB, patch)
2015-07-23 16:48 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 12252: (qa-follow-up) Remove perlcritic error (1.22 KB, patch)
2015-07-23 16:48 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 12252 Include item data only in extended mode (4.91 KB, patch)
2015-07-23 16:49 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 12252 - OAI-PMH result includes item data [SIGNED-OFF] (5.14 KB, patch)
2015-07-24 16:41 UTC, Gaetan Boisson
Details | Diff | Splinter Review
Bug 12252 - OAI-PMH items respect OpacHiddenItems [SIGNED-OFF] (4.90 KB, patch)
2015-07-24 16:42 UTC, Gaetan Boisson
Details | Diff | Splinter Review
Bug 12252 - include item data in OAI-PMH ListRecords result [SIGNED-OFF] (2.16 KB, patch)
2015-07-24 16:42 UTC, Gaetan Boisson
Details | Diff | Splinter Review
Bug 12252: Add tests for EmbedItemsInMarcBiblio [SIGNED-OFF] (4.31 KB, patch)
2015-07-24 16:42 UTC, Gaetan Boisson
Details | Diff | Splinter Review
Bug 12252: (qa-follow-up) Remove perlcritic error [SIGNED-OFF] (1.29 KB, patch)
2015-07-24 16:42 UTC, Gaetan Boisson
Details | Diff | Splinter Review
Bug 12252 Include item data only in extended mode [SIGNED-OFF] (4.21 KB, patch)
2015-07-24 16:42 UTC, Gaetan Boisson
Details | Diff | Splinter Review
Bug 12252 - OAI-PMH result includes item data [SIGNED-OFF] (5.14 KB, patch)
2015-08-27 12:30 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12252 - OAI-PMH items respect OpacHiddenItems [SIGNED-OFF] (4.94 KB, patch)
2015-08-27 12:30 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12252 - include item data in OAI-PMH ListRecords result [SIGNED-OFF] (2.16 KB, patch)
2015-08-27 12:31 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12252: Add tests for EmbedItemsInMarcBiblio [SIGNED-OFF] (4.39 KB, patch)
2015-08-27 12:31 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12252: (qa-follow-up) Remove perlcritic error [SIGNED-OFF] (1.29 KB, patch)
2015-08-27 12:31 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12252 Include item data only in extended mode [SIGNED-OFF] (4.22 KB, patch)
2015-08-27 12:31 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12252: (follow-up) Fix error raised on comment 69 http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12252#c69 (917 bytes, patch)
2015-08-27 12:32 UTC, Alex Arnaud
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12252: (follow-up) Fix error raised on comment 69 http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12252#c69 (984 bytes, patch)
2015-09-03 12:44 UTC, Gaetan Boisson
Details | Diff | Splinter Review
Bug 12252 - OAI-PMH result includes item data (5.22 KB, patch)
2015-09-16 09:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12252 - OAI-PMH items respect OpacHiddenItems (5.02 KB, patch)
2015-09-16 09:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12252 - include item data in OAI-PMH ListRecords result (2.24 KB, patch)
2015-09-16 09:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12252: Add tests for EmbedItemsInMarcBiblio (4.44 KB, patch)
2015-09-16 09:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12252: (qa-follow-up) Remove perlcritic error (1.37 KB, patch)
2015-09-16 09:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12252 Include item data only in extended mode (4.29 KB, patch)
2015-09-16 09:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12252: (follow-up) Include item data only in extended mode (1.00 KB, patch)
2015-09-16 09:53 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 Robin Sheat 2014-05-14 02:35:33 UTC
Fetching the MARCXML via the OAI-PMH interface doesn't include item data, however for harvesting purposes, it should do.
Comment 1 Robin Sheat 2014-05-14 02:52:55 UTC Comment hidden (obsolete)
Comment 2 David Cook 2014-05-14 03:41:51 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2014-05-14 08:04:41 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2014-05-14 08:06:06 UTC
It looks like a bug, isn't it? I changed from enh to normal.
Comment 5 Robin Sheat 2014-05-14 08:49:43 UTC
I wasn't sure, but I don't disagree :-)
Comment 6 Galen Charlton 2014-05-19 22:10:43 UTC
Setting to in discussion -- arguably, it would be a bug that the proposed change does not respect the OpacHiddenItems syspref.
Comment 7 Galen Charlton 2014-05-19 22:12:03 UTC
Also, changing back to an enhancement, as it's not clear to me that folks wanting to run Koha as an OAI-PMH provider would *always* want to include item data.
Comment 8 Robin Sheat 2014-05-19 22:51:30 UTC
(In reply to Galen Charlton from comment #6)
> Setting to in discussion -- arguably, it would be a bug that the proposed
> change does not respect the OpacHiddenItems syspref.

It's probable that supporting it wouldn't be too hard. I think support would have to be added at the GetMarcBiblio level.

(In reply to Galen Charlton from comment #7)
> Also, changing back to an enhancement, as it's not clear to me that folks
> wanting to run Koha as an OAI-PMH provider would *always* want to include
> item data.

On the flip side, I'm going to argue that not having item data was a regression caused by the removal of item data from the stored MARC record, because it would have worked then. This means it's indeed a bug! :)
Comment 9 David Cook 2014-05-19 23:23:18 UTC
(In reply to Galen Charlton from comment #6)
> Setting to in discussion -- arguably, it would be a bug that the proposed
> change does not respect the OpacHiddenItems syspref.

I think that this one can be tricky. 

On one hand, I would want the OAI-PMH server to honor OpacHiddenItems and the MARC frameworks, so that we weren't sending records with private/hidden information (whether stored at the item-level or bib-level).

On the other hand, I wonder if there are times where you do want to serve complete (i.e. staff side) records. 

I suppose we could assume the former case of treating the OAI-PMH server as "public", and if there arose a need for a "private" stream, we could add authentication. I've seen a few OAI-PMH servers out there that require username and password to access them.
Comment 10 Robin Sheat 2014-05-20 05:17:51 UTC Comment hidden (obsolete)
Comment 11 Robin Sheat 2014-05-20 05:30:03 UTC Comment hidden (obsolete)
Comment 12 David Cook 2014-06-06 05:15:41 UTC
I suppose I'm a bit hesitant to sign off on this one at the moment because of Mark's bug 11592.

That said, I'm not going to sign off on Mark's patch either, as I'm not sold on the code there...
Comment 13 Robin Sheat 2014-06-06 05:31:18 UTC
Assuming Mark's code is backwards compatible, it should work anyway.
Comment 14 Robin Sheat 2014-06-23 05:19:07 UTC Comment hidden (obsolete)
Comment 15 Robin Sheat 2014-06-23 05:24:56 UTC Comment hidden (obsolete)
Comment 16 Robin Sheat 2014-06-23 05:25:45 UTC
...I forgot the OpacHiddenItems thing again...
Comment 17 Frédéric Demians 2014-11-01 15:34:17 UTC Comment hidden (obsolete)
Comment 18 Frédéric Demians 2014-11-01 15:35:55 UTC Comment hidden (obsolete)
Comment 19 Frédéric Demians 2014-11-01 15:36:38 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2014-12-18 10:34:19 UTC
*** Bug 13475 has been marked as a duplicate of this bug. ***
Comment 21 Jonathan Druart 2014-12-18 10:46:27 UTC
Comment on attachment 33121 [details] [review]
Bug 12252 - OAI-PMH items respect OpacHiddenItems

Review of attachment 33121 [details] [review]:
-----------------------------------------------------------------

::: C4/Biblio.pm
@@ +1263,5 @@
> +=item C<$embeditems>
> +
> +set to true to include item information.
> +
> +=item C<$opac>

This new parameter will be used in all opac scripts where we want to embed items.

@@ +2928,5 @@
>      my @item_fields;
>      my ( $itemtag, $itemsubfield ) = GetMarcFromKohaField( "items.itemnumber", $frameworkcode );
> +    my @items;
> +    my $opachiddenitems = $opac
> +      && ( C4::Context->preference('OpacHiddenItems') !~ /^\s*$/ );

It would have been better to use /\S/ to match the existing test.

::: opac/oai.pl
@@ +280,4 @@
>  
>      # We fetch it using this method, rather than the database directly,
>      # so it'll include the item data
> +    my $record = GetMarcBiblio($biblionumber, 1, 1);

We really want to avoid this kind of calls, it's not readable.
Comment 22 Jonathan Druart 2014-12-18 10:49:29 UTC
I would like to get other QA point of view here.

The changes in GetMarcBiblio does not provided any tests. But none of GetMarcBiblio and EmbedItemsInMarcBiblio have tests!
It would be very good nice to see these 2 subroutines tested, but should we block this patch set for this reason?
Comment 23 Marcel de Rooy 2014-12-18 11:48:44 UTC
(In reply to Jonathan Druart from comment #22)
> I would like to get other QA point of view here.
> 
> The changes in GetMarcBiblio does not provided any tests. But none of
> GetMarcBiblio and EmbedItemsInMarcBiblio have tests!
> It would be very good nice to see these 2 subroutines tested, but should we
> block this patch set for this reason?

From a glance, it looks like adding some tests for at least EmbedItems would not be a bad thing here.
Comment 24 Jonathan Druart 2015-01-02 15:14:58 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2015-01-07 14:25:14 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2015-01-07 14:25:18 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2015-01-07 14:25:22 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2015-01-07 14:25:26 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2015-01-07 14:30:29 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2015-01-07 14:32:35 UTC Comment hidden (obsolete)
Comment 31 Katrin Fischer 2015-01-18 18:12:07 UTC
Didn't test yet, but wondering: how are we dealing with the subfields marked as 'don't show in OPAC'? Thinking of the internal notes especially.
Comment 32 Gaetan Boisson 2015-01-19 09:00:23 UTC
I might be arriving late to the party but, if i am not mistaken, OAI in Koha can work in extended mode, with a custom xslt. I think that would be the right way of filtering things out in the exported data. Otherwise we might have situations where we need to hide certain fields on the opac, but want to have them in the exported data, which wouldn't be possible then.
Comment 33 Katrin Fischer 2015-01-19 09:04:17 UTC
Hm, not sure I agree. I think internal note holds the promise that noone outside the library can see it and afaik the extended mode is not documented in a way that it would be easy to add those constraints for a library.
Comment 34 Katrin Fischer 2015-01-19 09:06:34 UTC
Maybe better documentation and a switch to include/not include? The export feature in the staff client allows to list fields to 'not export'.
Comment 35 Gaetan Boisson 2015-01-19 10:04:53 UTC
That's more complicated than i first thought indeed. My understanding at first was that the library would want to filter out some fields to import in the OAI harvester, but Jonathan pointed out that anyone can query the koha OAI provider and that the real issue is data that would be considered confidential and that should never be exported.

I would say we have two separate issues though:

- Having items data in the OAI export. If we don't have it, this is a problem for harvesting purposes, which even defeats the point of being an OAI provider in some situations.

- Some confidential data can be exposed through this. But Jonathan and i just tried it, and it's not just the OAI interface, if you save the record as marc or marcxml from the opac, this data will be in there. So we don't respect this confidentiality here either.

I would say this ticket should remain about adding item data, and we should create another bug report, that would tackle the issue of exporting sensitive data everywhere this could happen. what do you think?
Comment 36 Katrin Fischer 2015-01-19 10:27:26 UTC
I am not sure if the current problem is something that 'used to work'. I filed a bug recently about the plain MARC view in OPAC where I was fairly sure that the fields would not be included - but they show.

So this is indeed a wider issue, just not sure if it's new or things we broke with some other changes over time :(

If we just quietly add the items and expose the data, this seems problematic to me. Could it be a switch with a warning for now?
Comment 37 Katrin Fischer 2015-01-19 12:11:24 UTC
OPAC visibility turns out to be quite a mess. Not blocking this, but still wondering if we can document this somewhere.

Maybe add a note to the pref that it will include all item data?
Comment 38 Gaetan Boisson 2015-01-19 13:38:31 UTC
Adding a note to the OAI-PMH preference so that it reads like the following would be ok for me:

able/disable Koha's OAI-PMH server. The data exposed will not include items hidden through the OpacHiddenItems system preference, but everything else will be available for harvest. If you need to hide some data, look into extended mode and the OAI-PMH:ConfFile system preference.

(It does look to me as enabling extended mode with a configuration file allowing only one type of metadata that would result from a custom xslt removing the confidential fields would be enough.)
Comment 39 Robin Sheat 2015-01-19 21:54:04 UTC
(In reply to Gaetan Boisson from comment #35)
> I would say this ticket should remain about adding item data, and we should
> create another bug report, that would tackle the issue of exporting
> sensitive data everywhere this could happen. what do you think?

Yes, this! :)

I kinda think that internal notes shouldn't ever go into the MARC anyway, it should perhaps be a separate table. Otherwise the chance of leak is quite high. But this is definitely a separate issue.
Comment 40 Katrin Fischer 2015-01-19 21:59:14 UTC
I like Gaetan's suggestion - we discussed this some more on IRC earlier today, the 'visibility' problem is indeed too big for this bug. 

But apart from how it should be... We have an 'internal note' in the item and the configuration options to hide in OPAC ... but we are not doing it right.
Comment 41 David Cook 2015-01-20 02:22:13 UTC
(In reply to Robin Sheat from comment #39)
> (In reply to Gaetan Boisson from comment #35)
> > I would say this ticket should remain about adding item data, and we should
> > create another bug report, that would tackle the issue of exporting
> > sensitive data everywhere this could happen. what do you think?
> 
> Yes, this! :)
> 
> I kinda think that internal notes shouldn't ever go into the MARC anyway, it
> should perhaps be a separate table. Otherwise the chance of leak is quite
> high. But this is definitely a separate issue.

+1

Gaetan: There's been a bit of work done on the opac detail page (see http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11592), but I'd definitely like to see a more consistent approach that respects the "visibility settings" everywhere in Koha (i.e. tools export, OAI, cart, search results, detail page, etc.)

I think Tomas had an idea about this... about using the frameworks to generate an "exclusion" XSLT which MARCXML would be run through after retrieving a record from Zebra or MySQL.

I suppose the only time that you might want to not use the "exclusion" XSLT would be for... well perhaps a few scenarios. Most places in the staff client would probably want to show it, and only hide it if you're sending the record elsewhere... (unless you're exporting it to a cataloguing tool where you want to re-import afteward).

Hmm, food for thought!
Comment 42 David Cook 2015-01-20 02:23:34 UTC
(In reply to Katrin Fischer from comment #40)
> I like Gaetan's suggestion - we discussed this some more on IRC earlier
> today, the 'visibility' problem is indeed too big for this bug. 
> 
> But apart from how it should be... We have an 'internal note' in the item
> and the configuration options to hide in OPAC ... but we are not doing it
> right.

Agreed.

Definitely too big for this bug... but perhaps the "visibility" problem blocks this bug?

Yeah, an "internal note" in the item and ineffectual configuration options to hide in the OPAC :/.
Comment 43 Tomás Cohen Arazi 2015-07-08 18:32:31 UTC
I definitely think this might not be a feature for some institutions and we should make it opt-in. Because as Galen mentioned, it is not obvious that everyone will want to include items data.
Comment 44 Robin Sheat 2015-07-08 22:45:13 UTC
(In reply to Tomás Cohen Arazi from comment #43)
> I definitely think this might not be a feature for some institutions and we
> should make it opt-in. Because as Galen mentioned, it is not obvious that
> everyone will want to include items data.

Well, it is there for harvesting. The reason I added it is because the harvesting required the item data so it could be presented.
Comment 45 Jonathan Druart 2015-07-09 07:23:29 UTC
(In reply to Tomás Cohen Arazi from comment #43)
> I definitely think this might not be a feature for some institutions and we
> should make it opt-in. Because as Galen mentioned, it is not obvious that
> everyone will want to include items data.

I am pretty sure it's a bug and a regression introduced by bug 5579.
Comment 46 Katrin Fischer 2015-07-09 12:33:52 UTC
(In reply to Gaetan Boisson from comment #38)
> Adding a note to the OAI-PMH preference so that it reads like the following
> would be ok for me:
> 
> able/disable Koha's OAI-PMH server. The data exposed will not include items
> hidden through the OpacHiddenItems system preference, but everything else
> will be available for harvest. If you need to hide some data, look into
> extended mode and the OAI-PMH:ConfFile system preference.
> 
> (It does look to me as enabling extended mode with a configuration file
> allowing only one type of metadata that would result from a custom xslt
> removing the confidential fields would be enough.)

I liked this idea from Gaetan. But thinking about it more I am still torn:

This is a change in behaviour. People might have built things assuming the item data is not there and might suddenly expose more data, than they wanted to.

I know that Jonathan is probably right in that this was initially a regression from bug 5579, but we missed to fix this for a really long time now :(

Would it be really bad to make the items a configuration option? I know some people hate new prefs... but the Web Services tab still has plenty of room...
Comment 47 Gaetan Boisson 2015-07-09 14:22:50 UTC
> This is a change in behaviour. People might have built things assuming the
> item data is not there and might suddenly expose more data, than they wanted
> to.

I agree but they will still be exposing this data, even if they switch it off for OAI, through the marc view, the marc downloads, etc.

It's true i think we have enough sysprefs, but especially here, i feel we will add one to do something that can already be achieved using extended mode, and that it will only very partially fix an issue we have in a lot of other places.
Comment 48 Tomás Cohen Arazi 2015-07-09 15:03:27 UTC
Maybe we should add an option to control this on the YAML configuration for OAI sets, so it is set-specific.
Comment 49 Tomás Cohen Arazi 2015-07-10 12:10:28 UTC
(In reply to Tomás Cohen Arazi from comment #48)
> Maybe we should add an option to control this on the YAML configuration for
> OAI sets, so it is set-specific.

Something like:

       format:
         with_items:
           metadataPrefix: with_items
           metadataNamespace: http://koha-community.org/marcxml/with_items
           schema: http://koha-community.org/marcxml/with_items/xsd
           xsl_file: /usr/local/koha/xslt/marcxml.xsl
           include_items: yes
Comment 50 Katrin Fischer 2015-07-12 21:50:41 UTC
Hm, I am not sure it works setting this on Set level as a lot of libraries might not be using sets - they are an optional feature and not needed if you want to get harvest the full catalog I think.

Maybe Gaetan is right - I am really not sure what's best here.
Comment 51 David Cook 2015-07-13 00:50:12 UTC
(In reply to Katrin Fischer from comment #50)
> Hm, I am not sure it works setting this on Set level as a lot of libraries
> might not be using sets - they are an optional feature and not needed if you
> want to get harvest the full catalog I think.

Yeah, doing it at the set level wouldn't make sense. 

I don't know anything about Koha's "OAI-PMH:ConfFile" or "extended mode", so can't really comment there...

I certainly get the idea of it being optional though and having the default be not to include items...
Comment 52 Frédéric Demians 2015-07-23 06:08:36 UTC
Any decision on the horizon? As said by Johnathan, this patch solves a regression introduced by bug 5579. The data visibility breach is a real issue. But most of the libraries using Koha OAI Server that I know need to expose item data, at least call number, and location. Without item data, their OAI server is almost useless... 

Couldn't we agree with Thomas solution? In Normal mode, no item data. In Extended mode, optional item data. This way, item will only appear back on purpose.
Comment 53 Gaetan Boisson 2015-07-23 12:22:21 UTC
I agree with Thomas' solution, although, to be picky, i would do it the other way around and would make exposing all the data the default, and filtering out things the specific behaviour ;)

But i would also like to see this solved soon, so i really don't mind what the default is in the end.
Comment 54 Frédéric Demians 2015-07-23 14:39:01 UTC
Bug 3206 prevents this patch to apply anymore.
Comment 55 Frédéric Demians 2015-07-23 15:08:29 UTC
> Bug 3206 prevents this patch to apply anymore.

and Bug 13568
Comment 56 Frédéric Demians 2015-07-23 16:47:56 UTC Comment hidden (obsolete)
Comment 57 Frédéric Demians 2015-07-23 16:48:15 UTC Comment hidden (obsolete)
Comment 58 Frédéric Demians 2015-07-23 16:48:27 UTC Comment hidden (obsolete)
Comment 59 Frédéric Demians 2015-07-23 16:48:45 UTC Comment hidden (obsolete)
Comment 60 Frédéric Demians 2015-07-23 16:48:58 UTC Comment hidden (obsolete)
Comment 61 Frédéric Demians 2015-07-23 16:49:12 UTC Comment hidden (obsolete)
Comment 62 Gaetan Boisson 2015-07-24 16:41:55 UTC Comment hidden (obsolete)
Comment 63 Gaetan Boisson 2015-07-24 16:42:03 UTC Comment hidden (obsolete)
Comment 64 Gaetan Boisson 2015-07-24 16:42:13 UTC Comment hidden (obsolete)
Comment 65 Gaetan Boisson 2015-07-24 16:42:20 UTC Comment hidden (obsolete)
Comment 66 Gaetan Boisson 2015-07-24 16:42:25 UTC Comment hidden (obsolete)
Comment 67 Gaetan Boisson 2015-07-24 16:42:31 UTC Comment hidden (obsolete)
Comment 68 Gaetan Boisson 2015-07-24 16:46:25 UTC
Works great, items will not be shown unless you set up a specific configuration file with 
    include_items: 1
in the options for the marcxml metadata prefix.
Comment 69 Matthias Meusburger 2015-08-04 07:23:04 UTC
When applying the patch on master, I get the following error when trying to display an existing record:

Request:
/cgi-bin/koha/oai.pl?verb=GetRecord&identifier=KOHA-OAI-TEST:10&metadataPrefix=oai_dc

Response:
Can't use an undefined value as a subroutine reference at /home/koha/versions/kohacommunity/opac/oai.pl line 337.
Comment 70 Gaetan Boisson 2015-08-04 07:40:24 UTC
Indeed, that was sloppy testing on my part: i tested the listrecord verb, verified deleted records and resumption token where properly handled, but it turns out the getrecord verb is broken. Marking as failed.
Comment 71 Alex Arnaud 2015-08-27 12:30:39 UTC Comment hidden (obsolete)
Comment 72 Alex Arnaud 2015-08-27 12:30:53 UTC Comment hidden (obsolete)
Comment 73 Alex Arnaud 2015-08-27 12:31:05 UTC Comment hidden (obsolete)
Comment 74 Alex Arnaud 2015-08-27 12:31:17 UTC Comment hidden (obsolete)
Comment 75 Alex Arnaud 2015-08-27 12:31:36 UTC Comment hidden (obsolete)
Comment 76 Alex Arnaud 2015-08-27 12:31:50 UTC Comment hidden (obsolete)
Comment 77 Alex Arnaud 2015-08-27 12:32:01 UTC Comment hidden (obsolete)
Comment 78 Alex Arnaud 2015-08-27 12:37:03 UTC
I have rebased this patch because i didn't manage to apply them (with or without git-bz) and added a follow up to fix the error on comment 69.

However, i didn't understand the way the OAI server's config file works. I created a YAML file containing only "include_items: 1" and now i get an error while retrieving a record: <error code="cannotDisseminateFormat">Dissemination as 'oai_dc' is not supported</error>
Comment 79 Gaetan Boisson 2015-08-31 07:45:26 UTC
The conf file needs to be more complete, an example conf file would be (note there was a small typo in Frederic's example in the metadataPrefix, but this one should work):

---
format:
  marcxml:
    metadataPrefix: marcxml
    metadataNamespace: http://www.loc.gov/MARC21/slim http://www.loc.gov/standards/marcxml/schema/MARC21slim
    schema: http://www.loc.gov/MARC21/slim http://www.loc.gov/standards/marcxml/schema/MARC21slim.xsd
    include_items: 1
  oai_dc:
    metadataNamespace: http://www.openarchives.org/OAI/2.0/oai_dc/
    metadataPrefix: oai_dc
    schema: http://www.openarchives.org/OAI/2.0/oai_dc.xsd
    xsl_file: /home/koha/src/koha-tmpl/intranet-tmpl/prog/en/xslt/UNIMARCslim2OAIDC.xsl
Comment 80 Fridolin SOMERS 2015-08-31 08:48:07 UTC
There is a problem I think in "OAI-PMH items respect OpacHiddenItems" :

This patch changes GetMarcBiblio adding "opac" arg.
But there is no : $opac = shift
Comment 81 Fridolin SOMERS 2015-08-31 08:58:19 UTC
(In reply to Fridolin SOMERS from comment #80)
> There is a problem I think in "OAI-PMH items respect OpacHiddenItems" :
> 
> This patch changes GetMarcBiblio adding "opac" arg.
> But there is no : $opac = shift
Oups sorry, its corrected by patch : "Include item data only in extended mode"
Comment 82 Alex Arnaud 2015-08-31 12:44:44 UTC
Thx Gaetan, it works fine with this configuration file. The QA script hasn't found any issues so the last patch could be checked for singing off.
Comment 83 Gaetan Boisson 2015-09-03 12:44:45 UTC Comment hidden (obsolete)
Comment 84 Gaetan Boisson 2015-09-03 12:46:44 UTC
Checked all the OAI verbs presented on this page : http://re.cs.uct.ac.za/ everything went smoothly.
Comment 85 Alex Arnaud 2015-09-04 08:12:56 UTC
apologies for my last commit message.
Comment 86 Jonathan Druart 2015-09-15 08:18:38 UTC
One question: How librarians will know this feature exists (No, they don't read release notes)?
There is no way to know about it.


Gaetan: Please don't add the "[SIGNED-OFF]" string in the commit messages :)
Comment 87 Gaetan Boisson 2015-09-15 21:54:33 UTC
I think it should go in the manual, in the section that documents the OAI-PMH:ConfFile syspref. (Somewhere in this section : http://translate.koha-community.org/manual/master/en/html-desktop/#webserviceprefs )
Comment 88 Jonathan Druart 2015-09-16 09:53:02 UTC
Created attachment 42575 [details] [review]
Bug 12252 - OAI-PMH result includes item data

GetRecord for OAI-PMH was pulling the MARCXML directly from the
database. Now it uses GetMarcBiblio and includes the item data with it,
making it more generally useful.

Test plan:
* Run an OAI-PMH query, for example:
http://koha/cgi-bin/koha/oai.pl?verb=GetRecord&identifier=KOHA-OAI-TEST:52&metadataPrefix=marcxml
  to fetch biblionumber 52
* Note that it doesn't include the 952 data
* Apply the patch
* Do the same thing, but this time see that the 952 data is at the
  bottom of the MARCXML.

Note:
* This patch also includes a small tidy-up in C4::Biblios to group
  things semantically a bit better, so I don't spend ages looking for a
  function that was staring me in the face all along again.

Signed-off-by: David Cook <dcook@prosentient.com.au>

Works as described. Simple yet useful patch.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Frederic Demians <f.demians@tamil.fr>

952/995 item fields are back in response to GetRecord OAI verb.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Signed-off-by: Gaetan Boisson <gaetan.boisson@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 89 Jonathan Druart 2015-09-16 09:53:07 UTC
Created attachment 42576 [details] [review]
Bug 12252 - OAI-PMH items respect OpacHiddenItems

This allows the OAI-PMH service to not provide item information when
there is a rule that would supress it in OpacHiddenItems.

Test plan:
* Find an OAI-PMH URL that shows you some items.
* Add an entry to OpacHiddenItems that would block that.
* Check that it's blocked.

Signed-off-by: Frederic Demians <f.demians@tamil.fr>

Tested, playing with OpacHiddenItems. GetRecord OAI verb returns a record
complying with OpacHiddenItems rules, for example without items from a specific
library.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Signed-off-by: Gaetan Boisson <gaetan.boisson@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 90 Jonathan Druart 2015-09-16 09:53:11 UTC
Created attachment 42577 [details] [review]
Bug 12252 - include item data in OAI-PMH ListRecords result

Same in spirit to the other patch, this also includes the item detail in
ListRecords.

Test plan:
* Fetch a URL like:
http://koha/cgi-bin/koha/oai.pl?verb=ListRecords&metadataPrefix=marcxml
* Verify that there are 952 entries in the returned records where
  appropriate.

Signed-off-by: Frederic Demians <f.demians@tamil.fr>

ListRecords OAI verb returns a list of records including items in 952/995 which
are not hidden based on OpacHiddenItems syspref.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Signed-off-by: Gaetan Boisson <gaetan.boisson@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 91 Jonathan Druart 2015-09-16 09:53:15 UTC
Created attachment 42578 [details] [review]
Bug 12252: Add tests for EmbedItemsInMarcBiblio

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Signed-off-by: Gaetan Boisson <gaetan.boisson@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 92 Jonathan Druart 2015-09-16 09:53:19 UTC
Created attachment 42579 [details] [review]
Bug 12252: (qa-follow-up) Remove perlcritic error

Signed-off-by: Signed-off-by: Gaetan Boisson <gaetan.boisson@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 93 Jonathan Druart 2015-09-16 09:53:24 UTC
Created attachment 42580 [details] [review]
Bug 12252 Include item data only in extended mode

Previous patches attached to this bug have been refactored to merge bug
3206 and bug 13568 features. So OAI server must be carrefully tested to
ensure that there is no regression in this area: deleted records and
resumption token.

This last patch fixed the way items are returned. They are returned only
if OAI server operates in extended mode, and specifically for format
having the parameter include_item set to 1 (true). For example this
configuration file set via OAI-PMH:ConfFile syspref will return items:

Signed-off-by: Signed-off-by: Gaetan Boisson <gaetan.boisson@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 94 Jonathan Druart 2015-09-16 09:53:28 UTC
Created attachment 42581 [details] [review]
Bug 12252: (follow-up) Include item data only in extended mode

Fix perlcritic error.

Signed-off-by: Signed-off-by: Gaetan Boisson <gaetan.boisson@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 95 Tomás Cohen Arazi 2015-09-18 15:08:45 UTC
Patches pushed to master.

Thanks Robin and everyone involved.