From 9e682d7b20606e6c6a3386d4965c0fbd516cc759 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Wed, 25 Feb 2026 09:33:05 +0000 Subject: [PATCH] Bug 10190: rewrite atomicupdate with correct 1:1 rule migration The previous migration had two fundamental errors: 1. It manufactured phantom empty rows for all patron categories that had no rule, which corrupted the "most frequent" calculation. 2. It created categorycode=NULL (all-categories) default rules for every branchcode seen, silently extending overdue notice coverage to patron categories that had no configured rule in the old system. The backend (overdue_notices.pl) does not consult has_rules, so these invented defaults would cause notices to be sent to patrons that were deliberately uncovered. The rewrite migrates only explicit (branchcode, categorycode) pairs that had a positive delay configured. No default rules are created. branchcode='' maps to NULL as before. exit is replaced with return to avoid killing subsequent atomicupdate scripts. --- .../data/mysql/atomicupdate/bug_10190.pl | 307 +++--------------- 1 file changed, 46 insertions(+), 261 deletions(-) diff --git a/installer/data/mysql/atomicupdate/bug_10190.pl b/installer/data/mysql/atomicupdate/bug_10190.pl index cdce92695d6..39e0cf5d8c8 100755 --- a/installer/data/mysql/atomicupdate/bug_10190.pl +++ b/installer/data/mysql/atomicupdate/bug_10190.pl @@ -8,9 +8,11 @@ return { my ($args) = @_; my ( $dbh, $out ) = @$args{qw(dbh out)}; - # This script first pulls data from these tables, then deletes them. -> Do not run if they do not exist. + # This script pulls data from these tables then drops them. + # Skip silently if they no longer exist (e.g. fresh install or already migrated). if ( !TableExists('overduerules_transport_types') || !TableExists('overduerules') ) { - exit; + say_info( $out, "Tables overduerules / overduerules_transport_types not found, skipping migration" ); + return; } # Remove the tools sub-permission for notice triggers; access is now @@ -19,49 +21,38 @@ return { $dbh->do(q|DELETE FROM permissions WHERE code = 'edit_notice_status_triggers'|); say_success( $out, "Removed deprecated permission 'edit_notice_status_triggers'" ); - # Populate empty overduerules table for missing categories - $dbh->do( - q| - INSERT IGNORE INTO overduerules (branchcode, categorycode) - SELECT '', categorycode - FROM categories - WHERE categorycode NOT IN (SELECT categorycode FROM overduerules); - | - ); - - # Fetch all overdue rules + # Fetch all explicit overdue rules with their transport types. + # No phantom rows are added - only rules that were explicitly configured are migrated. my $rules = $dbh->selectall_arrayref( q| SELECT - o.overduerules_id, o.branchcode, o.categorycode, o.delay1, o.letter1, o.debarred1, o.delay2, - o.debarred2, o.letter2, + o.debarred2, o.delay3, o.letter3, o.debarred3, GROUP_CONCAT( CASE WHEN ott.letternumber = 1 THEN ott.message_transport_type END ORDER BY ott.message_transport_type ASC - ) AS message_transport_type_1, + ) AS mtt_1, GROUP_CONCAT( CASE WHEN ott.letternumber = 2 THEN ott.message_transport_type END ORDER BY ott.message_transport_type ASC - ) AS message_transport_type_2, + ) AS mtt_2, GROUP_CONCAT( CASE WHEN ott.letternumber = 3 THEN ott.message_transport_type END ORDER BY ott.message_transport_type ASC - ) AS message_transport_type_3 + ) AS mtt_3 FROM overduerules o LEFT JOIN - overduerules_transport_types ott - ON o.overduerules_id = ott.overduerules_id + overduerules_transport_types ott ON o.overduerules_id = ott.overduerules_id GROUP BY o.overduerules_id, o.branchcode, @@ -70,8 +61,8 @@ return { o.letter1, o.debarred1, o.delay2, - o.debarred2, o.letter2, + o.debarred2, o.delay3, o.letter3, o.debarred3 @@ -79,266 +70,60 @@ return { { Slice => {} } ); - # Initialize hashes to store the frequency of delays, notices, and mtt per group (1, 2, 3) - my %branchcodes; - my %delay_count; - my %notice_count; - my %mtt_count; - my %restrict_count; - - # Populate the hashes with the frequency of each value by group - for my $rule ( @{$rules} ) { - my $branchcode = $rule->{'branchcode'}; - $branchcodes{$branchcode} = 1; - - foreach my $i ( 1 .. 3 ) { - my $delay = $rule->{"delay$i"} // ''; - my $notice = $rule->{"letter$i"} // ''; - my $mtt = $rule->{"message_transport_type_$i"} // ''; - my $restrict = $rule->{"debarred$i"} // ''; - - # Increment counts only if values are defined - $delay_count{$branchcode}{$i}{$delay}++ if defined $delay; - $notice_count{$branchcode}{$i}{$notice}++ if defined $notice; - $mtt_count{$branchcode}{$i}{$mtt}++ if defined $mtt; - $restrict_count{$branchcode}{$i}{$restrict}++ if defined $restrict; - } - } - - # Find the most frequent delay, notice, and mtt for each branchcode and group - my ( %most_frequent_delay, %most_frequent_notice, %most_frequent_mtt, %most_frequent_restrict ); - for my $branchcode ( keys %branchcodes ) { - foreach my $i ( 1 .. 3 ) { - - # Find the most frequent delay in group $i - my $max_delay_count = 0; - for my $delay ( keys %{ $delay_count{$branchcode}{$i} } ) { - if ( $delay_count{$branchcode}{$i}{$delay} > $max_delay_count ) { - $max_delay_count = $delay_count{$branchcode}{$i}{$delay}; - $most_frequent_delay{$branchcode}{$i} = $delay; - } - } - - # Find the most frequent notice in group $i - my $max_notice_count = 0; - for my $notice ( keys %{ $notice_count{$branchcode}{$i} } ) { - if ( $notice_count{$branchcode}{$i}{$notice} > $max_notice_count ) { - $max_notice_count = $notice_count{$branchcode}{$i}{$notice}; - $most_frequent_notice{$branchcode}{$i} = $notice; - } - } - - # Find the most frequent mtt in group $i - my $max_mtt_count = 0; - for my $mtt ( keys %{ $mtt_count{$branchcode}{$i} } ) { - if ( $mtt_count{$branchcode}{$i}{$mtt} > $max_mtt_count ) { - $max_mtt_count = $mtt_count{$branchcode}{$i}{$mtt}; - $most_frequent_mtt{$branchcode}{$i} = $mtt; - } - } - - # Find the most frequent mtt in group $i - my $max_restrict_count = 0; - for my $restrict ( keys %{ $restrict_count{$branchcode}{$i} } ) { - if ( $restrict_count{$branchcode}{$i}{$restrict} > $max_restrict_count ) { - $max_restrict_count = $restrict_count{$branchcode}{$i}{$restrict}; - $most_frequent_restrict{$branchcode}{$i} = $restrict; - } - } - } - } - - # Migrate rules from overduerules to circulation_rules, skipping most frequent values as those will be our defaults my $insert = $dbh->prepare( "INSERT IGNORE INTO circulation_rules (branchcode, categorycode, itemtype, rule_name, rule_value) VALUES (?, ?, ?, ?, ?)" ); - my $itemtype = undef; + my $migrated = 0; + for my $rule ( @{$rules} ) { - my $branchcode = $rule->{'branchcode'} || undef; - my $categorycode = $rule->{'categorycode'} || undef; - my $branchcode_key = $branchcode // ''; + # In the old system branchcode='' is the global/default library. + # In circulation_rules this is represented as branchcode=NULL. + my $branchcode = $rule->{branchcode} || undef; + my $categorycode = $rule->{categorycode} || undef; + my $itemtype = undef; + + my $context_label = ( $branchcode // '*' ) . ':' . ( $categorycode // '*' ); + foreach my $i ( 1 .. 3 ) { + my $delay = $rule->{"delay$i"}; + my $notice = $rule->{"letter$i"}; + my $mtt = $rule->{"mtt_$i"}; + my $restrict = $rule->{"debarred$i"}; - my $has_rules = 0; + # A zero or null delay means this trigger was not configured in the old system. + # Only migrate explicitly configured triggers. + next unless defined $delay && $delay > 0; - # Insert the delay rule for group $i, skipping if it matches the most frequent delay - if ( my $delay = $rule->{"delay$i"} ) { - $delay ||= ''; - unless ( $delay eq $most_frequent_delay{$branchcode_key}{$i} ) { - $delay ||= undef; - say $out "Inserting " - . ( $branchcode // '' ) . ":" - . ( $categorycode // '' ) . ':' - . ( $itemtype // '' ) - . " overdue_$i" - . '_delay: ' - . ( $delay // '' ); - $insert->execute( - $branchcode, - $categorycode, - $itemtype, - "overdue_$i" . '_delay', - $delay - ); - $has_rules = 1; - } - } + say $out "Migrating trigger $i for $context_label (delay=$delay)"; - # Insert the notice rule for group $i, skipping if it matches the most frequent notice - if ( my $notice = $rule->{"letter$i"} ) { - unless ( $notice eq $most_frequent_notice{$branchcode_key}{$i} ) { - say $out "Inserting " - . ( $branchcode // '' ) . ":" - . ( $categorycode // '' ) . ':' - . ( $itemtype // '' ) - . " overdue_$i" - . '_notice: ' - . ( $notice // '' ); - $insert->execute( - $branchcode, - $categorycode, - $itemtype, - "overdue_$i" . '_notice', - $notice - ); - $has_rules = 1; - } - } + $insert->execute( $branchcode, $categorycode, $itemtype, "overdue_${i}_delay", $delay ); - # Insert the message transport type rule for group $i, skipping if it matches the most frequent mtt - if ( my $mtt = $rule->{"message_transport_type_$i"} ) { - unless ( $mtt eq $most_frequent_mtt{$branchcode_key}{$i} ) { - say $out "Inserting " - . ( $branchcode // '' ) . ":" - . ( $categorycode // '' ) . ':' - . ( $itemtype // '' ) - . " overdue_$i" - . '_mtt: ' - . ( $mtt // '' ); - $insert->execute( - $branchcode, - $categorycode, - $itemtype, - "overdue_$i" . '_mtt', - $mtt - ); - $has_rules = 1; - } + if ($notice) { + $insert->execute( $branchcode, $categorycode, $itemtype, "overdue_${i}_notice", $notice ); } - # Insert the restrict rule for group $i - if ( my $restrict = $rule->{"debarred$i"} ) { - unless ( $restrict eq $most_frequent_restrict{$branchcode_key}{$i} ) { - say $out "Inserting " - . ( $branchcode // '' ) . ":" - . ( $categorycode // '' ) . ':' - . ( $itemtype // '' ) - . " overdue_$i" - . '_restrict: ' - . ( $restrict // '' ); - $insert->execute( - $branchcode, - $categorycode, - $itemtype, - "overdue_$i" . '_restrict', - $restrict - ); - $has_rules = 1; - } + if ($mtt) { + $insert->execute( $branchcode, $categorycode, $itemtype, "overdue_${i}_mtt", $mtt ); } - # Insert the has_rules rule for group $i if and only if a rule set has been added into the db for this context. - if ( $has_rules == 1 ) { - say $out "Inserting " - . ( $branchcode // '' ) . ":" - . ( $categorycode // '' ) . ':' - . ( $itemtype // '' ) - . " overdue_$i" - . "has_rules: 1"; - $insert->execute( - $branchcode, - $categorycode, - $itemtype, - "overdue_$i" . '_has_rules', - 1 - ); + # Only insert restrict when explicitly enabled (=1); absence defaults to no restriction. + if ($restrict) { + $insert->execute( $branchcode, $categorycode, $itemtype, "overdue_${i}_restrict", $restrict ); } - } - } - - # Insert the default rules for each group - for my $branchcode ( keys %branchcodes ) { - my $branchcode_value = $branchcode || undef; - foreach my $i ( 1 .. 3 ) { - my $most_frequent_delay = $most_frequent_delay{$branchcode}{$i}; - say $out "Inserting " - . ( $branchcode_value // '' ) . ":all:" - . ( $itemtype // '' ) - . " default most frequent delay for overdue_$i: " - . $most_frequent_delay; - $insert->execute( - $branchcode_value, - undef, - $itemtype, - "overdue_${i}_delay", - $most_frequent_delay - ); - - my $most_frequent_notice = $most_frequent_notice{$branchcode}{$i}; - say $out "Inserting " - . ( $branchcode_value // '' ) - . ":all:all default most frequent notice for overdue_$i: " - . $most_frequent_notice; - $insert->execute( - $branchcode_value, - undef, - undef, - "overdue_${i}_notice", - $most_frequent_notice - ); - - my $most_frequent_mtt = $most_frequent_mtt{$branchcode}{$i}; - say $out "Inserting " - . ( $branchcode_value // '' ) - . ":all:all default most frequent mtt for overdue_$i: " - . $most_frequent_mtt; - $insert->execute( - $branchcode_value, - undef, - undef, - "overdue_${i}_mtt", - $most_frequent_mtt - ); - my $most_frequent_restrict = $most_frequent_restrict{$branchcode}{$i}; - say $out "Inserting " - . ( $branchcode_value // '' ) - . ":all:all default most frequent restrict for overdue_$i: " - . $most_frequent_restrict; - $insert->execute( - $branchcode_value, - undef, - undef, - "overdue_${i}_restrict", - $most_frequent_restrict - ); - - # Insert the has_rules rule for group $i's default context - say $out "Inserting " . ( $branchcode_value // '' ) . ":all:all default has_rules for overdue_$i: 1"; - $insert->execute( - $branchcode_value, - undef, - undef, - "overdue_${i}_has_rules", - 1 - ); + # has_rules is a UI sentinel that marks an explicit ruleset for this context/trigger. + # It is not used by the backend (overdue_notices.pl). + $insert->execute( $branchcode, $categorycode, $itemtype, "overdue_${i}_has_rules", 1 ); + $migrated++; } } - $dbh->do(q|DROP TABLE overduerules_transport_types|) if TableExists('overduerules_transport_types'); - $dbh->do(q|DROP TABLE overduerules|) if TableExists('overduerules'); + say_success( $out, "Migrated $migrated trigger(s) from overduerules to circulation_rules" ); + + $dbh->do(q|DROP TABLE overduerules_transport_types|); + $dbh->do(q|DROP TABLE overduerules|); } }; -- 2.53.0