We don't yet expose the lists functionality over API's, this would be a beneficial enhancement both for internal use and thrid party integrations wishing to integrate with this functionality.
Created attachment 174807 [details] [review] Bug 38050: create admin CRUD endpoint definitions this patch adds api endpoints for lists/virtualshelves. Tests to follow
Created attachment 174808 [details] [review] Bug 38050: create methods for admin CRUD list endpoints This patch implements the methods for the admin CRUD endpoints for lists/virtualshelves
Created attachment 174809 [details] [review] Bug 38050: create public CRUD list endpoint definitions This patch creates the yaml definitions for the public CRUD enpoints for lists/virtualshelves
Created attachment 174810 [details] [review] Bug 38050: create methods for public CRUD list endpoints This patch creates the methods for the public CRUD enpoints for lists/virtualshelves
You're going to need to add unit tests for all these for QA Jacob.
Created attachment 176462 [details] [review] Bug 38050: create admin CRUD endpoint definitions This patch creates the yaml definitions for the CRUD enpoints for virtualshelves/lists
Created attachment 176463 [details] [review] Bug 38050: create methods for admin CRUD list endpoints This patch implements the methods for the admin CRUD endpoints for lists/virtualshelves
Created attachment 176464 [details] [review] Bug 38050: create public CRUD list endpoint definitions This patch creates the yaml definitions for the public CRUD enpoints for lists/virtualshelves
Created attachment 176465 [details] [review] Bug 38050: create methods for public CRUD list endpoints This patch creates the methods for the public CRUD enpoints for lists/virtualshelves
Created attachment 176466 [details] [review] Bug 38050: create tests for admin CRUD list endpoints
Created attachment 176467 [details] [review] Bug 38050: create tests for public CRUD list endpoints
This patch adds API endpoints for lists, both public and admin. To Test: prove /t/db_dependent/api/v1/lists.t prove /t/db_dependent/api/v1/public/lists.t These currently respect "allow_changes_from_others" and "allow_changes_from_staff" as to whether you can modify a given list with the admin endpoints. The public endpoints can only modify lists owned by the authenticated user. As a consequence, to allow you to be able to always edit any lists you create with the admin endpoints (even with another user as the owner) unless changed, any created lists will be created with "allow_changes_from_staff=1" Additionally, passing ->filter_by_readable also means that private lists are not displayed, even with the admin endpoints. Is this intended functionality here or should the admin endpoints (which require administer lists permissions) allow you to edit/view all lists regardless?
Created attachment 176769 [details] [review] Bug 38050: create admin CRUD endpoint definitions This patch creates the yaml definitions for the CRUD enpoints for virtualshelves/lists Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176770 [details] [review] Bug 38050: create methods for admin CRUD list endpoints This patch implements the methods for the admin CRUD endpoints for lists/virtualshelves Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176771 [details] [review] Bug 38050: create public CRUD list endpoint definitions This patch creates the yaml definitions for the public CRUD enpoints for lists/virtualshelves Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176772 [details] [review] Bug 38050: create methods for public CRUD list endpoints This patch creates the methods for the public CRUD enpoints for lists/virtualshelves Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176773 [details] [review] Bug 38050: create tests for admin CRUD list endpoints Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 176774 [details] [review] Bug 38050: create tests for public CRUD list endpoints Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Test failures * Proving /usr/share/koha/t/db_dependent/api/v1/lists.t KO! # Failed test 'Anonymous users cannot access admin lists endpoint' # at /usr/share/koha/t/db_dependent/api/v1/lists.t line 62. # got: '404' # expected: '401' # Failed test 'Regular patrons cannot access admin lists endpoint' # at /usr/share/koha/t/db_dependent/api/v1/lists.t line 64. # got: '404' # expected: '403' # Failed test '200 OK' # at /usr/share/koha/t/db_dependent/api/v1/lists.t line 70. # got: '404' # expected: '200' # Failed test 'exact match for JSON Pointer ""' # at /usr/share/koha/t/db_dependent/api/v1/lists.t line 70. # Structures begin differing at: # $got = HASH(0x55dda459a920) # $expected = ARRAY(0x55dda43fba30) # Looks like you failed 4 tests of 7. etc etc
(In reply to Jacob O'Mara from comment #12) > Additionally, passing ->filter_by_readable also means that private lists are > not displayed, even with the admin endpoints. Is this intended functionality > here or should the admin endpoints (which require administer lists > permissions) allow you to edit/view all lists regardless? Probably not. But that is a long debate ;)
Created attachment 180788 [details] [review] Bug 38050: create admin CRUD endpoint definitions This patch creates the yaml definitions for the CRUD enpoints for virtualshelves/lists
Created attachment 180789 [details] [review] Bug 38050: create methods for admin CRUD list endpoints This patch implements the methods for the admin CRUD endpoints for lists/virtualshelves
Created attachment 180790 [details] [review] Bug 38050: create public CRUD list endpoint definitions This patch creates the yaml definitions for the public CRUD enpoints for lists/virtualshelves
Created attachment 180791 [details] [review] Bug 38050: create methods for public CRUD list endpoints This patch creates the methods for the public CRUD enpoints for lists/virtualshelves
Created attachment 180792 [details] [review] Bug 38050: create tests for admin CRUD list endpoints
Created attachment 180793 [details] [review] Bug 38050: create tests for public CRUD list endpoints
Created attachment 180838 [details] [review] Bug 38050: create admin CRUD endpoint definitions The delete endpoint tests were intermittently failing but this is now resolved. This patch creates the yaml definitions for the CRUD enpoints for virtualshelves/lists Current status: Signed Off
Created attachment 180839 [details] [review] Bug 38050: create methods for admin CRUD list endpoints This patch implements the methods for the admin CRUD endpoints for lists/virtualshelves
Created attachment 180840 [details] [review] Bug 38050: create public CRUD list endpoint definitions This patch creates the yaml definitions for the public CRUD enpoints for lists/virtualshelves
Created attachment 180841 [details] [review] Bug 38050: create methods for public CRUD list endpoints This patch creates the methods for the public CRUD enpoints for lists/virtualshelves
Created attachment 180842 [details] [review] Bug 38050: create tests for admin CRUD list endpoints
Created attachment 180843 [details] [review] Bug 38050: create tests for public CRUD list endpoints
Created attachment 180879 [details] [review] Bug 38050: create admin CRUD endpoint definitions This patch creates the yaml definitions for the CRUD enpoints for virtualshelves/lists
Created attachment 180880 [details] [review] Bug 38050: create methods for admin CRUD list endpoints This patch implements the methods for the admin CRUD endpoints for lists/virtualshelves
Created attachment 180881 [details] [review] Bug 38050: create public CRUD list endpoint definitions This patch creates the yaml definitions for the public CRUD enpoints for lists/virtualshelves
Created attachment 180882 [details] [review] Bug 38050: create methods for public CRUD list endpoints This patch creates the methods for the public CRUD enpoints for lists/virtualshelves
Created attachment 180883 [details] [review] Bug 38050: create tests for admin CRUD list endpoints
Created attachment 180884 [details] [review] Bug 38050: create tests for public CRUD list endpoints
Let's not talk about the fact that I re-attached this again. That's what happens when you forget to re-run the qa script before attaching changes. Let me be a lesson for others.
(In reply to Martin Renvoize (ashimema) from comment #0) > We don't yet expose the lists functionality over API's, this would be a > beneficial enhancement both for internal use and thrid party integrations > wishing to integrate with this functionality. I would love this
description: "This resource returns a list of existing bibliographic lists." - summary: List bibliographic lists + x-koha-authorization: + permissions: + tools: manage_patron_lists This is the wrong "list" module.
QA comment: 1. See previous comment re the permission 2. 2 occurrences of Data::Dumper left in the patches 3. Why isn't Koha::REST::V1::List::update using $shelf->can_be_managed to know if the logged in user can edit the list? 4. What about the other logic in virtualshelves/shelves.pl? 126 $sortfield = 'title' 127 unless grep { $_ eq $sortfield } qw( title author copyrightdate publicationyear itemcallnumber dateadded ); 128 if ( $sortfield eq 'copyrightdate' and C4::Context->preference('marcflavour') eq 'UNIMARC' ) { 129 $sortfield = 'publicationyear'; 130 } Shouldn't it be port to the REST API endpoint as well? 5. Same as 3 for deletion: why isn't the logic the same as the shelves.pl controller? + if ( $list->can_be_deleted( $user->id ) + || $list->allow_change_from_staff + || $list->allow_change_from_others + || ( $list->is_public && C4::Auth::haspermission( $user->userid, { lists => 'delete_public_lists' } ) ) ) vs 158 if ($shelf) { 159 if ( $shelf->can_be_deleted($loggedinuser) ) { 160 eval { $shelf->delete; }; I don't want to know which one is correct, but I expect to see the exact same code in both places. 6. What if the pref `virtualshelves` is off? 7. QA script is failing: [FAIL] t/db_dependent/api/v1/lists.t FAIL test_no_warnings 'use Test::NoWarnings' must be present in this test file. [FAIL] t/db_dependent/api/v1/public/lists.t FAIL file_permissions File must have the exec flag
Created attachment 189219 [details] [review] Bug 38050: create admin CRUD endpoint definitions This patch creates the yaml definitions for the CRUD enpoints for virtualshelves/lists
Created attachment 189220 [details] [review] Bug 38050: create methods for admin CRUD list endpoints This patch implements the methods for the admin CRUD endpoints for lists/virtualshelves
Created attachment 189221 [details] [review] Bug 38050: create public CRUD list endpoint definitions This patch creates the yaml definitions for the public CRUD enpoints for lists/virtualshelves
Created attachment 189222 [details] [review] Bug 38050: create methods for public CRUD list endpoints This patch creates the methods for the public CRUD enpoints for lists/virtualshelves
Created attachment 189223 [details] [review] Bug 38050: create tests for admin CRUD list endpoints
Created attachment 189224 [details] [review] Bug 38050: create tests for public CRUD list endpoints
Created attachment 189225 [details] [review] Bug 38050: move sortfield validation to method for reuse Call method instead of repeating the validation logic
Thanks for the above! This latest patchset should address all of the above with one notable exception. All of the logic is now consistently uses the can_be_x checks and the sortfield logic is also integrated. My understanding of the allow_change_x columns was flawed as these seem to be for the add/removal of items from lists. Not typically lists themselves so those have been removed from this logic, list items endpoints will need to integrate with these though. The notable exception: There is no handling for if the virtualshelves pref is disabled, though this is currently consistent with other api’s. I couldn’t find any instances where we DO handle that. For instance the ERM api’s seem to all still work while it’s disabled and I couldn’t find any logic that should stop them from doing so. While I could technically add a syspref check at the top of all of the api methods, I think we would be better suited handling this at the router/swagger level. I can add a bug to that effect if you agree? Or, if I’m wrong, if you could point me at an example of endpoints that handle this, I would be happy to follow up.