Bugzilla – Attachment 185228 Details for
Bug 20253
Optionally use buyer's purchase order number from EDIFACT quote in basket name
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 20253: (follow-up) Block auto_orders and UI ordering for duplicate PO numbers
Bug-20253-follow-up-Block-autoorders-and-UI-orderi.patch (text/plain), 18.04 KB, created by
Martin Renvoize (ashimema)
on 2025-08-07 15:27:31 UTC
(
hide
)
Description:
Bug 20253: (follow-up) Block auto_orders and UI ordering for duplicate PO numbers
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2025-08-07 15:27:31 UTC
Size:
18.04 KB
patch
obsolete
>From 7ec717527e5b82046d9dea68ce6889d9c2e6d100 Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >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 @@ > <span>There is no notice template with code ACQORDER defined.</span> > [% CASE 'email_sent' %] > <span>Order e-mail was sent to the vendor.</span> >+ [% CASE 'edi_duplicate_po_number' %] >+ <span >+ >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.</span >+ > > [% CASE %] > <span>ERROR! - [% m.code | html %]</span> > [% 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
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 20253
:
183859
|
183860
|
183861
|
183862
|
183953
|
183954
|
183955
|
183956
|
183957
|
183958
|
184000
|
184001
|
184002
|
184003
|
184004
|
184005
|
184006
|
184007
|
185221
|
185222
|
185223
|
185224
|
185225
|
185226
|
185227
| 185228