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 }
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.
Created attachment 111401 [details] [review] Bug 26635: Add tests Add tests in t/db_dependent/Koha/Object.t
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.
Created attachment 111411 [details] [review] Bug 26635: Add tests Add tests in t/db_dependent/Koha/Object.t
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.
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
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.
Can't apply this patch in order to apply and test 8179
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
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.
(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
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
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>
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
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>
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>
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?
(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.
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>
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>
(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?
(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
(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
(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 '';
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>
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>
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>
Created attachment 124268 [details] [review] Bug 26635: Add tests for av-expand => 0
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.
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...
(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
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..
(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
(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.
It would also be nice if it could be embedded in the self-documentation somehow.
Started an API Users Guide on the Wiki: https://wiki.koha-community.org/wiki/Koha_REST_API_Users_Guide
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.
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.
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.
(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.
Would love to see this get unstuck, authorized values descriptions from the REST API would be really handy!
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.
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.
*** Bug 31683 has been marked as a duplicate of this bug. ***
Another wrinkle could be translations for authorised values: https://lists.katipo.co.nz/public/koha/2021-December/057052.html
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...
(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.
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.
Tomas, This is marked as 'ASSIGNED', is there work to do or is ready for testing?
(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.
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>
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>
Created attachment 142763 [details] [review] Bug 26635: Add tests for av-expand => 0
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.
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>
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>
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>
Created attachment 142794 [details] [review] Bug 26635: Add tests for av-expand => 0
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>
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>
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..
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>
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>
Created attachment 142803 [details] [review] Bug 26635: Add tests for av-expand => 0 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
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>
All works for me.. Signing Off
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.
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.
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!
OK, with that example I understand better where you were going with it.. makes sense to me.
(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.
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>
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>
Created attachment 143041 [details] [review] Bug 26635: Add tests for av-expand => 0
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>
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>
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
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!
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>
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>
Created attachment 143075 [details] [review] Bug 26635: Add tests for av-expand => 0 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
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>
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>
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>
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>
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`
(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`.
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>
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>
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>
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>
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>
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>
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>
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>
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.
(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.
(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.
Created attachment 143276 [details] [review] Bug 26635: (QA follow-up) Rename key _str to _strings
> 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.
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>
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>
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>
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>
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>
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>
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>
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>
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>
As discussed.. I updated the embed call to also refer to 'strings' as aposed to 'av_expand'. Passed QA now :)
Pushed to master for 22.11. Nice work everyone, thanks!