Bug 26635 - Expand coded values in REST API call
Summary: Expand coded values in REST API call
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement with 10 votes (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Kyle M Hall
URL:
Keywords: rel_22_11_candidate
: 31683 (view as bug list)
Depends on: 26636
Blocks: 8179 22440 32118 33161 34211
  Show dependency treegraph
 
Reported: 2020-10-08 17:18 UTC by Agustín Moyano
Modified: 2023-12-28 20:47 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00


Attachments
Bug 26635: Expand authorised values in to_api method (5.07 KB, patch)
2020-10-09 13:33 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 26635: Add tests (3.09 KB, patch)
2020-10-09 13:35 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 26635: Expand authorised values in to_api method (5.07 KB, patch)
2020-10-09 13:35 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 26635: Add tests (3.09 KB, patch)
2020-10-09 14:56 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 26635: Expand authorised values in to_api method (5.07 KB, patch)
2020-10-09 14:56 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 26635: Add tests (8.12 KB, patch)
2020-10-09 15:01 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 26635: Expand authorised values in to_api method (5.07 KB, patch)
2020-10-09 15:01 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 26635: Add tests (8.28 KB, patch)
2020-10-26 19:09 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 26635: Expand authorised values in to_api method (5.07 KB, patch)
2020-10-26 19:09 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 26635: Add tests (11.41 KB, patch)
2021-02-02 20:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26635: Expand authorised values in to_api method (6.28 KB, patch)
2021-02-02 20:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26635: Add tests (11.31 KB, patch)
2021-02-02 21:03 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26635: Expand authorised values in to_api method (6.35 KB, patch)
2021-02-02 21:03 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26635: Add tests (11.50 KB, patch)
2021-02-09 15:34 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 26635: Expand authorised values in to_api method (6.36 KB, patch)
2021-02-09 15:34 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 26635: Add tests (11.57 KB, patch)
2021-05-07 15:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26635: Add tests (11.57 KB, patch)
2021-05-07 15:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26635: Expand authorised values in to_api method (6.43 KB, patch)
2021-05-07 15:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26635: Add tests for av-expand => 0 (2.81 KB, patch)
2021-08-31 08:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26635: Add tests (11.59 KB, patch)
2022-10-28 12:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26635: Expand authorised values in to_api method (4.35 KB, patch)
2022-10-28 12:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26635: Add tests for av-expand => 0 (2.83 KB, patch)
2022-10-28 12:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26635: AV expansion should ackowledge $params->{public} (4.62 KB, patch)
2022-10-28 15:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26635: Add tests (11.59 KB, patch)
2022-10-28 22:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26635: Expand authorised values in to_api method (4.35 KB, patch)
2022-10-28 22:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26635: Add tests for av-expand => 0 (2.83 KB, patch)
2022-10-28 22:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26635: AV expansion should ackowledge $params->{public} (4.62 KB, patch)
2022-10-28 22:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26635: Refined data structure and methods (11.19 KB, patch)
2022-10-28 22:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26635: Add tests (11.65 KB, patch)
2022-10-30 08:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26635: Expand authorised values in to_api method (4.41 KB, patch)
2022-10-30 08:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26635: Add tests for av-expand => 0 (2.89 KB, patch)
2022-10-30 08:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26635: AV expansion should ackowledge $params->{public} (4.68 KB, patch)
2022-10-30 08:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26635: Refined data structure and methods (11.25 KB, patch)
2022-10-30 08:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26635: Add tests (11.59 KB, patch)
2022-11-02 22:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26635: Expand authorised values in to_api method (4.35 KB, patch)
2022-11-02 22:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26635: Add tests for av-expand => 0 (2.83 KB, patch)
2022-11-02 22:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26635: AV expansion should ackowledge $params->{public} (4.62 KB, patch)
2022-11-02 22:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26635: Refined data structure and methods (13.38 KB, patch)
2022-11-02 22:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26635: Move expand syntax to x-koha-embed (12.09 KB, patch)
2022-11-02 22:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26635: Add tests (11.65 KB, patch)
2022-11-03 11:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26635: Expand authorised values in to_api method (4.41 KB, patch)
2022-11-03 11:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26635: Add tests for av-expand => 0 (2.89 KB, patch)
2022-11-03 11:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26635: AV expansion should ackowledge $params->{public} (4.68 KB, patch)
2022-11-03 11:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26635: Refined data structure and methods (13.44 KB, patch)
2022-11-03 11:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26635: Move expand syntax to x-koha-embed (12.16 KB, patch)
2022-11-03 11:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26635: (follow-up) Add optional support for `:` as the modifier (2.02 KB, patch)
2022-11-03 11:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26635: (QA follow-up) Remove test for x-koha-av-expand header (1.18 KB, patch)
2022-11-03 11:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26635: Add tests (11.70 KB, patch)
2022-11-03 15:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26635: Expand authorised values in to_api method (4.46 KB, patch)
2022-11-03 15:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26635: Add tests for av-expand => 0 (2.95 KB, patch)
2022-11-03 15:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26635: AV expansion should ackowledge $params->{public} (4.74 KB, patch)
2022-11-03 15:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26635: Refined data structure and methods (13.50 KB, patch)
2022-11-03 15:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26635: Move expand syntax to x-koha-embed (12.21 KB, patch)
2022-11-03 15:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26635: (follow-up) Add optional support for `:` as the modifier (2.08 KB, patch)
2022-11-03 15:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26635: (QA follow-up) Remove test for x-koha-av-expand header (1.24 KB, patch)
2022-11-03 15:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26635: (QA follow-up) Rename key _str to _strings (5.58 KB, patch)
2022-11-04 18:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26635: Add tests (11.70 KB, patch)
2022-11-07 12:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26635: Expand authorised values in to_api method (4.46 KB, patch)
2022-11-07 12:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26635: Add tests for av-expand => 0 (2.95 KB, patch)
2022-11-07 12:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26635: AV expansion should ackowledge $params->{public} (4.74 KB, patch)
2022-11-07 12:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26635: Refined data structure and methods (13.50 KB, patch)
2022-11-07 12:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26635: Move expand syntax to x-koha-embed (12.21 KB, patch)
2022-11-07 12:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26635: (QA follow-up) Remove test for x-koha-av-expand header (1.30 KB, patch)
2022-11-07 12:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26635: (QA follow-up) Rename key _str to _strings (5.64 KB, patch)
2022-11-07 12:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26635: (follow-up) Rename av_expand to strings (12.32 KB, patch)
2022-11-07 12:30 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 Agustín Moyano 2020-10-08 17:18:55 UTC
We need a way to embed expanded authorised values in a REST API call. It's specially needed when you have to show authorised values descriptions in tables that are fetched through api.

The proposed solution adds to object's api definition (even embedded content) the following key: _authorised_values

For each column that contains an authorised value, it will be reflected inside this property, with the authorised value expanded. So for example if we embed the authorised values of an item, the result from the API call could look like this:

GET /api/v1/items/1

{
    "_authorised_values": {
        "location": {
            "authorised_value": "GEN",
            "category": "LOC",
            "id": 27,
            "imageurl": null,
            "lib": "General Stacks",
            "lib_opac": null
        }
    },
    "acquisition_date": "2014-05-07",
    "acquisition_source": null,
    "biblio_id": 1,
    "item_id": 1,
    ...
    "location": "GEN",
    ...
    "uri": null,
    "withdrawn": 0,
    "withdrawn_date": null
}
Comment 1 Agustín Moyano 2020-10-09 13:33:58 UTC
Created attachment 111400 [details] [review]
Bug 26635: Expand authorised values in to_api method

This patch adds the posibility to expand authorised values when to_api
method is called.

The classes where authorised values should expand must implememnt the
_fetch_authorised_values method, and must return a hash like the
following

{
  column_name => Koha::AuthorisedValue->unblessed
  ...
}

This patch will be used in bug 8179, so please test there.
Comment 2 Agustín Moyano 2020-10-09 13:35:08 UTC
Created attachment 111401 [details] [review]
Bug 26635: Add tests

Add tests in t/db_dependent/Koha/Object.t
Comment 3 Agustín Moyano 2020-10-09 13:35:15 UTC
Created attachment 111402 [details] [review]
Bug 26635: Expand authorised values in to_api method

This patch adds the posibility to expand authorised values when to_api
method is called.

The classes where authorised values should expand must implememnt the
_fetch_authorised_values method, and must return a hash like the
following

{
  column_name => Koha::AuthorisedValue->unblessed
  ...
}

This patch will be used in bug 8179, so please test there.
Comment 4 Agustín Moyano 2020-10-09 14:56:00 UTC
Created attachment 111411 [details] [review]
Bug 26635: Add tests

Add tests in t/db_dependent/Koha/Object.t
Comment 5 Agustín Moyano 2020-10-09 14:56:07 UTC
Created attachment 111412 [details] [review]
Bug 26635: Expand authorised values in to_api method

This patch adds the posibility to expand authorised values when to_api
method is called.

The classes where authorised values should expand must implememnt the
_fetch_authorised_values method, and must return a hash like the
following

{
  column_name => Koha::AuthorisedValue->unblessed
  ...
}

This patch will be used in bug 8179, so please test there.
Comment 6 Agustín Moyano 2020-10-09 15:01:08 UTC
Created attachment 111413 [details] [review]
Bug 26635: Add tests

Add tests in t/db_dependent/Koha/Object.t and
t/db_dependent/Koha/REST/Plugin/Objects.t
Comment 7 Agustín Moyano 2020-10-09 15:01:13 UTC
Created attachment 111414 [details] [review]
Bug 26635: Expand authorised values in to_api method

This patch adds the posibility to expand authorised values when to_api
method is called.

The classes where authorised values should expand must implememnt the
_fetch_authorised_values method, and must return a hash like the
following

{
  column_name => Koha::AuthorisedValue->unblessed
  ...
}

This patch will be used in bug 8179, so please test there.
Comment 8 Andrew Fuerste-Henry 2020-10-22 19:53:07 UTC
Can't apply this patch in order to apply and test 8179
Comment 9 Agustín Moyano 2020-10-26 19:09:33 UTC
Created attachment 112538 [details] [review]
Bug 26635: Add tests

Add tests in t/db_dependent/Koha/Object.t and
t/db_dependent/Koha/REST/Plugin/Objects.t
Comment 10 Agustín Moyano 2020-10-26 19:09:39 UTC
Created attachment 112539 [details] [review]
Bug 26635: Expand authorised values in to_api method

This patch adds the posibility to expand authorised values when to_api
method is called.

The classes where authorised values should expand must implememnt the
_fetch_authorised_values method, and must return a hash like the
following

{
  column_name => Koha::AuthorisedValue->unblessed
  ...
}

This patch will be used in bug 8179, so please test there.
Comment 11 Agustín Moyano 2020-10-26 19:47:22 UTC
(In reply to Andrew Fuerste-Henry from comment #8)
> Can't apply this patch in order to apply and test 8179

Hi Andrew, could you please try again?

Thanks
Comment 12 David Nind 2021-01-25 21:12:10 UTC
Patches apply, but running prove t/db_dependent/Koha/REST/Plugin/Objects.t fails (prove t/db_dependent/Koha/Object.t passes):

prove t/db_dependent/Koha/REST/Plugin/Objects.t
t/db_dependent/Koha/REST/Plugin/Objects.t .. Global symbol "$t" requires explicit package name (did you forget to declare "my $t"?) at t/db_dependent/Koha/REST/Plugin/Objects.t line 470.
Execution of t/db_dependent/Koha/REST/Plugin/Objects.t aborted due to compilation errors.
# Looks like your test exited with 255 before it could output anything.
t/db_dependent/Koha/REST/Plugin/Objects.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 11/11 subtests 

Test Summary Report
-------------------
t/db_dependent/Koha/REST/Plugin/Objects.t (Wstat: 65280 Tests: 0 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 11 tests but ran 0.
Files=1, Tests=0,  1 wallclock secs ( 0.03 usr  0.00 sys +  1.04 cusr  0.09 csys =  1.16 CPU)
Result: FAIL
Comment 13 Tomás Cohen Arazi 2021-02-02 20:30:58 UTC
Created attachment 116232 [details] [review]
Bug 26635: Add tests

Add tests in t/db_dependent/Koha/Object.t and
t/db_dependent/Koha/REST/Plugin/Objects.t

Sponsored-by: Virginia Tech Libraries

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Tomás Cohen Arazi 2021-02-02 20:31:05 UTC
Created attachment 116233 [details] [review]
Bug 26635: Expand authorised values in to_api method

This patch adds the posibility to expand authorised values when to_api
method is called.

The classes where authorised values should expand must implememnt the
_fetch_authorised_values method, and must return a hash like the
following

{
  column_name => Koha::AuthorisedValue->unblessed
  ...
}

This patch will be used in bug 8179, so please test there.

Sponsored-by: Virginia Tech Libraries
Comment 15 Andrew Fuerste-Henry 2021-02-02 21:03:45 UTC
Created attachment 116234 [details] [review]
Bug 26635: Add tests

Add tests in t/db_dependent/Koha/Object.t and
t/db_dependent/Koha/REST/Plugin/Objects.t

Sponsored-by: Virginia Tech Libraries

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 16 Andrew Fuerste-Henry 2021-02-02 21:03:48 UTC
Created attachment 116235 [details] [review]
Bug 26635: Expand authorised values in to_api method

This patch adds the posibility to expand authorised values when to_api
method is called.

The classes where authorised values should expand must implememnt the
_fetch_authorised_values method, and must return a hash like the
following

{
  column_name => Koha::AuthorisedValue->unblessed
  ...
}

This patch will be used in bug 8179, so please test there.

Sponsored-by: Virginia Tech Libraries

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 17 Martin Renvoize 2021-02-03 08:43:28 UTC
This is interesting, and the code is nice to follow.

I wonder a bit about the output form however... should we not expand AV's inline?

So rather than:

GET /api/v1/items/1

{
    "_authorised_values": {
        "location": {
            "authorised_value": "GEN",
            "category": "LOC",
            "id": 27,
            "imageurl": null,
            "lib": "General Stacks",
            "lib_opac": null
        }
    },
    "acquisition_date": "2014-05-07",
    "acquisition_source": null,
    "biblio_id": 1,
    "item_id": 1,
    ...
    "location": "GEN",
    ...
    "uri": null,
    "withdrawn": 0,
    "withdrawn_date": null
}

We would do

GET /api/v1/items/1

{
    "acquisition_date": "2014-05-07",
    "acquisition_source": null,
    "biblio_id": 1,
    "item_id": 1,
    ...
    "location": {
        "code": "GEN",
        "description": "General Stacks",
        "imageurl": null
    },
    ...
    "uri": null,
    "withdrawn": 0,
    "withdrawn_date": null
}

and

GET /public/items/1

{
    "acquisition_date": "2014-05-07",
    "acquisition_source": null,
    "biblio_id": 1,
    "item_id": 1,
    ...
    "location": {
        "code": "GEN",
        "description": null,
        "imageurl": null
    },
    ...
    "uri": null,
    "withdrawn": 0,
    "withdrawn_date": null
}

This would more closely resemble how we return the data in the existing UI's.

I'm not sure we need the full AV object expanded.. and I'm thinking if would be 'nicer' to access it directly rather than have to do a subsequent lookup in JS?
Comment 18 Tomás Cohen Arazi 2021-02-03 11:40:44 UTC
(In reply to Martin Renvoize from comment #17)
> This would more closely resemble how we return the data in the existing UI's.

It is possible. But that means you will need to look at the attribute and determine if it is linked to an AV or not anyway. You could have a scalar or the attribute be an object. And if it is an object it could be some generated structure or an embed. We would still need to define a generic way to identify it is the case of an av-linked attribute. I'm not sure what's best, but having a separate lookup table felt cleaner to use. In the templates you would have proper objects that can be asked if they are av-linked.

This certainly needs ideas. We are open to proposals, obviously!

> I'm not sure we need the full AV object expanded.. and I'm thinking if would
> be 'nicer' to access it directly rather than have to do a subsequent lookup
> in JS?

I would go for an $av->to_api object, that names things better: lib/lib_opac => description, picking the right one according to the caller context.
Comment 19 Agustín Moyano 2021-02-09 15:34:12 UTC
Created attachment 116592 [details] [review]
Bug 26635: Add tests

Add tests in t/db_dependent/Koha/Object.t and
t/db_dependent/Koha/REST/Plugin/Objects.t

Sponsored-by: Virginia Polytechnic Institute and State University

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 20 Agustín Moyano 2021-02-09 15:34:15 UTC
Created attachment 116593 [details] [review]
Bug 26635: Expand authorised values in to_api method

This patch adds the posibility to expand authorised values when to_api
method is called.

The classes where authorised values should expand must implememnt the
_fetch_authorised_values method, and must return a hash like the
following

{
  column_name => Koha::AuthorisedValue->unblessed
  ...
}

This patch will be used in bug 8179, so please test there.

Sponsored-by: Virginia Polytechnic Institute and State University

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 21 Tomás Cohen Arazi 2021-03-15 11:43:47 UTC
(In reply to Tomás Cohen Arazi from comment #18)
> (In reply to Martin Renvoize from comment #17)
> > I'm not sure we need the full AV object expanded.. and I'm thinking if would
> > be 'nicer' to access it directly rather than have to do a subsequent lookup
> > in JS?
> 
> I would go for an $av->to_api object, that names things better: lib/lib_opac
> => description, picking the right one according to the caller context.

What do you think, Agustín?
Comment 22 Martin Renvoize 2021-04-01 12:42:50 UTC
(In reply to Tomás Cohen Arazi from comment #21)
> (In reply to Tomás Cohen Arazi from comment #18)
> > (In reply to Martin Renvoize from comment #17)
> > > I'm not sure we need the full AV object expanded.. and I'm thinking if would
> > > be 'nicer' to access it directly rather than have to do a subsequent lookup
> > > in JS?
> > 
> > I would go for an $av->to_api object, that names things better: lib/lib_opac
> > => description, picking the right one according to the caller context.
> 
> What do you think, Agustín?

I'm onboard with this... embedding them and using to_api to make the naming clear.. I reckon we could be onto a winner
Comment 23 Agustín Moyano 2021-04-05 10:44:49 UTC
(In reply to Martin Renvoize from comment #22)
> (In reply to Tomás Cohen Arazi from comment #21)
> > (In reply to Tomás Cohen Arazi from comment #18)
> > > (In reply to Martin Renvoize from comment #17)
> > > > I'm not sure we need the full AV object expanded.. and I'm thinking if would
> > > > be 'nicer' to access it directly rather than have to do a subsequent lookup
> > > > in JS?
> > > 
> > > I would go for an $av->to_api object, that names things better: lib/lib_opac
> > > => description, picking the right one according to the caller context.
> > 
> > What do you think, Agustín?
> 
> I'm onboard with this... embedding them and using to_api to make the naming
> clear.. I reckon we could be onto a winner

Now it is actually doing a "to_api" on avs when it does:


+        $avs = $self->_do_api_mapping($avs);

and then

+sub _do_api_mapping {
+    my ($self, $json_object) = @_;
+    # Rename attributes if there's a mapping
+    if ( $self->can('to_api_mapping') ) {
+        foreach my $column ( keys %{ $self->to_api_mapping } ) {
+            my $mapped_column = $self->to_api_mapping->{$column};
+            if ( exists $json_object->{$column}
+                && defined $mapped_column )
+            {
+                # key != undef
+                $json_object->{$mapped_column} = delete $json_object->{$column};
+            }
+            elsif ( exists $json_object->{$column}
+                && !defined $mapped_column )
+            {
+                # key == undef
+                delete $json_object->{$column};
+            }
+        }
+    }
     return $json_object;
 }

what we are currently lacking is "to_api_mapping" in AuthorisedValues
Comment 24 Tomás Cohen Arazi 2021-04-05 19:17:30 UTC
(In reply to Agustín Moyano from comment #23)
> 
> Now it is actually doing a "to_api" on avs when it does:
> 
> 
> +        $avs = $self->_do_api_mapping($avs);
> 
> and then
> 
> +sub _do_api_mapping {
> +    my ($self, $json_object) = @_;
> +    # Rename attributes if there's a mapping
> +    if ( $self->can('to_api_mapping') ) {
> +        foreach my $column ( keys %{ $self->to_api_mapping } ) {
> +            my $mapped_column = $self->to_api_mapping->{$column};
> +            if ( exists $json_object->{$column}
> +                && defined $mapped_column )
> +            {
> +                # key != undef
> +                $json_object->{$mapped_column} = delete
> $json_object->{$column};
> +            }
> +            elsif ( exists $json_object->{$column}
> +                && !defined $mapped_column )
> +            {
> +                # key == undef
> +                delete $json_object->{$column};
> +            }
> +        }
> +    }
>      return $json_object;
>  }
> 
> what we are currently lacking is "to_api_mapping" in AuthorisedValues

Well, we certainly need that, but we also need to call ->to_api instead of the hand-crafted _do_api_mapping(). That's because we could want to call it with { public => 1 } and thus making the av representation dependent on the context. This example snippet from an hypothetical Koha::AuthorisedValue->to_api() might illustrate what I mean:

my $json = $self->SUPER::to_api($params);

my $description = $json->{description};
$description = $json->{description_opac}
    if $params->{public}
       and defined $json->{description_opac}
       and $json->{description_opac} ne '';
Comment 25 Kyle M Hall 2021-05-07 15:00:45 UTC
Created attachment 120698 [details] [review]
Bug 26635: Add tests

Add tests in t/db_dependent/Koha/Object.t and
t/db_dependent/Koha/REST/Plugin/Objects.t

Sponsored-by: Virginia Polytechnic Institute and State University

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 26 Kyle M Hall 2021-05-07 15:01:15 UTC
Created attachment 120699 [details] [review]
Bug 26635: Add tests

Add tests in t/db_dependent/Koha/Object.t and
t/db_dependent/Koha/REST/Plugin/Objects.t

Sponsored-by: Virginia Polytechnic Institute and State University

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 27 Kyle M Hall 2021-05-07 15:01:30 UTC
Created attachment 120700 [details] [review]
Bug 26635: Expand authorised values in to_api method

This patch adds the posibility to expand authorised values when to_api
method is called.

The classes where authorised values should expand must implememnt the
_fetch_authorised_values method, and must return a hash like the
following

{
  column_name => Koha::AuthorisedValue->unblessed
  ...
}

This patch will be used in bug 8179, so please test there.

Sponsored-by: Virginia Polytechnic Institute and State University

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 28 Jonathan Druart 2021-08-31 08:04:59 UTC
Created attachment 124268 [details] [review]
Bug 26635: Add tests for av-expand => 0
Comment 29 Jonathan Druart 2021-08-31 14:29:25 UTC
What about having it more generic? At least the naming to start.

We will want to "expand" itemtypes as well for instance.
Do we really need the whole Koha::AV object or do we finally only need the description?

It's similar to the 'columns_to_str' methods I introduced on bug 27526 (yes Tomas, I know you told me already), where we want to have stringified values of object's attributes.
Comment 30 Martin Renvoize 2021-08-31 15:38:22 UTC
I have another question regards the output representation.. I've come round to the lookup table approach.. but shouldn't there be another level in there?

{
  "_authorised_values": {
    "location": {
      "authorised_value": "GEN",
      "category": "LOC",
      "id": 27,
      "imageurl": null,
      "lib": "General Stacks",
      "lib_opac": null
    }
  },
  "location": "GEN"
}
    

What about for searches.. it's unlikely all items will be in the 'GEN' location.. so wouldn't we actually need:

{
  "_authorised_values": {
    "location": {
      "GEN": "General Stacks",
      "SHELVES": "Shelves"
    },
  },
  "location": "GEN"
}

Where the key is the AV code and the value is the appropriate description (based on public vs staff.

Not sure we need any of the rest of the AV object detail honestly.. perhaps the imageurl later...
Comment 31 Agustín Moyano 2021-08-31 16:23:51 UTC
(In reply to Martin Renvoize from comment #30)
> I have another question regards the output representation.. I've come round
> to the lookup table approach.. but shouldn't there be another level in there?
> 
> {
>   "_authorised_values": {
>     "location": {
>       "authorised_value": "GEN",
>       "category": "LOC",
>       "id": 27,
>       "imageurl": null,
>       "lib": "General Stacks",
>       "lib_opac": null
>     }
>   },
>   "location": "GEN"
> }
>     
> 
> What about for searches.. it's unlikely all items will be in the 'GEN'
> location.. so wouldn't we actually need:
> 
> {
>   "_authorised_values": {
>     "location": {
>       "GEN": "General Stacks",
>       "SHELVES": "Shelves"
>     },
>   },
>   "location": "GEN"
> }
> 
> Where the key is the AV code and the value is the appropriate description
> (based on public vs staff.
> 
> Not sure we need any of the rest of the AV object detail honestly.. perhaps
> the imageurl later...

Hi Martin, I did not think this to fill a search or select options (I thought better for that to have an authorised values endpoint).

I designed the _authorised_values attribute to just bring the value of an authorised value in the original json.

I'm not saying we shouldn't do it.. I just don't think it wise to bring every possible value here.. I've seen AV categories with hundreds of AV

About what info of the AV should we bring, it's true that in most cases you would only need lib or lib_opac, but I didn't new if someone else for some reason would need any of the other info.. maybe we should sheep category too, to be able to populate a search or a select with the category options
Comment 32 Martin Renvoize 2021-09-01 08:19:15 UTC
Hmmm, if that's the case I'm not sure I understand why there are tests for the search helper.. surely that will generally return multiple results?

I'm vaguely feeling like a single page web app would actually grab the full AV list when it first needs it and cache it locally to use for subsequent uses.. but as Koha isn't at that point yet we're unlikely to be able to really utilise that methodology..
Comment 33 Tomás Cohen Arazi 2021-09-07 09:33:35 UTC
(In reply to Martin Renvoize from comment #32)
> Hmmm, if that's the case I'm not sure I understand why there are tests for
> the search helper.. surely that will generally return multiple results?
> 
> I'm vaguely feeling like a single page web app would actually grab the full
> AV list when it first needs it and cache it locally to use for subsequent
> uses.. but as Koha isn't at that point yet we're unlikely to be able to
> really utilise that methodology..

If you need to render a list of (say) items, you will need to be able to make the GET /items call return not only the 'damaged' attribute, but also a way to return the linked AV, for rendering purposes.

The only thing that I'm not sure about this dev, is that (maybe) we want more fine-grained control of what objects should have their AVs expanded. For example, if we wanted the list of the items on a bib to display them grouped by the library they were checked out (I know, rare example heh):

GET /biblio/123
x-koha-av-expand: true
x-koha-embed: items.checkouts

would mean we expand AVs for biblio, items, but also for the related checkouts (a probably expensive thing, as we don't have relationships in place for prefetching).

I would prefer a syntax that allows us to do:

GET /biblio/123
x-koha-embed: items+av,items.checkouts
Comment 34 Kyle M Hall 2021-09-07 10:51:21 UTC
(In reply to Tomás Cohen Arazi from comment #33)
> (In reply to Martin Renvoize from comment #32)
> > Hmmm, if that's the case I'm not sure I understand why there are tests for
> > the search helper.. surely that will generally return multiple results?
> > 
> > I'm vaguely feeling like a single page web app would actually grab the full
> > AV list when it first needs it and cache it locally to use for subsequent
> > uses.. but as Koha isn't at that point yet we're unlikely to be able to
> > really utilise that methodology..
> 
> If you need to render a list of (say) items, you will need to be able to
> make the GET /items call return not only the 'damaged' attribute, but also a
> way to return the linked AV, for rendering purposes.
> 
> The only thing that I'm not sure about this dev, is that (maybe) we want
> more fine-grained control of what objects should have their AVs expanded.
> For example, if we wanted the list of the items on a bib to display them
> grouped by the library they were checked out (I know, rare example heh):
> 
> GET /biblio/123
> x-koha-av-expand: true
> x-koha-embed: items.checkouts
> 
> would mean we expand AVs for biblio, items, but also for the related
> checkouts (a probably expensive thing, as we don't have relationships in
> place for prefetching).
> 
> I would prefer a syntax that allows us to do:
> 
> GET /biblio/123
> x-koha-embed: items+av,items.checkouts

Where will this be documented? I think the biggest weakness with our API is a derth of documentation for end users. We have some much great functionality that most API users don't even know exists let alone how to use it! This is a great example.

I'd really love to see at a minimum, this be documented on a wiki page. We could start there, and perhaps build an API User's Handbook, or even just an API User's Cookbook just like the SQL Reports and Javscript library pages.
Comment 35 Kyle M Hall 2021-09-07 10:52:09 UTC
It would also be nice if it could be embedded in the self-documentation somehow.
Comment 36 Kyle M Hall 2021-09-13 15:39:54 UTC
Started an API Users Guide on the Wiki: https://wiki.koha-community.org/wiki/Koha_REST_API_Users_Guide
Comment 37 Jonathan Druart 2022-01-13 06:56:53 UTC
What's next? Should we go with the proposed patches or think something else?

(In reply to Jonathan Druart from comment #29)
> What about having it more generic? At least the naming to start.
> 
> We will want to "expand" itemtypes as well for instance.
> Do we really need the whole Koha::AV object or do we finally only need the
> description?
> 
> It's similar to the 'columns_to_str' methods I introduced on bug 27526 (yes
> Tomas, I know you told me already), where we want to have stringified values
> of object's attributes.

This comment didn't get a reply, and I still think it's what we need here. A stringified version of codes seem sensible. It will work for AVs, branchcode, itemtypes, etc.
However it will bring the problem of translatability (but same problem will appear with the current version). In a first step we only want to support AVs so this problem is for later.
Comment 38 Tomás Cohen Arazi 2022-01-13 10:23:49 UTC
We name itemtype => item_type_id, branchcode => library_id, etc. We do that so we can do things like:

x-koha-embed: item_type,library

So we already have a generic way if doing it. You just add a method that returns the thing and add an embed definition on the API.

The problem is, frameworks define how attributes are mapped to avs, and as such you cannot know in advance what you would need to embed. And even if you know in advance that some attribute is married to an av, it could happen that there isn't a match just because (of data). So maybe one item has a corresponding av, and the other doesn't, on a resultset.

It felt (some time ago already) that it was better to return a structure in which you could query for the descriptions.

I'm ok with whatever we decide, as long as it is practical and consistent. And we can use so we can push bug 8179 to master, which is hurting the sponsors for a year already.

Bug 8179 needs some more polishing for sure, but uncertainty made it get stuck.
Comment 39 Katrin Fischer 2022-02-26 20:14:10 UTC
Trying to comment here in order to get this unstuck... maybe.

As this is going to be used for bug 8179 which I very much would like to see moving :)

If I understand Tomas last comment correctly the problem might be with result lists that contain objects that can have different sets of AV mappings... does that sound right?

I am thinking about a list of order lines (aqorders) where every order line is linked to a fund... that can be linked to different or none AV for the 2 statistical values.
Comment 40 Tomás Cohen Arazi 2022-02-26 20:24:53 UTC
(In reply to Katrin Fischer from comment #39)
> Trying to comment here in order to get this unstuck... maybe.
> 
> As this is going to be used for bug 8179 which I very much would like to see
> moving :)
> 
> If I understand Tomas last comment correctly the problem might be with
> result lists that contain objects that can have different sets of AV
> mappings... does that sound right?
> 
> I am thinking about a list of order lines (aqorders) where every order line
> is linked to a fund... that can be linked to different or none AV for the 2
> statistical values.

Right, the idea is to make each object return (inside of it) done generic structure that lets the caller know how to find the av description. I'll get back to this next week, as we are responsible for having 8179 done, as the sponsor requested. We just need that people having different pov on the matter just say how they'd do it. And eventually vote something.
Comment 41 Lucas Gass 2022-06-09 19:09:18 UTC
Would love to see this get unstuck, authorized values descriptions from the REST API would be really handy!
Comment 42 Martin Renvoize 2022-06-10 08:32:17 UTC
I'm still coming back around to comment #32, in particular:

> a single page web app would actually grab the full AV list when it first
> needs it and cache it locally to use for subsequent uses..

I think for efficiency we're going to really struggle with embedding AV's dynamically in the results.  I'm thinking more and more we need to focus on having AV and Framework routes and then building a JS equivalent to the RecordProcessor pattern for the UI.
Comment 43 Tomás Cohen Arazi 2022-06-10 13:08:12 UTC
One thing to consider is we should (ideally) not change the API often so devs can rely on it. That said what if we streamline the data structure (i.e. do not return the AV object, but just the description, making it context (opac?)) and try this.

Once we find a better pattern in real-life uses, we can flag this as 'slow' and propose a different one, while keeping it for some cases in which it really doesn't hurt performance. Bug 8179 is a real example of using this feature without any noticeable overhead.

So maybe when this is *not enough* we look for other approaches.
Comment 44 Lucas Gass 2022-10-04 22:50:54 UTC
*** Bug 31683 has been marked as a duplicate of this bug. ***
Comment 45 David Cook 2022-10-04 23:53:18 UTC
Another wrinkle could be translations for authorised values: https://lists.katipo.co.nz/public/koha/2021-December/057052.html
Comment 46 David Cook 2022-10-05 01:55:27 UTC
In any case, I reckon backwards compatibility is important, especially since we're still using "v1" for the API, so I don't think we can change the existing "location" field.

Whether we embed the data or provide a lookup reference, I think it needs to be as a separate field. 

On non-Koha projects where I've needed to maintain backwards compatibility, I've added fields like "location_description". 

--

Sometimes, I think that authorised values should be removed from item data, and replaced with proper foreign key references. 

Location is an important field which by rights deserves its own database table I think. 

Of course, that data migration is easier said than done...
Comment 47 Tomás Cohen Arazi 2022-10-05 11:01:58 UTC
(In reply to David Cook from comment #46)
> In any case, I reckon backwards compatibility is important, especially since
> we're still using "v1" for the API, so I don't think we can change the
> existing "location" field.

Agreed!

> On non-Koha projects where I've needed to maintain backwards compatibility,
> I've added fields like "location_description". 

Right. We also need the category it is linked to, do we can draw things like drop-downs.

> Sometimes, I think that authorised values should be removed from item data,
> and replaced with proper foreign key references. 
> 
> Location is an important field which by rights deserves its own database
> table I think.

Locations, collections... We've done that with credit/debit tired recently.

> Of course, that data migration is easier said than done...

Generally speaking, the problem is lack of infinite time.
Comment 48 Jonathan Druart 2022-10-17 14:20:31 UTC
Make the REST API returns a "_str" key (generated only if you requested it) which would contains the stringified values
{
  _str: { attr: "stringified_value" },
  attr: "coded_value"
}

JS code would be
  let string_to_display = json._str.attr || json.attr

Not sure why this is still stuck.

I needed it for ERM and ended with retrieving the AVs from the TT plugin, serialized to JSON and cached them.

The main problem is that we don't even have a route for AVs.
Comment 49 Lucas Gass 2022-10-26 17:14:30 UTC
Tomas,

This is marked as 'ASSIGNED', is there work to do or is ready for testing?
Comment 50 Tomás Cohen Arazi 2022-10-27 19:09:44 UTC
(In reply to Lucas Gass from comment #49)
> Tomas,
> 
> This is marked as 'ASSIGNED', is there work to do or is ready for testing?

We need to rework this a bit. I marked it for inclusion in 22.11 and will submit a revisited version ASAP.
Comment 51 Tomás Cohen Arazi 2022-10-28 12:45:52 UTC
Created attachment 142761 [details] [review]
Bug 26635: Add tests

Add tests in t/db_dependent/Koha/Object.t and
t/db_dependent/Koha/REST/Plugin/Objects.t

Sponsored-by: Virginia Polytechnic Institute and State University

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 52 Tomás Cohen Arazi 2022-10-28 12:45:58 UTC
Created attachment 142762 [details] [review]
Bug 26635: Expand authorised values in to_api method

This patch adds the posibility to expand authorised values when to_api
method is called.

The classes where authorised values should expand must implememnt the
_fetch_authorised_values method, and must return a hash like the
following

{
  column_name => Koha::AuthorisedValue->unblessed
  ...
}

This patch will be used in bug 8179, so please test there.

Sponsored-by: Virginia Polytechnic Institute and State University

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 53 Tomás Cohen Arazi 2022-10-28 12:46:04 UTC
Created attachment 142763 [details] [review]
Bug 26635: Add tests for av-expand => 0
Comment 54 Tomás Cohen Arazi 2022-10-28 12:48:47 UTC
Rebased so it works and devs can take a look. I will refactor this into 'coded values' (not just AV) and will stick to a similar approach to Agustin's.

While I think just adding <attribute>_str to the response might suffice some use cases, I think having a lookup table will be better for knowing things like 'where to pick the values for a dropdown' etc.
Comment 55 Tomás Cohen Arazi 2022-10-28 15:10:03 UTC
Created attachment 142776 [details] [review]
Bug 26635: AV expansion should ackowledge $params->{public}

This patch simplifies the original implementation so it is more
readable, removing extra loops and methods and just dealing with the
coded values inline on the existing loops.

It also adds non-public attributes removal from the original list, to
address the changes introduced by bug 28948.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t \
           t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 56 Tomás Cohen Arazi 2022-10-28 22:30:45 UTC
Created attachment 142792 [details] [review]
Bug 26635: Add tests

Add tests in t/db_dependent/Koha/Object.t and
t/db_dependent/Koha/REST/Plugin/Objects.t

Sponsored-by: Virginia Polytechnic Institute and State University

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 57 Tomás Cohen Arazi 2022-10-28 22:30:52 UTC
Created attachment 142793 [details] [review]
Bug 26635: Expand authorised values in to_api method

This patch adds the posibility to expand authorised values when to_api
method is called.

The classes where authorised values should expand must implememnt the
_fetch_authorised_values method, and must return a hash like the
following

{
  column_name => Koha::AuthorisedValue->unblessed
  ...
}

This patch will be used in bug 8179, so please test there.

Sponsored-by: Virginia Polytechnic Institute and State University

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 58 Tomás Cohen Arazi 2022-10-28 22:30:59 UTC
Created attachment 142794 [details] [review]
Bug 26635: Add tests for av-expand => 0
Comment 59 Tomás Cohen Arazi 2022-10-28 22:31:05 UTC
Created attachment 142795 [details] [review]
Bug 26635: AV expansion should ackowledge $params->{public}

This patch simplifies the original implementation so it is more
readable, removing extra loops and methods and just dealing with the
coded values inline on the existing loops.

It also adds non-public attributes removal from the original list, to
address the changes introduced by bug 28948.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t \
           t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 60 Tomás Cohen Arazi 2022-10-28 22:31:13 UTC
Created attachment 142796 [details] [review]
Bug 26635: Refined data structure and methods

This patch makes the returned data structure be simpler:

_authorised_values => {
    attribute_1 => { category => 'some_category_name', description =>
'description' }

}

The description is sensible to context, so if public => 1 is passed,
then lib_opac is passed, and lib is returned otherwise. Whenever we add
language to the combo, we will add it to the implementation.

Tests are adjusted accordingly, also to reflect the public => 1 use
case.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t \
           t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 61 Martin Renvoize 2022-10-29 12:28:33 UTC
So.. for my own sanity.. the final structure is this right:

GET /api/v1/items/1

{
    "_authorized_values": {
        "location": {
            "category": "LOC",
            "description": "General Stacks",
        }
    },
    "acquisition_date": "2014-05-07",
    "acquisition_source": null,
    "biblio_id": 1,
    "item_id": 1,
    ...
    "location": "GEN",
    ...
    "uri": null,
    "withdrawn": 0,
    "withdrawn_date": null
}

GET /api/v1/items

[
  {
    "_authorized_values": {
        "location": {
            "category": "LOC",
            "description": "General Stacks",
        }
    },
    "acquisition_date": "2014-05-07",
    "acquisition_source": null,
    "biblio_id": 1,
    "item_id": 1,
    ...
    "location": "GEN",
    ...
    "uri": null,
    "withdrawn": 0,
    "withdrawn_date": null
  },
  {
    "_authorized_values": {
        "location": {
            "category": "LOC",
            "description": "Birchwood Shelves",
        }
    },
    "acquisition_date": "2017-02-14",
    "acquisition_source": null,
    "biblio_id": 2,
    "item_id": 12,
    ...
    "location": "BIR",
    ...
    "uri": null,
    "withdrawn": 0,
    "withdrawn_date": null
  }
]

So.. an embedded _authorized_values structure per individual object (not at a top level.. as we can't do that for things like biblios where the framework governs stuff..)

We keep the lookup as '_authorized' so we know it's an authorized value (not another coded lookup).. and we include 'category' so we can know the link between the field and which av category we're looking at (for later lookups should we want to produce a pick list).. and then we have the interface specific return for description.

I'm no against also including image url if we think that's helpful.. I could see if being so.. but it also feels fairly specific to only a minority of AV cases.. so maybe not too..
Comment 62 Martin Renvoize 2022-10-30 08:34:53 UTC
Created attachment 142801 [details] [review]
Bug 26635: Add tests

Add tests in t/db_dependent/Koha/Object.t and
t/db_dependent/Koha/REST/Plugin/Objects.t

Sponsored-by: Virginia Polytechnic Institute and State University
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 63 Martin Renvoize 2022-10-30 08:34:59 UTC
Created attachment 142802 [details] [review]
Bug 26635: Expand authorised values in to_api method

This patch adds the posibility to expand authorised values when to_api
method is called.

The classes where authorised values should expand must implememnt the
_fetch_authorised_values method, and must return a hash like the
following

{
  column_name => Koha::AuthorisedValue->unblessed
  ...
}

This patch will be used in bug 8179, so please test there.

Sponsored-by: Virginia Polytechnic Institute and State University
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 64 Martin Renvoize 2022-10-30 08:35:05 UTC
Created attachment 142803 [details] [review]
Bug 26635: Add tests for av-expand => 0

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 65 Martin Renvoize 2022-10-30 08:35:11 UTC
Created attachment 142804 [details] [review]
Bug 26635: AV expansion should ackowledge $params->{public}

This patch simplifies the original implementation so it is more
readable, removing extra loops and methods and just dealing with the
coded values inline on the existing loops.

It also adds non-public attributes removal from the original list, to
address the changes introduced by bug 28948.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t \
           t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 66 Martin Renvoize 2022-10-30 08:35:16 UTC
Created attachment 142805 [details] [review]
Bug 26635: Refined data structure and methods

This patch makes the returned data structure be simpler:

_authorised_values => {
    attribute_1 => { category => 'some_category_name', description =>
'description' }

}

The description is sensible to context, so if public => 1 is passed,
then lib_opac is passed, and lib is returned otherwise. Whenever we add
language to the combo, we will add it to the implementation.

Tests are adjusted accordingly, also to reflect the public => 1 use
case.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t \
           t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 67 Martin Renvoize 2022-10-30 08:36:30 UTC
All works for me.. Signing Off
Comment 68 Jonathan Druart 2022-10-31 06:38:38 UTC
As said earlier (comment 29), IMO this should not be specific to AVs. It (the new hash we are injecting) needs to reflect that it's a stringified version of a coded field.
Comment 69 Martin Renvoize 2022-10-31 06:50:29 UTC
I though Tomas had answered your comment already Jonathan.

I think the key piece is that he wants both the string and the av category so that one can produce the correct pick lists when needed.

That of course also assumes we have and av endpoint and but it does let us off adding framework endpoints at this time.
Comment 70 Jonathan Druart 2022-10-31 07:28:13 UTC
Then we could use the following structure
{
  _str: {
    location: { str: "General Stacks", type: "authorised_values" },
    itemtype: { str: "Books", type: "itemtypes" }
  },
  location: "GEN",
  itemtype: "BK",
}


From a dev pov it will be easier to
  const location = r._str.location.str || r.location
  const itemtype = r._str.itemtype.str || r.itemtype

than to *know* that location is in _authorised_values and itemtype in _itemtypes!
Comment 71 Martin Renvoize 2022-10-31 07:40:35 UTC
OK, with that example I understand better where you were going with it.. makes sense to me.
Comment 72 Tomás Cohen Arazi 2022-10-31 16:53:05 UTC
(In reply to Jonathan Druart from comment #70)
> Then we could use the following structure
> {
>   _str: {
>     location: { str: "General Stacks", type: "authorised_values" },
>     itemtype: { str: "Books", type: "itemtypes" }
>   },
>   location: "GEN",
>   itemtype: "BK",
> }
> 
> 
> From a dev pov it will be easier to
>   const location = r._str.location.str || r.location
>   const itemtype = r._str.itemtype.str || r.itemtype
> 
> than to *know* that location is in _authorised_values and itemtype in
> _itemtypes!


I like this. Was thinking the same way after Martin's recap highlighted I didn't approach the coded fields approach we chatted about. Follow-ups welcome or wait for me while I recover from this virus.
Comment 73 Tomás Cohen Arazi 2022-11-02 22:05:13 UTC
Created attachment 143039 [details] [review]
Bug 26635: Add tests

Add tests in t/db_dependent/Koha/Object.t and
t/db_dependent/Koha/REST/Plugin/Objects.t

Sponsored-by: Virginia Polytechnic Institute and State University

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 74 Tomás Cohen Arazi 2022-11-02 22:05:20 UTC
Created attachment 143040 [details] [review]
Bug 26635: Expand authorised values in to_api method

This patch adds the posibility to expand authorised values when to_api
method is called.

The classes where authorised values should expand must implememnt the
_fetch_authorised_values method, and must return a hash like the
following

{
  column_name => Koha::AuthorisedValue->unblessed
  ...
}

This patch will be used in bug 8179, so please test there.

Sponsored-by: Virginia Polytechnic Institute and State University

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 75 Tomás Cohen Arazi 2022-11-02 22:05:27 UTC
Created attachment 143041 [details] [review]
Bug 26635: Add tests for av-expand => 0
Comment 76 Tomás Cohen Arazi 2022-11-02 22:05:35 UTC
Created attachment 143042 [details] [review]
Bug 26635: AV expansion should ackowledge $params->{public}

This patch simplifies the original implementation so it is more
readable, removing extra loops and methods and just dealing with the
coded values inline on the existing loops.

It also adds non-public attributes removal from the original list, to
address the changes introduced by bug 28948.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t \
           t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 77 Tomás Cohen Arazi 2022-11-02 22:05:44 UTC
Created attachment 143043 [details] [review]
Bug 26635: Refined data structure and methods

This patch makes the returned data structure be simpler:

_str => {
    attribute_1 => {
        category => 'some_category_name',
        str      => 'description',
        type     => 'av'
    },
    ...
}

The description is sensible to context, so if public => 1 is passed,
then lib_opac is passed, and lib is returned otherwise. Whenever we add
language to the combo, we will add it to the implementation.

Tests are adjusted accordingly, also to reflect the public => 1 use
case.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t \
           t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 78 Tomás Cohen Arazi 2022-11-02 22:05:51 UTC
Created attachment 143044 [details] [review]
Bug 26635: Move expand syntax to x-koha-embed

Instead of a global av-expand flag (through a header) this patch
proposes to allow specifying +av_expand at the x-koha-embed header
level. This allows a more fine-grained control on what objects get avs
expanded. e.g.:

GET /patrons/123
x-koha-expand: +av_expand,checkouts.item+av_expand
=>
{
    "_str": {
        "city": { "str": "Córdoba", ... }
    },
    ...
    "checkouts": [
        {
            ...,
            "item": {
                "_str": {
                    "not_for_loan_status": { "str": "Reference material", ... },
                    ...
                },
                ...
            }
        }
    ]
}

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t \
           t/Koha/REST/Plugin/Query.t \
           t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Tests pass!
2. Sign off :-D
Comment 79 Tomás Cohen Arazi 2022-11-02 22:10:09 UTC
I haven't fully recovered, but... I get bored so...

I have submitted a revisited patch for revisiting the data structure and methods, and then another interesting follow-up for getting rid of the x-koha-expand-av flag, and just add a syntax for +av_expand on the x-koha-embed, which makes a lot more sense and provides a fine-grained approach instead of an all-or-nothing one.

I implemented things mostly as Jonathan proposed (I only added an obvious 'category' attribute).

We'll see how we later add 'str' counterpart for other types of things.

Please test!
Comment 80 Martin Renvoize 2022-11-03 11:29:42 UTC
Created attachment 143073 [details] [review]
Bug 26635: Add tests

Add tests in t/db_dependent/Koha/Object.t and
t/db_dependent/Koha/REST/Plugin/Objects.t

Sponsored-by: Virginia Polytechnic Institute and State University
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 81 Martin Renvoize 2022-11-03 11:29:48 UTC
Created attachment 143074 [details] [review]
Bug 26635: Expand authorised values in to_api method

This patch adds the posibility to expand authorised values when to_api
method is called.

The classes where authorised values should expand must implememnt the
_fetch_authorised_values method, and must return a hash like the
following

{
  column_name => Koha::AuthorisedValue->unblessed
  ...
}

This patch will be used in bug 8179, so please test there.

Sponsored-by: Virginia Polytechnic Institute and State University
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 82 Martin Renvoize 2022-11-03 11:29:54 UTC
Created attachment 143075 [details] [review]
Bug 26635: Add tests for av-expand => 0

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 83 Martin Renvoize 2022-11-03 11:29:59 UTC
Created attachment 143076 [details] [review]
Bug 26635: AV expansion should ackowledge $params->{public}

This patch simplifies the original implementation so it is more
readable, removing extra loops and methods and just dealing with the
coded values inline on the existing loops.

It also adds non-public attributes removal from the original list, to
address the changes introduced by bug 28948.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t \
           t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 84 Martin Renvoize 2022-11-03 11:30:06 UTC
Created attachment 143077 [details] [review]
Bug 26635: Refined data structure and methods

This patch makes the returned data structure be simpler:

_str => {
    attribute_1 => {
        category => 'some_category_name',
        str      => 'description',
        type     => 'av'
    },
    ...
}

The description is sensible to context, so if public => 1 is passed,
then lib_opac is passed, and lib is returned otherwise. Whenever we add
language to the combo, we will add it to the implementation.

Tests are adjusted accordingly, also to reflect the public => 1 use
case.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t \
           t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 85 Martin Renvoize 2022-11-03 11:30:12 UTC
Created attachment 143078 [details] [review]
Bug 26635: Move expand syntax to x-koha-embed

Instead of a global av-expand flag (through a header) this patch
proposes to allow specifying +av_expand at the x-koha-embed header
level. This allows a more fine-grained control on what objects get avs
expanded. e.g.:

GET /patrons/123
x-koha-expand: +av_expand,checkouts.item+av_expand
=>
{
    "_str": {
        "city": { "str": "Córdoba", ... }
    },
    ...
    "checkouts": [
        {
            ...,
            "item": {
                "_str": {
                    "not_for_loan_status": { "str": "Reference material", ... },
                    ...
                },
                ...
            }
        }
    ]
}

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t \
           t/Koha/REST/Plugin/Query.t \
           t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Tests pass!
2. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 86 Martin Renvoize 2022-11-03 11:30:19 UTC
Created attachment 143079 [details] [review]
Bug 26635: (follow-up) Add optional support for `:` as the modifier

This patch adds optional support for using `:` as the modifier for
invoking the 'count' method for relations as discussed.

We retain support for `+` for backward compatability to prevent a
breaking change.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 87 Martin Renvoize 2022-11-03 11:30:25 UTC
Created attachment 143080 [details] [review]
Bug 26635: (QA follow-up) Remove test for x-koha-av-expand header

This patch updates the last remaining test that mentioned the now defunkt
x-koha-av-expand header.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 88 Martin Renvoize 2022-11-03 11:33:44 UTC
I'm loving this… signing off.

The one very, very minor niggle… I'm wondering if `+av_expand` is right; With us switching to _str to generalize it, perhaps the key string should be `+str_expand` or `+code_expand`
Comment 89 Tomás Cohen Arazi 2022-11-03 12:19:22 UTC
(In reply to Martin Renvoize from comment #88)
> I'm loving this… signing off.
> 
> The one very, very minor niggle… I'm wondering if `+av_expand` is right;
> With us switching to _str to generalize it, perhaps the key string should be
> `+str_expand` or `+code_expand`

Oh I didn't mention that... On our chat we agreed that `attribute+count` was not the best syntax, as it is not 'adding', and that we should push for `attribute:count`. That said, I feel like we will end up implementing something like: `item_type_id:str` or `pickup_location_id:str`.
Comment 90 Kyle M Hall 2022-11-03 15:21:59 UTC
Created attachment 143127 [details] [review]
Bug 26635: Add tests

Add tests in t/db_dependent/Koha/Object.t and
t/db_dependent/Koha/REST/Plugin/Objects.t

Sponsored-by: Virginia Polytechnic Institute and State University
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 91 Kyle M Hall 2022-11-03 15:22:30 UTC
Created attachment 143128 [details] [review]
Bug 26635: Expand authorised values in to_api method

This patch adds the posibility to expand authorised values when to_api
method is called.

The classes where authorised values should expand must implememnt the
_fetch_authorised_values method, and must return a hash like the
following

{
  column_name => Koha::AuthorisedValue->unblessed
  ...
}

This patch will be used in bug 8179, so please test there.

Sponsored-by: Virginia Polytechnic Institute and State University
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 92 Kyle M Hall 2022-11-03 15:22:36 UTC
Created attachment 143129 [details] [review]
Bug 26635: Add tests for av-expand => 0

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 93 Kyle M Hall 2022-11-03 15:22:41 UTC
Created attachment 143130 [details] [review]
Bug 26635: AV expansion should ackowledge $params->{public}

This patch simplifies the original implementation so it is more
readable, removing extra loops and methods and just dealing with the
coded values inline on the existing loops.

It also adds non-public attributes removal from the original list, to
address the changes introduced by bug 28948.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t \
           t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 94 Kyle M Hall 2022-11-03 15:22:46 UTC
Created attachment 143131 [details] [review]
Bug 26635: Refined data structure and methods

This patch makes the returned data structure be simpler:

_str => {
    attribute_1 => {
        category => 'some_category_name',
        str      => 'description',
        type     => 'av'
    },
    ...
}

The description is sensible to context, so if public => 1 is passed,
then lib_opac is passed, and lib is returned otherwise. Whenever we add
language to the combo, we will add it to the implementation.

Tests are adjusted accordingly, also to reflect the public => 1 use
case.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t \
           t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 95 Kyle M Hall 2022-11-03 15:22:51 UTC
Created attachment 143132 [details] [review]
Bug 26635: Move expand syntax to x-koha-embed

Instead of a global av-expand flag (through a header) this patch
proposes to allow specifying +av_expand at the x-koha-embed header
level. This allows a more fine-grained control on what objects get avs
expanded. e.g.:

GET /patrons/123
x-koha-expand: +av_expand,checkouts.item+av_expand
=>
{
    "_str": {
        "city": { "str": "Córdoba", ... }
    },
    ...
    "checkouts": [
        {
            ...,
            "item": {
                "_str": {
                    "not_for_loan_status": { "str": "Reference material", ... },
                    ...
                },
                ...
            }
        }
    ]
}

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t \
           t/Koha/REST/Plugin/Query.t \
           t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Tests pass!
2. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 96 Kyle M Hall 2022-11-03 15:22:57 UTC
Created attachment 143133 [details] [review]
Bug 26635: (follow-up) Add optional support for `:` as the modifier

This patch adds optional support for using `:` as the modifier for
invoking the 'count' method for relations as discussed.

We retain support for `+` for backward compatability to prevent a
breaking change.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 97 Kyle M Hall 2022-11-03 15:23:02 UTC
Created attachment 143134 [details] [review]
Bug 26635: (QA follow-up) Remove test for x-koha-av-expand header

This patch updates the last remaining test that mentioned the now defunkt
x-koha-av-expand header.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 98 Jonathan Druart 2022-11-04 09:34:03 UTC
1. "Add optional support for `:` as the modifier"
We should document that. Why do we have it here actually? Shouldn't it be on its own bug?

2. Cannot we come with something better than "_str" and "str"?

3. We still have specific reference to "av" when I think we should focus on "stringify coded values" as a general concept.
Comment 99 Tomás Cohen Arazi 2022-11-04 12:24:28 UTC
(In reply to Jonathan Druart from comment #98)
> 1. "Add optional support for `:` as the modifier"
> We should document that. Why do we have it here actually? Shouldn't it be on
> its own bug?

I agree.

> 2. Cannot we come with something better than "_str" and "str"?

I agree with that, you proposed. You don't like?

> 3. We still have specific reference to "av" when I think we should focus on
> "stringify coded values" as a general concept.

AV expansion can be expensive. And so other strings. The idea of ':', was to later add <attribute>:str, so things that are not linked to av, might be generated on demand as well.
Comment 100 Jonathan Druart 2022-11-04 13:11:55 UTC
(In reply to Tomás Cohen Arazi from comment #99)
> (In reply to Jonathan Druart from comment #98)
> > 1. "Add optional support for `:` as the modifier"
> > We should document that. Why do we have it here actually? Shouldn't it be on
> > its own bug?
> 
> I agree.
> 
> > 2. Cannot we come with something better than "_str" and "str"?
> 
> I agree with that, you proposed. You don't like?

Yes, the idea, but I was expecting someone to tell me that "str" is ugly (especially when appearing twice :D)
The second one can be 'str', not the first one.

> > 3. We still have specific reference to "av" when I think we should focus on
> > "stringify coded values" as a general concept.
> 
> AV expansion can be expensive. And so other strings. The idea of ':', was to
> later add <attribute>:str, so things that are not linked to av, might be
> generated on demand as well.

In my mind consumers don't want to ask for each attributes to be expanded, but one "expand everything" (actually "stringify everything") switch. I may be wrong.

Maybe we are mixing "expand" and "stringify" into the same concept and we should not.
Comment 101 Kyle M Hall 2022-11-04 18:11:57 UTC
Created attachment 143276 [details] [review]
Bug 26635: (QA follow-up) Rename key _str to _strings
Comment 102 Kyle M Hall 2022-11-04 18:16:02 UTC
> Yes, the idea, but I was expecting someone to tell me that "str" is ugly
> (especially when appearing twice :D)
> The second one can be 'str', not the first one.

Fix in the latest followup!
 
> In my mind consumers don't want to ask for each attributes to be expanded,
> but one "expand everything" (actually "stringify everything") switch. I may
> be wrong.

I think it depends on the use case. For speed, it is good to specify each expansion you'd like. Adding an 'expand all' switch would be a nice enhancement on top of this, but I don't think it's necessary for this to move forward.
Comment 103 Martin Renvoize 2022-11-07 12:29:32 UTC
Created attachment 143354 [details] [review]
Bug 26635: Add tests

Add tests in t/db_dependent/Koha/Object.t and
t/db_dependent/Koha/REST/Plugin/Objects.t

Sponsored-by: Virginia Polytechnic Institute and State University
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 104 Martin Renvoize 2022-11-07 12:29:40 UTC
Created attachment 143355 [details] [review]
Bug 26635: Expand authorised values in to_api method

This patch adds the posibility to expand authorised values when to_api
method is called.

The classes where authorised values should expand must implememnt the
_fetch_authorised_values method, and must return a hash like the
following

{
  column_name => Koha::AuthorisedValue->unblessed
  ...
}

This patch will be used in bug 8179, so please test there.

Sponsored-by: Virginia Polytechnic Institute and State University
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 105 Martin Renvoize 2022-11-07 12:29:48 UTC
Created attachment 143356 [details] [review]
Bug 26635: Add tests for av-expand => 0

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 106 Martin Renvoize 2022-11-07 12:29:54 UTC
Created attachment 143357 [details] [review]
Bug 26635: AV expansion should ackowledge $params->{public}

This patch simplifies the original implementation so it is more
readable, removing extra loops and methods and just dealing with the
coded values inline on the existing loops.

It also adds non-public attributes removal from the original list, to
address the changes introduced by bug 28948.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t \
           t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 107 Martin Renvoize 2022-11-07 12:30:01 UTC
Created attachment 143358 [details] [review]
Bug 26635: Refined data structure and methods

This patch makes the returned data structure be simpler:

_str => {
    attribute_1 => {
        category => 'some_category_name',
        str      => 'description',
        type     => 'av'
    },
    ...
}

The description is sensible to context, so if public => 1 is passed,
then lib_opac is passed, and lib is returned otherwise. Whenever we add
language to the combo, we will add it to the implementation.

Tests are adjusted accordingly, also to reflect the public => 1 use
case.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t \
           t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 108 Martin Renvoize 2022-11-07 12:30:25 UTC
Created attachment 143359 [details] [review]
Bug 26635: Move expand syntax to x-koha-embed

Instead of a global av-expand flag (through a header) this patch
proposes to allow specifying +av_expand at the x-koha-embed header
level. This allows a more fine-grained control on what objects get avs
expanded. e.g.:

GET /patrons/123
x-koha-expand: +av_expand,checkouts.item+av_expand
=>
{
    "_str": {
        "city": { "str": "Córdoba", ... }
    },
    ...
    "checkouts": [
        {
            ...,
            "item": {
                "_str": {
                    "not_for_loan_status": { "str": "Reference material", ... },
                    ...
                },
                ...
            }
        }
    ]
}

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t \
           t/Koha/REST/Plugin/Query.t \
           t/db_dependent/Koha/REST/Plugin/Objects.t
=> SUCCESS: Tests pass!
2. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 109 Martin Renvoize 2022-11-07 12:30:33 UTC
Created attachment 143360 [details] [review]
Bug 26635: (QA follow-up) Remove test for x-koha-av-expand header

This patch updates the last remaining test that mentioned the now defunkt
x-koha-av-expand header.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 110 Martin Renvoize 2022-11-07 12:30:39 UTC
Created attachment 143361 [details] [review]
Bug 26635: (QA follow-up) Rename key _str to _strings

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 111 Martin Renvoize 2022-11-07 12:30:44 UTC
Created attachment 143362 [details] [review]
Bug 26635: (follow-up) Rename av_expand to strings

This patch updates the `av_expand` occurences to `strings` to match the
change of the return structure of `_strings`.

We replace `+av_expand` in the headers with `+strings`, the expected
object method name from `api_av_mapping` to `api_strings_mapping` and
the internal hash key from `av_expand` to `strings`.

Test plan
1) Run the included unit tests.. all should still pass.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 112 Martin Renvoize 2022-11-07 12:32:58 UTC
As discussed.. I updated the embed call to also refer to 'strings' as aposed to 'av_expand'.

Passed QA now :)
Comment 113 Tomás Cohen Arazi 2022-11-09 17:39:24 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!