Created attachment 172943 [details] [review] Bug 38204: Add api endpoint to list baskets This patch adds an API endpoint to list baskets It uses CanUserManageBasket to respect the users permissions and sysprefs Test plan: 1) prove t/db_dependent/api/v1/acquisitions_baskets.t
Created attachment 172944 [details] [review] Bug 38204: Add unit test
The tests fail after applying the patches: prove t/db_dependent/api/v1/acquisitions_baskets.t t/db_dependent/api/v1/acquisitions_baskets.t .. 1/2 # Failed test '200 OK' # at t/db_dependent/api/v1/acquisitions_baskets.t line 89. # got: '404' # expected: '200' # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/acquisitions_baskets.t line 89. # Structures begin differing at: # $got = HASH(0x58ebb493a0e8) # $expected = ARRAY(0x58ebb49bd280) # Failed test '200 OK' # at t/db_dependent/api/v1/acquisitions_baskets.t line 105. # got: '404' # expected: '200' # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/acquisitions_baskets.t line 105. # Structures begin differing at: # $got = HASH(0x58ebb4992410) # $expected = ARRAY(0x58ebb4792a58) # Looks like you failed 4 tests of 6. # Failed test 'list() tests' # at t/db_dependent/api/v1/acquisitions_baskets.t line 109. # Looks like you failed 1 test of 2. t/db_dependent/api/v1/acquisitions_baskets.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/2 subtests Test Summary Report ------------------- t/db_dependent/api/v1/acquisitions_baskets.t (Wstat: 256 (exited 1) Tests: 2 Failed: 1) Failed test: 2 Non-zero exit status: 1 Files=1, Tests=2, 4 wallclock secs ( 0.01 usr 0.00 sys + 3.33 cusr 0.38 csys = 3.72 CPU) Result: FAIL
Hi David, my fault - I forgot to add the following steps to the test plan: 1) yarn api:bundle 2) restart_all 3) Run prove t/db_dependent/api/v1/acquisitions_baskets.t
Created attachment 173020 [details] [review] Bug 38204: Add api endpoint to list baskets This patch adds an API endpoint to list baskets It uses CanUserManageBasket to respect the users permissions and sysprefs Test plan: 1) prove t/db_dependent/api/v1/acquisitions_baskets.t Signed-off-by: David Nind <david@davidnind.com>
Created attachment 173021 [details] [review] Bug 38204: Add unit test Signed-off-by: David Nind <david@davidnind.com>
(In reply to Matt Blenkinsop from comment #4) > Hi David, my fault - I forgot to add the following steps to the test plan: > > 1) yarn api:bundle > 2) restart_all > 3) Run prove t/db_dependent/api/v1/acquisitions_baskets.t Thanks Matt! Now signed off.
1. Why do you make the basket's name not null? name: - type: string + type: + - string + - "null" It can be NULL at DB level, but the UI is enforcing it. I think we should keep the constraint at the REST API level. 2. create_items: - type: string + type: + - string + - "null" Same, why allowing null here? 3. - authorised_by: + creator_id: It has been there for a while now, isn't it a problem to modify it, especially without advertising it. There may be existing consumers.
(In reply to Jonathan Druart from comment #8) > 1. Why do you make the basket's name not null? > name: > - type: string > + type: > + - string > + - "null" > > It can be NULL at DB level, but the UI is enforcing it. I think we should > keep the constraint at the REST API level. I can reset this and do checks in the controller > 2. > create_items: > - type: string > + type: > + - string > + - "null" > Same, why allowing null here? In the description for create_items it says "When items should be created for orders in this basket (Options: 'ordering', 'receiving', 'cataloguing'. Null means system wide config)". Null is supposed to be allowed as an option but the endpoint crashes if it is set to null as we don't allow null to be passed > 3. > - authorised_by: > + creator_id: > > It has been there for a while now, isn't it a problem to modify it, > especially without advertising it. There may be existing consumers. How does this work with the to_api_mapping? That maps authorisedby to creator_id which suggests there would be conflict?
(In reply to Matt Blenkinsop from comment #9) > (In reply to Jonathan Druart from comment #8) > > 1. Why do you make the basket's name not null? > > name: > > - type: string > > + type: > > + - string > > + - "null" > > > > It can be NULL at DB level, but the UI is enforcing it. I think we should > > keep the constraint at the REST API level. > > I can reset this and do checks in the controller If you remove the "null" option in the spec, the OpenAPI plugin will take care of validation. > > 2. > > create_items: > > - type: string > > + type: > > + - string > > + - "null" > > Same, why allowing null here? > > In the description for create_items it says "When items should be created > for orders in this basket (Options: 'ordering', 'receiving', 'cataloguing'. > Null means system wide config)". Null is supposed to be allowed as an option > but the endpoint crashes if it is set to null as we don't allow null to be > passed I don't like enum fields to not be advertised as such in the spec. But I'm not sure how's that gonna work with the 'nullable' option. You should try. And maybe add a 'system_wide' option that would map to NULL on its way to the DB. > > 3. > > - authorised_by: > > + creator_id: > > > > It has been there for a while now, isn't it a problem to modify it, > > especially without advertising it. There may be existing consumers. > > How does this work with the to_api_mapping? That maps authorisedby to > creator_id which suggests there would be conflict? Having consumers already use the API as-is should be considered. I suggest we: * Add the new mapping * Manually add the old field in the Koha::Acquisition::Basket::to_api method (the same way we do it for `closed`. * Update the spec to *add* the new field, and add a [DEPRECATED] message on the old one * File a bug for removing it on 25.05. * Make sure we mention this in the release notes.
The to_api_mapping was changed way back in 2020 by Bug 24918: Fix attribute mappings in Koha::Acquisition::Basket As such, I don't think anyone can be using the old nomenclature, as surely the routes working with baskets have been failing since then since the specification should have been validating against it.
Ha.. nothing has actually been validating against this basket spec before now.. so he's just fixing the spec to match what has already been the case in the actual returns for some time. We don't ref to the basket spec for either of the existing embeds (I seem to remember trying to with edi but discovering I couldn't due to library limitations and the 'or null' option. Correct me if I'm wrong, but we still don't have anything better than: basket: type: - object - "null" description: Koha basket For the case where we need to support no basket key at all, basket key but null as the content and basket key with a full basket object as the response... or can we ref the basket definition for the object type
> > > 2. > > > create_items: > > > - type: string > > > + type: > > > + - string > > > + - "null" > > > Same, why allowing null here? > > > > In the description for create_items it says "When items should be created > > for orders in this basket (Options: 'ordering', 'receiving', 'cataloguing'. > > Null means system wide config)". Null is supposed to be allowed as an option > > but the endpoint crashes if it is set to null as we don't allow null to be > > passed > > I don't like enum fields to not be advertised as such in the spec. But I'm > not sure how's that gonna work with the 'nullable' option. You should try. > And maybe add a 'system_wide' option that would map to NULL on its way to > the DB. The nullable option does work in the spec, I've tested it. I think if we had a "system_wide" option it should match the AcqCreateItem syspref. Having said that we don't currently have a POST/PUT endpoint for baskets so its only relevant for listing them currently
Created attachment 173259 [details] [review] Bug 38204: (QA follow-up) Require name property
(In reply to Matt Blenkinsop from comment #13) > > The nullable option does work in the spec, I've tested it. I think if we had > a "system_wide" option it should match the AcqCreateItem syspref. Having > said that we don't currently have a POST/PUT endpoint for baskets so its > only relevant for listing them currently As y'all have noticed, even if there was no baskets endpoint, I added a spec and stuff for being able to embed them. In this case, we don't have POST or PUT, but when time comes (probably soon?) we don't want to change the spec again. I'd be in favor of even refactoring the nullable nature of the field in the DB, which makes us have to deal with it here, adding technical debt. It would mean just changing some: ```perl if (!$basket->create_items) { } ``` for ```perl if ( $basket->create_items eq 'system_wide' ) { } ```
Created attachment 173366 [details] [review] Bug 38204: Add api endpoint to list baskets This patch adds an API endpoint to list baskets It uses CanUserManageBasket to respect the users permissions and sysprefs Test plan: 1) prove t/db_dependent/api/v1/acquisitions_baskets.t Signed-off-by: David Nind <david@davidnind.com>
Created attachment 173367 [details] [review] Bug 38204: Add unit test Signed-off-by: David Nind <david@davidnind.com>
Created attachment 173368 [details] [review] Bug 38204: (QA follow-up) Require name property
Created attachment 173369 [details] [review] Bug 38204: Tidy Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pushed for 24.11! Well done everyone, thank you!