From 7ec717527e5b82046d9dea68ce6889d9c2e6d100 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Thu, 7 Aug 2025 15:39:51 +0100 Subject: [PATCH] Bug 20253: (follow-up) Block auto_orders and UI ordering for duplicate PO numbers When po_is_basketname is enabled for a vendor, duplicate purchase order numbers should prevent both automatic ordering (auto_orders) and manual UI-triggered ordering to avoid conflicts with existing baskets. This patch implements blocking functionality that: 1. Auto-orders blocking: - Tracks baskets with PO number conflicts during quote processing - Skips auto-ordering for baskets with duplicate PO numbers - Logs clear warnings about blocked baskets 2. UI basket ordering blocking: - Adds duplicate checking in create_edi_order function - Returns detailed error information when conflicts detected - Prevents EDI order creation for conflicting baskets 3. User-friendly error handling: - Updates basket.pl to handle duplicate PO number errors - Stays on basket page to display error messages - Provides clear messaging about conflicts and resolution steps 4. Template messaging: - Adds clear error message explaining duplicate PO conflicts - Shows which existing basket uses the same PO number - Guides users on next steps to resolve the issue This ensures data integrity and prevents order processing conflicts while providing clear feedback to users about duplicate PO number issues. Test plan: 1. Set up a vendor with EDI account and enable "po_is_basketname" 2. Create a basket with a specific name (e.g., "PO12345") 3. Process an EDIFACT quote that contains the same PO number: - If auto_orders is enabled: verify the new basket is created but NOT automatically ordered - Check the logs show "Auto-order blocked" message 4. For UI testing, create another basket manually with the same name 5. Try to generate EDI order via "Generate EDI order" button 6. Verify: Error message appears stating duplicate PO number conflict 7. Verify: Basket remains open (not closed) 8. Verify: Error message shows which existing basket uses the PO number 9. Test with po_is_basketname disabled: verify normal operation continues 10. Test with unique PO numbers: verify orders process normally --- Koha/EDI.pm | 57 ++++- acqui/basket.pl | 14 +- .../prog/en/modules/acqui/basket.tt | 5 + t/db_dependent/Koha/EDI.t | 235 +++++++++++++++++- 4 files changed, 306 insertions(+), 5 deletions(-) diff --git a/Koha/EDI.pm b/Koha/EDI.pm index 60bef57ba08..f3789367d23 100644 --- a/Koha/EDI.pm +++ b/Koha/EDI.pm @@ -95,6 +95,32 @@ sub create_edi_order { } )->single; + # Check for duplicate PO numbers when po_is_basketname is enabled + if ( $vendor && $vendor->po_is_basketname ) { + my $basket = $orderlines[0]->basketno; + if ( $basket->basketname ) { + my $existing_basket = $schema->resultset('Aqbasket')->search( + { + basketname => $basket->basketname, + booksellerid => $basket->booksellerid, + basketno => { '!=' => $basketno }, # Exclude current basket + } + )->first; + + if ($existing_basket) { + $logger->error( "EDI order blocked for basket $basketno due to duplicate PO number '" + . $basket->basketname + . "' (existing basket: " + . $existing_basket->basketno + . ")" ); + return { + error => "duplicate_po_number", existing_basket => $existing_basket->basketno, + po_number => $basket->basketname + }; + } + } + } + my $ean_search_keys = { ean => $ean, }; if ($branchcode) { $ean_search_keys->{branchcode} = $branchcode; @@ -647,7 +673,8 @@ sub process_quote { my $schema = Koha::Database->new()->schema(); my $message_count = 0; my $split_message; - my @added_baskets; # if auto & multiple baskets need to order all + my @added_baskets; # if auto & multiple baskets need to order all + my %baskets_with_po_conflicts; # track baskets that have PO number conflicts # Get vendor EDI account configuration my $v = $schema->resultset('VendorEdiAccount')->search( @@ -663,13 +690,16 @@ sub process_quote { # Determine basket name based on vendor configuration my $basket_name = $quote_message->filename; + my $purchase_order_number; + my $existing_basket; + if ( $v && $v->po_is_basketname ) { - my $purchase_order_number = $msg->purchase_order_number; + $purchase_order_number = $msg->purchase_order_number; if ($purchase_order_number) { $basket_name = $purchase_order_number; # Check for duplicate purchase order numbers for this vendor (including closed baskets) - my $existing_basket = $schema->resultset('Aqbasket')->search( + $existing_basket = $schema->resultset('Aqbasket')->search( { basketname => $basket_name, booksellerid => $quote_message->vendor_id, @@ -701,6 +731,14 @@ sub process_quote { q{} . q{} ); push @added_baskets, $basketno; + + # Record if this basket has a PO conflict (for auto_orders blocking) + if ($existing_basket) { + $baskets_with_po_conflicts{$basketno} = { + po_number => $purchase_order_number, + existing_basket => $existing_basket->basketno + }; + } if ( $message_count > 1 ) { my $m_filename = $quote_message->filename; $m_filename .= "_$message_count"; @@ -741,6 +779,19 @@ sub process_quote { # Do we automatically generate orders for this vendor if ( $v && $v->auto_orders ) { for my $b (@added_baskets) { + + # Check if this basket has a PO number conflict + if ( exists $baskets_with_po_conflicts{$b} ) { + my $conflict_info = $baskets_with_po_conflicts{$b}; + $logger->warn( "Auto-order blocked for basket $b due to duplicate PO number '" + . $conflict_info->{po_number} + . "' (existing basket: " + . $conflict_info->{existing_basket} + . ")" ); + $logger->info("Basket $b remains open due to PO number conflict"); + next; + } + create_edi_order( { ean => $messages->[0]->buyer_ean, diff --git a/acqui/basket.pl b/acqui/basket.pl index 57621b4e7ac..30cb08862ce 100755 --- a/acqui/basket.pl +++ b/acqui/basket.pl @@ -578,7 +578,19 @@ sub edi_close_and_order { if ( $basket->{branch} ) { $edi_params->{branchcode} = $basket->{branch}; } - if ( create_edi_order($edi_params) ) { + my $edi_result = create_edi_order($edi_params); + if ( ref $edi_result eq 'HASH' && $edi_result->{error} ) { + if ( $edi_result->{error} eq 'duplicate_po_number' ) { + push @messages, { + type => 'error', + code => 'edi_duplicate_po_number', + po_number => $edi_result->{po_number}, + existing_basket => $edi_result->{existing_basket} + }; + $op = 'list'; # Stay on basket page to show error + return; + } + } elsif ($edi_result) { #$template->param( edifile => 1 ); } diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt index 2176e799594..f50b69b499d 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt @@ -306,6 +306,11 @@ There is no notice template with code ACQORDER defined. [% CASE 'email_sent' %] Order e-mail was sent to the vendor. + [% CASE 'edi_duplicate_po_number' %] + EDI order blocked: Duplicate purchase order number '[% m.po_number | html %]' found. This purchase order number is already used by basket [% m.existing_basket | html %]. Please check with your vendor or + use a different purchase order number. [% CASE %] ERROR! - [% m.code | html %] [% END %] diff --git a/t/db_dependent/Koha/EDI.t b/t/db_dependent/Koha/EDI.t index 5b4912bf29f..cb3f1ee6c54 100755 --- a/t/db_dependent/Koha/EDI.t +++ b/t/db_dependent/Koha/EDI.t @@ -21,7 +21,7 @@ use Modern::Perl; use FindBin qw( $Bin ); use Test::NoWarnings; -use Test::More tests => 5; +use Test::More tests => 6; use Test::MockModule; use t::lib::Mocks; @@ -1067,6 +1067,239 @@ subtest 'process_invoice_without_tax_rate' => sub { $schema->storage->txn_rollback; }; +subtest 'duplicate_po_number_blocking' => sub { + plan tests => 2; + + $schema->storage->txn_begin; + + my $test_san = '5013546098818'; + my $dirname = ( $Bin =~ /^(.*\/t\/)/ ? $1 . 'edi_testfiles/' : q{} ); + my $active_period = $builder->build( + { + source => 'Aqbudgetperiod', + value => { budget_period_active => 1 } + } + ); + + # Test 1: Auto-orders blocking for duplicate PO numbers + subtest 'auto_orders_blocking' => sub { + plan tests => 9; + + $schema->storage->txn_begin; + + # Create vendor EDI account with auto_orders and po_is_basketname enabled + my $account = $builder->build( + { + source => 'VendorEdiAccount', + value => { + description => 'auto_orders duplicate test vendor', + transport => 'FILE', + plugin => '', + san => $test_san, + auto_orders => 1, + po_is_basketname => 1, + } + } + ); + + my $ean = $builder->build( + { + source => 'EdifactEan', + value => { + description => 'test ean', + branchcode => undef, + ean => $test_san + } + } + ); + + # Create existing basket with PO number that will conflict + my $existing_basket = $builder->build( + { + source => 'Aqbasket', + value => { + basketname => 'orders 23/1', # Same as in QUOTES_SMALL.CEQ + booksellerid => $account->{vendor_id}, + closedate => undef, + } + } + ); + + # Setup the fund + $builder->build( + { + source => 'Aqbudget', + value => { + budget_code => 'REF', + budget_period_id => $active_period->{budget_period_id} + } + } + ); + + my $filename = 'QUOTES_SMALL.CEQ'; + my $trans = Koha::Edifact::Transport->new( $account->{id} ); + $trans->working_directory($dirname); + + my $mhash = $trans->message_hash(); + $mhash->{message_type} = 'QUOTE'; + $trans->ingest( $mhash, $filename ); + + my $quote = $schema->resultset('EdifactMessage')->find( { filename => $filename } ); + + # Clear logger before processing + $logger->clear(); + + # Process quote - should create basket but NOT auto-order due to duplicate PO + my $die; + eval { + process_quote($quote); + 1; + } or do { + $die = $@; + }; + ok( !$die, 'Quote with duplicate PO processed without dying' ); + + # Verify duplicate PO error was logged during quote processing + my $errors = $quote->edifact_errors; + ok( $errors->count >= 1, 'Error logged for duplicate PO number' ); + + my $duplicate_error = $errors->search( { section => 'RFF+ON:orders 23/1' } )->first; + ok( $duplicate_error, 'Duplicate PO error found in quote errors' ); + + # Check that new basket was created + my $baskets = Koha::Acquisition::Baskets->search( + { + booksellerid => $account->{vendor_id}, + basketno => { '!=' => $existing_basket->{basketno} } + } + ); + is( $baskets->count, 1, "New basket created despite duplicate PO" ); + + my $new_basket = $baskets->next; + is( $new_basket->basketname, 'orders 23/1', "New basket uses duplicate PO number as name" ); + + # Critical test: Verify basket was NOT closed (auto-order was blocked) + ok( !$new_basket->closedate, 'New basket with duplicate PO was NOT closed (auto-order blocked)' ); + + # Verify no EDI order was created for the conflicting basket + my $edi_orders = $schema->resultset('EdifactMessage')->search( + { + message_type => 'ORDERS', + basketno => $new_basket->basketno, + } + ); + is( $edi_orders->count, 0, 'No EDI order created for basket with duplicate PO' ); + + # Check that the blocking is working by verifying no EDI orders were created + # This is the most important test - the basket should NOT be auto-ordered + ok( $edi_orders->count == 0, 'Auto-order was blocked - no EDI order created for duplicate PO' ); + + # Verify that duplicate PO error was logged (this shows detection is working) + ok( $duplicate_error, 'Duplicate PO number detection and logging is working' ); + + $schema->storage->txn_rollback; + }; + + # Test 2: No blocking when po_is_basketname is disabled + subtest 'no_blocking_when_feature_disabled' => sub { + plan tests => 5; + + $schema->storage->txn_begin; + + # Create vendor EDI account with po_is_basketname DISABLED + my $account = $builder->build( + { + source => 'VendorEdiAccount', + value => { + description => 'feature disabled test vendor', + transport => 'FILE', + plugin => '', + san => $test_san, + auto_orders => 1, + po_is_basketname => 0, # Feature disabled + } + } + ); + + my $ean = $builder->build( + { + source => 'EdifactEan', + value => { + description => 'test ean', + branchcode => undef, + ean => $test_san + } + } + ); + + # Create existing basket with any name + my $existing_basket = $builder->build( + { + source => 'Aqbasket', + value => { + basketname => 'orders 23/1', + booksellerid => $account->{vendor_id}, + closedate => undef, + } + } + ); + + # Setup the fund + $builder->build( + { + source => 'Aqbudget', + value => { + budget_code => 'REF', + budget_period_id => $active_period->{budget_period_id} + } + } + ); + + my $filename = 'QUOTES_SMALL.CEQ'; + my $trans = Koha::Edifact::Transport->new( $account->{id} ); + $trans->working_directory($dirname); + + my $mhash = $trans->message_hash(); + $mhash->{message_type} = 'QUOTE'; + $trans->ingest( $mhash, $filename ); + + my $quote = $schema->resultset('EdifactMessage')->find( { filename => $filename } ); + + $logger->clear(); + process_quote($quote); + + # Verify new basket was created and auto-ordered (no blocking) + my $baskets = Koha::Acquisition::Baskets->search( + { + booksellerid => $account->{vendor_id}, + basketno => { '!=' => $existing_basket->{basketno} } + } + ); + is( $baskets->count, 1, "New basket created" ); + + my $new_basket = $baskets->next; + + # When po_is_basketname is disabled, basket uses filename not PO number + is( $new_basket->basketname, $filename, "Basket uses filename when po_is_basketname disabled" ); + + # The key test is that normal processing occurred without duplicate detection + # Since po_is_basketname is disabled, basket should use filename not PO number + ok( $new_basket->basketname eq $filename, 'Normal basket naming when feature disabled' ); + + # With po_is_basketname disabled, quote processing should work normally + # The absence of duplicate errors in the database is the key indicator + my $quote_errors = $schema->resultset('EdifactError')->search( {} ); + my $has_duplicate_db_error = grep { $_->details =~ /Duplicate purchase order/ } $quote_errors->all; + ok( !$has_duplicate_db_error, 'No duplicate PO database errors when feature disabled' ); + + pass('Feature correctly disabled - normal processing occurred'); + + $schema->storage->txn_rollback; + }; + + $schema->storage->txn_rollback; +}; + subtest 'create_edi_order_logging' => sub { plan tests => 8; -- 2.50.1