[Rt-commit] rt branch, 4.4/asset-custom-roles, updated. rt-4.4.1-12-gc61e068

Shawn Moore shawn at bestpractical.com
Wed May 17 15:33:18 EDT 2017


The branch, 4.4/asset-custom-roles has been updated
       via  c61e0684d2e7d9fadec327622ae43ada0c5d5e66 (commit)
       via  8e35aa6f6f6708d2e5900f13b8223bb54f01f090 (commit)
      from  03ded030430f6269e0d9250b33427a07ea93a9d1 (commit)

Summary of changes:
 lib/RT/Action/Notify.pm                      | 11 ++++++----
 lib/RT/CustomRole.pm                         |  9 +-------
 lib/RT/Tickets.pm                            | 10 ++++++---
 sbin/rt-validator.in                         | 16 --------------
 share/html/Elements/ColumnMap                |  7 +++++-
 share/html/Search/Bulk.html                  |  2 ++
 share/html/Search/Elements/BuildFormatString |  1 +
 share/html/Search/Elements/PickCustomRoles   |  1 +
 t/customroles/basic.t                        | 33 ----------------------------
 9 files changed, 25 insertions(+), 65 deletions(-)

- Log -----------------------------------------------------------------
commit 8e35aa6f6f6708d2e5900f13b8223bb54f01f090
Author: Shawn M Moore <shawn at bestpractical.com>
Date:   Wed May 17 18:45:06 2017 +0000

    Exclude asset custom roles from ticket search
    
    This covers both search builder and bulk update.

diff --git a/lib/RT/Tickets.pm b/lib/RT/Tickets.pm
index 64b0646..d6c7595 100644
--- a/lib/RT/Tickets.pm
+++ b/lib/RT/Tickets.pm
@@ -1022,6 +1022,7 @@ sub _CustomRoleDecipher {
 
     if ( $field =~ /\D/ ) {
         my $roles = RT::CustomRoles->new( $self->CurrentUser );
+        $roles->LimitToLookupType(RT::Ticket->CustomFieldLookupType);
         $roles->Limit( FIELD => 'Name', VALUE => $field, CASESENSITIVE => 0 );
 
         # custom roles are named uniquely, but just in case there are
diff --git a/share/html/Elements/ColumnMap b/share/html/Elements/ColumnMap
index d08a4fc..c744ddc 100644
--- a/share/html/Elements/ColumnMap
+++ b/share/html/Elements/ColumnMap
@@ -157,7 +157,12 @@ $WCOLUMN_MAP = $COLUMN_MAP = {
                 my $role_type = $m->notes($key);
                 if (!$role_type) {
                     my $role_obj = RT::CustomRole->new($object->CurrentUser);
-                    $role_obj->Load($role_name);
+                    if ($role_name =~ /^\d+$/) {
+                        $role_obj->Load($role_name);
+                    }
+                    else {
+                        $role_obj->LoadByCols(Name => $role_name, LookupType => $object->CustomFieldLookupType);
+                    }
 
                     RT->Logger->notice("Unable to load custom role $role_name")
                         unless $role_obj->Id;
diff --git a/share/html/Search/Bulk.html b/share/html/Search/Bulk.html
index 326e830..15c78d0 100644
--- a/share/html/Search/Bulk.html
+++ b/share/html/Search/Bulk.html
@@ -96,6 +96,7 @@
 <td class="value"> <& /Elements/EmailInput, Name => "DeleteAdminCc", Size=> 20, Default => $ARGS{DeleteAdminCc} &> </td></tr>
 
 % my $single_roles = RT::CustomRoles->new($session{CurrentUser});
+% $single_roles->LimitToLookupType(RT::Ticket->CustomFieldLookupType);
 % $single_roles->LimitToSingleValue;
 % $single_roles->LimitToObjectId($_) for keys %$seen_queues;
 % while (my $role = $single_roles->Next) {
@@ -106,6 +107,7 @@
 % }
 
 % my $multi_roles = RT::CustomRoles->new($session{CurrentUser});
+% $multi_roles->LimitToLookupType(RT::Ticket->CustomFieldLookupType);
 % $multi_roles->LimitToMultipleValue;
 % $multi_roles->LimitToObjectId($_) for keys %$seen_queues;
 % while (my $role = $multi_roles->Next) {
diff --git a/share/html/Search/Elements/BuildFormatString b/share/html/Search/Elements/BuildFormatString
index 4cbcde4..9c9bf80 100644
--- a/share/html/Search/Elements/BuildFormatString
+++ b/share/html/Search/Elements/BuildFormatString
@@ -117,6 +117,7 @@ while ( my $CustomField = $CustomFields->Next ) {
 }
 
 my $CustomRoles = RT::CustomRoles->new( $session{'CurrentUser'});
+$CustomRoles->LimitToLookupType(RT::Ticket->CustomFieldLookupType);
 foreach my $id (keys %queues) {
     # Gotta load up the $queue object, since queues get stored by name now.
     my $queue = RT::Queue->new($session{'CurrentUser'});
diff --git a/share/html/Search/Elements/PickCustomRoles b/share/html/Search/Elements/PickCustomRoles
index e1d0669..a1ebe52 100644
--- a/share/html/Search/Elements/PickCustomRoles
+++ b/share/html/Search/Elements/PickCustomRoles
@@ -50,6 +50,7 @@
 </%ARGS>
 <%INIT>
 my $CustomRoles = RT::CustomRoles->new( $session{'CurrentUser'});
+$CustomRoles->LimitToLookupType(RT::Ticket->CustomFieldLookupType);
 foreach my $id (keys %queues) {
     # Gotta load up the $queue object, since queues get stored by name now.
     my $queue = RT::Queue->new($session{'CurrentUser'});

commit c61e0684d2e7d9fadec327622ae43ada0c5d5e66
Author: Shawn M Moore <shawn at bestpractical.com>
Date:   Wed May 17 19:11:17 2017 +0000

    Remove custom role name uniqueness restriction
    
    This is especially important now that custom roles can be applied to
    different object types (specifically assets).
    
    Now that we can have multiple custom roles with the same name, we have
    to be a little bit more careful any time we handle a custom role by
    name. Internally custom roles are referenced as RT::CustomRole-$id, so
    it's largely only a concern when interpreting user input. Since we were
    already careful to use LimitToLookupType, there should never be any
    confusion internally between a ticket custom role named Foo and an asset
    custom role named Foo.

diff --git a/lib/RT/Action/Notify.pm b/lib/RT/Action/Notify.pm
index 5c3e0f0..b0ba5b0 100644
--- a/lib/RT/Action/Notify.pm
+++ b/lib/RT/Action/Notify.pm
@@ -141,14 +141,17 @@ sub SetRecipients {
                  || $name eq 'OtherRecipients'
                  || $name eq 'AlwaysNotifyActor';
 
-            my $roles = RT::CustomRoles->new( $self->CurrentUser );
+            my $roles = $self->TicketObj->QueueObj->CustomRoles;
             $roles->Limit( FIELD => 'Name', VALUE => $name, CASESENSITIVE => 0 );
 
-            # custom roles are named uniquely, but just in case there are
-            # multiple matches, bail out as we don't know which one to use
+            # in case there are multiple matches, bail out as we
+            # don't know which one to use
             $role = $roles->First;
             if ( $role ) {
-                $role = undef if $roles->Next;
+                if ($roles->Next) {
+                    RT->Logger->error("Ambiguous custom role named '$name' in Notify action for queue #" . $self->TicketObj->Queue . "; skipping. Perhaps specify RT::CustomRole-# instead.");
+                    $role = undef;
+                }
             }
         }
         else {
diff --git a/lib/RT/CustomRole.pm b/lib/RT/CustomRole.pm
index 8c9d69e..3efa899 100644
--- a/lib/RT/CustomRole.pm
+++ b/lib/RT/CustomRole.pm
@@ -253,7 +253,7 @@ sub Load {
 =head2 ValidateName NAME
 
 Takes a custom role name. Returns true if it's an ok name for
-a new custom role. Returns undef if there's already a role by that name.
+a new custom role. Returns undef if it's not valid.
 
 =cut
 
@@ -277,13 +277,6 @@ sub _ValidateName {
         return ($ok, $self->loc("'[_1]' is not a valid name.", $name));
     }
 
-    my $temp = RT::CustomRole->new(RT->SystemUser);
-    $temp->LoadByCols(Name => $name);
-
-    if ( $temp->Name && $temp->id != ($self->id||0))  {
-        return (undef, $self->loc("Role already exists") );
-    }
-
     return (1);
 }
 
diff --git a/lib/RT/Tickets.pm b/lib/RT/Tickets.pm
index d6c7595..51afe8f 100644
--- a/lib/RT/Tickets.pm
+++ b/lib/RT/Tickets.pm
@@ -1025,11 +1025,14 @@ sub _CustomRoleDecipher {
         $roles->LimitToLookupType(RT::Ticket->CustomFieldLookupType);
         $roles->Limit( FIELD => 'Name', VALUE => $field, CASESENSITIVE => 0 );
 
-        # custom roles are named uniquely, but just in case there are
-        # multiple matches, bail out as we don't know which one to use
+        # in case there are multiple matches, bail out as we
+        # don't know which one to use
         $role = $roles->First;
         if ( $role ) {
-            $role = undef if $roles->Next;
+            if ($roles->Next) {
+                RT->Logger->error("Ambiguous custom role named '$field' in TicketSQL; skipping. Perhaps specify __CustomRole.{id}__ instead.");
+                $role = undef;
+            }
         }
     }
     else {
diff --git a/sbin/rt-validator.in b/sbin/rt-validator.in
index f54b984..37366a5 100644
--- a/sbin/rt-validator.in
+++ b/sbin/rt-validator.in
@@ -382,22 +382,6 @@ push @CHECKS, 'User Defined Group Name uniqueness' => sub {
     );
 };
 
-push @CHECKS, 'Custom Role Name uniqueness' => sub {
-    return check_uniqueness(
-        'CustomRoles',
-        columns         => ['Name'],
-        action          => sub {
-            return unless prompt(
-                'Rename', "Found a custom role with a non-unique Name."
-            );
-
-            my $id = shift;
-            my %cols = @_;
-            update_records('CustomRoles', { id => $id }, { Name => join('-', $cols{'Name'}, $id) });
-        },
-    );
-};
-
 push @CHECKS, 'GMs -> Groups, Members' => sub {
     my $msg = "A record in GroupMembers references an object that doesn't exist."
         ." Maybe you deleted a group or principal directly from the database?"
diff --git a/t/customroles/basic.t b/t/customroles/basic.t
index d703eee..a0696af 100644
--- a/t/customroles/basic.t
+++ b/t/customroles/basic.t
@@ -232,39 +232,6 @@ diag 'role names' if $ENV{'TEST_VERBOSE'};
     my $playground = RT::CustomRole->new(RT->SystemUser);
     ($ok, $msg) = $playground->Create(Name => 'Playground-' . $$, MaxValues => 1);
     ok($ok, "playground role: $msg");
-
-    for my $name (
-        'Programmer-' . $$,
-        'proGRAMMER-' . $$,
-        'Cc',
-        'CC',
-        'AdminCc',
-        'ADMIN CC',
-        'Requestor',
-        'requestors',
-        'Owner',
-        'OWNer',
-    ) {
-        # creating a role with that name should fail
-        my $new = RT::CustomRole->new(RT->SystemUser);
-        ($ok, $msg) = $new->Create(Name => $name, MaxValues => 1);
-        ok(!$ok, "creating a role with duplicate name $name should fail: $msg");
-
-        # updating an existing role with the dupe name should fail too
-        ($ok, $msg) = $playground->SetName($name);
-        ok(!$ok, "updating an existing role with duplicate name $name should fail: $msg");
-        is($playground->Name, 'Playground-' . $$, 'name stayed the same');
-    }
-
-    # make sure we didn't create any new roles
-    my $roles = RT::CustomRoles->new(RT->SystemUser);
-    $roles->UnLimit;
-    is($roles->Count, 3, 'three roles (original two plus playground)');
-
-    is_deeply([sort RT::System->Roles], ['AdminCc', 'Cc', 'Contact', 'HeldBy', 'Owner', 'RT::CustomRole-1', 'RT::CustomRole-2', 'RT::CustomRole-3', 'Requestor'], 'No new System->Roles');
-    is_deeply([sort RT::Queue->Roles], ['AdminCc', 'Cc', 'Owner', 'RT::CustomRole-1', 'RT::CustomRole-2', 'RT::CustomRole-3', 'Requestor'], 'No new Queue->Roles');
-    is_deeply([sort RT::Ticket->Roles], ['AdminCc', 'Cc', 'Owner', 'RT::CustomRole-1', 'RT::CustomRole-2', 'RT::CustomRole-3', 'Requestor'], 'No new Ticket->Roles');
-    is_deeply([sort RT::Queue->ManageableRoleGroupTypes], ['AdminCc', 'Cc', 'RT::CustomRole-2'], 'No new Queue->ManageableRoleGroupTypes');
 }
 
 diag 'load by name and id' if $ENV{'TEST_VERBOSE'};

-----------------------------------------------------------------------


More information about the rt-commit mailing list