Bug 35469 - Cannot create bookings without circulation permissions
Summary: Cannot create bookings without circulation permissions
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 29002
Blocks:
  Show dependency treegraph
 
Reported: 2023-12-01 17:51 UTC by Nick Clemens
Modified: 2024-03-19 20:39 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.04


Attachments
Bug 35469: Allow fetching checkouts on API (1.03 KB, patch)
2023-12-15 11:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35469: Unit test (1.75 KB, patch)
2023-12-15 11:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35469: Add 'manage_bookings' subpermission to /checkouts (2.40 KB, patch)
2023-12-19 13:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35469: Add 'manage_bookings' permission to biblios/checkouts (3.45 KB, patch)
2023-12-19 13:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35469: Add 'manage_bookings' subpermission to /checkouts (2.45 KB, patch)
2023-12-19 13:55 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 35469: Add 'manage_bookings' permission to biblios/checkouts (3.50 KB, patch)
2023-12-19 13:55 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 35469: (follow-up) Tidy (1.21 KB, patch)
2023-12-19 13:55 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 35469: Add 'manage_bookings' permission to biblios/checkouts (3.54 KB, patch)
2024-01-26 08:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35469: Add 'manage_bookings' subpermission to /checkouts (2.49 KB, patch)
2024-01-26 08:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35469: Add 'manage_bookings' permission to biblios/checkouts (3.54 KB, patch)
2024-01-26 08:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35469: Add 'manage_bookings' subpermission to /checkouts (2.40 KB, patch)
2024-01-30 14:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35469: Add 'manage_bookings' permission to biblios/checkouts (3.45 KB, patch)
2024-01-30 14:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35469: Add 'manage_bookings' subpermission to /checkouts (2.45 KB, patch)
2024-01-30 14:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35469: Add 'manage_bookings' permission to biblios/checkouts (3.50 KB, patch)
2024-01-30 14:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35469: Add 'manage_bookings' subpermission to /checkouts (2.54 KB, patch)
2024-03-01 08:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35469: Add 'manage_bookings' permission to biblios/checkouts (3.60 KB, patch)
2024-03-01 08:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2023-12-01 17:51:15 UTC
When attempting to place a booking on an item in the catalogue a user needs several permissions:
1 - Patron search permissions (see bug 29503)
2 - circulate: manage_bookings
3 - circulate_ circulate_remaning_permissions

If you don't have the third, the loading of patrons seems to die on looking up checkouts via the api - this prevents the JS from enabling the 'Period' field for choosing booking dats and the booking cannot proceed.

That third permission is not expected I think
Comment 1 Martin Renvoize 2023-12-12 16:28:45 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.
Comment 2 Martin Renvoize 2023-12-12 16:29:31 UTC
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?
Comment 3 Martin Renvoize 2023-12-12 16:29:56 UTC
Or... maybe we need a 'View current checkouts' additional sub-permission.
Comment 4 Nick Clemens 2023-12-12 20:29:45 UTC
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
Comment 5 Martin Renvoize 2023-12-15 11:46:53 UTC
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'
Comment 6 Martin Renvoize 2023-12-15 11:46:56 UTC
Created attachment 159883 [details] [review]
Bug 35469: Unit test

This patch adds a unit test to check that manage_bookings allows
fetching of checkouts.
Comment 7 Nick Clemens 2023-12-15 15:12:19 UTC
I think this needs to adjust biblios.yaml as the call is:
/api/v1/biblio/{id}/checkouts
Comment 8 Martin Renvoize 2023-12-19 13:34:32 UTC
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'
Comment 9 Martin Renvoize 2023-12-19 13:34:35 UTC
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.
Comment 10 Martin Renvoize 2023-12-19 13:35:41 UTC
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.
Comment 11 Nick Clemens 2023-12-19 13:55:34 UTC
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>
Comment 12 Nick Clemens 2023-12-19 13:55:36 UTC
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>
Comment 13 Nick Clemens 2023-12-19 13:55:38 UTC
Created attachment 160007 [details] [review]
Bug 35469: (follow-up) Tidy

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 14 Jonathan Druart 2024-01-11 09:20:16 UTC
Same, cannot create without patron's permission. I get 403 on /patrons.
Comment 15 Jonathan Druart 2024-01-11 09:32:18 UTC
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.
Comment 16 Martin Renvoize 2024-01-15 15:40:03 UTC
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.
Comment 17 Marcel de Rooy 2024-01-26 08:21:09 UTC
Looking here
Comment 18 Marcel de Rooy 2024-01-26 08:24:52 UTC
#   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.
Comment 19 Marcel de Rooy 2024-01-26 08:25:27 UTC Comment hidden (obsolete)
Comment 20 Marcel de Rooy 2024-01-26 08:25:38 UTC
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>
Comment 21 Marcel de Rooy 2024-01-26 08:25:41 UTC
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>
Comment 22 Marcel de Rooy 2024-01-26 08:26:27 UTC
If possible, no separate tidy patches. Applied it on first patch.

Fix test?
Comment 23 Martin Renvoize 2024-01-30 14:09:11 UTC
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'
Comment 24 Martin Renvoize 2024-01-30 14:09:14 UTC
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.
Comment 25 Martin Renvoize 2024-01-30 14:10:05 UTC
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.
Comment 26 Martin Renvoize 2024-01-30 14:10:59 UTC
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>
Comment 27 Martin Renvoize 2024-01-30 14:11:02 UTC
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>
Comment 28 Marcel de Rooy 2024-03-01 08:52:57 UTC
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>
Comment 29 Marcel de Rooy 2024-03-01 08:52:59 UTC
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>
Comment 30 Katrin Fischer 2024-03-07 16:37:15 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 31 Fridolin Somers 2024-03-11 10:23:26 UTC
Pushed to 23.11.x for 23.11.04
Comment 32 Lucas Gass 2024-03-19 20:39:02 UTC
Missing 23.05.x dependencies, no backport.