[Rt-commit] rt branch, unique-group-names, created. rt-3.9.4-530-g84b68b3

Thomas Sibley trs at bestpractical.com
Thu Nov 18 20:48:25 EST 2010


The branch, unique-group-names has been created
        at  84b68b3784bddb2d30fe342e86308d6e570eed0d (commit)

- Log -----------------------------------------------------------------
commit 90da0c0bff03450eedb1f55edfac8dfa6872a16f
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Thu Nov 18 19:54:26 2010 -0500

    Enforce unique group names for user defined groups
    
    Existing non-unique groups will continue to function, but it is now not
    possible through the UI or API to create another non-unique group or
    rename an existing group to an already taken name.
    
    The Pg and SQLite schemas have required this for 7-8 years, but the
    MySQL and Oracle schemas never have.  A tool to report non-unique group
    names will appear in the near future.

diff --git a/lib/RT/Group_Overlay.pm b/lib/RT/Group_Overlay.pm
index 686f455..3250b63 100755
--- a/lib/RT/Group_Overlay.pm
+++ b/lib/RT/Group_Overlay.pm
@@ -413,6 +413,12 @@ sub _Create {
         @_
     );
 
+    # Enforce uniqueness on user defined group names
+    if ($args{'Domain'} and $args{'Domain'} eq 'UserDefined') {
+        my ($ok, $msg) = $self->_ValidateUserDefinedName($args{'Name'});
+        return ($ok, $msg) if not $ok;
+    }
+
     $RT::Handle->BeginTransaction() unless ($args{'InsideTransaction'});
     # Groups deal with principal ids, rather than user ids.
     # When creating this group, set up a principal Id for it.
@@ -423,7 +429,6 @@ sub _Create {
     );
     $principal->__Set(Field => 'ObjectId', Value => $principal_id);
 
-
     $self->SUPER::Create(
         id          => $principal_id,
         Name        => $args{'Name'},
@@ -487,7 +492,37 @@ sub CreateUserDefinedGroup {
     return($self->_Create( Domain => 'UserDefined', Type => '', Instance => '', @_));
 }
 
+=head2 ValidateName VALUE
+
+Enforces unique user defined group names when updating
 
+=cut
+
+sub ValidateName {
+    my ($self, $value) = @_;
+
+    if ($self->Domain and $self->Domain eq 'UserDefined') {
+        my ($ok, $msg) = $self->_ValidateUserDefinedName($value);
+        # It's really too bad we can't pass along the actual error
+        return 0 if not $ok;
+    }
+    return 1;
+}
+
+=head2 _ValidateUserDefinedName VALUE
+
+Returns true if the user defined group name isn't in use, false otherwise.
+
+=cut
+
+sub _ValidateUserDefinedName {
+    my ($self, $value) = @_;
+    my $dupcheck = RT::Group->new(RT->SystemUser);
+    $dupcheck->LoadUserDefinedGroup($value);
+    return (0, $self->loc("Group name '[_1]' is already in use", $value))
+        if $dupcheck->id;
+    return 1;
+}
 
 =head2 _CreateACLEquivalenceGroup { Principal }
 
diff --git a/t/web/admin_groups.t b/t/web/admin_groups.t
new file mode 100644
index 0000000..45c1b9f
--- /dev/null
+++ b/t/web/admin_groups.t
@@ -0,0 +1,60 @@
+#!/usr/bin/perl
+use strict;
+use warnings;
+
+use RT::Test tests => 25;
+
+my ( $url, $m ) = RT::Test->started_ok;
+ok( $m->login(), 'logged in' );
+
+{
+    diag "test creating a group" if $ENV{TEST_VERBOSE};
+    $m->get_ok( $url . '/Admin/Groups/Modify.html?Create=1' );
+    $m->content_contains('Create a new group', 'found title');
+    $m->submit_form_ok({
+        form_number => 3,
+        fields => { Name => 'test group' },
+    });
+    $m->content_contains('Group created', 'found results');
+    $m->content_contains('Modify the group test group', 'found title');
+}
+
+{
+    diag "test creating another group" if $ENV{TEST_VERBOSE};
+    $m->get_ok( $url . '/Admin/Groups/Modify.html?Create=1' );
+    $m->content_contains('Create a new group', 'found title');
+    $m->submit_form_ok({
+        form_number => 3,
+        fields => { Name => 'test group2' },
+    });
+    $m->content_contains('Group created', 'found results');
+    $m->content_contains('Modify the group test group2', 'found title');
+}
+
+{
+    diag "test creating an overlapping group" if $ENV{TEST_VERBOSE};
+    $m->get_ok( $url . '/Admin/Groups/Modify.html?Create=1' );
+    $m->content_contains('Create a new group', 'found title');
+    $m->submit_form_ok({
+        form_number => 3,
+        fields => { Name => 'test group' },
+    });
+    $m->content_contains('RT Error', 'found error title');
+    $m->content_contains('Group could not be created', 'found results');
+    $m->content_like(qr/Group name .+? is already in use/, 'found message');
+}
+
+{
+    diag "test updating a group name to overlap" if $ENV{TEST_VERBOSE};
+    $m->get_ok( $url . '/Admin/Groups/' );
+    $m->follow_link_ok({text => 'test group2'}, 'found title');
+    $m->content_contains('Modify the group test group2');
+    $m->submit_form_ok({
+        form_number => 3,
+        fields => { Name => 'test group' },
+    });
+    $m->content_lacks('Name changed', "name not changed");
+    $m->content_contains('Illegal value for Name', 'found error message');
+    $m->content_contains('test group', 'did not find new name');
+}
+

commit 84b68b3784bddb2d30fe342e86308d6e570eed0d
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Thu Nov 18 20:27:19 2010 -0500

    Warn about existing duplicate group names

diff --git a/share/html/Admin/Groups/Modify.html b/share/html/Admin/Groups/Modify.html
index bc5c585..697483f 100755
--- a/share/html/Admin/Groups/Modify.html
+++ b/share/html/Admin/Groups/Modify.html
@@ -96,7 +96,7 @@
 </form>
 <%INIT>
 
-my  ($title, @results, $Disabled, $EnabledChecked);
+my  ($title, @results, @warnings, $Disabled, $EnabledChecked);
 
 my $Group = RT::Group->new($session{'CurrentUser'});
 
@@ -132,6 +132,14 @@ if ($id) {
 					    ARGSRef => \%ARGS );
     push (@results, at fieldresults);
     push @results, ProcessObjectCustomFieldUpdates( ARGSRef => \%ARGS, Object => $Group );
+
+    # Warn about duplicate groups
+    my $dupcheck = RT::Groups->new(RT->SystemUser);
+    $dupcheck->LimitToUserDefinedGroups();
+    $dupcheck->Limit( FIELD => 'Name', VALUE => $Group->Name );
+    if ($dupcheck->Count > 1) {
+        push @warnings, loc("There is more than one group with the name '[_1]'.  This may cause inconsistency in the UI, and it's recommended you rename the groups.", $Group->Name);
+    }
 }
 
 #we're asking about enabled on the web page but really care about disabled.
@@ -154,6 +162,8 @@ $m->comp(
     ARGSRef     => \%ARGS
 );
 
+push @results, @warnings;
+
 unless ($Group->Disabled()) {
     $EnabledChecked ='checked="checked"';
 }

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


More information about the Rt-commit mailing list