[Rt-commit] rt branch, 4.2/group-member-sort-order, created. rt-4.2.10-222-ga5f62fd

Alex Vandiver alexmv at bestpractical.com
Thu Mar 19 15:16:07 EDT 2015


The branch, 4.2/group-member-sort-order has been created
        at  a5f62fd949806162f8a86acbd9f811f9f5f3f8d3 (commit)

- Log -----------------------------------------------------------------
commit 77c5646484088e5fdde7a0dad6a9ccfdaf0f5a47
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Thu Mar 19 12:35:48 2015 -0400

    Stop wrapping ShowUser in <a> tags
    
    This leads to nested <a> tags, only the inner of which is usable; in
    general, explicitly linking to /Admin/User/Modify.html is unnecessary,
    as the ShowUser links to the user summary, which has a tab to the Modify
    page if the user has rights to do so.

diff --git a/share/html/Admin/Groups/Members.html b/share/html/Admin/Groups/Members.html
index a4ff4be..2248a14 100644
--- a/share/html/Admin/Groups/Members.html
+++ b/share/html/Admin/Groups/Members.html
@@ -82,7 +82,7 @@ my @users = sort { lc($a->[0]) cmp lc($b->[0]) }
 % my ($rendered, $user) = @$_;
 % $UsersSeen{ $user->id } = 1 if $SkipSeenUsers;
 <li><input type="checkbox" class="checkbox" name="DeleteMember-<% $user->PrincipalObj->Id %>" value="1" />
-<a href="<% RT->Config->Get('WebPath') %>/Admin/Users/Modify.html?id=<% $user->id %>"><% $rendered |n%></a></li>
+<% $rendered |n%></li>
 % }
 </ul>
 <&|/l&>Groups</&>
diff --git a/share/html/Elements/Crypt/KeyIssues b/share/html/Elements/Crypt/KeyIssues
index 35c1264..1c3ff4a 100644
--- a/share/html/Elements/Crypt/KeyIssues
+++ b/share/html/Elements/Crypt/KeyIssues
@@ -68,7 +68,7 @@
 % foreach my $issue ( @$Issues ) {
 <li>
 % if ( $issue->{'User'} ) {
-User <a href="<% RT->Config->Get('WebPath') %>/Admin/Users/Modify.html?id=<% $issue->{'User'}->id %>"><&/Elements/ShowUser, User => $issue->{'User'} &></a> has a problem.
+User <&/Elements/ShowUser, User => $issue->{'User'} &> has a problem.
 % } else {
 There is a problem with key/certificate(s) for address <% $issue->{'EmailAddress'} %>, but there is no user in the DB for this address.
 % }
diff --git a/share/html/Ticket/Elements/EditWatchers b/share/html/Ticket/Elements/EditWatchers
index 0f613ca..db4a0b3 100644
--- a/share/html/Ticket/Elements/EditWatchers
+++ b/share/html/Ticket/Elements/EditWatchers
@@ -57,14 +57,8 @@
 <li>
 <input type="checkbox" class="checkbox" name="Ticket-DeleteWatcher-Type-<% $Watchers->Name %>-Principal-<% $watcher->MemberId %>" value="1" unchecked />
 % if ( $member->isa( 'RT::User' ) ) { 
-% if ( $session{CurrentUser}->HasRight( Right => 'AdminUsers', Object => $RT::System ) &&
-%      $session{CurrentUser}->HasRight( Right => 'ShowConfigTab', Object =>$RT::System ) ) {
-<a href="<% RT->Config->Get('WebPath') %>/Admin/Users/Modify.html?id=<% $watcher->MemberId %>">
-<& /Elements/ShowUser, User => $member &></a> <& /Elements/ShowUserEmailFrequency, User => $member, Ticket => $TicketObj &>
-% } else {
 <& /Elements/ShowUser, User => $member &> <& /Elements/ShowUserEmailFrequency, User => $member, Ticket => $TicketObj &>
-% } }
-% else {
+% } else {
 % if ( $session{CurrentUser}->HasRight( Right => 'AdminGroup', Object => $RT::System ) &&
 %      $session{CurrentUser}->HasRight( Right => 'ShowConfigTab', Object =>$RT::System ) ) {
 <a href="<% RT->Config->Get('WebPath') %>/Admin/Groups/Modify.html?id=<% $watcher->MemberId %>">
diff --git a/t/web/ticket_modify_people.t b/t/web/ticket_modify_people.t
index cefbf91..f4821a5 100644
--- a/t/web/ticket_modify_people.t
+++ b/t/web/ticket_modify_people.t
@@ -1,7 +1,7 @@
 use strict;
 use warnings;
 
-use RT::Test tests => 25;
+use RT::Test tests => undef;
 
 my $root = RT::Test->load_or_create_user( Name => 'root' );
 my $group_foo = RT::Group->new($RT::SystemUser);
@@ -36,11 +36,11 @@ ok( $m->login( 'user', 'password' ), 'logged in' );
 $m->get_ok( $url . "/Ticket/ModifyPeople.html?id=" . $ticket->id );
 
 ok(
-    !$m->find_link(
-        text      => 'Enoch Root',
-        url_regex => qr!/Admin/Users/Modify\.html!,
+    $m->find_link(
+        text      => 'root (Enoch Root)',
+        url_regex => qr!/User/Summary\.html!,
     ),
-    'no link to modify user'
+    'contains link to user summary page'
 );
 $m->content_contains('Enoch Root', 'still has the user name' );
 
@@ -54,24 +54,6 @@ ok(
 
 $m->content_contains('group_foo', 'still has the group name' );
 
-ok( RT::Test->add_rights( { Principal => $user, Right => ['AdminUsers'] }, ),
-    'added AdminUsers right' );
-$m->reload;
-ok(
-    !$m->find_link(
-        text      => 'Enoch Root',
-        url_regex => qr!/Admin/Users/Modify\.html!,
-    ),
-    'still no link to modify user'
-);
-ok(
-    !$m->find_link(
-        text      => 'group_foo',
-        url_regex => qr!/Admin/Groups/Modify\.html!,
-    ),
-    'still no link to modify group'
-);
-
 ok(
     RT::Test->add_rights( { Principal => $user, Right => ['ShowConfigTab'] }, ),
     'added ShowConfigTab right',
@@ -81,9 +63,9 @@ $m->reload;
 ok(
     $m->find_link(
         text      => 'root (Enoch Root)',
-        url_regex => qr!/Admin/Users/Modify\.html!,
+        url_regex => qr!/User/Summary\.html!,
     ),
-    'got link to modify user'
+    'still contains link to user summary page'
 );
 
 ok(
@@ -119,5 +101,8 @@ $m->submit_form_ok({
 my $foo = RT::Test->load_or_create_user( EmailAddress => 'foo at example.com' );
 is $foo->RealName, "Foo Bar", "RealName matches";
 
+undef $m;
+done_testing;
+
 # TODO test Add|Delete people
 

commit 7eccdee28eedff9151e2b8b6942c396683c2fe97
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Thu Mar 19 12:40:39 2015 -0400

    Sort by text-only representation of the user, not HTML
    
    /Elements/ShowUser always includes a <span data-user-id="..."> element;
    thus, sorting the result leads to sorting ASCII-betically by id -- this
    is unhelpful, to say the least.
    
    Switch to sorting by $user->Format; this is, strictly, not guaranteed to
    be the same as the ShowUser component's text, as the latter has a
    callback that might modify it.  However, it keeps compatibility with
    this callsite respecting said callback, and still wraps in data-user-id
    for metadata (currently unused), while sorting by the presumed display.
    
    The computational overhead to effectively calling ->Format twice per
    user is judged to be small; should that cease being the case, it could
    be cached on the user object.
    
    Fixes: I#30771

diff --git a/share/html/Admin/Groups/Members.html b/share/html/Admin/Groups/Members.html
index 2248a14..7a1f41a 100644
--- a/share/html/Admin/Groups/Members.html
+++ b/share/html/Admin/Groups/Members.html
@@ -74,12 +74,12 @@
 % my $Users = $Group->UserMembersObj( Recursively => 0 );
 <%perl>
 my @users = sort { lc($a->[0]) cmp lc($b->[0]) }
-            map { [$m->scomp("/Elements/ShowUser", User => $_), $_] }
+            map { [$_->Format, $m->scomp( "/Elements/ShowUser", User => $_), $_] }
             @{ $Users->ItemsArrayRef };
 </%perl>
 <ul>
 % for (@users) {
-% my ($rendered, $user) = @$_;
+% my (undef, $rendered, $user) = @{$_};
 % $UsersSeen{ $user->id } = 1 if $SkipSeenUsers;
 <li><input type="checkbox" class="checkbox" name="DeleteMember-<% $user->PrincipalObj->Id %>" value="1" />
 <% $rendered |n%></li>

commit a5f62fd949806162f8a86acbd9f811f9f5f3f8d3
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Thu Mar 19 12:51:00 2015 -0400

    Stop pre-computing ShowUser
    
    There is no longer a reason to look up ShowUser prior to rendering; this
    turns the array into a simple Schwartzian transform.

diff --git a/share/html/Admin/Groups/Members.html b/share/html/Admin/Groups/Members.html
index 7a1f41a..91da6f1 100644
--- a/share/html/Admin/Groups/Members.html
+++ b/share/html/Admin/Groups/Members.html
@@ -73,16 +73,16 @@
 <&|/l&>Users</&>
 % my $Users = $Group->UserMembersObj( Recursively => 0 );
 <%perl>
-my @users = sort { lc($a->[0]) cmp lc($b->[0]) }
-            map { [$_->Format, $m->scomp( "/Elements/ShowUser", User => $_), $_] }
+my @users = map {$_->[1]}
+            sort { lc($a->[0]) cmp lc($b->[0]) }
+            map { [$_->Format, $_] }
             @{ $Users->ItemsArrayRef };
 </%perl>
 <ul>
-% for (@users) {
-% my (undef, $rendered, $user) = @{$_};
+% for my $user (@users) {
 % $UsersSeen{ $user->id } = 1 if $SkipSeenUsers;
 <li><input type="checkbox" class="checkbox" name="DeleteMember-<% $user->PrincipalObj->Id %>" value="1" />
-<% $rendered |n%></li>
+<& /Elements/ShowUser, User => $user &></li>
 % }
 </ul>
 <&|/l&>Groups</&>

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


More information about the rt-commit mailing list