From 9839bd7c25c727134e8331c3f43b884c5c4f02a2 Mon Sep 17 00:00:00 2001 From: Martin Renvoize 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 --- 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