From b4531d64f0b7c1a6289767d1065e83ec209c5306 Mon Sep 17 00:00:00 2001 From: sunnavy Date: Thu, 22 Jul 2021 05:30:39 +0800 Subject: [PATCH 01/11] Fix typo of "nonexistent" --- lib/RT/Lifecycle.pm | 22 +++++++++++----------- lib/RT/Record.pm | 2 +- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/lib/RT/Lifecycle.pm b/lib/RT/Lifecycle.pm index 29382ef2a22..f872404c008 100644 --- a/lib/RT/Lifecycle.pm +++ b/lib/RT/Lifecycle.pm @@ -995,7 +995,7 @@ sub ValidateLifecycle { for my $state ( keys %{ $lifecycle->{defaults} || {} } ) { my $status = $lifecycle->{defaults}{$state}; if ( $status ) { - push @warnings, $current_user->loc( "Nonexistant default [_1] status [_2] in [_3] lifecycle", $state, lc $status, $name ) + push @warnings, $current_user->loc( "Nonexistent default [_1] status [_2] in [_3] lifecycle", $state, lc $status, $name ) unless $lifecycle->{canonical_case}{ lc $status }; } else { @@ -1003,11 +1003,11 @@ sub ValidateLifecycle { } } for my $from ( keys %{ $lifecycle->{transitions} || {} } ) { - push @warnings, $current_user->loc( "Nonexistant status [_1] in transitions in [_2] lifecycle", lc $from, $name ) + push @warnings, $current_user->loc( "Nonexistent status [_1] in transitions in [_2] lifecycle", lc $from, $name ) unless $from eq '' || $lifecycle->{canonical_case}{ lc $from }; for my $status ( @{ ( $lifecycle->{transitions}{$from} ) || [] } ) { - push @warnings, $current_user->loc( "Nonexistant status [_1] in transitions in [_2] lifecycle", lc $status, $name ) + push @warnings, $current_user->loc( "Nonexistent status [_1] in transitions in [_2] lifecycle", lc $status, $name ) unless $lifecycle->{canonical_case}{ lc $status }; } } @@ -1018,10 +1018,10 @@ sub ValidateLifecycle { push @warnings, $current_user->loc( "Invalid right transition [_1] in [_2] lifecycle", $schema, $name ); next; } - push @warnings, $current_user->loc( "Nonexistant status [_1] in right transition in [_2] lifecycle", lc $from, $name ) + push @warnings, $current_user->loc( "Nonexistent status [_1] in right transition in [_2] lifecycle", lc $from, $name ) unless $from eq '*' or $lifecycle->{canonical_case}{ lc $from }; - push @warnings, $current_user->loc( "Nonexistant status [_1] in right transition in [_2] lifecycle", lc $to, $name ) + push @warnings, $current_user->loc( "Nonexistent status [_1] in right transition in [_2] lifecycle", lc $to, $name ) unless $to eq '*' || $lifecycle->{canonical_case}{ lc $to }; push @warnings, @@ -1050,10 +1050,10 @@ sub ValidateLifecycle { push @warnings, $current_user->loc( "Invalid action status change [_1], in [_2] lifecycle", $transition, $name ); next; } - push @warnings, $current_user->loc( "Nonexistant status [_1] in actions in [_2] lifecycle", lc $from, $name ) + push @warnings, $current_user->loc( "Nonexistent status [_1] in actions in [_2] lifecycle", lc $from, $name ) unless $from eq '*' or $lifecycle->{canonical_case}{ lc $from }; - push @warnings, $current_user->loc( "Nonexistant status [_1] in actions in [_2] lifecycle", lc $to, $name ) + push @warnings, $current_user->loc( "Nonexistent status [_1] in actions in [_2] lifecycle", lc $to, $name ) unless $to eq '*' or $lifecycle->{canonical_case}{ lc $to }; } @@ -1084,9 +1084,9 @@ sub ValidateLifecycleMaps { push @warnings, $current_user->loc( "Invalid lifecycle mapping [_1]", $mapname ); next; } - push @warnings, $current_user->loc( "Nonexistant lifecycle [_1] in [_2] lifecycle map", $from, $mapname ) + push @warnings, $current_user->loc( "Nonexistent lifecycle [_1] in [_2] lifecycle map", $from, $mapname ) unless $LIFECYCLES_CACHE{$from}; - push @warnings, $current_user->loc( "Nonexistant lifecycle [_1] in [_2] lifecycle map", $to, $mapname ) + push @warnings, $current_user->loc( "Nonexistent lifecycle [_1] in [_2] lifecycle map", $to, $mapname ) unless $LIFECYCLES_CACHE{$to}; # Ignore mappings referring to disabled lifecycles @@ -1095,9 +1095,9 @@ sub ValidateLifecycleMaps { my $map = $LIFECYCLES_CACHE{'__maps__'}{$mapname}; for my $status ( keys %{$map} ) { - push @warnings, $current_user->loc( "Nonexistant status [_1] in [_2] in [_3] lifecycle map", lc $status, $from, $mapname ) + push @warnings, $current_user->loc( "Nonexistent status [_1] in [_2] in [_3] lifecycle map", lc $status, $from, $mapname ) if $LIFECYCLES_CACHE{$from} && !$LIFECYCLES_CACHE{$from}{canonical_case}{ lc $status }; - push @warnings, $current_user->loc( "Nonexistant status [_1] in [_2] in [_3] lifecycle map", lc $map->{$status}, $to, $mapname ) + push @warnings, $current_user->loc( "Nonexistent status [_1] in [_2] in [_3] lifecycle map", lc $map->{$status}, $to, $mapname ) if $LIFECYCLES_CACHE{$to} && !$LIFECYCLES_CACHE{$to}{canonical_case}{ lc $map->{$status} }; } } diff --git a/lib/RT/Record.pm b/lib/RT/Record.pm index 3a87daf590b..96729ed08be 100644 --- a/lib/RT/Record.pm +++ b/lib/RT/Record.pm @@ -1010,7 +1010,7 @@ sub _UpdateAttributes { "The new value has been set.", # loc "No column specified", # loc "Immutable field", # loc - "Nonexistant field?", # loc + "Nonexistent field?", # loc "Invalid data", # loc "Couldn't find row", # loc "Missing a primary key?: [_1]", # loc From f7b3dc76cd7c7b2753fad43712cb19a0f2369bfd Mon Sep 17 00:00:00 2001 From: sunnavy Date: Thu, 22 Jul 2021 04:00:57 +0800 Subject: [PATCH 02/11] Clean up lifecycles on save when possible Here we just clean up obvious errors like nonexistent lifecycles, statuses, etc. This is initially to sync __maps__ when statuses are deleted from lifecycles. --- lib/RT/Lifecycle.pm | 162 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 130 insertions(+), 32 deletions(-) diff --git a/lib/RT/Lifecycle.pm b/lib/RT/Lifecycle.pm index f872404c008..f29c32aeda8 100644 --- a/lib/RT/Lifecycle.pm +++ b/lib/RT/Lifecycle.pm @@ -799,6 +799,22 @@ sub _SaveLifecycles { my $lifecycles = shift; my $CurrentUser = shift; + for my $key ( keys %$lifecycles ) { + next if $key eq '__maps__'; + my ( $ret, @warnings ) + = $class->ValidateLifecycle( Lifecycle => $lifecycles->{$key}, Name => $key, Cleanup => 1 ); + unless ($ret) { + RT->Logger->debug($_) for @warnings; + } + } + + if ( $lifecycles->{__maps__} ) { + my ( $ret, @warnings ) = $class->ValidateLifecycleMaps( Lifecycles => $lifecycles, Cleanup => 1 ); + unless ($ret) { + RT->Logger->debug($_) for @warnings; + } + } + my $setting = RT::Configuration->new($CurrentUser); $setting->LoadByCols(Name => 'Lifecycles', Disabled => 0); if ($setting->Id) { @@ -953,10 +969,13 @@ sub UpdateMaps { return (1, $CurrentUser->loc("Lifecycle mappings updated")); } -=head2 ValidateLifecycle( CurrentUser => undef, Lifecycle => undef, Name => undef ) +=head2 ValidateLifecycle( CurrentUser => undef, Lifecycle => undef, Name => undef, Cleanup => undef ) Validate passed Lifecycle data structure. +If Cleanup is true, clean up passed Lifecycle data structure, e.g. removing +nonexistent statuses. + Returns (STATUS, MESSAGE). STATUS is true if succeeded, otherwise false. =cut @@ -967,6 +986,7 @@ sub ValidateLifecycle { CurrentUser => undef, Lifecycle => undef, Name => undef, + Cleanup => undef, @_, ); my $current_user = $args{CurrentUser} || RT->SystemUser; @@ -995,43 +1015,79 @@ sub ValidateLifecycle { for my $state ( keys %{ $lifecycle->{defaults} || {} } ) { my $status = $lifecycle->{defaults}{$state}; if ( $status ) { - push @warnings, $current_user->loc( "Nonexistent default [_1] status [_2] in [_3] lifecycle", $state, lc $status, $name ) - unless $lifecycle->{canonical_case}{ lc $status }; + unless ( $lifecycle->{canonical_case}{ lc $status } ) { + push @warnings, $current_user->loc( "Nonexistent default [_1] status [_2] in [_3] lifecycle", $state, lc $status, $name ) + unless $lifecycle->{canonical_case}{ lc $status }; + if ( $args{Cleanup} ) { + delete $lifecycle->{defaults}{$state}; + } + } } else { push @warnings, $current_user->loc( "Empty default [_1] status in [_2] Lifecycle", $state, $name ); } } for my $from ( keys %{ $lifecycle->{transitions} || {} } ) { - push @warnings, $current_user->loc( "Nonexistent status [_1] in transitions in [_2] lifecycle", lc $from, $name ) - unless $from eq '' || $lifecycle->{canonical_case}{ lc $from }; + unless ( $from eq '' || $lifecycle->{canonical_case}{ lc $from } ) { + push @warnings, + $current_user->loc( "Nonexistent status [_1] in transitions in [_2] lifecycle", lc $from, $name ); + if ( $args{Cleanup} ) { + delete $lifecycle->{transitions}{$from}; + next; + } + } for my $status ( @{ ( $lifecycle->{transitions}{$from} ) || [] } ) { push @warnings, $current_user->loc( "Nonexistent status [_1] in transitions in [_2] lifecycle", lc $status, $name ) unless $lifecycle->{canonical_case}{ lc $status }; } + + if ( $args{Cleanup} ) { + $lifecycle->{transitions}{$from} + = [ grep { $lifecycle->{canonical_case}{ lc $_ } } @{ ( $lifecycle->{transitions}{$from} ) || [] } ]; + } } for my $schema ( keys %{ $lifecycle->{rights} || {} } ) { my ( $from, $to ) = split /\s*->\s*/, $schema, 2; unless ( $from and $to ) { push @warnings, $current_user->loc( "Invalid right transition [_1] in [_2] lifecycle", $schema, $name ); + if ( $args{Cleanup} ) { + delete $lifecycle->{rights}{$schema}; + } next; } - push @warnings, $current_user->loc( "Nonexistent status [_1] in right transition in [_2] lifecycle", lc $from, $name ) - unless $from eq '*' - or $lifecycle->{canonical_case}{ lc $from }; - push @warnings, $current_user->loc( "Nonexistent status [_1] in right transition in [_2] lifecycle", lc $to, $name ) - unless $to eq '*' || $lifecycle->{canonical_case}{ lc $to }; - - push @warnings, - $current_user->loc( "Invalid right name ([_1]) in [_2] lifecycle; right names must be ASCII", $lifecycle->{rights}{$schema}, $name ) - if $lifecycle->{rights}{$schema} =~ /\P{ASCII}/; - - push @warnings, - $current_user - ->loc( "Invalid right name ([_1]) in [_2] lifecycle; right names must be <= 25 characters", $lifecycle->{rights}{$schema}, $name ) - if length( $lifecycle->{rights}{$schema} ) > 25; + + my $invalid; + unless ( $from eq '*' or $lifecycle->{canonical_case}{ lc $from } ) { + push @warnings, + $current_user->loc( "Nonexistent status [_1] in right transition in [_2] lifecycle", lc $from, $name ); + $invalid = 1; + } + + unless ( $to eq '*' or $lifecycle->{canonical_case}{ lc $to } ) { + push @warnings, + $current_user->loc( "Nonexistent status [_1] in right transition in [_2] lifecycle", lc $to, $name ); + $invalid = 1; + } + + if ( $lifecycle->{rights}{$schema} =~ /\P{ASCII}/ ) { + push @warnings, + $current_user->loc( "Invalid right name ([_1]) in [_2] lifecycle; right names must be ASCII", + $lifecycle->{rights}{$schema}, $name ); + $invalid = 1; + } + + if ( length( $lifecycle->{rights}{$schema} ) > 25 ) { + push @warnings, + $current_user->loc( "Invalid right name ([_1]) in [_2] lifecycle; right names must be <= 25 characters", + $lifecycle->{rights}{$schema}, $name ); + $invalid = 1; + } + + if ( $args{Cleanup} && $invalid ) { + delete $lifecycle->{rights}{$schema}; + } } my @actions; @@ -1044,27 +1100,44 @@ sub ValidateLifecycle { @actions = @{ $lifecycle->{'actions'} }; } + my @valid_actions; while ( my ( $transition, $info ) = splice @actions, 0, 2 ) { my ( $from, $to ) = split /\s*->\s*/, $transition, 2; unless ( $from and $to ) { push @warnings, $current_user->loc( "Invalid action status change [_1], in [_2] lifecycle", $transition, $name ); next; } - push @warnings, $current_user->loc( "Nonexistent status [_1] in actions in [_2] lifecycle", lc $from, $name ) - unless $from eq '*' - or $lifecycle->{canonical_case}{ lc $from }; - push @warnings, $current_user->loc( "Nonexistent status [_1] in actions in [_2] lifecycle", lc $to, $name ) - unless $to eq '*' - or $lifecycle->{canonical_case}{ lc $to }; + + my $invalid; + unless ( $from eq '*' or $lifecycle->{canonical_case}{ lc $from } ) { + push @warnings, $current_user->loc( "Nonexistent status [_1] in actions in [_2] lifecycle", lc $from, $name ); + $invalid = 1; + } + + unless ( $to eq '*' or $lifecycle->{canonical_case}{ lc $to } ) { + push @warnings, $current_user->loc( "Nonexistent status [_1] in actions in [_2] lifecycle", lc $to, $name ); + $invalid = 1; + } + + if ( $args{Cleanup} && !$invalid ) { + push @valid_actions, $transition, $info; + } + } + + if ( $args{Cleanup} ) { + $lifecycle->{'actions'} = \@valid_actions; } return @warnings ? ( 0, uniq @warnings ) : 1; } -=head2 ValidateLifecycleMaps( CurrentUser => undef ) +=head2 ValidateLifecycleMaps( CurrentUser => undef, Lifecycles => undef, Maps => undef, Cleanup => undef ) Validate lifecycle Maps. +If Cleanup is true, clean up maps structure, e.g. removing nonexistent +statuses. + Returns (STATUS, MESSAGES). STATUS is true if succeeded, otherwise false. =cut @@ -1073,32 +1146,57 @@ sub ValidateLifecycleMaps { my $self = shift; my %args = ( CurrentUser => undef, + Lifecycles => undef, + Maps => undef, + Cleanup => undef, @_, ); my $current_user = $args{CurrentUser} || RT->SystemUser; my @warnings; - for my $mapname ( keys %{ $LIFECYCLES_CACHE{'__maps__'} || {} } ) { + my $lifecycles = $args{Lifecycles} || \%LIFECYCLES_CACHE; + my $maps = $args{Maps} || $lifecycles->{__maps__} || $LIFECYCLES_CACHE{__maps__} || {}; + + for my $mapname ( keys %$maps ) { my ( $from, $to ) = split /\s*->\s*/, $mapname, 2; unless ( $from and $to ) { push @warnings, $current_user->loc( "Invalid lifecycle mapping [_1]", $mapname ); + if ( $args{Cleanup} ) { + delete $maps->{$mapname}; + } next; } push @warnings, $current_user->loc( "Nonexistent lifecycle [_1] in [_2] lifecycle map", $from, $mapname ) - unless $LIFECYCLES_CACHE{$from}; + unless $lifecycles->{$from}; push @warnings, $current_user->loc( "Nonexistent lifecycle [_1] in [_2] lifecycle map", $to, $mapname ) - unless $LIFECYCLES_CACHE{$to}; + unless $lifecycles->{$to}; # Ignore mappings referring to disabled lifecycles next if $LIFECYCLES_CACHE{$from} && $LIFECYCLES_CACHE{$from}{disabled}; next if $LIFECYCLES_CACHE{$to} && $LIFECYCLES_CACHE{$to}{disabled}; - my $map = $LIFECYCLES_CACHE{'__maps__'}{$mapname}; + if ( $args{Cleanup} && ( !$lifecycles->{$from} || !$lifecycles->{$to} ) ) { + delete $maps->{$mapname}; + next; + } + + my $map = $maps->{$mapname}; + my $new_map = {}; for my $status ( keys %{$map} ) { push @warnings, $current_user->loc( "Nonexistent status [_1] in [_2] in [_3] lifecycle map", lc $status, $from, $mapname ) - if $LIFECYCLES_CACHE{$from} && !$LIFECYCLES_CACHE{$from}{canonical_case}{ lc $status }; + if $lifecycles->{$from} && !$lifecycles->{$from}{canonical_case}{ lc $status }; push @warnings, $current_user->loc( "Nonexistent status [_1] in [_2] in [_3] lifecycle map", lc $map->{$status}, $to, $mapname ) - if $LIFECYCLES_CACHE{$to} && !$LIFECYCLES_CACHE{$to}{canonical_case}{ lc $map->{$status} }; + if $lifecycles->{$to} && !$lifecycles->{$to}{canonical_case}{ lc $map->{$status} }; + $new_map->{$status} = $map->{$status} + if $args{Cleanup} + && $lifecycles->{$from} + && $lifecycles->{$from}{canonical_case}{ lc $status } + && $lifecycles->{$to} + && $lifecycles->{$to}{canonical_case}{ lc $map->{$status} }; + } + + if ( $args{Cleanup} ) { + $maps->{$mapname} = $new_map; } } From aa4426ade886ad53573601f3f4be09cb5b14a300 Mon Sep 17 00:00:00 2001 From: sunnavy Date: Wed, 31 Jan 2024 16:02:11 -0500 Subject: [PATCH 03/11] Trim any leading and trailing spaces from name on lifecycle create This is initially to fix broken lifecycle links because the URL with Name=... doesn't retain the trailing space. --- share/html/Admin/Lifecycles/Create.html | 2 ++ 1 file changed, 2 insertions(+) diff --git a/share/html/Admin/Lifecycles/Create.html b/share/html/Admin/Lifecycles/Create.html index e3a25973f0c..8e02113b8bc 100644 --- a/share/html/Admin/Lifecycles/Create.html +++ b/share/html/Admin/Lifecycles/Create.html @@ -126,6 +126,8 @@ } if ($Create) { + $Name =~ s!^\s+!!; + $Name =~ s!\s+$!!; my ($ok, $msg) = RT::Lifecycle->CreateLifecycle( CurrentUser => $session{CurrentUser}, Name => $Name, From dbfa304457251493c0fb6c4bb7753548582c6756 Mon Sep 17 00:00:00 2001 From: sunnavy Date: Wed, 31 Jan 2024 19:33:54 -0500 Subject: [PATCH 04/11] Do not merge old values for hash configs in database When admins update configs from web UI, the default values of inputs fully contain current config values, so there is no need to merge old values after update. By not merging old values, admins now are able to delete keys from hash configs. This is initially to support lifecycle deletions. --- lib/RT/Config.pm | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/RT/Config.pm b/lib/RT/Config.pm index 96a79943389..e670f3ca451 100644 --- a/lib/RT/Config.pm +++ b/lib/RT/Config.pm @@ -3077,10 +3077,9 @@ sub LoadConfigFromDatabase { : $type eq 'HASH' ? [ %$value ] : [ $value ]; - # hashes combine, but by default previous config settings shadow - # later changes, here we want database configs to shadow file ones. + # Unlike hashes in files that merge together, database configs are supposed to contain all the data, so no need + # to merge file configs. With it, admins are able to delete keys. if ($type eq 'HASH') { - $val = [ $self->Get($name), @$val ]; $self->Set($name, ()); } From 9fa6147b63f2807c842598ee089549ac04e3b474 Mon Sep 17 00:00:00 2001 From: sunnavy Date: Wed, 31 Jan 2024 19:39:47 -0500 Subject: [PATCH 05/11] Support to delete lifecycles --- lib/RT/Lifecycle.pm | 60 +++++++++++++++++++++++ share/html/Admin/Lifecycles/Advanced.html | 45 +++++++++++++---- 2 files changed, 96 insertions(+), 9 deletions(-) diff --git a/lib/RT/Lifecycle.pm b/lib/RT/Lifecycle.pm index f29c32aeda8..cff0275ebc5 100644 --- a/lib/RT/Lifecycle.pm +++ b/lib/RT/Lifecycle.pm @@ -905,6 +905,66 @@ sub CreateLifecycle { return $class->_CreateLifecycle(%args); } +=head2 DeleteLifecycle( CurrentUser => undef, Name => undef ) + +Delete a lifecycle. + +Returns (STATUS, MESSAGE). STATUS is true if succeeded, otherwise false. + +=cut + +sub DeleteLifecycle { + my $class = shift; + my %args = ( + CurrentUser => undef, + Name => undef, + @_, + ); + + my $CurrentUser = $args{CurrentUser}; + my $Name = $args{Name}; + + return ( 0, $CurrentUser->loc("Lifecycle Name required") ) unless length $Name; + + my $lifecycles = RT->Config->Get('Lifecycles'); + if ( $lifecycles->{$Name} ) { + my $dep_class = ( $lifecycles->{$Name}{'type'} // '' ) eq 'asset' ? 'RT::Catalogs' : 'RT::Queues'; + my $dep_objects = $dep_class->new( $args{CurrentUser} ); + $dep_objects->Limit( FIELD => 'Lifecycle', VALUE => $Name ); + my @dep_names = map { $_->Name } @{ $dep_objects->ItemsArrayRef }; + + if (@dep_names) { + return ( + 0, + $CurrentUser->loc( + "Could not delete lifecycle [_1]: it is used by [_2] [_3]", + $Name, + ( + $dep_class eq 'RT::Queues' + ? ( @dep_names > 1 ? 'queues' : 'queue' ) + : ( @dep_names > 1 ? 'catalogs' : 'catalog' ) + ), + join( ', ', @dep_names ), + ) + ); + } + else { + delete $lifecycles->{$Name}; + my $maps = $lifecycles->{__maps__}; + for my $key ( keys %$maps ) { + if ( $key =~ m/^$Name -> / || $key =~ m/ -> $Name$/ ) { + delete $maps->{$key}; + } + } + my ( $ok, $msg ) = $class->_SaveLifecycles( $lifecycles, $CurrentUser ); + return ( $ok, $msg ) if !$ok; + } + } + + return ( 1, $CurrentUser->loc( 'Lifecycle [_1] deleted', $args{Name} ) ); +} + + =head2 UpdateLifecycle( CurrentUser => undef, LifecycleObj => undef, NewConfig => undef, Maps => undef ) Update passed lifecycle to the new configuration. diff --git a/share/html/Admin/Lifecycles/Advanced.html b/share/html/Admin/Lifecycles/Advanced.html index 0282c9e062a..09e0c1355e7 100644 --- a/share/html/Admin/Lifecycles/Advanced.html +++ b/share/html/Admin/Lifecycles/Advanced.html @@ -78,7 +78,11 @@ <& /Elements/Submit, Label => loc('Save Changes'), Name => 'Update' &> - +
+
+ <& /Elements/Submit, Label => loc('Delete'), Name => 'Delete' &> +
+
<%INIT> @@ -96,6 +100,9 @@ push @results, loc("The graphical lifecycle builder is not currently supported on IE 11. You can update the lifecycle configuration using the Advanced tab or access the lifecycle editor in a supported browser."); } +my $redirect_to ='/Admin/Lifecycles/Advanced.html'; +my %redirect_args; + if ( $Validate || $Update ) { my $lifecycle = JSON::from_json($Config); my ( $valid, @warnings ) @@ -126,17 +133,36 @@ push @results, @warnings; } - MaybeRedirectForResults( - Actions => \@results, - Arguments => { - Name => $LifecycleObj->Name, - Type => $LifecycleObj->Type, + %redirect_args = ( + Name => $Name, + Type => $Type, - # Get the latest canonicalized data if updated successfully - $updated ? () : ( Config => $Config ), - }, + # Get the latest canonicalized data if updated successfully + $updated ? () : ( Config => $Config ), ); } +elsif ( $Delete ) { + my ( $ret, $msg ) = RT::Lifecycle->DeleteLifecycle( + CurrentUser => $session{CurrentUser}, + Name => $Name, + ); + push @results, $msg; + if ( $ret ) { + $redirect_to = '/Admin/Lifecycles/'; + } + else { + %redirect_args = ( + Name => $Name, + Type => $Type, + ); + } +} + +MaybeRedirectForResults( + Actions => \@results, + Path => $redirect_to, + Arguments => \%redirect_args, +); <%ARGS> @@ -145,4 +171,5 @@ $Config => undef $Validate => undef $Update => undef +$Delete => undef From da8373268f81bc4aa7643205e474d522506d5889 Mon Sep 17 00:00:00 2001 From: sunnavy Date: Mon, 5 Feb 2024 10:56:06 -0500 Subject: [PATCH 06/11] Test lifecycle deletions --- t/web/lifecycle.t | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 t/web/lifecycle.t diff --git a/t/web/lifecycle.t b/t/web/lifecycle.t new file mode 100644 index 00000000000..a1652532436 --- /dev/null +++ b/t/web/lifecycle.t @@ -0,0 +1,46 @@ +use strict; +use warnings; + +BEGIN { require './t/lifecycles/utils.pl' } + +my ( $url, $m ) = RT::Test->started_ok; +ok( $m->login(), 'logged in' ); + +diag "Test lifecycle creation"; + +$m->get_ok('/Admin/Lifecycles/Create.html'); +$m->submit_form_ok( + { + form_name => 'CreateLifecycle', + fields => { Name => ' foobar ', }, # Intentially add spaces to test the auto cleanup. + button => 'Create', + }, + 'Create lifecycle foobar' +); + +$m->text_contains( 'foobar', 'Lifecycle foobar created' ); + +# Test if index page has it too +$m->follow_link_ok( { text => 'Select', url_regex => qr{/Admin/Lifecycles} } ); +$m->follow_link_ok( { text => 'foobar' } ); + + +# Test more updates + + +diag "Test lifecycle deletion"; + +$m->follow_link_ok( { url_regex => qr{/Admin/Lifecycles/Advanced.html} } ); +$m->submit_form_ok( + { + form_name => 'ModifyLifecycleAdvanced', + button => 'Delete', + }, + 'Delete lifecycle foobar' +); + +$m->text_contains('Lifecycle foobar deleted'); +$m->follow_link_ok( { text => 'Select', url_regex => qr{/Admin/Lifecycles} } ); +$m->text_lacks( 'foobar', 'foobar is gone' ); + +done_testing; From d2543f35389aa1bda558788771b340a959a6da8c Mon Sep 17 00:00:00 2001 From: sunnavy Date: Fri, 2 Feb 2024 15:14:07 -0500 Subject: [PATCH 07/11] Show lifecycle warnings to admins who are accessing lifecycle pages This gives admins hints of the issue of %Lifecycles configuration. --- lib/RT/Lifecycle.pm | 6 ++++++ share/html/Admin/Lifecycles/autohandler | 18 +++++++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/lib/RT/Lifecycle.pm b/lib/RT/Lifecycle.pm index cff0275ebc5..0f7d08b2575 100644 --- a/lib/RT/Lifecycle.pm +++ b/lib/RT/Lifecycle.pm @@ -625,9 +625,12 @@ sub CanonicalCase { return($self->{data}{canonical_case}{lc $status} || lc $status); } +our @WARNINGS; sub FillCache { my $self = shift; + @WARNINGS = (); + my $map = RT->Config->Get('Lifecycles') or return; { @@ -643,6 +646,7 @@ sub FillCache { for my $name ( @lifecycles ) { unless ( $map->{$name} ) { warn "Lifecycle $name is missing in %Lifecycles config"; + push @WARNINGS, loc( "Lifecycle [_1] is missing in %Lifecycles config", $name ); } } } @@ -667,6 +671,7 @@ sub FillCache { my ( $ret, @warnings ) = $self->ValidateLifecycle(Lifecycle => $lifecycle, Name => $name); unless ( $ret ) { warn $_ for @warnings; + push @WARNINGS, @warnings; } my @statuses; @@ -739,6 +744,7 @@ sub FillCache { my ( $ret, @warnings ) = $self->ValidateLifecycleMaps(); unless ( $ret ) { warn $_ for @warnings; + push @WARNINGS, @warnings; } # Lower-case the transition maps diff --git a/share/html/Admin/Lifecycles/autohandler b/share/html/Admin/Lifecycles/autohandler index db74cbe3050..a7be91f9d60 100644 --- a/share/html/Admin/Lifecycles/autohandler +++ b/share/html/Admin/Lifecycles/autohandler @@ -46,9 +46,21 @@ %# %# END BPS TAGGED BLOCK }}} <%init> -return $m->call_next(%ARGS) - if RT->Config->Get('ShowEditLifecycleConfig') +$m->clear_and_abort(403) + unless RT->Config->Get('ShowEditLifecycleConfig') && $session{'CurrentUser'}->UserObj->HasRight( Right => 'SuperUser', Object => $RT::System, ); -$m->clear_and_abort(403); +if ( RT::Interface::Web::RequestENV('REQUEST_METHOD') eq 'GET' && ( my @warnings = @RT::Lifecycle::WARNINGS ) ) { + + # Filter by lifecycle name to show warnings only for that lifecycle + if ( $DECODED_ARGS->{Name} ) { + @warnings = grep { / \Q$DECODED_ARGS->{Name}\E / } @warnings; + } + + if ( @warnings ) { + push @{ $session{'Actions'}{''} ||= []}, @warnings; + } +} + +$m->call_next(%ARGS); From 04b1c87d950971e01f901329b7ca7aa60a093d9a Mon Sep 17 00:00:00 2001 From: sunnavy Date: Mon, 5 Feb 2024 10:09:24 -0500 Subject: [PATCH 08/11] Add protective code in case of invalid lifecycles This is initially to get rid of server errors for our tests that contain other unsupported types(e.g. "racecar" type in "t/lifecycles/utils.pl"). --- share/html/Admin/Lifecycles/index.html | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/share/html/Admin/Lifecycles/index.html b/share/html/Admin/Lifecycles/index.html index dc1555c8ce2..c9f97f4e314 100644 --- a/share/html/Admin/Lifecycles/index.html +++ b/share/html/Admin/Lifecycles/index.html @@ -157,6 +157,10 @@ RT::Logger->error( 'Unable to load lifecycle ' . $key ); return; } + + # No applied objects if lifecycle is not a subclass, in case of invalid data + return if ref $lifecycle eq 'RT::Lifecycle'; + my $list_method = $lifecycle->Type() eq 'asset' ? 'Catalogs' : 'Queues'; my $appliedobjs = $lifecycle->$list_method(); $appliedobjs->RowsPerPage($applied_list_limit); From 72926900222cbf38cb25ed6bfb31ae12b4a7483d Mon Sep 17 00:00:00 2001 From: sunnavy Date: Mon, 5 Feb 2024 10:17:51 -0500 Subject: [PATCH 09/11] Test lifecycle warnings on admin web UI --- t/lifecycles/basics.t | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/t/lifecycles/basics.t b/t/lifecycles/basics.t index f53f077cca4..bee73b43bb9 100644 --- a/t/lifecycles/basics.t +++ b/t/lifecycles/basics.t @@ -242,4 +242,16 @@ diag "'!inactive -> inactive' actions are shown even if ticket has unresolved de ); } +diag "Test lifecycle warnings on admin pages"; +{ + no warnings 'redefine'; + local *RT::Queue::ValidateLifecycle = sub {1}; + my ( $ret, $msg ) = $general->SetLifecycle('foobar'); + ok( $ret, $msg ); + RT->System->LifecycleCacheNeedsUpdate(1); + $m->get_ok('/Admin/Lifecycles/'); + $m->warning_like(qr/Lifecycle foobar is missing/); + $m->text_contains('Lifecycle foobar is missing'); +} + done_testing; From 367982377a92682f3da5b06022294ca69a2195f4 Mon Sep 17 00:00:00 2001 From: sunnavy Date: Fri, 2 Feb 2024 16:42:48 -0500 Subject: [PATCH 10/11] Support to update maps of a lifecycle via JSON on Advanced page --- lib/RT/Lifecycle.pm | 11 ++- share/html/Admin/Lifecycles/Advanced.html | 82 ++++++++++++++++++++++- t/web/lifecycle_mappings.t | 56 ++++++++++++++++ 3 files changed, 145 insertions(+), 4 deletions(-) diff --git a/lib/RT/Lifecycle.pm b/lib/RT/Lifecycle.pm index 0f7d08b2575..b978a4c04ed 100644 --- a/lib/RT/Lifecycle.pm +++ b/lib/RT/Lifecycle.pm @@ -1005,12 +1005,15 @@ sub UpdateLifecycle { return (1, $CurrentUser->loc("Lifecycle [_1] updated", $name)); } -=head2 UpdateMaps( CurrentUser => undef, Maps => undef ) +=head2 UpdateMaps( CurrentUser => undef, Maps => undef, Name => undef ) Update lifecycle maps. Returns (STATUS, MESSAGE). STATUS is true if succeeded, otherwise false. +Note that the Maps in argument will be merged into existing maps. To delete +all existing items for a lifecycle before merging, pass its Name. + =cut sub UpdateMaps { @@ -1018,14 +1021,18 @@ sub UpdateMaps { my %args = ( CurrentUser => undef, Maps => undef, + Name => undef, @_, ); my $CurrentUser = $args{CurrentUser}; my $lifecycles = RT->Config->Get('Lifecycles'); + my $all_maps = $lifecycles->{__maps__} || {}; + my @keep_keys = grep { $args{Name} && !/^\Q$args{Name}\E -> | -> \Q$args{Name}\E$/ } keys %$all_maps; + %{ $lifecycles->{__maps__} } = ( - %{ $lifecycles->{__maps__} || {} }, + map( { $_ => $all_maps->{$_} } @keep_keys ), %{ $args{Maps} }, ); diff --git a/share/html/Admin/Lifecycles/Advanced.html b/share/html/Admin/Lifecycles/Advanced.html index 09e0c1355e7..ddc7452a64b 100644 --- a/share/html/Admin/Lifecycles/Advanced.html +++ b/share/html/Admin/Lifecycles/Advanced.html @@ -54,10 +54,11 @@ -
+ + - + <&| /Widgets/TitleBox, title => loc('Basics'), content_class => 'mx-auto width-sm' &>
@@ -83,8 +84,40 @@ <& /Elements/Submit, Label => loc('Delete'), Name => 'Delete' &>
+
+ + +
+ + + <&| /Widgets/TitleBox, title => loc('Mappings'), content_class => 'mx-auto width-sm' &> + +
+ + + +
+ + + +
+
+ <& /Elements/Submit, Label => loc('Validate Mappings'), Name => 'ValidateMaps' &> +
+
+ <& /Elements/Submit, Label => loc('Save Mappings'), Name => 'UpdateMaps' &> +
+
+ +
+ + <%INIT> my ($title, @results); my $LifecycleObj = RT::Lifecycle->new(); @@ -100,6 +133,13 @@ push @results, loc("The graphical lifecycle builder is not currently supported on IE 11. You can update the lifecycle configuration using the Advanced tab or access the lifecycle editor in a supported browser."); } +if ( !defined $Maps && ( my $all_maps = RT->Config->Get('Lifecycles')->{__maps__} ) ) { + for my $item ( grep {/^\Q$Name\E -> | -> \Q$Name\E$/} keys %$all_maps ) { + $Maps->{$item} = $all_maps->{$item}; + } + $Maps = JSON::to_json( $Maps || {}, { canonical => 1, pretty => 1 } ); +} + my $redirect_to ='/Admin/Lifecycles/Advanced.html'; my %redirect_args; @@ -157,6 +197,41 @@ ); } } +elsif ( $ValidateMaps || $UpdateMaps ) { + my $maps = JSON::from_json($Maps || '{}'); + + my ( $valid, @warnings ) + = $LifecycleObj->ValidateLifecycleMaps( Maps => $maps, CurrentUser => $session{CurrentUser} ); + + my $updated; + if ($valid) { + if ($ValidateMaps) { + push @results, loc('Mappings is valid'); + } + else { + # Maps will be merged into existing value, here we remove existing values so admins can delete items + + ( $updated, my $msg ) = RT::Lifecycle->UpdateMaps( + CurrentUser => $session{CurrentUser}, + Maps => $maps, + Name => $Name, + ); + push @results, $msg; + } + + } + else { + push @results, @warnings; + } + + %redirect_args = ( + Name => $Name, + Type => $Type, + + # Get the latest canonicalized data if updated successfully + $updated ? () : ( Maps => $Maps ), + ); +} MaybeRedirectForResults( Actions => \@results, @@ -172,4 +247,7 @@ $Validate => undef $Update => undef $Delete => undef +$ValidateMaps => undef +$UpdateMaps => undef +$Maps => undef diff --git a/t/web/lifecycle_mappings.t b/t/web/lifecycle_mappings.t index f9efafa1b79..dcaec2b6511 100644 --- a/t/web/lifecycle_mappings.t +++ b/t/web/lifecycle_mappings.t @@ -172,6 +172,62 @@ diag "Test updating sales-engineering mappings"; ); } +diag "Test advanced mappings"; +{ + $m->get_ok( $url . '/Admin/Lifecycles/Advanced.html?Type=ticket&Name=sales-engineering' ); + my $form = $m->form_name('ModifyLifecycleAdvancedMappings'); + + require JSON; + my $maps = JSON::from_json( $form->value('Maps') ); + is_deeply( + $maps, + { + 'sales-engineering -> default' => { + 'rejected' => 'rejected', + 'resolved' => 'resolved', + 'deleted' => 'deleted', + 'engineering' => 'open', + 'stalled' => 'stalled', + 'sales' => 'new' + } + }, + 'Correct current value' + ); + + $maps->{'default -> sales-engineering'} = { 'new' => 'sales', }; + + $m->submit_form_ok( + { + fields => { + Maps => JSON::to_json($maps), + Name => "sales-engineering", + Type => "ticket", + }, + button => 'UpdateMaps', + }, + 'Update maps' + ); + $m->content_contains('Lifecycle mappings updated'); + $form = $m->form_name('ModifyLifecycleAdvancedMappings'); + is_deeply( $maps, JSON::from_json( $form->value('Maps') ), 'Correct updated value' ); + + $m->submit_form_ok( + { + fields => { + Maps => '', + Name => "sales-engineering", + Type => "ticket", + }, + button => 'UpdateMaps', + }, + 'Clear maps' + ); + $m->content_contains('Lifecycle mappings updated'); + $form = $m->form_name('ModifyLifecycleAdvancedMappings'); + $form->value( "Maps", "{}", 'Maps got cleared' ); +} + + sub reload_lifecycle { # to get rid of the warning of: # you're changing config option in a test file when server is active From 500d3857d5aef2cdba73884bd54cf3acd3a3ca6d Mon Sep 17 00:00:00 2001 From: sunnavy Date: Thu, 8 Feb 2024 09:15:01 -0500 Subject: [PATCH 11/11] Add links to help map statuses that have the same name --- share/html/Admin/Lifecycles/Mappings.html | 29 ++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/share/html/Admin/Lifecycles/Mappings.html b/share/html/Admin/Lifecycles/Mappings.html index 38f3426a236..a4b1273ce27 100644 --- a/share/html/Admin/Lifecycles/Mappings.html +++ b/share/html/Admin/Lifecycles/Mappings.html @@ -64,7 +64,12 @@ % my $ToMapping = $LifecycleObj->MoveMap($Other); % my @OtherStatuses = $Other->Valid; -<&| /Widgets/TitleBox, title => $Other->Name &> +% require List::MoreUtils; +<&| /Widgets/TitleBox, title => $Other->Name, + List::MoreUtils::duplicates( map { lc } @MyStatuses, @OtherStatuses ) + ? ( titleright_raw => '' . loc('Map same names if unset') . '' ) + : (), +&>
<&|/l, $Other->Name, $LifecycleObj->Name &>From [_1] to [_2]
@@ -104,6 +109,28 @@
<&|/l, $LifecycleObj->Name, $Other->Name &>From [_1
+ + % } <%INIT> my ($title, @results);