|
Description
Nick Clemens (kidclamp)
2023-12-01 17:51:15 UTC
Ah, I see... The third permission is required as we need to lookup current checkouts for all items on the biblio so we can prevent a booking being added during the current checkout of an item. Should we disable the 'Place booking' option if they don't have the circulate remaining permission.. or do we think the permission set on the checkouts api endpoint is set incorrectly in the first place? Or... maybe we need a 'View current checkouts' additional sub-permission. Could it be a new bookings endpoint, or a parameter on the bookings endpoint to include circulations? I think 'manage_bookings' covers the ability to place/update/delete bookings, so shouldn't require more than this. It looks like we fetch the checkouts, and turn them into bookings for the purposes here anyway - so seems it would make sense to have a 'current bookings' which includes the checkouts Created attachment 159882 [details] [review] Bug 35469: Allow fetching checkouts on API This patch adds the 'manage_bookings' permission to allow fetching of checkouts on the API should the user have 'manage_bookings' but not have 'circulate_remaining_permissions' Created attachment 159883 [details] [review] Bug 35469: Unit test This patch adds a unit test to check that manage_bookings allows fetching of checkouts. I think this needs to adjust biblios.yaml as the call is:
/api/v1/biblio/{id}/checkouts
Created attachment 159996 [details] [review] Bug 35469: Add 'manage_bookings' subpermission to /checkouts This patch adds the 'manage_bookings' permission to allow fetching of checkouts on the API should the user have 'manage_bookings' but not have 'circulate_remaining_permissions' Created attachment 159997 [details] [review] Bug 35469: Add 'manage_bookings' permission to biblios/checkouts This patch adds the manage_bookings subpermission check to the biblios/{biblio_id}/checkouts endpoint and updates the corresponding unit test too. Good catch... I added a new patch, but also opted to leave the original here as I think it makes sense for consistency in the api permissions across these two similar endpoints. Created attachment 160005 [details] [review] Bug 35469: Add 'manage_bookings' subpermission to /checkouts This patch adds the 'manage_bookings' permission to allow fetching of checkouts on the API should the user have 'manage_bookings' but not have 'circulate_remaining_permissions' Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 160006 [details] [review] Bug 35469: Add 'manage_bookings' permission to biblios/checkouts This patch adds the manage_bookings subpermission check to the biblios/{biblio_id}/checkouts endpoint and updates the corresponding unit test too. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 160007 [details] [review] Bug 35469: (follow-up) Tidy Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Same, cannot create without patron's permission. I get 403 on /patrons. I am not sure this is correct. It feels more like we need a dedicated route for bookings. Maybe we need an endpoint that will return the existing dates? That will simplify the JS code and make us move the logic to a Perl module. Whilst this might be the longer term solution, I don't think we should focus on that here. We need a rethink of permissions in general as discussed a little on bug 25909. As for merging current checkouts into the bookings endpoints, I think that will add needless complexity into the bookings api controllers.. we'd need to also map fields for searching, joining, embedding etc etc to make that work as expected in all cases.. that is not a trivial piece of code to write or test. Looking here # Failed test 'exact match for JSON Pointer "/due_date"' # at /usr/share/koha/t/db_dependent/api/v1/checkouts.t line 215. # got: '2024-01-12T23:59:00+00:00' # expected: '2024-02-16T23:59:00+00:00' # Failed test 'exact match for JSON Pointer "/due_date"' # at /usr/share/koha/t/db_dependent/api/v1/checkouts.t line 245. # got: '2024-01-12T23:59:00+00:00' # expected: '2024-02-16T23:59:00+00:00' # Looks like you failed 2 tests of 108. Created attachment 161490 [details] [review] Bug 35469: Add 'manage_bookings' permission to biblios/checkouts This patch adds the manage_bookings subpermission check to the biblios/{biblio_id}/checkouts endpoint and updates the corresponding unit test too. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 161491 [details] [review] Bug 35469: Add 'manage_bookings' subpermission to /checkouts This patch adds the 'manage_bookings' permission to allow fetching of checkouts on the API should the user have 'manage_bookings' but not have 'circulate_remaining_permissions' Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 161492 [details] [review] Bug 35469: Add 'manage_bookings' permission to biblios/checkouts This patch adds the manage_bookings subpermission check to the biblios/{biblio_id}/checkouts endpoint and updates the corresponding unit test too. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> If possible, no separate tidy patches. Applied it on first patch. Fix test? Created attachment 161639 [details] [review] Bug 35469: Add 'manage_bookings' subpermission to /checkouts This patch adds the 'manage_bookings' permission to allow fetching of checkouts on the API should the user have 'manage_bookings' but not have 'circulate_remaining_permissions' Created attachment 161640 [details] [review] Bug 35469: Add 'manage_bookings' permission to biblios/checkouts This patch adds the manage_bookings subpermission check to the biblios/{biblio_id}/checkouts endpoint and updates the corresponding unit test too. Weird, the tests pass for me.. and I was sure I'd run tidy prior to submitting these up. Anyway.. I've rebased my branch and squashed a tidy in. Created attachment 161641 [details] [review] Bug 35469: Add 'manage_bookings' subpermission to /checkouts This patch adds the 'manage_bookings' permission to allow fetching of checkouts on the API should the user have 'manage_bookings' but not have 'circulate_remaining_permissions' Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 161642 [details] [review] Bug 35469: Add 'manage_bookings' permission to biblios/checkouts This patch adds the manage_bookings subpermission check to the biblios/{biblio_id}/checkouts endpoint and updates the corresponding unit test too. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 162644 [details] [review] Bug 35469: Add 'manage_bookings' subpermission to /checkouts This patch adds the 'manage_bookings' permission to allow fetching of checkouts on the API should the user have 'manage_bookings' but not have 'circulate_remaining_permissions' Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 162645 [details] [review] Bug 35469: Add 'manage_bookings' permission to biblios/checkouts This patch adds the manage_bookings subpermission check to the biblios/{biblio_id}/checkouts endpoint and updates the corresponding unit test too. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Pushed for 24.05! Well done everyone, thank you! Pushed to 23.11.x for 23.11.04 Missing 23.05.x dependencies, no backport. |