[Rt-commit] rt branch 5.0/rest2-custom-roles-keys created. rt-5.0.3-294-g6fbdd2bcb3

BPS Git Server git at git.bestpractical.com
Fri Mar 10 20:40:34 UTC 2023


This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "rt".

The branch, 5.0/rest2-custom-roles-keys has been created
        at  6fbdd2bcb30719d69a5d57cbe893b83879b764e3 (commit)

- Log -----------------------------------------------------------------
commit 6fbdd2bcb30719d69a5d57cbe893b83879b764e3
Author: sunnavy <sunnavy at bestpractical.com>
Date:   Fri Mar 10 04:50:37 2023 +0800

    Add upgrading note for the change of custom role keys in REST2 ticket endpoints

diff --git a/docs/UPGRADING-5.0 b/docs/UPGRADING-5.0
index f99cc20bac..501c9e6831 100644
--- a/docs/UPGRADING-5.0
+++ b/docs/UPGRADING-5.0
@@ -525,6 +525,55 @@ as intended. However, if you were using this callback for other
 reasons, you may need to update your code to use the C<BeforeSessionDelete>
 callback instead.
 
+=item * Custom role keys in REST2 ticket endpoints changed
+
+We update custom role keys from "GroupType" like "RT::CustomRole-1" to
+"Name" in REST2 ticket endpoints, to be consistent with core roles.
+We also add "CustomRoles" entry there to cover all custom roles, like
+"CustomFields".
+
+E.g. the GET response of C<XX_RT_URL_XX/REST/2.0/ticket/ID> changes
+from
+
+    {
+        ...
+        "RT::CustomRole-1" : [
+           {
+              "id" : "root",
+              "type" : "user",
+              "_url" : "XX_RT_URL_XX/REST/2.0/user/root"
+           }
+        ],
+        ...
+    }
+
+to
+
+    {
+        ...
+        "Manager" : [
+           {
+              "id" : "root",
+              "type" : "user",
+              "_url" : "XX_RT_URL_XX/REST/2.0/user/root"
+           }
+        ],
+        "CustomRoles" : {
+           "Manager" : [
+              {
+                 "id" : "root",
+                 "type" : "user",
+                 "_url" : "XX_RT_URL_XX/REST/2.0/user/root"
+              }
+           ]
+        },
+        ...
+    }
+
+We suggest use "CustomRoles" because individual top level custom role
+entries like the "Manager" following "..." in above example will be
+removed in the future.
+
 =back
 
 =cut

commit 5fe728a8935d59164f47df2640d726ff5083df41
Author: sunnavy <sunnavy at bestpractical.com>
Date:   Wed May 18 20:43:18 2022 +0800

    Update REST2 doc to prefer "CustomRoles" top key for ticket search
    
    Fields like "RT::CustomRole-1" is still supported for back
    compatibility, but using "CustomRoles" is more pragmatic, and also
    consistent with other examples that create/update tickets.

diff --git a/lib/RT/REST2.pm b/lib/RT/REST2.pm
index b0b8512527..d611debaf2 100644
--- a/lib/RT/REST2.pm
+++ b/lib/RT/REST2.pm
@@ -1305,7 +1305,7 @@ You can use additional fields parameters to expand child blocks, for
 example (line wrapping inserted for readability):
 
     XX_RT_URL_XX/REST/2.0/tickets
-      ?fields=Owner,Status,Created,Subject,Queue,CustomFields,Requestor,Cc,AdminCc,RT::CustomRole-1
+      ?fields=Owner,Status,Created,Subject,Queue,CustomFields,Requestor,Cc,AdminCc,CustomRoles
       &fields[Queue]=Name,Description
 
 Says that in the result set for tickets, the extra fields for Owner, Status,
@@ -1360,13 +1360,13 @@ ticket is displayed in this example):
             }
          ],
          "AdminCc" : [],
-         "RT::CustomRole-1" : [
-            {
+         "CustomRoles" : {
+            "Manager": {
                "_url" : "XX_RT_URL_XX/REST/2.0/user/foo at example.com",
                "type" : "user",
                "id" : "foo at example.com"
             }
-         ]
+         }
       }
       { … },
       …

commit 8af3d0f8956492036c44e7745e80d93475618398
Author: sunnavy <sunnavy at bestpractical.com>
Date:   Wed May 18 20:26:03 2022 +0800

    Update tests for the keys change of CustomRoles in REST2

diff --git a/t/rest2/ticket-correspond-customroles.t b/t/rest2/ticket-correspond-customroles.t
index f52cb5a947..c629c97a74 100644
--- a/t/rest2/ticket-correspond-customroles.t
+++ b/t/rest2/ticket-correspond-customroles.t
@@ -71,10 +71,12 @@ my ($ticket_url, $ticket_id);
     is($content->{Status}, 'new');
     is($content->{Subject}, 'Ticket creation using REST');
 
-    ok(exists $content->{$_}, "Content exists for $_") for qw(AdminCc TimeEstimated Started Cc
-                                     LastUpdated TimeWorked Resolved
-                                     RT::CustomRole-1 RT::CustomRole-2
-                                     Created Due Priority EffectiveId);
+    ok( exists $content->{$_}, "Content exists for $_" ) for qw(AdminCc TimeEstimated Started Cc
+        LastUpdated TimeWorked Resolved Created Due Priority EffectiveId CustomRoles);
+
+    # Remove this in RT 5.2
+    ok( exists $content->{$_}, "Content exists for $_" ) for 'Single Member', 'Multi Member';
+
 }
 
 diag "Correspond with custom roles";
diff --git a/t/rest2/ticket-customroles.t b/t/rest2/ticket-customroles.t
index 8831c6869f..c7aeab547a 100644
--- a/t/rest2/ticket-customroles.t
+++ b/t/rest2/ticket-customroles.t
@@ -58,8 +58,8 @@ $user->PrincipalObj->GrantRight( Right => $_ )
     is($res->code, 200);
 
     my $content = $mech->json_response;
-    cmp_deeply($content->{$multi->GroupType}, [], 'no Multi Member');
-    cmp_deeply($content->{$single->GroupType}, {
+    cmp_deeply($content->{$multi->Name}, [], 'no Multi Member');
+    cmp_deeply($content->{$single->Name}, {
         type => 'user',
         id   => 'Nobody',
         _url => re(qr{$rest_base_path/user/Nobody$}),
@@ -86,7 +86,7 @@ $user->PrincipalObj->GrantRight( Right => $_ )
     my ($single_url) = map { $_->{_url} } grep { $_->{ref} eq 'customrole' && $_->{id} == $single_id } @{ $content->{'_hyperlinks'} };
     my ($multi_url) = map { $_->{_url} } grep { $_->{ref} eq 'customrole' && $_->{id} == $multi_id } @{ $content->{'_hyperlinks'} };
 
-    $res = $mech->get($content->{$single->GroupType}{_url},
+    $res = $mech->get($content->{$single->Name}{_url},
         'Authorization' => $auth,
     );
     is($res->code, 200);
@@ -142,13 +142,13 @@ $user->PrincipalObj->GrantRight( Right => $_ )
     is($res->code, 200);
 
     my $content = $mech->json_response;
-    cmp_deeply($content->{$multi->GroupType}, [{
+    cmp_deeply($content->{$multi->Name}, [{
         type => 'user',
         id   => 'multi at example.com',
         _url => re(qr{$rest_base_path/user/multi\@example\.com$}),
     }], 'one Multi Member');
 
-    cmp_deeply($content->{$single->GroupType}, {
+    cmp_deeply($content->{$single->Name}, {
         type => 'user',
         id   => 'test at localhost',
         _url => re(qr{$rest_base_path/user/test\@localhost$}),
@@ -178,7 +178,7 @@ $user->PrincipalObj->GrantRight( Right => $_ )
     is($res->code, 200);
 
     my $content = $mech->json_response;
-    cmp_deeply($content->{$multi->GroupType}, [{
+    cmp_deeply($content->{$multi->Name}, [{
         type => 'user',
         id   => 'multi at example.com',
         _url => re(qr{$rest_base_path/user/multi\@example\.com$}),
@@ -188,7 +188,7 @@ $user->PrincipalObj->GrantRight( Right => $_ )
         _url => re(qr{$rest_base_path/user/multi2\@example\.com$}),
     }], 'two Multi Members');
 
-    cmp_deeply($content->{$single->GroupType}, {
+    cmp_deeply($content->{$single->Name}, {
         type => 'user',
         id   => 'test at localhost',
         _url => re(qr{$rest_base_path/user/test\@localhost}),
@@ -218,7 +218,7 @@ diag 'Create and view ticket with custom roles by name';
     is($res->code, 200);
 
     my $content = $mech->json_response;
-    cmp_deeply($content->{$multi->GroupType}, [{
+    cmp_deeply($content->{$multi->Name}, [{
         type => 'user',
         id   => 'multi at example.com',
         _url => re(qr{$rest_base_path/user/multi\@example\.com$}),
@@ -228,7 +228,7 @@ diag 'Create and view ticket with custom roles by name';
         _url => re(qr{$rest_base_path/user/multi2\@example\.com$}),
     }], 'two Multi Members');
 
-    cmp_deeply($content->{$single->GroupType}, {
+    cmp_deeply($content->{$single->Name}, {
         type => 'user',
         id   => 'test at localhost',
         _url => re(qr{$rest_base_path/user/test\@localhost}),
@@ -255,7 +255,7 @@ diag 'Create and view ticket with custom roles by name';
     );
     is($res->code, 200);
 
-    cmp_deeply($mech->json_response->{$single->GroupType}, {
+    cmp_deeply($mech->json_response->{$single->Name}, {
         type => 'user',
         id   => 'Nobody',
         _url => re(qr{$rest_base_path/user/Nobody$}),
@@ -277,7 +277,7 @@ diag 'Create and view ticket with custom roles by name';
         );
         is($res->code, 200);
 
-        cmp_deeply($mech->json_response->{$single->GroupType}, {
+        cmp_deeply($mech->json_response->{$single->Name}, {
             type => 'user',
             id   => 'test',
             _url => re(qr{$rest_base_path/user/test$}),
@@ -298,7 +298,7 @@ diag 'Create and view ticket with custom roles by name';
         );
         is($res->code, 200);
 
-        cmp_deeply($mech->json_response->{$single->GroupType}, {
+        cmp_deeply($mech->json_response->{$single->Name}, {
             type => 'user',
             id   => 'Nobody',
             _url => re(qr{$rest_base_path/user/Nobody$}),
@@ -321,7 +321,7 @@ diag 'Create and view ticket with custom roles by name';
         );
         is($res->code, 200);
 
-        cmp_deeply($mech->json_response->{$single->GroupType}, {
+        cmp_deeply($mech->json_response->{$single->Name}, {
             type => 'user',
             id   => 'test',
             _url => re(qr{$rest_base_path/user/test$}),
@@ -342,7 +342,7 @@ diag 'Create and view ticket with custom roles by name';
         );
         is($res->code, 200);
 
-        cmp_deeply($mech->json_response->{$single->GroupType}, {
+        cmp_deeply($mech->json_response->{$single->Name}, {
             type => 'user',
             id   => 'Nobody',
             _url => re(qr{$rest_base_path/user/Nobody$}),
@@ -371,7 +371,7 @@ diag 'Create and view ticket with custom roles by name';
     is($res->code, 200);
 
     my $content = $mech->json_response;
-    cmp_deeply($content->{$multi->GroupType}, [], 'no Multi Member');
+    cmp_deeply($content->{$multi->Name}, [], 'no Multi Member');
 
     $payload = {
         $multi->GroupType => 'multi at example.com',
@@ -388,7 +388,7 @@ diag 'Create and view ticket with custom roles by name';
     );
     is($res->code, 200);
     $content = $mech->json_response;
-    cmp_deeply($content->{$multi->GroupType}, [{
+    cmp_deeply($content->{$multi->Name}, [{
         type => 'user',
         id   => 'multi at example.com',
         _url => re(qr{$rest_base_path/user/multi\@example\.com$}),
@@ -409,7 +409,7 @@ diag 'Create and view ticket with custom roles by name';
     );
     is($res->code, 200);
     $content = $mech->json_response;
-    cmp_deeply($content->{$multi->GroupType}, [{
+    cmp_deeply($content->{$multi->Name}, [{
         type => 'user',
         id   => 'multi2 at example.com',
         _url => re(qr{$rest_base_path/user/multi2\@example\.com$}),
@@ -430,7 +430,7 @@ diag 'Create and view ticket with custom roles by name';
     );
     is($res->code, 200);
     $content = $mech->json_response;
-    cmp_deeply($content->{$multi->GroupType}, bag({
+    cmp_deeply($content->{$multi->Name}, bag({
         type => 'user',
         id   => 'multi2 at example.com',
         _url => re(qr{$rest_base_path/user/multi2\@example\.com$}),
@@ -478,7 +478,7 @@ diag 'Create and view ticket with custom roles by name';
         );
         is($res->code, 200);
         $content = $mech->json_response;
-        cmp_deeply($content->{$multi->GroupType}, bag({
+        cmp_deeply($content->{$multi->Name}, bag({
             type => 'user',
             id   => 'multi2 at example.com',
             _url => re(qr{$rest_base_path/user/multi2\@example\.com$}),
@@ -502,7 +502,7 @@ diag 'Create and view ticket with custom roles by name';
     is( $res->code, 200 );
     $content = $mech->json_response;
     cmp_deeply(
-        $content->{ $multi->GroupType },
+        $content->{ $multi->Name },
         bag({   type => 'user',
                 id   => 'test at localhost',
                 _url => re(qr{$rest_base_path/user/test\@localhost$}),
@@ -549,7 +549,7 @@ diag 'Create and view ticket with custom roles by name';
     is($res->code, 200);
 
     my $content = $mech->json_response;
-    cmp_deeply($content->{$multi->GroupType}, [{
+    cmp_deeply($content->{$multi->Name}, [{
         id   => $group_id,
         type => 'group',
         _url => re(qr{$rest_base_path/group/$group_id$}),
@@ -570,7 +570,7 @@ diag 'Create and view ticket with custom roles by name';
     );
     is($res->code, 200);
     $content = $mech->json_response;
-    cmp_deeply($content->{$multi->GroupType}, [{
+    cmp_deeply($content->{$multi->Name}, [{
         type => 'user',
         id   => 'multi at example.com',
         _url => re(qr{$rest_base_path/user/multi\@example\.com$}),
@@ -591,7 +591,7 @@ diag 'Create and view ticket with custom roles by name';
     );
     is($res->code, 200);
     $content = $mech->json_response;
-    cmp_deeply($content->{$multi->GroupType}, [{
+    cmp_deeply($content->{$multi->Name}, [{
         type => 'user',
         id   => 'multi at example.com',
         _url => re(qr{$rest_base_path/user/multi\@example\.com$}),
@@ -602,7 +602,7 @@ diag 'Create and view ticket with custom roles by name';
         _url => re(qr{$rest_base_path/group/$group_id$}),
     }], 'Multi Member user and group');
 
-    $res = $mech->get($content->{$multi->GroupType}[1]{_url},
+    $res = $mech->get($content->{$multi->Name}[1]{_url},
         'Authorization' => $auth,
     );
     is($res->code, 200);
@@ -660,8 +660,8 @@ diag 'Create and view ticket with custom roles by name';
     is($res->code, 200);
 
     my $content = $mech->json_response;
-    cmp_deeply($content->{$later_multi->GroupType}, [], 'no Later Multi Member');
-    cmp_deeply($content->{$later_single->GroupType}, {
+    cmp_deeply($content->{$later_multi->Name}, [], 'no Later Multi Member');
+    cmp_deeply($content->{$later_single->Name}, {
         type => 'user',
         id   => 'Nobody',
         _url => re(qr{$rest_base_path/user/Nobody$}),
@@ -704,9 +704,79 @@ diag 'Create and view ticket with custom roles by name';
     is( scalar @{ $content->{items} }, 1 );
 
     $ticket = $content->{items}->[0];
-    is( $ticket->{CustomRoles}{ $single->GroupType }{id}, 'single2 at example.com',  'Single Member id in search result' );
-    is( $ticket->{CustomRoles}{ $multi->GroupType }[0]{id}, 'multi at example.com',  'Multi Member id in search result' );
-    is( $ticket->{CustomRoles}{ $multi->GroupType }[1]{id}, 'multi2 at example.com', 'Multi Member id in search result' );
+    is( $ticket->{CustomRoles}{ $single->Name }{id}, 'single2 at example.com',  'Single Member id in search result' );
+    is( $ticket->{CustomRoles}{ $multi->Name }[0]{id}, 'multi at example.com',  'Multi Member id in search result' );
+    is( $ticket->{CustomRoles}{ $multi->Name }[1]{id}, 'multi2 at example.com', 'Multi Member id in search result' );
+}
+
+diag 'Test custom role name conflicts';
+{
+    my $creator = RT::CustomRole->new( RT->SystemUser );
+    ( $ok, $msg ) = $creator->Create( Name => 'Creator' );
+    ok( $ok, $msg );
+    my $creator_id = $creator->Id;
+
+    ( $ok, $msg ) = $creator->AddToObject( $queue->id );
+    ok( $ok, $msg );
+
+    my $payload = {
+        Subject     => 'Ticket with creator watchers',
+        Queue       => 'General',
+        CustomRoles => { Creator => [ 'multi at example.com', 'multi2 at example.com' ] },
+    };
+
+    my $res = $mech->post_json( "$rest_base_path/ticket", $payload, 'Authorization' => $auth, );
+    is( $res->code, 201 );
+    ok( my $ticket_url = $res->header('location') );
+    ok( ( my $ticket_id ) = $ticket_url =~ qr[/ticket/(\d+)] );
+
+    my @warnings;
+    local $SIG{__WARN__} = sub {
+        push @warnings, @_;
+    };
+
+    $res = $mech->get( $ticket_url, 'Authorization' => $auth, );
+    is( $res->code, 200 );
+    my $content = $mech->json_response;
+    cmp_deeply(
+        $content->{ 'CustomRole.{' . $creator->Name . '}' },
+        [
+            {
+                type => 'user',
+                id   => 'multi at example.com',
+                _url => re(qr{$rest_base_path/user/multi\@example\.com$}),
+            },
+            {
+                type => 'user',
+                id   => 'multi2 at example.com',
+                _url => re(qr{$rest_base_path/user/multi2\@example\.com$}),
+            }
+        ],
+        'two Creator Members'
+    );
+    cmp_deeply(
+        $content->{Creator},
+        {
+            type => 'user',
+            id   => 'test',
+            _url => re(qr{$rest_base_path/user/test$}),
+        },
+        'two Multi Members'
+    );
+    is( scalar @warnings, 1, 'Got one warning' );
+    like(
+        $warnings[0],
+        qr/\QCustomRole Creator conflicts with core field Creator, renaming its key to CustomRole.{Creator}\E/,
+        'Got the name conflict warning'
+    );
+
+    is_deeply(
+        $content->{_comments},
+        [
+            'Top level individual custom role keys are deprecated and will be removed in RT 5.2. Please use "CustomRoles" instead.'
+        ],
+        'Got the deprecated comment'
+    );
 }
 
 done_testing;

commit b10c70991fe3e257badc9305e2af7747b7c9aed6
Author: sunnavy <sunnavy at bestpractical.com>
Date:   Tue May 17 23:36:37 2022 +0800

    Use custom role names as keys for ticket endpoints in REST2
    
    Previously we used the GroupType(e.g. "RT::CustomRole-2"), which didn't
    contain useful info compared to Name. This commit changes it to Name to
    be more consistent with core roles. It also adds a separate
    "CustomRoles" top key, just like "CustomFields".
    
    As there is a "CustomRoles" top key, we can deprecate the usage of
    individual top level custom role keys, considering it could conflict
    with other fields(e.g. custom role "Creator" VS core field "Creator").
    If there is a collsion, the key for custom role is automatically
    renamed for now, e.g. from "Creator" to "CustomRole.{Creator}".

diff --git a/lib/RT/REST2/Resource.pm b/lib/RT/REST2/Resource.pm
index ddca9571dd..d081547d5f 100644
--- a/lib/RT/REST2/Resource.pm
+++ b/lib/RT/REST2/Resource.pm
@@ -116,23 +116,34 @@ sub expand_field {
         if ( $item->DOES("RT::Record::Role::Roles") ) {
             my %data;
             for my $role ( $item->Roles( ACLOnly => 0 ) ) {
-                next unless $role =~ /^RT::CustomRole-/;
-                $data{$role} = [];
+                next unless $role =~ /^RT::CustomRole-(\d+)$/;
+                my $role_id = $1;
+                my $role_object = RT::CustomRole->new( $item->CurrentUser );
+                $role_object->Load($role_id);
+
+                if ( !$role_object->Id ) {
+                    RT->Logger->warning("Couldn't load custom role $role_id");
+                    next;
+                }
+
+                my $role_name = $role_object->Name;
+
+                $data{$role_name} = [];
 
                 my $group = $item->RoleGroup($role);
                 if ( !$group->Id ) {
-                    $data{$role} = $self->_expand_object( RT->Nobody->UserObj, $field, $param_prefix )
+                    $data{$role_name} = $self->_expand_object( RT->Nobody->UserObj, $field, $param_prefix )
                         if $item->_ROLES->{$role}{Single};
                     next;
                 }
 
                 my $gms = $group->MembersObj;
                 while ( my $gm = $gms->Next ) {
-                    push @{ $data{$role} }, $self->_expand_object( $gm->MemberObj->Object, $field, $param_prefix );
+                    push @{ $data{$role_name} }, $self->_expand_object( $gm->MemberObj->Object, $field, $param_prefix );
                 }
 
                 # Avoid the extra array ref for single member roles
-                $data{$role} = shift @{$data{$role}} if $group->SingleMemberRoleGroup;
+                $data{$role_name} = shift @{$data{$role_name}} if $group->SingleMemberRoleGroup;
             }
             return \%data;
         }
diff --git a/lib/RT/REST2/Util.pm b/lib/RT/REST2/Util.pm
index e704b661c2..8ae91a7552 100644
--- a/lib/RT/REST2/Util.pm
+++ b/lib/RT/REST2/Util.pm
@@ -164,11 +164,33 @@ sub serialize_record {
 
     # Include role members, if applicable
     if ($record->DOES("RT::Record::Role::Roles")) {
+        my %custom_role;
         for my $role ($record->Roles(ACLOnly => 0)) {
-            my $members = $data{$role} = [];
+            my $role_name;
+            if ( $role =~ /^RT::CustomRole-(\d+)$/ ) {
+                my $role_id = $1;
+                my $role_object = RT::CustomRole->new( $record->CurrentUser );
+                $role_object->Load($role_id);
+                next unless $role_object->Id;
+                $role_name = $role_object->Name;
+                $custom_role{$role_name} = 1;
+
+                if ( $record->_Accessible( $role_name => 'read' ) ) {
+                    RT->Logger->warning(
+                        "CustomRole $role_name conflicts with core field $role_name, renaming its key to CustomRole.{$role_name}"
+                    );
+                    $role_name = "CustomRole.{$role_name}";
+                }
+            }
+            else {
+                # Core role
+                $role_name = $role;
+            }
+
+            my $members = $data{$role_name} = [];
             my $group = $record->RoleGroup($role);
             if ( !$group->Id ) {
-                $data{$role} = expand_uid( RT->Nobody->UserObj->UID ) if $record->_ROLES->{$role}{Single};
+                $data{$role_name} = expand_uid( RT->Nobody->UserObj->UID ) if $record->_ROLES->{$role}{Single};
                 next;
             }
 
@@ -178,9 +200,24 @@ sub serialize_record {
             }
 
             # Avoid the extra array ref for single member roles
-            $data{$role} = shift @$members
+            $data{$role_name} = shift @$members
                 if $group->SingleMemberRoleGroup;
         }
+
+        if (%custom_role) {
+            $data{CustomRoles} = { map { $_ => $data{$_} || $data{"CustomRole.{$_}"} } keys %custom_role };
+            push @{ $data{_comments} },
+                $record->loc(
+                'Top level individual custom role keys are deprecated and will be removed in RT 5.2. Please use "CustomRoles" instead.'
+                );
+            # Does not actually trigger deprecated warnings because of the localization of RT::Deprecated above.
+            # This is to help developers clean up outdated code on new releases.
+            RT->Deprecated(
+                Message => 'Top level individual custom role keys are deprecated',
+                Instead => 'CustomRoles',
+                Remove  => '5.2',
+            );
+        }
     }
 
     if (my $cfs = custom_fields_for($record)) {

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


hooks/post-receive
-- 
rt


More information about the rt-commit mailing list