[Rt-commit] rt branch, 4.0/detailed-admin-update-validation-messages, created. rt-4.0.2-189-ga916310
Jason May
jasonmay at bestpractical.com
Thu Oct 13 13:51:48 EDT 2011
The branch, 4.0/detailed-admin-update-validation-messages has been created
at a916310c053acbf7abd6a4fb3f74bbe7c099cd02 (commit)
- Log -----------------------------------------------------------------
commit ef01c3a2c55d8a87c6cf66e91d530951ec0a1e5b
Author: Jason May <jasonmay at bestpractical.com>
Date: Fri Sep 16 14:46:30 2011 -0400
Validate group names on update
We validate group names on 'create', which is well and good, but
not having constraints on 'update' as well defeats the purpose.
diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index 1ee0591..b0a7bd9 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -506,10 +506,10 @@ Enforces unique user defined group names when updating
=cut
sub ValidateName {
- my ($self, $value) = @_;
+ my ($self, $value, %args) = @_;
if ($self->Domain and $self->Domain eq 'UserDefined') {
- my ($ok, $msg) = $self->_ValidateUserDefinedName($value);
+ my ($ok, $msg) = $self->_ValidateUserDefinedName($value, %args);
# It's really too bad we can't pass along the actual error
return 0 if not $ok;
}
@@ -523,11 +523,14 @@ 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;
+ my ($self, $value, %args) = @_;
+ return (0, $self->loc("A group name is required")) unless $value;
+ unless ($args{for_update_only}) {
+ 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;
}
diff --git a/share/html/Admin/Groups/Modify.html b/share/html/Admin/Groups/Modify.html
index b5b0aad..86ed7e4 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" />
@@ -128,19 +128,26 @@ if ($Create) {
if ($Group->Id) {
my @fields = qw(Description Name );
- my @fieldresults = UpdateRecordObject ( AttributesRef => \@fields,
- Object => $Group,
- 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 parts of the admin interface, and therefore it's recommended you rename the conflicting groups.", $Group->Name);
+ my ($val_ok, $val_msg) = $Group->ValidateName($Name, for_update_only => 1);
+ if ($val_ok) {
+ my @fieldresults = UpdateRecordObject ( AttributesRef => \@fields,
+ Object => $Group,
+ 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 parts of the admin interface, and therefore it's recommended you rename the conflicting groups.", $Group->Name);
+ }
+ }
+ else {
+ # Not sure why we can't pass a message
+ push @warnings, $val_msg || 'The group name you supplied is invalid.';
}
+
}
#we're asking about enabled on the web page but really care about disabled.
diff --git a/t/web/group_create.t b/t/web/group_create.t
new file mode 100644
index 0000000..9cf97f1
--- /dev/null
+++ b/t/web/group_create.t
@@ -0,0 +1,53 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use RT::Test tests => 10;
+
+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('A group name is required');
+ $m->submit_form(
+ form_name => 'ModifyGroup',
+ fields => {
+ Name => '0',
+ },
+ );
+ $m->text_contains('A group name is required');
+ $m->submit_form(
+ form_name => 'ModifyGroup',
+ fields => {
+ Name => $group_name,
+ },
+ );
+ $m->content_contains('Group created', 'created group sucessfully' );
+ $group_id = $m->form_name('ModifyGroup')->value('id');
+ ok $group_id, "found id of the group in the form, it's #$group_id";
+
+ $m->submit_form(
+ form_name => 'ModifyGroup',
+ fields => {
+ Name => '',
+ },
+ );
+ $m->text_contains('Illegal value for Name');
+}
+
commit 58865d34d4793740c73d36f18fa455a37fb87320
Author: Jason May <jasonmay at bestpractical.com>
Date: Tue Sep 27 14:05:03 2011 -0400
Use wantarray so we don't get false positives on column updates
diff --git a/lib/RT/Queue.pm b/lib/RT/Queue.pm
index 73733fc..7cdf126 100644
--- a/lib/RT/Queue.pm
+++ b/lib/RT/Queue.pm
@@ -522,7 +522,6 @@ sub Load {
}
return ( $self->Id );
-
}
@@ -542,16 +541,16 @@ sub ValidateName {
$tempqueue->Load($name);
#If this queue exists, return undef
- if ( $tempqueue->Name() && $tempqueue->id != $self->id) {
- return (undef, "Queue already exists.");
+ if ( $tempqueue->Name() && $tempqueue->id != $self->id) {
+ return wantarray ? (undef, "Queue already exists.") : 0;
}
#If the queue doesn't exist, return 1
elsif (my $q = $self->SUPER::ValidateName($name)) {
- return ($q);
+ return wantarray ? ($q) : $q;
}
else {
- return (undef, "That's not a valid name.");
+ return wantarray ? (undef, "That's not a valid name.") : 0;
}
}
commit bbaf6abfc8083d1b4aea09f3e7a7146399394825
Author: Jason May <jasonmay at bestpractical.com>
Date: Tue Sep 27 18:52:27 2011 -0400
Do not validate the group name in the component
This already gets validated when the record gets updated.
diff --git a/share/html/Admin/Groups/Modify.html b/share/html/Admin/Groups/Modify.html
index 86ed7e4..01777e8 100755
--- a/share/html/Admin/Groups/Modify.html
+++ b/share/html/Admin/Groups/Modify.html
@@ -128,26 +128,18 @@ if ($Create) {
if ($Group->Id) {
my @fields = qw(Description Name );
- my ($val_ok, $val_msg) = $Group->ValidateName($Name, for_update_only => 1);
- if ($val_ok) {
- my @fieldresults = UpdateRecordObject ( AttributesRef => \@fields,
- Object => $Group,
- 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 parts of the admin interface, and therefore it's recommended you rename the conflicting groups.", $Group->Name);
- }
- }
- else {
- # Not sure why we can't pass a message
- push @warnings, $val_msg || 'The group name you supplied is invalid.';
+ my @fieldresults = UpdateRecordObject ( AttributesRef => \@fields,
+ Object => $Group,
+ 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 parts of the admin interface, and therefore it's recommended you rename the conflicting groups.", $Group->Name);
}
-
}
#we're asking about enabled on the web page but really care about disabled.
commit a916310c053acbf7abd6a4fb3f74bbe7c099cd02
Author: Jason May <jasonmay at bestpractical.com>
Date: Tue Sep 27 18:53:17 2011 -0400
If the caller wants an array, also provide the validation message
This allows for back compat and potentially more descriptive errors at
the same time.
diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index b0a7bd9..b232899 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -510,8 +510,9 @@ sub ValidateName {
if ($self->Domain and $self->Domain eq 'UserDefined') {
my ($ok, $msg) = $self->_ValidateUserDefinedName($value, %args);
- # It's really too bad we can't pass along the actual error
- return 0 if not $ok;
+ if (not $ok) {
+ return wantarray ? (0, $msg) : 0;
+ }
}
return 1;
}
-----------------------------------------------------------------------
More information about the Rt-commit
mailing list