git grep manage_bookings The api routes require: parameters: manage_bookings The permission is defined as: circulate: manage_bookings
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.
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.
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>
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>
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>
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>
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 :)
Created attachment 159789 [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>
Created attachment 159790 [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>
Created attachment 159791 [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>
Created attachment 159792 [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>
Trivial fix. Nice to see tests finally added for the feature. QA script happy.
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>
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>
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>
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>
Pushed for 24.05! Well done everyone, thank you!
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
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>
That follow-up should make Jenkins happy again everywhere :)
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?
(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.
(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)
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.
Pushed to 23.11.x for 23.11.02
Dependencies missing for 23.05.x, no backport.