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 9cdf37182900936c865c0ba605a7a7debccccf56 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..e04f67d5f80 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 ); + 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 }; } + + # Remove nonexistent statuses + 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 4f0700d3d3f1dd01c44e41e8dd326ac1ce24ccf3 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 ce3adb04c2d0e82ecd66d0ed4445caa4f356e7d8 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 e19e0556be7d3e271e61ad2756d56a5c839c6d34 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 e04f67d5f80..09a3479eaf1 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 ae7daecc310c9e731c9fa0663f7f0c366e4d7cd6 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 | 53 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 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..a052321c80e --- /dev/null +++ b/t/web/lifecycle.t @@ -0,0 +1,53 @@ +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' } ); + +RT->Config->RefreshConfigFromDatabase(); +RT::Lifecycle->FillCache; +my $lifecycle = RT::Lifecycle->new; +$lifecycle->Load(' foobar '); +ok( !$lifecycle->Name, 'Lifecycle " foo bar " does not exist' ); +$lifecycle->Load('foobar'); +is( $lifecycle->Name, 'foobar', 'Lifecycle name is corrected to "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 302af2af2e6337fedb3c3d17ca741a019a58b336 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 09a3479eaf1..85544535bf9 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 24342d3ff3756cf94ecaeb6ad73f5e04e8e585b7 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 1753c6384ef1e52a05c8fd1c1a07162c4bc6ef38 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 3e9651f2c1b7e0e6c19b6cac3508150f982515b9 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 ++++++++++++++++++++++- share/static/js/util.js | 2 +- t/web/lifecycle_mappings.t | 56 ++++++++++++++++ 4 files changed, 146 insertions(+), 5 deletions(-) diff --git a/lib/RT/Lifecycle.pm b/lib/RT/Lifecycle.pm index 85544535bf9..6e48938ec78 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/share/static/js/util.js b/share/static/js/util.js index bd422412560..00a5bd4b389 100644 --- a/share/static/js/util.js +++ b/share/static/js/util.js @@ -823,7 +823,7 @@ jQuery(function() { return true; }; - jQuery('[name=Config]').bind('input propertychange', function() { + jQuery('[name=Config], [name=Maps]').bind('input propertychange', function() { var form = jQuery(this).closest('form'); if ( validate_json(jQuery(this).val()) ) { form.find('input[type=submit]').prop('disabled', false); 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 169ea4a05dd7f9703c4aa9bf3a8e63362a3ab903 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 --- etc/cpanfile | 2 +- share/html/Admin/Lifecycles/Mappings.html | 43 ++++++++++++++++++++++- 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/etc/cpanfile b/etc/cpanfile index 07ee82d1825..e20d748af1e 100644 --- a/etc/cpanfile +++ b/etc/cpanfile @@ -52,7 +52,7 @@ requires 'HTTP::Message', '>= 6.07'; requires 'IPC::Run3'; requires 'JavaScript::Minifier::XS'; requires 'JSON'; -requires 'List::MoreUtils'; +requires 'List::MoreUtils', '>= 0.420'; requires 'Locale::Maketext', '>= 1.06'; requires 'Locale::Maketext::Fuzzy', '>= 0.11'; requires 'Locale::Maketext::Lexicon', '>= 0.32'; diff --git a/share/html/Admin/Lifecycles/Mappings.html b/share/html/Admin/Lifecycles/Mappings.html index 38f3426a236..a39a4d4529a 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 => $titleright ) + : (), +&>
<&|/l, $Other->Name, $LifecycleObj->Name &>From [_1] to [_2]
@@ -104,6 +109,28 @@
<&|/l, $LifecycleObj->Name, $Other->Name &>From [_1
+ + % } <%INIT> my ($title, @results); @@ -147,6 +174,20 @@
<&|/l, $LifecycleObj->Name, $Other->Name &>From [_1 @lifecycle_names; push @results, loc("You only need mappings once you have two more [_1] lifecycles", loc($Type)) unless @lifecycles; + +my $alt = loc('Options'); +my $map_same_name = loc('Map same names if unset'); +my $titleright = qq{ +
+ + +
+}; + <%ARGS> $Name => undef