[Bug 38204] New: Add an API endpoint to list baskets
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 Bug ID: 38204 Summary: Add an API endpoint to list baskets Change sponsored?: --- Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Acquisitions Assignee: koha-bugs@lists.koha-community.org Reporter: matt.blenkinsop@ptfs-europe.com QA Contact: testopia@bugs.koha-community.org -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |matt.blenkinsop@ptfs-europe |ity.org |.com -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |38010 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38010 [Bug 38010] Migrate vendors to Vue -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 --- Comment #1 from Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> --- Created attachment 172943 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=172943&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 --- Comment #2 from Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> --- Created attachment 172944 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=172944&action=edit Bug 38204: Add unit test -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA CC| |david@davidnind.com --- Comment #3 from David Nind <david@davidnind.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 --- Comment #4 from Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #172943|0 |1 is obsolete| | --- Comment #5 from David Nind <david@davidnind.com> --- Created attachment 173020 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173020&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #172944|0 |1 is obsolete| | --- Comment #6 from David Nind <david@davidnind.com> --- Created attachment 173021 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173021&action=edit Bug 38204: Add unit test Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 --- Comment #7 from 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@gmail.com --- Comment #8 from Jonathan Druart <jonathan.druart@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 --- Comment #9 from Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com --- Comment #10 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com --- Comment #11 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 --- Comment #12 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 --- Comment #13 from Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> ---
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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 --- Comment #14 from Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> --- Created attachment 173259 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173259&action=edit Bug 38204: (QA follow-up) Require name property -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 --- Comment #15 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (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' ) { } ``` -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #173020|0 |1 is obsolete| | Attachment #173021|0 |1 is obsolete| | Attachment #173259|0 |1 is obsolete| | --- Comment #16 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 173366 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173366&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 --- Comment #17 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 173367 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173367&action=edit Bug 38204: Add unit test Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 --- Comment #18 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 173368 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173368&action=edit Bug 38204: (QA follow-up) Require name property -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 --- Comment #19 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 173369 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173369&action=edit Bug 38204: Tidy Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |tomascohen@gmail.com |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Add an API endpoint to list |Add `GET |baskets |/acquisitions/baskets` -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |This enhancement adds a new release notes| |API endpoint to list | |baskets for acquisitions. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |24.11.00 released in| | Status|Passed QA |Pushed to main -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 --- Comment #20 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed for 24.11! Well done everyone, thank you! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |roadmap_24_11 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the|This enhancement adds a new |This enhancement adds a new release notes|API endpoint to list |API endpoint to list |baskets for acquisitions. |baskets for acquisitions. | | | |**Sponsored by** *PTFS | |Europe* and *ByWater | |Solutions* -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com Status|Pushed to main |Needs documenting -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs documenting |RESOLVED Resolution|--- |FIXED --- Comment #21 from David Nind <david@davidnind.com> --- The API is not covered in the manual, so no changes to the manual are required. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 --- Comment #22 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (In reply to David Nind from comment #21)
The API is not covered in the manual, so no changes to the manual are required.
The API is self-documented! And any missing docs should be caught by QA! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 carthur@slolibrary.org <carthur@slolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |carthur@slolibrary.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38204 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |29668 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29668 [Bug 29668] Add API route to create a basket -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org