[Rt-commit] rt branch, 4.0/disallow-blank-field-names, created. rt-4.0.2-193-gb101a39

Jason May jasonmay at bestpractical.com
Fri Oct 14 16:55:27 EDT 2011


The branch, 4.0/disallow-blank-field-names has been created
        at  b101a39552c290f5924e67247e1ef1f2c9ae32d5 (commit)

- Log -----------------------------------------------------------------
commit 4adf2a4f271d7e156c50701b1df03567b4b07318
Author: Jason May <jasonmay at bestpractical.com>
Date:   Thu Oct 13 16:39:02 2011 -0400

    Disallow '0' as well as other integers for the Name column

diff --git a/lib/RT/Record.pm b/lib/RT/Record.pm
index fcc7bed..2833c9f 100644
--- a/lib/RT/Record.pm
+++ b/lib/RT/Record.pm
@@ -591,10 +591,10 @@ Validate the name of the record we're creating. Mostly, just make sure it's not
 sub ValidateName {
     my $self = shift;
     my $value = shift;
-    if ($value && $value=~ /^\d+$/) {
+    if (defined $value && $value=~ /^\d+$/) {
         return(0);
     } else  {
-         return (1);
+        return(1);
     }
 }
 

commit 0832ade8678192576d12d4718fe0cce47e79ab05
Author: Jason May <jasonmay at bestpractical.com>
Date:   Fri Oct 14 10:59:10 2011 -0400

    Do not set CF properties unless it was created.
    
    In order to set these properties. The CF has to be a valid object (i.e.
    have an id) to check user rights.

diff --git a/lib/RT/CustomField.pm b/lib/RT/CustomField.pm
index 031fa9e..3f3f1c5 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -388,22 +388,24 @@ sub Create {
         Repeated    => $args{'Repeated'},
     );
 
-    if ( exists $args{'LinkValueTo'}) {
-	$self->SetLinkValueTo($args{'LinkValueTo'});
-    }
+    if ($rv) {
+        if ( exists $args{'LinkValueTo'}) {
+        $self->SetLinkValueTo($args{'LinkValueTo'});
+        }
 
-    if ( exists $args{'IncludeContentForValue'}) {
-	$self->SetIncludeContentForValue($args{'IncludeContentForValue'});
-    }
+        if ( exists $args{'IncludeContentForValue'}) {
+        $self->SetIncludeContentForValue($args{'IncludeContentForValue'});
+        }
 
-    return ($rv, $msg) unless exists $args{'Queue'};
+        return ($rv, $msg) unless exists $args{'Queue'};
 
-    # Compat code -- create a new ObjectCustomField mapping
-    my $OCF = RT::ObjectCustomField->new( $self->CurrentUser );
-    $OCF->Create(
-        CustomField => $self->Id,
-        ObjectId => $args{'Queue'},
-    );
+        # Compat code -- create a new ObjectCustomField mapping
+        my $OCF = RT::ObjectCustomField->new( $self->CurrentUser );
+        $OCF->Create(
+            CustomField => $self->Id,
+            ObjectId => $args{'Queue'},
+        );
+    }
 
     return ($rv, $msg);
 }

commit f0d04a788987ac26a2a81ad8e1dd66ffe590cb14
Author: Jason May <jasonmay at bestpractical.com>
Date:   Wed Sep 14 09:39:03 2011 -0400

    Do not allow CFs to be created with the name '' or an integer
    
    It does not make sense to have a CF with a blank name.
    
    Allowing CF to have an integer for a name confuses the Load method, as
    it acccepts both 'name' values and IDs.

diff --git a/lib/RT/CustomField.pm b/lib/RT/CustomField.pm
index 3f3f1c5..52d4423 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -589,6 +589,20 @@ sub DeleteValue {
 }
 
 
+=head2 ValidateQueue Queue
+
+Make sure that the name specified is valid
+
+=cut
+
+sub ValidateName {
+    my $self = shift;
+    my $value = shift;
+
+    return 0 unless length $value;
+
+    return $self->SUPER::ValidateName($value);
+}
 
 =head2 ValidateQueue Queue
 
diff --git a/t/web/cf_access.t b/t/web/cf_access.t
index 0eaf7b3..73b7765 100644
--- a/t/web/cf_access.t
+++ b/t/web/cf_access.t
@@ -1,7 +1,7 @@
 #!/usr/bin/perl -w
 use strict;
 
-use RT::Test tests => 25;
+use RT::Test tests => 32;
 
 my ($baseurl, $m) = RT::Test->started_ok;
 
@@ -13,6 +13,42 @@ ok $m->login, 'logged in';
 diag "Create a CF";
 {
     $m->follow_link( id => 'tools-config-custom-fields-create');
+
+    # Test form validation
+    $m->submit_form(
+        form_name => "ModifyCustomField",
+        fields => {
+            TypeComposite => 'Image-0',
+            LookupType => 'RT::Queue-RT::Ticket',
+            Name => '',
+            Description => 'img',
+        },
+    );
+    $m->text_contains('Invalid value for Name');
+
+    $m->submit_form(
+        form_name => "ModifyCustomField",
+        fields => {
+            TypeComposite => 'Image-0',
+            LookupType => 'RT::Queue-RT::Ticket',
+            Name => '0',
+            Description => 'img',
+        },
+    );
+    $m->text_contains('Invalid value for Name');
+
+    $m->submit_form(
+        form_name => "ModifyCustomField",
+        fields => {
+            TypeComposite => 'Image-0',
+            LookupType => 'RT::Queue-RT::Ticket',
+            Name => '1',
+            Description => 'img',
+        },
+    );
+    $m->text_contains('Invalid value for Name');
+
+    # The real submission
     $m->submit_form(
         form_name => "ModifyCustomField",
         fields => {
@@ -22,6 +58,36 @@ diag "Create a CF";
             Description => 'img',
         },
     );
+    $m->text_contains('Object created');
+
+    # Validation on update
+    $m->form_name("ModifyCustomField");
+    $m->set_fields(
+        TypeComposite => 'Image-0',
+        LookupType => 'RT::Queue-RT::Ticket',
+        Name => '',
+        Description => 'img',
+    );
+    $m->click('Update');
+    $m->text_contains('Illegal value for Name');
+    $m->form_name("ModifyCustomField");
+    $m->set_fields(
+        TypeComposite => 'Image-0',
+        LookupType => 'RT::Queue-RT::Ticket',
+        Name => '0',
+        Description => 'img',
+    );
+    $m->click('Update');
+    $m->text_contains('Illegal value for Name');
+    $m->form_name("ModifyCustomField");
+    $m->set_fields(
+        TypeComposite => 'Image-0',
+        LookupType => 'RT::Queue-RT::Ticket',
+        Name => '1',
+        Description => 'img',
+    );
+    $m->click('Update');
+    $m->text_contains('Illegal value for Name');
 }
 
 diag "apply the CF to General queue";

commit f8102a3319d6248eed1aafaf606ac34df169f0a5
Author: Jason May <jasonmay at bestpractical.com>
Date:   Wed Sep 14 11:15:05 2011 -0400

    Do not allow groups to be created with the name '' or an integer
    
    See 35e1428e12c for a similar rationale.

diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index 1ee0591..2c42553 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -506,14 +506,15 @@ Enforces unique user defined group names when updating
 =cut
 
 sub ValidateName {
-    my ($self, $value) = @_;
+    my $self  = shift;
+    my $value = shift;
 
     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;
+    return $self->SUPER::ValidateName($value);
 }
 
 =head2 _ValidateUserDefinedName VALUE
@@ -524,6 +525,9 @@ Returns true if the user defined group name isn't in use, false otherwise.
 
 sub _ValidateUserDefinedName {
     my ($self, $value) = @_;
+
+    return (0, 'Name is required') unless length $value;
+
     my $dupcheck = RT::Group->new(RT->SystemUser);
     $dupcheck->LoadUserDefinedGroup($value);
     return (0, $self->loc("Group name '[_1]' is already in use", $value))
diff --git a/share/html/Admin/Groups/Modify.html b/share/html/Admin/Groups/Modify.html
index b5b0aad..9ad48e8 100755
--- a/share/html/Admin/Groups/Modify.html
+++ b/share/html/Admin/Groups/Modify.html
@@ -52,7 +52,7 @@
 
 
 
-<form action="<%RT->Config->Get('WebPath')%>/Admin/Groups/Modify.html" method="post" enctype="multipart/form-data">
+<form action="<%RT->Config->Get('WebPath')%>/Admin/Groups/Modify.html" name="ModifyGroup" method="post" enctype="multipart/form-data">
 
 %unless ($Group->Id) {
 <input type="hidden" class="hidden" name="id" value="new" />
diff --git a/t/web/group_create.t b/t/web/group_create.t
new file mode 100644
index 0000000..26da592
--- /dev/null
+++ b/t/web/group_create.t
@@ -0,0 +1,75 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use RT::Test tests => 13;
+
+my ( $baseurl, $m ) = RT::Test->started_ok;
+ok $m->login, 'logged in as root';
+my $root = RT::User->new(RT->SystemUser);
+ok( $root->Load('root'), 'load root user' );
+
+my $group_name = 'test group';
+
+my $group_id;
+diag "Create a group";
+{
+    $m->follow_link( id => 'tools-config-groups-create');
+
+    # Test group form validation
+    $m->submit_form(
+        form_name => 'ModifyGroup',
+        fields => {
+            Name => '',
+        },
+    );
+    $m->text_contains('Name is required');
+    $m->submit_form(
+        form_name => 'ModifyGroup',
+        fields => {
+            Name => '0',
+        },
+    );
+    $m->text_contains('Could not create group');
+    $m->submit_form(
+        form_name => 'ModifyGroup',
+        fields => {
+            Name => '1',
+        },
+    );
+    $m->text_contains('Could not create group');
+    $m->submit_form(
+        form_name => 'ModifyGroup',
+        fields => {
+            Name => $group_name,
+        },
+    );
+    $m->content_contains('Group created', 'created group sucessfully' );
+
+    # Test validation on updae
+    $m->form_name('ModifyGroup');
+    $m->set_fields(
+        Name => '',
+    );
+    $m->click_button(value => 'Save Changes');
+    $m->text_contains('Illegal value for Name');
+
+    $m->form_name('ModifyGroup');
+    $m->set_fields(
+        Name => '0',
+    );
+    $m->click_button(value => 'Save Changes');
+    $m->text_contains('Illegal value for Name');
+
+    $m->form_name('ModifyGroup');
+    $m->set_fields(
+        Name => '1',
+    );
+    $m->click_button(value => 'Save Changes');
+    $m->text_contains('Illegal value for Name');
+
+    $group_id           = $m->form_name('ModifyGroup')->value('id');
+    ok $group_id, "found id of the group in the form, it's #$group_id";
+}
+

commit 4834d77f3bcd4691640b0ee578f3398fa47ed70a
Author: Jason May <jasonmay at bestpractical.com>
Date:   Wed Sep 14 13:14:36 2011 -0400

    Bring us back to Create state if validation is false, not true
    
    Without this fix, form assumes a queue was created if validation really
    does fail and a queue was never created.

diff --git a/share/html/Admin/Queues/Modify.html b/share/html/Admin/Queues/Modify.html
index 767ea62..fcbfcd6 100755
--- a/share/html/Admin/Queues/Modify.html
+++ b/share/html/Admin/Queues/Modify.html
@@ -171,7 +171,7 @@ if ($Create) {
 } else {
     if ( defined $id && $id eq 'new' ) {
         my ($val, $msg) = $QueueObj->Create( Name => $Name );
-        if ($val) {
+        if (!$val) {
             $Create = 1; # Create failed, so bring us back to step 1
         }
         push @results, $msg;

commit e42335dc0db4cf1e9e4276d5c57b9032809faf98
Author: Jason May <jasonmay at bestpractical.com>
Date:   Wed Sep 14 15:28:46 2011 -0400

    Prevent undef in loc()'s argument to avoid a cryptic warning:
    
    Use of uninitialized value $_[1] in join or string at (eval 1755) line
    2.

diff --git a/share/html/Admin/Queues/Modify.html b/share/html/Admin/Queues/Modify.html
index fcbfcd6..74d929d 100755
--- a/share/html/Admin/Queues/Modify.html
+++ b/share/html/Admin/Queues/Modify.html
@@ -179,7 +179,7 @@ if ($Create) {
     else {
         $QueueObj->Load($id) || $QueueObj->Load($Name) || Abort(loc("Couldn't load queue '[_1]'", $Name));
     }
-    $title = loc('Configuration for queue [_1]', $QueueObj->Name);
+    $title = loc('Configuration for queue [_1]', $QueueObj->Name||'');
 }
 if ( $QueueObj->Id ) {
     my @attribs= qw(Description CorrespondAddress CommentAddress Name

commit ae23f22a5dfc5347485047beae2de54ea96edd03
Author: Jason May <jasonmay at bestpractical.com>
Date:   Fri Oct 14 12:33:04 2011 -0400

    Factor validation message returning to _ValidateName
    
    SearchBuilder expects a scalar value from ValidateFoo so we can't return
    a list. However, it would still be nice to have specific error messages
    in some cases, so we'll keep the old logic in _ValidateName in this
    case.

diff --git a/lib/RT/Queue.pm b/lib/RT/Queue.pm
index 73733fc..b41a215 100644
--- a/lib/RT/Queue.pm
+++ b/lib/RT/Queue.pm
@@ -405,7 +405,7 @@ sub Create {
     }
 
     {
-        my ($val, $msg) = $self->ValidateName( $args{'Name'} );
+        my ($val, $msg) = $self->_ValidateName( $args{'Name'} );
 
         if (!$val) {
             return ($val, $self->loc($msg));
@@ -538,6 +538,15 @@ sub ValidateName {
     my $self = shift;
     my $name = shift;
 
+    my ($ok, $msg) = $self->_ValidateName($name);
+
+    return $ok ? 1 : 0;
+}
+
+sub _ValidateName {
+    my $self = shift;
+    my $name = shift;
+
     my $tempqueue = RT::Queue->new(RT->SystemUser);
     $tempqueue->Load($name);
 
@@ -553,7 +562,6 @@ sub ValidateName {
     else {
         return (undef, "That's not a valid name.");
     }
-
 }
 
 

commit b101a39552c290f5924e67247e1ef1f2c9ae32d5
Author: Jason May <jasonmay at bestpractical.com>
Date:   Wed Sep 14 15:34:10 2011 -0400

    Do not allow queues to be created with the name '' or an integer
    
    See 35e1428e12c for a similar rationale.

diff --git a/lib/RT/Queue.pm b/lib/RT/Queue.pm
index b41a215..1a4f85c 100644
--- a/lib/RT/Queue.pm
+++ b/lib/RT/Queue.pm
@@ -547,6 +547,15 @@ sub _ValidateName {
     my $self = shift;
     my $name = shift;
 
+    return (undef, "Queue name is required") unless length $name;
+
+    # Validate via the superclass first
+    # Case: short circuit if it's an integer so we don't have
+    # fale negatives when loading a temp queue
+    unless ( my $q = $self->SUPER::ValidateName($name) ) {
+        return ($q, "That's not a valid name.");
+    }
+
     my $tempqueue = RT::Queue->new(RT->SystemUser);
     $tempqueue->Load($name);
 
@@ -555,13 +564,7 @@ sub _ValidateName {
         return (undef, "Queue already exists.");
     }
 
-    #If the queue doesn't exist, return 1
-    elsif (my $q = $self->SUPER::ValidateName($name)) {
-        return ($q);
-    }
-    else {
-        return (undef, "That's not a valid name.");
-    }
+    return (1);
 }
 
 
diff --git a/share/html/Admin/Queues/Modify.html b/share/html/Admin/Queues/Modify.html
index 74d929d..153fd88 100755
--- a/share/html/Admin/Queues/Modify.html
+++ b/share/html/Admin/Queues/Modify.html
@@ -51,7 +51,7 @@
 
 
 
-<form action="<%RT->Config->Get('WebPath')%>/Admin/Queues/Modify.html" method="post">
+<form action="<%RT->Config->Get('WebPath')%>/Admin/Queues/Modify.html" name="ModifyQueue" method="post">
 <input type="hidden" class="hidden" name="SetEnabled" value="1" />
 <input type="hidden" class="hidden" name="id" value="<% $Create? 'new': $QueueObj->Id %>" />
 
diff --git a/t/web/queue_create.t b/t/web/queue_create.t
new file mode 100644
index 0000000..ae7106b
--- /dev/null
+++ b/t/web/queue_create.t
@@ -0,0 +1,75 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use RT::Test tests => 13;
+
+my ( $baseurl, $m ) = RT::Test->started_ok;
+ok $m->login, 'logged in as root';
+my $root = RT::User->new(RT->SystemUser);
+ok( $root->Load('root'), 'load root user' );
+
+my $queue_name = 'test queue';
+
+my $queue_id;
+diag "Create a queue";
+{
+    $m->follow_link( id => 'tools-config-queues-create');
+
+    # Test queue form validation
+    $m->submit_form(
+        form_name => 'ModifyQueue',
+        fields => {
+            Name => '',
+        },
+    );
+    $m->text_contains('Queue name is required');
+    $m->submit_form(
+        form_name => 'ModifyQueue',
+        fields => {
+            Name => '0',
+        },
+    );
+    $m->text_contains("That's not a valid name");
+    $m->submit_form(
+        form_name => 'ModifyQueue',
+        fields => {
+            Name => '1',
+        },
+    );
+    $m->text_contains("That's not a valid name");
+    $m->submit_form(
+        form_name => 'ModifyQueue',
+        fields => {
+            Name => $queue_name,
+        },
+    );
+    $m->content_contains('Queue created', 'created queue sucessfully' );
+
+    # Test validation on update
+    $m->form_name('ModifyQueue');
+    $m->set_fields(
+        Name => '',
+    );
+    $m->click_button(value => 'Save Changes');
+    $m->content_contains('Illegal value for Name');
+
+    $m->form_name('ModifyQueue');
+    $m->set_fields(
+        Name => '0',
+    );
+    $m->click_button(value => 'Save Changes');
+    $m->content_contains('Illegal value for Name');
+
+    $m->form_name('ModifyQueue');
+    $m->set_fields(
+        Name => '1',
+    );
+    $m->click_button(value => 'Save Changes');
+    $m->content_contains('Illegal value for Name');
+
+    $queue_id = $m->form_name('ModifyQueue')->value('id');
+    ok $queue_id, "found id of the queue in the form, it's #$queue_id";
+}
+

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


More information about the Rt-commit mailing list