Bug 35468 - Bookings permission mismatch
Summary: Bookings permission mismatch
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 29002
Blocks: 36100
  Show dependency treegraph
 
Reported: 2023-12-01 17:47 UTC by Nick Clemens
Modified: 2024-02-15 10:14 UTC (History)
6 users (show)

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


Attachments
Bug 35468: Correct api bookings permission (1.34 KB, patch)
2023-12-12 13:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35468: Unit tests (14.57 KB, patch)
2023-12-12 16:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35468: Correct api bookings permission (1.39 KB, patch)
2023-12-12 16:55 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 35468: Unit tests (14.63 KB, patch)
2023-12-12 16:55 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 35468: (follow-up) Fix QA complaints (5.50 KB, patch)
2023-12-12 16:55 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 35468: (follow-up) Test only the specific permission needed (2.64 KB, patch)
2023-12-12 16:55 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 35468: Correct api bookings permission (1.39 KB, patch)
2023-12-13 12:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 35468: Unit tests (14.63 KB, patch)
2023-12-13 12:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 35468: (follow-up) Fix QA complaints (5.50 KB, patch)
2023-12-13 12:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 35468: (follow-up) Test only the specific permission needed (2.64 KB, patch)
2023-12-13 12:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 35468: Correct api bookings permission (1.45 KB, patch)
2023-12-13 12:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 35468: Unit tests (14.68 KB, patch)
2023-12-13 12:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 35468: (follow-up) Fix QA complaints (5.55 KB, patch)
2023-12-13 12:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 35468: (follow-up) Test only the specific permission needed (2.70 KB, patch)
2023-12-13 12:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 35468: (follow-up) Fix for older DateTimes (5.40 KB, patch)
2023-12-15 11:29 UTC, Martin Renvoize
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:47:07 UTC
git grep manage_bookings

The api routes require:
parameters: manage_bookings

The permission is defined as:
circulate: manage_bookings
Comment 1 Martin Renvoize 2023-12-12 13:12:02 UTC
Created attachment 159734 [details] [review]
Bug 35468: Correct api bookings permission

At some point during development we migrated from 'parameters' to
'circulate' for the manage_bookings sub-permission. Unfortunately, the
corresponsing API definition missed the update.
Comment 2 Martin Renvoize 2023-12-12 16:25:36 UTC
Created attachment 159746 [details] [review]
Bug 35468: Unit tests

This patch adds all the 'basic' unit tests one would expect to find with
the api endpoints.  I've been working on this as a follow-up to bug
29002 as promised, but hadn't yet submitted them as I wanted to get the
full coverage.. but having these basics in early makes sense whilst I
continue on the more advanced cases.
Comment 3 Nick Clemens 2023-12-12 16:55:07 UTC
Created attachment 159754 [details] [review]
Bug 35468: Correct api bookings permission

At some point during development we migrated from 'parameters' to
'circulate' for the manage_bookings sub-permission. Unfortunately, the
corresponsing API definition missed the update.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 4 Nick Clemens 2023-12-12 16:55:09 UTC
Created attachment 159755 [details] [review]
Bug 35468: Unit tests

This patch adds all the 'basic' unit tests one would expect to find with
the api endpoints.  I've been working on this as a follow-up to bug
29002 as promised, but hadn't yet submitted them as I wanted to get the
full coverage.. but having these basics in early makes sense whilst I
continue on the more advanced cases.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 5 Nick Clemens 2023-12-12 16:55:11 UTC
Created attachment 159756 [details] [review]
Bug 35468: (follow-up) Fix QA complaints

Tidyness
Use dt_from_string, not DateTime->now
Add exec flag

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 6 Nick Clemens 2023-12-12 16:55:12 UTC
Created attachment 159757 [details] [review]
Bug 35468: (follow-up) Test only the specific permission needed

The tests were granting circulation top-level permissions - we should be able to
perform the actions with only 'manage_bookings'. This patch updates the tests to
use the narrower permission.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 7 Martin Renvoize 2023-12-13 09:26:11 UTC
That's a great follow-up, thanks Nick.. I hadn't the brain power at the moment I was writing those tests to remember how to narrow it down that far :)
Comment 8 Tomás Cohen Arazi 2023-12-13 12:36:07 UTC Comment hidden (obsolete)
Comment 9 Tomás Cohen Arazi 2023-12-13 12:36:24 UTC Comment hidden (obsolete)
Comment 10 Tomás Cohen Arazi 2023-12-13 12:36:44 UTC Comment hidden (obsolete)
Comment 11 Tomás Cohen Arazi 2023-12-13 12:37:06 UTC Comment hidden (obsolete)
Comment 12 Tomás Cohen Arazi 2023-12-13 12:37:49 UTC
Trivial fix. Nice to see tests finally added for the feature. QA script happy.
Comment 13 Tomás Cohen Arazi 2023-12-13 12:44:33 UTC
Created attachment 159793 [details] [review]
Bug 35468: Correct api bookings permission

At some point during development we migrated from 'parameters' to
'circulate' for the manage_bookings sub-permission. Unfortunately, the
corresponsing API definition missed the update.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Tomás Cohen Arazi 2023-12-13 12:44:54 UTC
Created attachment 159794 [details] [review]
Bug 35468: Unit tests

This patch adds all the 'basic' unit tests one would expect to find with
the api endpoints.  I've been working on this as a follow-up to bug
29002 as promised, but hadn't yet submitted them as I wanted to get the
full coverage.. but having these basics in early makes sense whilst I
continue on the more advanced cases.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi 2023-12-13 12:45:10 UTC
Created attachment 159795 [details] [review]
Bug 35468: (follow-up) Fix QA complaints

Tidyness
Use dt_from_string, not DateTime->now
Add exec flag

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Tomás Cohen Arazi 2023-12-13 12:45:28 UTC
Created attachment 159796 [details] [review]
Bug 35468: (follow-up) Test only the specific permission needed

The tests were granting circulation top-level permissions - we should be able to
perform the actions with only 'manage_bookings'. This patch updates the tests to
use the narrower permission.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 17 Katrin Fischer 2023-12-14 15:31:41 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 18 Jonathan Druart 2023-12-15 05:46:09 UTC
koha_1       | #   Failed test 'list() tests'
koha_1       | #   at t/db_dependent/api/v1/bookings.t line 142.
koha_1       | Can't locate object method "rfc3339" via package "DateTime" at t/db_dependent/api/v1/bookings.t line 124.

Oops

https://metacpan.org/dist/DateTime/changes
1.53   2020-11-08
- Added a $dt->rfc3339 method. Based on discussion in GH #109 by worthmine.

buster (oldoldstable): 2:1.50
bullseye (oldstable): 2:1.54
bookworm (stable): 2:1.59

focal (20.04LTS): 2:1.51
jammy (22.04LTS): 2:1.55
lunar (23.04): 2:1.59
Comment 19 Martin Renvoize 2023-12-15 11:29:02 UTC
Created attachment 159881 [details] [review]
Bug 35468: (follow-up) Fix for older DateTimes

We can't use the rfc3339 method from DateTime itself as older Debians
don't ship with a recent enough version.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2023-12-15 11:29:52 UTC
That follow-up should make Jenkins happy again everywhere :)
Comment 21 Katrin Fischer 2023-12-18 10:51:54 UTC
I fail to get the perltidy QA test happy on this last patch. I verified all changed lines would not change when I perltidied them in my editor.
Any ideas?
Comment 22 Tomás Cohen Arazi 2023-12-18 12:50:28 UTC
(In reply to Katrin Fischer from comment #21)
> I fail to get the perltidy QA test happy on this last patch. I verified all
> changed lines would not change when I perltidied them in my editor.
> Any ideas?

I fully tidied the file and the diff was empty. I would skip that as a false positive. Maybe worth using it as a test case to enhance the QA tools, but I don't have the time today to get a deep dive into it.
Comment 23 Katrin Fischer 2023-12-18 13:01:43 UTC
(In reply to Tomás Cohen Arazi from comment #22)
> (In reply to Katrin Fischer from comment #21)
> > I fail to get the perltidy QA test happy on this last patch. I verified all
> > changed lines would not change when I perltidied them in my editor.
> > Any ideas?
> 
> I fully tidied the file and the diff was empty. I would skip that as a false
> positive. Maybe worth using it as a test case to enhance the QA tools, but I
> don't have the time today to get a deep dive into it.

Same for me, twice (once with older, once with current perltidy version)
Comment 24 Katrin Fischer 2023-12-18 14:25:27 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 25 Jonathan Druart 2023-12-19 11:01:30 UTC
root@kohadevbox:koha$ perltidy t/db_dependent/api/v1/bookings.t

root@kohadevbox:koha$ diff t/db_dependent/api/v1/bookings.t*
89,90c89,90
<     my $start_1   = dt_from_string->add( days => 1 )->truncate( to => 'day' );
<     my $end_1     = dt_from_string->add( days => 6 )->truncate( to => 'day' );
---
>     my $start_1 = dt_from_string->add( days => 1 )->truncate( to => 'day' );
>     my $end_1   = dt_from_string->add( days => 6 )->truncate( to => 'day' );
101,102c101,102
<     my $start_2   = dt_from_string->add( days => 4 )->truncate( to => 'day' );
<     my $end_2     = dt_from_string->add( days => 8 )->truncate( to => 'day' );
---
>     my $start_2 = dt_from_string->add( days => 4 )->truncate( to => 'day' );
>     my $end_2   = dt_from_string->add( days => 8 )->truncate( to => 'day' );

Seems easy to recreate.

ktd: This is perltidy, v20200110


However new version has a different output (host): v20210717

This will appear until we embed the new version in ktd, see bug 30002 comment 49.
Comment 26 Fridolin Somers 2024-01-03 14:05:48 UTC
Pushed to 23.11.x for 23.11.02
Comment 27 Lucas Gass 2024-01-04 18:43:20 UTC
Dependencies missing for 23.05.x, no backport.