Bug 26635 - Expand authorised values in REST API call
Summary: Expand authorised values in REST API call
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement with 10 votes (vote)
Assignee: Agustín Moyano
QA Contact:
URL:
Keywords:
Depends on: 26636
Blocks: 8179
  Show dependency treegraph
 
Reported: 2020-10-08 17:18 UTC by Agustín Moyano
Modified: 2021-04-05 19:17 UTC (History)
6 users (show)

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


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

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 '';