From 020417e8c5a84af5011c7bbbf33ff7a0870a7de2 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Mon, 20 Oct 2025 10:16:51 +0100 Subject: [PATCH] Bug 41020: (follow-up) Modernize code to use Koha objects and relationship accessors This follow-up improves the code to better follow Koha best practices: 1. Added file_transport() relationship accessor to Koha::MarcOrderAccount - Returns the correct polymorphic subclass (SFTP, FTP, or Local) - Respects prefetched relationships to avoid N+1 queries - Follows the same pattern as vendor() and budget() accessors 2. Updated admin/marc_order_accounts.pl to use Koha::File::Transports - Replaced direct DBIx::Class schema access with Koha objects - Removed unused $schema variable 3. Updated misc/cronjobs/marc_ordering_process.pl - Changed manual transport lookup to use the new relationship accessor - Cleaner code that can benefit from prefetching 4. Added unit tests (t/db_dependent/Koha/MarcOrderAccount.t) - Tests for all relationship accessors (vendor, budget, file_transport) - Verifies polymorphism works correctly for all transport types - Includes test demonstrating that prefetched relationships don't trigger additional database queries Test plan: 1. prove t/db_dependent/Koha/MarcOrderAccount.t 2. Verify all tests pass 3. Run marc_ordering_process.pl cronjob and verify it still works Signed-off-by: Martin Renvoize --- Koha/MarcOrderAccount.pm | 21 ++ admin/marc_order_accounts.pl | 6 +- misc/cronjobs/marc_ordering_process.pl | 3 +- t/db_dependent/Koha/MarcOrderAccount.t | 263 +++++++++++++++++++++++++ 4 files changed, 288 insertions(+), 5 deletions(-) create mode 100644 t/db_dependent/Koha/MarcOrderAccount.t diff --git a/Koha/MarcOrderAccount.pm b/Koha/MarcOrderAccount.pm index 517ab5c7f62..1e591aa737d 100644 --- a/Koha/MarcOrderAccount.pm +++ b/Koha/MarcOrderAccount.pm @@ -52,6 +52,27 @@ sub budget { return Koha::Acquisition::Fund->_new_from_dbic($budget_rs); } +=head3 file_transport + +Returns the Koha::File::Transport::(S)FTP object for this account, if one is configured. + + my $transport = $account->file_transport; + +=cut + +sub file_transport { + my ($self) = @_; + my $transport_rs = $self->_result->file_transport; + return unless $transport_rs; + + # Use Koha::File::Transports to determine the correct polymorphic class + require Koha::File::Transports; + my $transports = Koha::File::Transports->new; + my $class = $transports->object_class($transport_rs); + + return $class->_new_from_dbic($transport_rs); +} + =head3 _type =cut diff --git a/admin/marc_order_accounts.pl b/admin/marc_order_accounts.pl index c4461b8df44..dc97ab2f6bb 100755 --- a/admin/marc_order_accounts.pl +++ b/admin/marc_order_accounts.pl @@ -36,9 +36,9 @@ use Koha::MarcOrder; use Koha::Acquisition::Booksellers; use Koha::MarcOrderAccount; use Koha::MarcOrderAccounts; +use Koha::File::Transports; my $input = CGI->new; -our $schema = Koha::Database->new()->schema(); my ( $template, $loggedinuser, $cookie ) = get_template_and_user( { @@ -62,10 +62,10 @@ if ( $op eq 'acct_form' ) { $template->param( available_matchers => \@matchers ); # Get available file transports for selection - my @file_transports = $schema->resultset('FileTransport')->search( + my @file_transports = Koha::File::Transports->search( {}, { order_by => { -asc => 'name' } } - ); + )->as_list; $template->param( file_transports => \@file_transports ); show_account( $input, $template ); diff --git a/misc/cronjobs/marc_ordering_process.pl b/misc/cronjobs/marc_ordering_process.pl index b61f89bc0e0..27327c21c44 100755 --- a/misc/cronjobs/marc_ordering_process.pl +++ b/misc/cronjobs/marc_ordering_process.pl @@ -70,7 +70,6 @@ use File::Copy qw( copy move ); use Koha::Script -cron; use Koha::MarcOrder; use Koha::MarcOrderAccounts; -use Koha::File::Transports; use C4::Log qw( cronlogaction ); @@ -112,7 +111,7 @@ foreach my $acct (@accounts) { my $working_dir = $acct->download_directory; - my $file_transport = $acct->file_transport_id ? Koha::File::Transports->find( $acct->file_transport_id ) : undef; + my $file_transport = $acct->file_transport; if ($file_transport) { if ( $file_transport->connect() ) { my $download_dir = $file_transport->download_directory; diff --git a/t/db_dependent/Koha/MarcOrderAccount.t b/t/db_dependent/Koha/MarcOrderAccount.t new file mode 100644 index 00000000000..08b7bb5a728 --- /dev/null +++ b/t/db_dependent/Koha/MarcOrderAccount.t @@ -0,0 +1,263 @@ +#!/usr/bin/perl + +# This file is part of Koha. +# +# Copyright 2025 Koha Development team +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; + +use Test::More tests => 5; +use Test::NoWarnings; + +use t::lib::TestBuilder; + +use Koha::Database; +use Koha::MarcOrderAccounts; + +my $schema = Koha::Database->new->schema; +my $builder = t::lib::TestBuilder->new; + +subtest 'vendor() relationship' => sub { + plan tests => 2; + + $schema->storage->txn_begin; + + my $vendor = $builder->build_object( { class => 'Koha::Acquisition::Booksellers' } ); + my $account = $builder->build_object( + { + class => 'Koha::MarcOrderAccounts', + value => { vendor_id => $vendor->id } + } + ); + + is( + ref( $account->vendor ), + 'Koha::Acquisition::Bookseller', + '->vendor should return a Koha::Acquisition::Bookseller object' + ); + + is( + $account->vendor->id, + $vendor->id, + 'Vendor relationship returns the correct vendor' + ); + + $schema->storage->txn_rollback; +}; + +subtest 'budget() relationship' => sub { + plan tests => 2; + + $schema->storage->txn_begin; + + my $budget = $builder->build_object( { class => 'Koha::Acquisition::Funds' } ); + my $account = $builder->build_object( + { + class => 'Koha::MarcOrderAccounts', + value => { budget_id => $budget->budget_id } + } + ); + + is( + ref( $account->budget ), + 'Koha::Acquisition::Fund', + '->budget should return a Koha::Acquisition::Fund object' + ); + + is( + $account->budget->budget_id, + $budget->budget_id, + 'Budget relationship returns the correct budget' + ); + + $schema->storage->txn_rollback; +}; + +subtest 'file_transport() relationship' => sub { + plan tests => 8; + + $schema->storage->txn_begin; + + # Test with no file transport (NULL foreign key) + my $account_no_transport = $builder->build_object( + { + class => 'Koha::MarcOrderAccounts', + value => { file_transport_id => undef } + } + ); + + is( + $account_no_transport->file_transport, + undef, + '->file_transport should return undef when no transport is configured' + ); + + # Test with SFTP transport + my $sftp_transport = $builder->build_object( + { + class => 'Koha::File::Transports', + value => { + transport => 'sftp', + name => 'Test SFTP', + host => 'test.example.com', + } + } + ); + + my $account_sftp = $builder->build_object( + { + class => 'Koha::MarcOrderAccounts', + value => { file_transport_id => $sftp_transport->id } + } + ); + + my $transport = $account_sftp->file_transport; + + is( + ref($transport), + 'Koha::File::Transport::SFTP', + '->file_transport should return polymorphic SFTP object for SFTP transport' + ); + + is( + $transport->id, + $sftp_transport->id, + 'Transport relationship returns the correct transport ID' + ); + + can_ok( $transport, 'connect' ); + + # Test with FTP transport + my $ftp_transport = $builder->build_object( + { + class => 'Koha::File::Transports', + value => { + transport => 'ftp', + name => 'Test FTP', + host => 'ftp.example.com', + } + } + ); + + my $account_ftp = $builder->build_object( + { + class => 'Koha::MarcOrderAccounts', + value => { file_transport_id => $ftp_transport->id } + } + ); + + my $ftp = $account_ftp->file_transport; + + is( + ref($ftp), + 'Koha::File::Transport::FTP', + '->file_transport should return polymorphic FTP object for FTP transport' + ); + + # Test with Local transport + my $local_transport = $builder->build_object( + { + class => 'Koha::File::Transports', + value => { + transport => 'local', + name => 'Test Local', + host => 'localhost', + } + } + ); + + my $account_local = $builder->build_object( + { + class => 'Koha::MarcOrderAccounts', + value => { file_transport_id => $local_transport->id } + } + ); + + my $local = $account_local->file_transport; + + is( + ref($local), + 'Koha::File::Transport::Local', + '->file_transport should return polymorphic Local object for local transport' + ); + + can_ok( $local, 'download_file' ); + can_ok( $local, 'delete_file' ); + + $schema->storage->txn_rollback; +}; + +subtest 'file_transport() respects prefetched relationships' => sub { + plan tests => 4; + + $schema->storage->txn_begin; + + # Create a transport and account + my $sftp_transport = $builder->build_object( + { + class => 'Koha::File::Transports', + value => { + transport => 'sftp', + name => 'Test SFTP Prefetch', + host => 'prefetch.example.com', + } + } + ); + + my $account = $builder->build_object( + { + class => 'Koha::MarcOrderAccounts', + value => { file_transport_id => $sftp_transport->id } + } + ); + + # Search with prefetch to load the relationship + my $account_with_prefetch = Koha::MarcOrderAccounts->search( + { id => $account->id }, + { prefetch => 'file_transport' } + )->next; + + # Enable query logging to count queries + my @queries; + my $original_debug = $schema->storage->debug; + $schema->storage->debugcb( + sub { + my ( $op, $info ) = @_; + push @queries, $info if $op eq 'SELECT'; + } + ); + $schema->storage->debug(1); + + # Access the prefetched relationship + my $transport = $account_with_prefetch->file_transport; + + # Disable query logging + $schema->storage->debug($original_debug); + + is( scalar @queries, 0, 'No additional queries when accessing prefetched file_transport' ); + + is( + ref($transport), + 'Koha::File::Transport::SFTP', + 'Prefetched transport returns correct polymorphic class' + ); + + is( $transport->id, $sftp_transport->id, 'Prefetched transport has correct ID' ); + + is( $transport->name, 'Test SFTP Prefetch', 'Prefetched transport has correct data' ); + + $schema->storage->txn_rollback; +}; -- 2.51.0