[Rt-commit] rt branch, 4.4/lazy-role-groups, created. rt-4.4.4-107-g0f51e463be

? sunnavy sunnavy at bestpractical.com
Mon Jun 1 16:53:12 EDT 2020


The branch, 4.4/lazy-role-groups has been created
        at  0f51e463be032b6001bd917a4d13ade80ebb8571 (commit)

- Log -----------------------------------------------------------------
commit b2aa7a96f2465557be88f8f856630653428718e3
Author: sunnavy <sunnavy at bestpractical.com>
Date:   Tue Jun 2 03:26:33 2020 +0800

    Support to create ticket/asset role groups lazily
    
    Since role groups may not exist, we need to use left join in searches.

diff --git a/lib/RT/Asset.pm b/lib/RT/Asset.pm
index d795759913..320eaaae53 100644
--- a/lib/RT/Asset.pm
+++ b/lib/RT/Asset.pm
@@ -192,6 +192,10 @@ by ID.
 Any of these link types accept either a single value or arrayref of values
 parseable by L<RT::URI>.
 
+=item LazyRoleGroups
+
+A boolean to control if to create role groups immediately or just when necessary
+
 =back
 
 Returns a tuple of (status, msg) on failure and (id, msg, non-fatal errors) on
@@ -212,6 +216,7 @@ sub Create {
         Contact         => undef,
 
         Status          => undef,
+        LazyRoleGroups  => 1,
         @_
     );
     my @non_fatal_errors;
@@ -270,11 +275,13 @@ sub Create {
     # Let users who just created an asset see it until the end of this method.
     $self->{_object_is_readable} = 1;
 
-    # Create role groups
-    unless ($self->_CreateRoleGroups()) {
-        RT->Logger->error("Couldn't create role groups for asset ". $self->id);
-        RT->DatabaseHandle->Rollback();
-        return (0, $self->loc("Couldn't create role groups for asset"));
+    if ( !$args{LazyRoleGroups} ) {
+        # Create role groups
+        unless ($self->_CreateRoleGroups()) {
+            RT->Logger->error("Couldn't create role groups for asset ". $self->id);
+            RT->DatabaseHandle->Rollback();
+            return (0, $self->loc("Couldn't create role groups for asset"));
+        }
     }
 
     # Figure out users for roles
diff --git a/lib/RT/Record/Role/Roles.pm b/lib/RT/Record/Role/Roles.pm
index b973eb21b1..ff5d5bf37f 100644
--- a/lib/RT/Record/Role/Roles.pm
+++ b/lib/RT/Record/Role/Roles.pm
@@ -692,7 +692,17 @@ sub _AddRolesOnCreate {
         my $changed = 0;
 
         for my $role (keys %{$roles}) {
+            next unless @{$roles->{$role}};
+
             my $group = $self->RoleGroup($role);
+            if ( !$group->id ) {
+                $group = $self->_CreateRoleGroup($role);
+                if ( !$group || !$group->id ) {
+                    push @errors, $self->loc( "Couldn't create role group '[_1]'", $role );
+                    next;
+                }
+            }
+
             my @left;
             for my $principal (@{$roles->{$role}}) {
                 if ($acls{$role}->($principal)) {
diff --git a/lib/RT/SearchBuilder/Role/Roles.pm b/lib/RT/SearchBuilder/Role/Roles.pm
index 5980882d31..703b2c979d 100644
--- a/lib/RT/SearchBuilder/Role/Roles.pm
+++ b/lib/RT/SearchBuilder/Role/Roles.pm
@@ -360,7 +360,7 @@ sub RoleLimit {
         # positive condition case
 
         $group_members ||= $self->_GroupMembersJoin(
-            GroupsAlias => $groups, New => 1, Left => 0
+            GroupsAlias => $groups, New => 1,
         );
         if ($args{FIELD} eq "id") {
             # Save a left join to Users, if possible
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 81224831cc..dce57c609e 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -192,6 +192,7 @@ Arguments: ARGS is a hash of named parameters.  Valid parameters are:
   Due -- an ISO date describing the ticket's due date and time in GMT
   MIMEObj -- a MIME::Entity object with the content of the initial ticket request.
   CustomField-<n> -- a scalar or array of values for the customfield with the id <n>
+  LazyRoleGroups -- a boolean to control if to create role groups immediately or just when necessary
 
 Ticket links can be set up during create by passing the link type as a hask key and
 the ticket id to be linked to as a value (or a URI when linking to other objects).
@@ -239,6 +240,7 @@ sub Create {
         SLA                => undef,
         MIMEObj            => undef,
         _RecordTransaction => 1,
+        LazyRoleGroups     => 1,
         @_
     );
 
@@ -439,15 +441,17 @@ sub Create {
     }
 
     # Create (empty) role groups
-    my $create_groups_ret = $self->_CreateRoleGroups();
-    unless ($create_groups_ret) {
-        $RT::Logger->crit( "Couldn't create ticket groups for ticket "
-              . $self->Id
-              . ". aborting Ticket creation." );
-        $RT::Handle->Rollback();
-        return ( 0, 0,
-            $self->loc("Ticket could not be created due to an internal error")
-        );
+    if ( !$args{LazyRoleGroups} ) {
+        my $create_groups_ret = $self->_CreateRoleGroups();
+        unless ($create_groups_ret) {
+            $RT::Logger->crit( "Couldn't create ticket groups for ticket "
+                  . $self->Id
+                  . ". aborting Ticket creation." );
+            $RT::Handle->Rollback();
+            return ( 0, 0,
+                $self->loc("Ticket could not be created due to an internal error")
+            );
+        }
     }
 
     # Codify what it takes to add each kind of group

commit 1c9c2d1bcca16b80941e029293b495776b156bb0
Author: sunnavy <sunnavy at bestpractical.com>
Date:   Tue Jun 2 03:32:58 2020 +0800

    Default PrincipalId to 0 to avoid SQL error for not-existing role groups
    
    The RoleGroup method returns an empty group object when current user
    doesn't have right or the corresponding role group doesn't exist. This
    could result in SQL syntax errors when calling methods like MembersObj:
    
      SELECT main.* FROM GroupMembers main  WHERE (main.GroupId = )  ORDER BY main.id ASC
    
    Defaulting PrincipalId to 0 could avoid SQL errors like above and return
    correct empty results.

diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index 19a41ef7a9..350fcaee2e 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -1295,7 +1295,7 @@ Returns this user's PrincipalId
 
 sub PrincipalId {
     my $self = shift;
-    return $self->Id;
+    return $self->Id || 0;
 }
 
 sub InstanceObj {

commit 0f51e463be032b6001bd917a4d13ade80ebb8571
Author: sunnavy <sunnavy at bestpractical.com>
Date:   Tue Jun 2 03:38:16 2020 +0800

    Update tests for the lazy role groups change

diff --git a/t/api/ticket.t b/t/api/ticket.t
index cfba3e97bf..fbd18b1a16 100644
--- a/t/api/ticket.t
+++ b/t/api/ticket.t
@@ -112,7 +112,8 @@ my ($id, $msg) = $ticket->Create(Subject => "Foo",
                 Owner => RT->SystemUser->Id,
                 Status => 'open',
                 Requestor => ['jesse at example.com'],
-                Queue => '1'
+                Queue => '1',
+                LazyRoleGroups => 0,
                 );
 ok ($id, "Ticket $id was created");
 ok(my $group = $ticket->RoleGroup('Requestor'));
@@ -343,4 +344,23 @@ diag("Test ticket types with different cases");
     is($t->Type, "approval", "Approvals, the third and final internal type, are also lc'd during Create");
 }
 
+diag("Test LazyRoleGroups");
+{
+    my $t = RT::Ticket->new( RT->SystemUser );
+    my ($ok) = $t->Create(
+        Queue     => 'General',
+        Subject   => 'LazyRoleGroup test',
+        Requestor => 'root at localhost',
+    );
+    ok( $t->RoleGroup($_)->Id, "$_ role group is created" ) for qw/Requestor Owner/;
+
+    for my $role (qw/Cc AdminCc/) {
+        ok( !$t->RoleGroup($role)->Id, "$role role group is not created yet" );
+        ok( $t->AddWatcher( Type => $role, Email => 'root at localhost' ) );
+        my $role_group = $t->RoleGroup($role);
+        ok( $role_group->Id,            "$role role group is created" );
+        is( $role_group->MemberEmailAddressesAsString, 'root at localhost', "$role role group has the member root" );
+    }
+}
+
 done_testing;
diff --git a/t/assets/roles.t b/t/assets/roles.t
index 1d8a647aa3..b6baa6cd65 100644
--- a/t/assets/roles.t
+++ b/t/assets/roles.t
@@ -4,7 +4,7 @@ use warnings;
 use RT::Test::Assets tests => undef;
 
 my $catalog = create_catalog( Name => "A catalog" );
-my $asset = create_asset( Name => "Test asset", Catalog => $catalog->id );
+my $asset = create_asset( Name => "Test asset", Catalog => $catalog->id, LazyRoleGroups => 0 );
 ok $asset && $asset->id, "Created asset";
 
 for my $object ($asset, $catalog, RT->System) {
diff --git a/t/web/download_user_info.t b/t/web/download_user_info.t
index 44155d49a7..bf41a899ba 100644
--- a/t/web/download_user_info.t
+++ b/t/web/download_user_info.t
@@ -62,9 +62,9 @@ EOF
 
     my $transaction_info_tsv = <<EOF;
 Ticket Id\tid\tCreated\tDescription\tOldValue\tNewValue\tContent
-1\t30\t$date_created\tTicket created\t\t\tThis transaction appears to have no content
-1\t32\t$date_commented\tComments added\t\t\tTest - Comment
-1\t33\t$date_correspondence\tCorrespondence added\t\t\tTest - Reply
+1\t28\t$date_created\tTicket created\t\t\tThis transaction appears to have no content
+1\t30\t$date_commented\tComments added\t\t\tTest - Comment
+1\t31\t$date_correspondence\tCorrespondence added\t\t\tTest - Reply
 EOF
 
     is $agent->content, $transaction_info_tsv,

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


More information about the rt-commit mailing list