Bugzilla – Attachment 190690 Details for
Bug 20956
BorrowersLog is not logging permission changes
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 20956: (QA follow-up) Match getuserflags() return structure
9839bd7.patch (text/plain), 6.90 KB, created by
Martin Renvoize (ashimema)
on 2025-12-22 17:14:49 UTC
(
hide
)
Description:
Bug 20956: (QA follow-up) Match getuserflags() return structure
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2025-12-22 17:14:49 UTC
Size:
6.90 KB
patch
obsolete
>From 9839bd7c25c727134e8331c3f43b884c5c4f02a2 Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >Date: Mon, 22 Dec 2025 17:06:31 +0000 >Subject: [PATCH] Bug 20956: (QA follow-up) Match getuserflags() return > structure > >This patch changes Koha::Patron->permissions() to return the same >structure as C4::Auth::getuserflags() for architectural consistency. > >Previously, permissions() only returned granted permissions: >{ > catalogue => 1, > circulate => { override_renewals => 1 } >} > >Now it returns ALL flags (matching getuserflags()): >{ > superlibrarian => 0, > catalogue => 1, > circulate => { override_renewals => 1 }, > borrowers => 0, > ... >} > >This provides several benefits: > >1. Consistency - Matches established C4::Auth::getuserflags() pattern >2. Simpler checks - Can use "if ($flags->{module})" instead of > "if (exists $flags->{module})" >3. Future compatibility - Enables eventual deprecation of getuserflags() > in favor of the ORM-based $patron->permissions() >4. Helper compatibility - Works with existing _dispatch() logic > >The logging functionality still works correctly - the diff will now >show explicit changes from 0 to 1 rather than absence to presence. > >Test plan: >1. prove t/db_dependent/Koha/Patron.t >2. prove t/db_dependent/Log.t >3. Verify all tests pass >4. Confirm permissions() now returns all flags with 0/1 values > >Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> >--- > Koha/Patron.pm | 52 ++++++++++++++++++++---------------- > t/db_dependent/Koha/Patron.t | 18 ++++++++----- > 2 files changed, 40 insertions(+), 30 deletions(-) > >diff --git a/Koha/Patron.pm b/Koha/Patron.pm >index 73a504da31f..d4217df010f 100644 >--- a/Koha/Patron.pm >+++ b/Koha/Patron.pm >@@ -3572,26 +3572,30 @@ sub is_anonymous { > > my $flags = $patron->permissions > >-Returns a structure such as: >+Returns a hashref of the patron's permissions. This method has the same >+return structure as C4::Auth::getuserflags for consistency. >+ >+Example return structure: > { >- "permissions": 1, >+ "superlibrarian": 0, >+ "permissions": 0, > "borrowers": 1, > "circulate": { > "manage_curbside_pickups": 1, > "overdues_report": 1, > "override_renewals": 1, >- "manage_restrictions": 1, >- "manage_checkout_notes": 1, >- "manage_bookings": 1 > }, > "catalogue": 1, >- "reserveforothers": { >- "place_holds": 1, >- "modify_holds_priority": 1 >- } >+ "reserveforothers": 0, >+ "tools": 0, >+ ... > } >-where a 1 indicates full permissions and a hash indicates >-partial permissions with the given permissions as hash keys >+ >+All permission flags are included in the returned hashref: >+- Flags set to 1 indicate the user has full permissions for that module >+- Flags set to 0 indicate the user does not have that permission >+- Flags set to a hashref indicate partial/granular permissions, where the >+ hash keys are the specific subpermissions granted > > =cut > >@@ -3603,27 +3607,29 @@ sub permissions { > my @userflags = $schema->resultset('Userflag')->search( {}, { order_by => 'bit' } )->all; > my %userflags_map = map { $_->bit => $_->flag } @userflags; > >- my $flags = $self->flags // 0; >- my $active_flags = {}; >+ my $flags = $self->flags // 0; >+ my $userflags = {}; > >- # Check which flags are active based on bit values >+ # Initialize all flags to 0, then set granted ones to 1 >+ # This matches the return structure of C4::Auth::getuserflags > foreach my $bit ( keys %userflags_map ) { >+ my $flag = $userflags_map{$bit}; > if ( $flags & ( 1 << $bit ) ) { >- my $flag = $userflags_map{$bit}; >- $active_flags->{$flag} = 1; >+ $userflags->{$flag} = 1; >+ } else { >+ $userflags->{$flag} = 0; > } > } > >- # Get granular subpermissions using existing helper >- my $user_perms = get_user_subpermissions( $self->userid ); >+ # Get granular subpermissions and merge with top-level permissions >+ my $user_subperms = get_user_subpermissions( $self->userid ); > >- for my $module ( keys %$user_perms ) { >- for my $code ( keys %{ $user_perms->{$module} } ) { >- $active_flags->{$module}->{$code} = 1; >- } >+ foreach my $module ( keys %$user_subperms ) { >+ next if $userflags->{$module} == 1; # user already has permission for everything in this module >+ $userflags->{$module} = $user_subperms->{$module}; > } > >- return $active_flags; >+ return $userflags; > } > > =head2 Internal methods >diff --git a/t/db_dependent/Koha/Patron.t b/t/db_dependent/Koha/Patron.t >index d58fcf4fbb9..a59331d9272 100755 >--- a/t/db_dependent/Koha/Patron.t >+++ b/t/db_dependent/Koha/Patron.t >@@ -808,7 +808,7 @@ subtest 'is_superlibrarian() tests' => sub { > > subtest 'permissions() tests' => sub { > >- plan tests => 8; >+ plan tests => 12; > > $schema->storage->txn_begin; > >@@ -824,9 +824,10 @@ subtest 'permissions() tests' => sub { > > my $permissions = $patron->permissions; > >- is( ref($permissions), 'HASH', "permissions() returns a hashref" ); >- is( scalar keys %$permissions, 1, "Patron has one module permission" ); >- is( $permissions->{catalogue}, 1, "Patron has catalogue permission" ); >+ is( ref($permissions), 'HASH', "permissions() returns a hashref" ); >+ ok( scalar keys %$permissions > 0, "permissions() returns all flags, not just granted ones" ); >+ is( $permissions->{catalogue}, 1, "Patron has catalogue permission" ); >+ is( $permissions->{superlibrarian}, 0, "Patron does not have superlibrarian (demonstrates all flags present)" ); > > # Test patron with no permissions > my $patron_no_perms = $builder->build_object( >@@ -836,7 +837,9 @@ subtest 'permissions() tests' => sub { > } > ); > my $no_perms = $patron_no_perms->permissions; >- is( scalar keys %$no_perms, 0, "Patron with no flags has no permissions" ); >+ ok( scalar keys %$no_perms > 0, "Patron with no flags still has hashref with all flags" ); >+ is( $no_perms->{catalogue}, 0, "Patron does not have catalogue permission" ); >+ is( $no_perms->{superlibrarian}, 0, "Patron does not have superlibrarian permission" ); > > # Test patron with multiple flags (catalogue=4 + circulate=2 = 6) > my $patron_multi = $builder->build_object( >@@ -846,8 +849,9 @@ subtest 'permissions() tests' => sub { > } > ); > my $multi_perms = $patron_multi->permissions; >- is( $multi_perms->{catalogue}, 1, "Multi-flag patron has catalogue permission" ); >- is( $multi_perms->{circulate}, 1, "Multi-flag patron has circulate permission" ); >+ is( $multi_perms->{catalogue}, 1, "Multi-flag patron has catalogue permission" ); >+ is( $multi_perms->{circulate}, 1, "Multi-flag patron has circulate permission" ); >+ is( $multi_perms->{superlibrarian}, 0, "Multi-flag patron does not have superlibrarian" ); > > # Test patron with granular subpermissions > my $patron_subperm = $builder->build_object( { class => 'Koha::Patrons', value => { flags => 0 } } ); >-- >2.52.0 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 20956
:
182955
|
185483
|
185491
|
186388
|
187592
|
187593
|
187594
|
188260
|
188261
|
188262
|
188263
|
190686
|
190687
|
190688
|
190689
| 190690