Bug 27358 - Add GET /public/biblios/:biblio_id/items
Summary: Add GET /public/biblios/:biblio_id/items
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
Depends on: 28948 24254 27352
Blocks: 28854
  Show dependency treegraph
 
Reported: 2021-01-07 13:50 UTC by Tomás Cohen Arazi
Modified: 2021-10-13 09:53 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 27358: Add a generic way to handle API privileged access attributes deny-list (8.08 KB, patch)
2021-01-07 14:38 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27358: Add a generic way to handle API privileged access attributes deny-list (8.08 KB, patch)
2021-01-07 18:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27358: Make is_public stashed on public routes (2.42 KB, patch)
2021-01-07 18:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27358: Teach objects.search about public requests (2.87 KB, patch)
2021-01-07 18:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27358: Add GET /public/biblios/:biblio_id/items (5.04 KB, patch)
2021-01-07 18:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27358: Unit tests for public items retrieval (3.34 KB, patch)
2021-01-07 18:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27358: Add a generic way to handle API privileged access attributes deny-list (8.14 KB, patch)
2021-02-02 09:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27358: Make is_public stashed on public routes (2.49 KB, patch)
2021-02-02 09:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27358: Teach objects.search about public requests (2.93 KB, patch)
2021-02-02 09:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27358: Add GET /public/biblios/:biblio_id/items (5.10 KB, patch)
2021-02-02 09:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27358: Unit tests for public items retrieval (3.40 KB, patch)
2021-02-02 09:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27358: Add a generic way to handle API privileged access attributes deny-list (8.15 KB, patch)
2021-08-13 20:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27358: Make is_public stashed on public routes (2.48 KB, patch)
2021-08-13 20:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27358: Teach objects.search about public requests (2.83 KB, patch)
2021-08-13 20:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27358: Add GET /public/biblios/:biblio_id/items (5.11 KB, patch)
2021-08-13 20:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27358: Unit tests for public items retrieval (3.43 KB, patch)
2021-08-13 20:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27358: Add a generic way to handle API privileged access attributes deny-list (8.14 KB, patch)
2021-09-03 11:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27358: Make is_public stashed on public routes (2.48 KB, patch)
2021-09-03 11:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27358: Teach objects.search about public requests (2.83 KB, patch)
2021-09-03 11:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27358: Add GET /public/biblios/:biblio_id/items (5.11 KB, patch)
2021-09-03 11:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27358: Unit tests for public items retrieval (3.43 KB, patch)
2021-09-03 11:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27358: (QA follow-up) Convert to allow-list (4.01 KB, patch)
2021-09-03 11:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27358: Add GET /public/biblios/:biblio_id/items (5.06 KB, patch)
2021-10-13 09:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27358: Unit tests for public items retrieval (3.43 KB, patch)
2021-10-13 09:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27358: Unit tests for public items retrieval (3.43 KB, patch)
2021-10-13 09:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27358: Add GET /public/biblios/:biblio_id/items (6.53 KB, patch)
2021-10-13 09:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27358: Unit tests for public items retrieval (3.43 KB, patch)
2021-10-13 09:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27358: Add GET /public/biblios/:biblio_id/items (5.96 KB, patch)
2021-10-13 09:42 UTC, Martin Renvoize
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 2021-01-07 13:50:27 UTC
Let's add a public route to get items. This requires a way to filter out attributes that are not meant for public access.
Comment 1 Tomás Cohen Arazi 2021-01-07 13:51:15 UTC
I will need advise on the fields to hide for public access :-D
Comment 2 Tomás Cohen Arazi 2021-01-07 14:38:27 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2021-01-07 18:49:29 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2021-01-07 18:49:37 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2021-01-07 18:49:44 UTC Comment hidden (obsolete)
Comment 6 Tomás Cohen Arazi 2021-01-07 18:49:52 UTC Comment hidden (obsolete)
Comment 7 Tomás Cohen Arazi 2021-01-07 18:49:59 UTC Comment hidden (obsolete)
Comment 8 Katrin Fischer 2021-01-09 13:40:15 UTC
Hi Tomas,

if I understood correctly you wanted me to check on what fields should not be exposed on the public route? One is easy:

itemnotes_nonpublic

We also have libraries requesting to hide the acq related information (price fields, booksellerid, etc.) and other fields describing internal processes too. 

Maybe it would be easier to work backwards on what fields make the most sense to expose? 

Database fields (not the names for the API)

itemnumber
biblionumber
homebranch
holdingbranch
location
collectioncode
itemcallnumber
copynumber
enumchron
barcode
dateaccessioned (for finding new things)
itemnotes
onloan
uri
itype

Not sure how useful these would be as they require a solution for resolving the internal codes into something more useful:

notforloan
damaged
itemlost
withdrawn
restricted

It might make more sense to replace these by a more general availability route? I think we had done some work to specify things like "can be renewed", "can be checked out", "can be placed on hold" etc.


A more difficult one: sometimes strictly internal, sometimes visible in the OPAC:
materials

How will we be handling more_subfields_xml? As it can contain about anything, probably best to hide by default.

Could it make sense to fall back on the "visible in OPAC" setting in the frameworks here?
Comment 9 Tomás Cohen Arazi 2021-01-11 13:07:09 UTC
Hi Katrina, I like both ideas:
- A core list of fields that should be returned
- Filter the rest out based on the frameworks

About more_subfields_xml, they are not present in the current implementation. But we could do something about that too. with 'x-koha-embed' and a method that translates the XML into a hash, and filtering them using the frameworks.
Comment 10 Martin Renvoize 2021-01-26 12:46:30 UTC
This looks pretty clever and lays a nice foundation for other routes of a similar nature.

Testing :)
Comment 11 Martin Renvoize 2021-02-02 09:29:42 UTC Comment hidden (obsolete)
Comment 12 Martin Renvoize 2021-02-02 09:29:45 UTC Comment hidden (obsolete)
Comment 13 Martin Renvoize 2021-02-02 09:29:48 UTC Comment hidden (obsolete)
Comment 14 Martin Renvoize 2021-02-02 09:29:51 UTC Comment hidden (obsolete)
Comment 15 Martin Renvoize 2021-02-02 09:29:55 UTC Comment hidden (obsolete)
Comment 16 Martin Renvoize 2021-02-02 09:32:59 UTC
This works well and I like it.. signing off..


However, I have one security/qa comment... I think it would be best to reverse the logic here.. 

Instead of the `BlockList` approach, we have with `api_privileged_attrs` I think we should instead implement an `AllowList` to 'fail safe' if someone forgets to add a field to the list: `api_public_atts` perhaps?
Comment 17 Katrin Fischer 2021-02-06 18:51:15 UTC
I am a little worried about the short list here:

+sub api_privileged_attrs {
+    return [
+        'checked_out_date',
+        'checkouts_count',
+        'holds_count',
+        'internal_notes',
+        'extended_subfields',
+    ];
+}
+

Can you help me? Just wondering if it also uses the framework visibility, then I'd be happy already :)
Comment 18 Martin Renvoize 2021-02-08 18:55:49 UTC
Hmmm, I hadn't considered the framework visibility at all... I suppose that could work with the infrastructure here.. I still think we should convert the generic handling to be an AllowList approach, but the list method could easily be looked up from the frameworks... I think?
Comment 19 Tomás Cohen Arazi 2021-08-13 20:34:25 UTC Comment hidden (obsolete)
Comment 20 Tomás Cohen Arazi 2021-08-13 20:34:30 UTC Comment hidden (obsolete)
Comment 21 Tomás Cohen Arazi 2021-08-13 20:34:36 UTC Comment hidden (obsolete)
Comment 22 Tomás Cohen Arazi 2021-08-13 20:34:42 UTC Comment hidden (obsolete)
Comment 23 Tomás Cohen Arazi 2021-08-13 20:34:48 UTC Comment hidden (obsolete)
Comment 24 Tomás Cohen Arazi 2021-08-13 20:50:39 UTC
(In reply to Katrin Fischer from comment #17)
> I am a little worried about the short list here:
> 
> +sub api_privileged_attrs {
> +    return [
> +        'checked_out_date',
> +        'checkouts_count',
> +        'holds_count',
> +        'internal_notes',
> +        'extended_subfields',
> +    ];
> +}
> +
> 
> Can you help me? Just wondering if it also uses the framework visibility,
> then I'd be happy already :)

If we leave more_subfields_xml/frameworks out of the item representation (we have plans for that), would y'all help me refine this deny-list for the items?

I have just rebased this work and it still works nicely. If I don't get feedback in a few days, I will move the 'public' layer work to another (simpler) table, so other devs see the benefit from this and can work on top of it.

My feeling is we can have a list of 'hidden in opac' attributes from the 'items' table,  and then we can sort visibility in the views. I might be wrong, though. Looking for feedback.

Please PM me if you feel like there's a good use case that could be simpler than this (I'm thinking accountlines).
Comment 25 Martin Renvoize 2021-08-16 09:56:14 UTC
(In reply to Tomás Cohen Arazi from comment #24)
> (In reply to Katrin Fischer from comment #17)
> > I am a little worried about the short list here:
> > 
> > +sub api_privileged_attrs {
> > +    return [
> > +        'checked_out_date',
> > +        'checkouts_count',
> > +        'holds_count',
> > +        'internal_notes',
> > +        'extended_subfields',
> > +    ];
> > +}
> > +
> > 
> > Can you help me? Just wondering if it also uses the framework visibility,
> > then I'd be happy already :)
> 
> If we leave more_subfields_xml/frameworks out of the item representation (we
> have plans for that), would y'all help me refine this deny-list for the
> items?

I still think we should switch from 'deny-list' to 'allow-list'.. security by default ;)

> 
> I have just rebased this work and it still works nicely. If I don't get
> feedback in a few days, I will move the 'public' layer work to another
> (simpler) table, so other devs see the benefit from this and can work on top
> of it.

Hmm, I don't think it would be a bad idea to move the core idea to another, simpler, endpoint/table so other work can be based upon it.

> 
> My feeling is we can have a list of 'hidden in opac' attributes from the
> 'items' table,  and then we can sort visibility in the views. I might be
> wrong, though. Looking for feedback.

Hmm, not sure I understand this one.. do you mean expose fields in the API and only use the 'hidden in opac' options for the final display.. I can see a use case for that.. but I can also see people complaining that some hidden fields are still publically available if you know how to use the API.

> 
> Please PM me if you feel like there's a good use case that could be simpler
> than this (I'm thinking accountlines).

Accountlines could work.. though I still have a way to go regarding the api's there.  Questions like '/credits vs /debits vs /lines' and how embeds should work for offsets and things.
Comment 26 Martin Renvoize 2021-09-03 11:26:52 UTC Comment hidden (obsolete)
Comment 27 Martin Renvoize 2021-09-03 11:26:56 UTC Comment hidden (obsolete)
Comment 28 Martin Renvoize 2021-09-03 11:27:00 UTC Comment hidden (obsolete)
Comment 29 Martin Renvoize 2021-09-03 11:27:04 UTC Comment hidden (obsolete)
Comment 30 Martin Renvoize 2021-09-03 11:27:08 UTC Comment hidden (obsolete)
Comment 31 Martin Renvoize 2021-09-03 11:27:12 UTC Comment hidden (obsolete)
Comment 32 Martin Renvoize 2021-09-03 15:47:07 UTC
Cloned the foundational code to bug 28948 so we can keep this public allow list stuff moving.

Lets come back to this once that's pushed and just add the API route and additional filter by framework visibility stuff in this one.
Comment 33 Martin Renvoize 2021-10-13 09:13:06 UTC Comment hidden (obsolete)
Comment 34 Martin Renvoize 2021-10-13 09:13:10 UTC Comment hidden (obsolete)
Comment 35 Martin Renvoize 2021-10-13 09:34:32 UTC Comment hidden (obsolete)
Comment 36 Martin Renvoize 2021-10-13 09:34:36 UTC Comment hidden (obsolete)
Comment 37 Martin Renvoize 2021-10-13 09:42:14 UTC
Created attachment 126164 [details] [review]
Bug 27358: Unit tests for public items retrieval

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 38 Martin Renvoize 2021-10-13 09:42:18 UTC
Created attachment 126165 [details] [review]
Bug 27358: Add GET /public/biblios/:biblio_id/items

This patch introduces a route to fetch items belonging to a biblio. It
is expected to return the 'public' representation of the Koha::Item
objects.

It is also enforcing the visibility rules, by using
Koha::Items->filter_by_visible_in_opac.

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/biblios.t
=> SUCCESS: Test pass and they cover all the cases!
3. Try your favourite REST tool against the new route.
4. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 39 Martin Renvoize 2021-10-13 09:47:09 UTC
Bug updated to reflect dependence on bug 28948 where we took the base code from here for a public allow list implementation and refined it using a simpler case, libraries.

Allowlist taken from Katrins list in comment 8 as I agree with that list.

We leave out more_subfields_xml so it can be handled separately, likely as an embed.


All looking good here now for the next QA round.. Signing off.