From 9ded76004b781cfacadd365a9c80fed9c5f2895e Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Mon, 22 Dec 2025 16:38:42 +0000 Subject: [PATCH] Bug 20956: (QA follow-up) Fix tests and refactor to use ORM This patch addresses several QA concerns: 1. Fixed duplicate subtest name - renamed second "is_superlibrarian() tests" to "permissions() tests" which accurately reflects what it tests 2. Fixed incorrect hash assignment - permissions() returns a hashref, not a hash. Changed `my %permissions` to `my $permissions` 3. Improved test coverage - expanded from 2 to 8 assertions covering: - Hashref return type verification - Single flag permission - No permissions - Multiple flags - Granular subpermissions from user_permissions table 4. Refactored permissions() method to use DBIx::Class ORM instead of raw SQL queries, following Koha::Object architecture principles All tests now pass without warnings. Test plan: 1. prove t/db_dependent/Koha/Patron.t 2. prove t/db_dependent/Log.t 3. Verify all tests pass 4. Confirm the test now properly validates the intended behavior Signed-off-by: Martin Renvoize --- Koha/Patron.pm | 17 +++++++------ t/db_dependent/Koha/Patron.t | 48 ++++++++++++++++++++++++++++++------ 2 files changed, 50 insertions(+), 15 deletions(-) diff --git a/Koha/Patron.pm b/Koha/Patron.pm index a0da88e4001..73a504da31f 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -3598,22 +3598,23 @@ partial permissions with the given permissions as hash keys sub permissions { my ($self) = @_; - my $dbh = C4::Context->dbh; - my $sth = $dbh->prepare("SELECT bit, flag FROM userflags ORDER BY bit"); - $sth->execute(); - my $userflags = { map { $_->[0] => $_->[1] } @{ $sth->fetchall_arrayref() } }; - - my $flags = $self->flags // 0; + # Get userflags using ORM + my $schema = Koha::Database->schema; + 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 = {}; - foreach my $bit ( keys %$userflags ) { + # Check which flags are active based on bit values + foreach my $bit ( keys %userflags_map ) { if ( $flags & ( 1 << $bit ) ) { - my $flag = $userflags->{$bit}; + my $flag = $userflags_map{$bit}; $active_flags->{$flag} = 1; } } + # Get granular subpermissions using existing helper my $user_perms = get_user_subpermissions( $self->userid ); for my $module ( keys %$user_perms ) { diff --git a/t/db_dependent/Koha/Patron.t b/t/db_dependent/Koha/Patron.t index be5875acf1e..d58fcf4fbb9 100755 --- a/t/db_dependent/Koha/Patron.t +++ b/t/db_dependent/Koha/Patron.t @@ -19,7 +19,7 @@ use Modern::Perl; -use Test::More tests => 45; +use Test::More tests => 46; use Test::NoWarnings; use Test::Exception; use Test::Warn; @@ -806,24 +806,58 @@ subtest 'is_superlibrarian() tests' => sub { $schema->storage->txn_rollback; }; -subtest 'is_superlibrarian() tests' => sub { +subtest 'permissions() tests' => sub { - plan tests => 2; + plan tests => 8; $schema->storage->txn_begin; + my $dbh = C4::Context->dbh; + + # Test patron with single flag permission (catalogue = bit 2, value 4) my $patron = $builder->build_object( { class => 'Koha::Patrons', - value => { flags => 4 } } ); - my %permissions = $patron->permissions; + my $permissions = $patron->permissions; - 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" ); + is( scalar keys %$permissions, 1, "Patron has one module permission" ); + is( $permissions->{catalogue}, 1, "Patron has catalogue permission" ); + + # Test patron with no permissions + my $patron_no_perms = $builder->build_object( + { + class => 'Koha::Patrons', + value => { flags => 0 } + } + ); + my $no_perms = $patron_no_perms->permissions; + is( scalar keys %$no_perms, 0, "Patron with no flags has no permissions" ); + + # Test patron with multiple flags (catalogue=4 + circulate=2 = 6) + my $patron_multi = $builder->build_object( + { + class => 'Koha::Patrons', + value => { flags => 6 } + } + ); + 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" ); + + # Test patron with granular subpermissions + my $patron_subperm = $builder->build_object( { class => 'Koha::Patrons', value => { flags => 0 } } ); + $dbh->do( + "INSERT INTO user_permissions (borrowernumber, module_bit, code) VALUES (?, ?, ?)", + undef, $patron_subperm->borrowernumber, 1, 'override_renewals' + ); + my $subperms = $patron_subperm->get_from_storage->permissions; + is( ref( $subperms->{circulate} ), 'HASH', "Granular permissions stored as hashref" ); + is( $subperms->{circulate}->{override_renewals}, 1, "Subpermission correctly detected" ); $schema->storage->txn_rollback; }; -- 2.52.0