TimeWorked-like custom fields, RT#11168
[freeside.git] / rt / lib / RT / Queue_Overlay.pm
index fcc185b..5245af4 100644 (file)
@@ -1,26 +1,51 @@
-# BEGIN LICENSE BLOCK
-# 
-# Copyright (c) 1996-2003 Jesse Vincent <jesse@bestpractical.com>
-# 
-# (Except where explictly superceded by other copyright notices)
-# 
+# BEGIN BPS TAGGED BLOCK {{{
+#
+# COPYRIGHT:
+#
+# This software is Copyright (c) 1996-2011 Best Practical Solutions, LLC
+#                                          <sales@bestpractical.com>
+#
+# (Except where explicitly superseded by other copyright notices)
+#
+#
+# LICENSE:
+#
 # This work is made available to you under the terms of Version 2 of
 # the GNU General Public License. A copy of that license should have
 # been provided with this software, but in any event can be snarfed
 # from www.gnu.org.
 # This work is made available to you under the terms of Version 2 of
 # the GNU General Public License. A copy of that license should have
 # been provided with this software, but in any event can be snarfed
 # from www.gnu.org.
-# 
+#
 # This work is distributed in the hope that it will be useful, but
 # WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 # General Public License for more details.
 # This work is distributed in the hope that it will be useful, but
 # WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 # General Public License for more details.
-# 
-# Unless otherwise specified, all modifications, corrections or
-# extensions to this work which alter its source code become the
-# property of Best Practical Solutions, LLC when submitted for
-# inclusion in the work.
-# 
-# 
-# END LICENSE BLOCK
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+# 02110-1301 or visit their web page on the internet at
+# http://www.gnu.org/licenses/old-licenses/gpl-2.0.html.
+#
+#
+# CONTRIBUTION SUBMISSION POLICY:
+#
+# (The following paragraph is not intended to limit the rights granted
+# to you to modify and distribute this software under the terms of
+# the GNU General Public License and is only of importance to you if
+# you choose to contribute your changes and enhancements to the
+# community by submitting them to Best Practical Solutions, LLC.)
+#
+# By intentionally submitting any modifications, corrections or
+# derivatives to this work, or any other work intended for use with
+# Request Tracker, to Best Practical Solutions, LLC, you confirm that
+# you are the copyright holder for those contributions and you grant
+# Best Practical Solutions,  LLC a nonexclusive, worldwide, irrevocable,
+# royalty-free, perpetual, license to use, copy, create derivative
+# works based on those contributions, and sublicense and distribute
+# those contributions and any derivatives thereof.
+#
+# END BPS TAGGED BLOCK }}}
+
 =head1 NAME
 
   RT::Queue - an RT Queue object
 =head1 NAME
 
   RT::Queue - an RT Queue object
 
 =head1 DESCRIPTION
 
 
 =head1 DESCRIPTION
 
+An RT queue object.
 
 =head1 METHODS
 
 
 =head1 METHODS
 
-=begin testing 
-
-use RT::Queue;
+=cut
 
 
-=end testing
 
 
-=cut
+package RT::Queue;
 
 use strict;
 no warnings qw(redefine);
 
 
 use strict;
 no warnings qw(redefine);
 
-use vars qw(@STATUS @ACTIVE_STATUS @INACTIVE_STATUS $RIGHTS);
 use RT::Groups;
 use RT::ACL;
 use RT::Groups;
 use RT::ACL;
+use RT::Interface::Email;
 
 
-
-@ACTIVE_STATUS = qw(new open stalled);
-@INACTIVE_STATUS = qw(resolved rejected deleted);
-@STATUS = (@ACTIVE_STATUS, @INACTIVE_STATUS);
+our @DEFAULT_ACTIVE_STATUS = qw(new open stalled);
+our @DEFAULT_INACTIVE_STATUS = qw(resolved rejected deleted);  
 
 # $self->loc('new'); # For the string extractor to get a string to localize
 # $self->loc('open'); # For the string extractor to get a string to localize
 
 # $self->loc('new'); # For the string extractor to get a string to localize
 # $self->loc('open'); # For the string extractor to get a string to localize
@@ -62,21 +83,24 @@ use RT::ACL;
 # $self->loc('deleted'); # For the string extractor to get a string to localize
 
 
 # $self->loc('deleted'); # For the string extractor to get a string to localize
 
 
-$RIGHTS = {
+our $RIGHTS = {
     SeeQueue            => 'Can this principal see this queue',       # loc_pair
     AdminQueue          => 'Create, delete and modify queues',        # loc_pair
     ShowACL             => 'Display Access Control List',             # loc_pair
     ModifyACL           => 'Modify Access Control List',              # loc_pair
     ModifyQueueWatchers => 'Modify the queue watchers',               # loc_pair
     SeeQueue            => 'Can this principal see this queue',       # loc_pair
     AdminQueue          => 'Create, delete and modify queues',        # loc_pair
     ShowACL             => 'Display Access Control List',             # loc_pair
     ModifyACL           => 'Modify Access Control List',              # loc_pair
     ModifyQueueWatchers => 'Modify the queue watchers',               # loc_pair
-    AdminCustomFields   => 'Create, delete and modify custom fields', # loc_pair
+    SeeCustomField     => 'See custom field values',                 # loc_pair
+    ModifyCustomField  => 'Modify custom field values',              # loc_pair
+    AssignCustomFields  => 'Assign and remove custom fields',         # loc_pair
     ModifyTemplate      => 'Modify Scrip templates for this queue',   # loc_pair
     ShowTemplate        => 'Display Scrip templates for this queue',  # loc_pair
 
     ModifyScrips => 'Modify Scrips for this queue',                   # loc_pair
     ShowScrips   => 'Display Scrips for this queue',                  # loc_pair
 
     ModifyTemplate      => 'Modify Scrip templates for this queue',   # loc_pair
     ShowTemplate        => 'Display Scrip templates for this queue',  # loc_pair
 
     ModifyScrips => 'Modify Scrips for this queue',                   # loc_pair
     ShowScrips   => 'Display Scrips for this queue',                  # loc_pair
 
-    ShowTicket         => 'Show ticket summaries',                    # loc_pair
-    ShowTicketComments => 'Show ticket private commentary',           # loc_pair
+    ShowTicket         => 'See ticket summaries',                    # loc_pair
+    ShowTicketComments => 'See ticket private commentary',           # loc_pair
+    ShowOutgoingEmail => 'See exact outgoing email messages and their recipeients',           # loc_pair
 
     Watch => 'Sign up as a ticket Requestor or ticket or queue Cc',   # loc_pair
     WatchAsAdminCc  => 'Sign up as a ticket or queue AdminCc',        # loc_pair
 
     Watch => 'Sign up as a ticket Requestor or ticket or queue Cc',   # loc_pair
     WatchAsAdminCc  => 'Sign up as a ticket or queue AdminCc',        # loc_pair
@@ -89,6 +113,8 @@ $RIGHTS = {
     TakeTicket      => 'Take tickets',                                # loc_pair
     StealTicket     => 'Steal tickets',                               # loc_pair
 
     TakeTicket      => 'Take tickets',                                # loc_pair
     StealTicket     => 'Steal tickets',                               # loc_pair
 
+    ForwardMessage  => 'Forward messages to third person(s)',         # loc_pair
+
 };
 
 # Tell RT::ACE that this sort of object can get acls granted
 };
 
 # Tell RT::ACE that this sort of object can get acls granted
@@ -100,7 +126,54 @@ $RT::ACE::OBJECT_TYPES{'RT::Queue'} = 1;
 foreach my $right ( keys %{$RIGHTS} ) {
     $RT::ACE::LOWERCASERIGHTNAMES{ lc $right } = $right;
 }
 foreach my $right ( keys %{$RIGHTS} ) {
     $RT::ACE::LOWERCASERIGHTNAMES{ lc $right } = $right;
 }
-    
+
+=head2 AddRights C<RIGHT>, C<DESCRIPTION> [, ...]
+
+Adds the given rights to the list of possible rights.  This method
+should be called during server startup, not at runtime.
+
+=cut
+
+sub AddRights {
+    my $self = shift;
+    my %new = @_;
+    $RIGHTS = { %$RIGHTS, %new };
+    %RT::ACE::LOWERCASERIGHTNAMES = ( %RT::ACE::LOWERCASERIGHTNAMES,
+                                      map { lc($_) => $_ } keys %new);
+}
+
+sub AddLink {
+    my $self = shift;
+    my %args = ( Target => '',
+                 Base   => '',
+                 Type   => '',
+                 Silent => undef,
+                 @_ );
+
+    unless ( $self->CurrentUserHasRight('ModifyQueue') ) {
+        return ( 0, $self->loc("Permission Denied") );
+    }
+
+    return $self->SUPER::_AddLink(%args);
+}
+
+sub DeleteLink {
+    my $self = shift;
+    my %args = (
+        Base   => undef,
+        Target => undef,
+        Type   => undef,
+        @_
+    );
+
+    #check acls
+    unless ( $self->CurrentUserHasRight('ModifyQueue') ) {
+        $RT::Logger->debug("No permission to delete links");
+        return ( 0, $self->loc('Permission Denied'))
+    }
+
+    return $self->SUPER::_DeleteLink(%args);
+}
 
 =head2 AvailableRights
 
 
 =head2 AvailableRights
 
@@ -123,7 +196,12 @@ Returns an array of all ActiveStatuses for this queue
 
 sub ActiveStatusArray {
     my $self = shift;
 
 sub ActiveStatusArray {
     my $self = shift;
-    return (@ACTIVE_STATUS);
+    if (RT->Config->Get('ActiveStatus')) {
+       return (RT->Config->Get('ActiveStatus'))
+    } else {
+        $RT::Logger->warning("RT::ActiveStatus undefined, falling back to deprecated defaults");
+        return (@DEFAULT_ACTIVE_STATUS);
+    }
 }
 
 # }}}
 }
 
 # }}}
@@ -138,7 +216,12 @@ Returns an array of all InactiveStatuses for this queue
 
 sub InactiveStatusArray {
     my $self = shift;
 
 sub InactiveStatusArray {
     my $self = shift;
-    return (@INACTIVE_STATUS);
+    if (RT->Config->Get('InactiveStatus')) {
+       return (RT->Config->Get('InactiveStatus'))
+    } else {
+        $RT::Logger->warning("RT::InactiveStatus undefined, falling back to deprecated defaults");
+        return (@DEFAULT_INACTIVE_STATUS);
+    }
 }
 
 # }}}
 }
 
 # }}}
@@ -153,7 +236,7 @@ Returns an array of all statuses for this queue
 
 sub StatusArray {
     my $self = shift;
 
 sub StatusArray {
     my $self = shift;
-    return (@STATUS);
+    return ($self->ActiveStatusArray(), $self->InactiveStatusArray());
 }
 
 # }}}
 }
 
 # }}}
@@ -162,12 +245,8 @@ sub StatusArray {
 
 =head2 IsValidStatus VALUE
 
 
 =head2 IsValidStatus VALUE
 
-Returns true if VALUE is a valid status.  Otherwise, returns 0
+Returns true if VALUE is a valid status.  Otherwise, returns 0.
 
 
-=for testing
-my $q = RT::Queue->new($RT::SystemUser);
-ok($q->IsValidStatus('new')== 1, 'New is a valid status');
-ok($q->IsValidStatus('f00')== 0, 'f00 is not a valid status');
 
 =cut
 
 
 =cut
 
@@ -175,7 +254,7 @@ sub IsValidStatus {
     my $self  = shift;
     my $value = shift;
 
     my $self  = shift;
     my $value = shift;
 
-    my $retval = grep ( /^$value$/, $self->StatusArray );
+    my $retval = grep ( $_ eq $value, $self->StatusArray );
     return ($retval);
 
 }
     return ($retval);
 
 }
@@ -188,11 +267,6 @@ sub IsValidStatus {
 
 Returns true if VALUE is a Active status.  Otherwise, returns 0
 
 
 Returns true if VALUE is a Active status.  Otherwise, returns 0
 
-=for testing
-my $q = RT::Queue->new($RT::SystemUser);
-ok($q->IsActiveStatus('new')== 1, 'New is a Active status');
-ok($q->IsActiveStatus('rejected')== 0, 'Rejected is an inactive status');
-ok($q->IsActiveStatus('f00')== 0, 'f00 is not a Active status');
 
 =cut
 
 
 =cut
 
@@ -200,7 +274,7 @@ sub IsActiveStatus {
     my $self  = shift;
     my $value = shift;
 
     my $self  = shift;
     my $value = shift;
 
-    my $retval = grep ( /^$value$/, $self->ActiveStatusArray );
+    my $retval = grep ( $_ eq $value, $self->ActiveStatusArray );
     return ($retval);
 
 }
     return ($retval);
 
 }
@@ -213,11 +287,6 @@ sub IsActiveStatus {
 
 Returns true if VALUE is a Inactive status.  Otherwise, returns 0
 
 
 Returns true if VALUE is a Inactive status.  Otherwise, returns 0
 
-=for testing
-my $q = RT::Queue->new($RT::SystemUser);
-ok($q->IsInactiveStatus('new')== 0, 'New is a Active status');
-ok($q->IsInactiveStatus('rejected')== 1, 'rejeected is an Inactive status');
-ok($q->IsInactiveStatus('f00')== 0, 'f00 is not a Active status');
 
 =cut
 
 
 =cut
 
@@ -225,7 +294,7 @@ sub IsInactiveStatus {
     my $self  = shift;
     my $value = shift;
 
     my $self  = shift;
     my $value = shift;
 
-    my $retval = grep ( /^$value$/, $self->InactiveStatusArray );
+    my $retval = grep ( $_ eq $value, $self->InactiveStatusArray );
     return ($retval);
 
 }
     return ($retval);
 
 }
@@ -235,11 +304,24 @@ sub IsInactiveStatus {
 
 # {{{ sub Create
 
 
 # {{{ sub Create
 
-=head2 Create
 
 
-Create takes the name of the new queue 
+
+
+=head2 Create(ARGS)
+
+Arguments: ARGS is a hash of named parameters.  Valid parameters are:
+
+  Name (required)
+  Description
+  CorrespondAddress
+  CommentAddress
+  InitialPriority
+  FinalPriority
+  DefaultDueIn
 If you pass the ACL check, it creates the queue and returns its queue id.
 
 If you pass the ACL check, it creates the queue and returns its queue id.
 
+
 =cut
 
 sub Create {
 =cut
 
 sub Create {
@@ -249,9 +331,13 @@ sub Create {
         CorrespondAddress => '',
         Description       => '',
         CommentAddress    => '',
         CorrespondAddress => '',
         Description       => '',
         CommentAddress    => '',
-        InitialPriority   => "0",
-        FinalPriority     => "0",
-        DefaultDueIn      => "0",
+        SubjectTag        => '',
+        InitialPriority   => 0,
+        FinalPriority     => 0,
+        DefaultDueIn      => 0,
+        Sign              => undef,
+        Encrypt           => undef,
+        _RecordTransaction => 1,
         @_
     );
 
         @_
     );
 
@@ -264,10 +350,11 @@ sub Create {
         return ( 0, $self->loc('Queue already exists') );
     }
 
         return ( 0, $self->loc('Queue already exists') );
     }
 
+    my %attrs = map {$_ => 1} $self->ReadableAttributes;
+
     #TODO better input validation
     $RT::Handle->BeginTransaction();
     #TODO better input validation
     $RT::Handle->BeginTransaction();
-
-    my $id = $self->SUPER::Create(%args);
+    my $id = $self->SUPER::Create( map { $_ => $args{$_} } grep exists $args{$_}, keys %attrs );
     unless ($id) {
         $RT::Handle->Rollback();
         return ( 0, $self->loc('Queue could not be created') );
     unless ($id) {
         $RT::Handle->Rollback();
         return ( 0, $self->loc('Queue could not be created') );
@@ -278,8 +365,24 @@ sub Create {
         $RT::Handle->Rollback();
         return ( 0, $self->loc('Queue could not be created') );
     }
         $RT::Handle->Rollback();
         return ( 0, $self->loc('Queue could not be created') );
     }
+    if ( $args{'_RecordTransaction'} ) {
+        $self->_NewTransaction( Type => "Create" );
+    }
+    $RT::Handle->Commit;
+
+    if ( defined $args{'Sign'} ) {
+        my ($status, $msg) = $self->SetSign( $args{'Sign'} );
+        $RT::Logger->error("Couldn't set attribute 'Sign': $msg")
+            unless $status;
+    }
+    if ( defined $args{'Encrypt'} ) {
+        my ($status, $msg) = $self->SetEncrypt( $args{'Encrypt'} );
+        $RT::Logger->error("Couldn't set attribute 'Encrypt': $msg")
+            unless $status;
+    }
+
+    RT->System->QueueCacheNeedsUpdate(1);
 
 
-    $RT::Handle->Commit();
     return ( $id, $self->loc("Queue created") );
 }
 
     return ( $id, $self->loc("Queue created") );
 }
 
@@ -300,11 +403,36 @@ sub Delete {
 =head2 SetDisabled
 
 Takes a boolean.
 =head2 SetDisabled
 
 Takes a boolean.
-1 will cause this queue to no longer be avaialble for tickets.
-0 will re-enable this queue
+1 will cause this queue to no longer be available for tickets.
+0 will re-enable this queue.
 
 =cut
 
 
 =cut
 
+sub SetDisabled {
+    my $self = shift;
+    my $val = shift;
+
+    $RT::Handle->BeginTransaction();
+    my $set_err = $self->SUPER::SetDisabled($val);
+    unless ($set_err) {
+        $RT::Handle->Rollback();
+        $RT::Logger->warning("Couldn't ".($val == 1) ? "disable" : "enable"." queue ".$self->PrincipalObj->Id);
+        return (undef);
+    }
+    $self->_NewTransaction( Type => ($val == 1) ? "Disabled" : "Enabled" );
+
+    $RT::Handle->Commit();
+
+    RT->System->QueueCacheNeedsUpdate(1);
+
+    if ( $val == 1 ) {
+        return (1, $self->loc("Queue disabled"));
+    } else {
+        return (1, $self->loc("Queue enabled"));
+    }
+
+}
+
 # }}}
 
 # {{{ sub Load 
 # }}}
 
 # {{{ sub Load 
@@ -352,26 +480,108 @@ sub ValidateName {
     my $tempqueue = new RT::Queue($RT::SystemUser);
     $tempqueue->Load($name);
 
     my $tempqueue = new RT::Queue($RT::SystemUser);
     $tempqueue->Load($name);
 
-    #If we couldn't load it :)
-    unless ( $tempqueue->id() ) {
-        return (1);
-    }
-
     #If this queue exists, return undef
     #If this queue exists, return undef
-    #Avoid the ACL check.
-    if ( $tempqueue->Name() ) {
+    if ( $tempqueue->Name() && $tempqueue->id != $self->id)  {
         return (undef);
     }
 
     #If the queue doesn't exist, return 1
     else {
         return (undef);
     }
 
     #If the queue doesn't exist, return 1
     else {
-        return (1);
+        return ($self->SUPER::ValidateName($name));
     }
 
 }
 
 # }}}
 
     }
 
 }
 
 # }}}
 
+=head2 SetSign
+
+=cut
+
+sub Sign {
+    my $self = shift;
+    my $value = shift;
+
+    return undef unless $self->CurrentUserHasRight('SeeQueue');
+    my $attr = $self->FirstAttribute('Sign') or return 0;
+    return $attr->Content;
+}
+
+sub SetSign {
+    my $self = shift;
+    my $value = shift;
+
+    return ( 0, $self->loc('Permission Denied') )
+        unless $self->CurrentUserHasRight('AdminQueue');
+
+    my ($status, $msg) = $self->SetAttribute(
+        Name        => 'Sign',
+        Description => 'Sign outgoing messages by default',
+        Content     => $value,
+    );
+    return ($status, $msg) unless $status;
+    return ($status, $self->loc('Signing enabled')) if $value;
+    return ($status, $self->loc('Signing disabled'));
+}
+
+sub Encrypt {
+    my $self = shift;
+    my $value = shift;
+
+    return undef unless $self->CurrentUserHasRight('SeeQueue');
+    my $attr = $self->FirstAttribute('Encrypt') or return 0;
+    return $attr->Content;
+}
+
+sub SetEncrypt {
+    my $self = shift;
+    my $value = shift;
+
+    return ( 0, $self->loc('Permission Denied') )
+        unless $self->CurrentUserHasRight('AdminQueue');
+
+    my ($status, $msg) = $self->SetAttribute(
+        Name        => 'Encrypt',
+        Description => 'Encrypt outgoing messages by default',
+        Content     => $value,
+    );
+    return ($status, $msg) unless $status;
+    return ($status, $self->loc('Encrypting enabled')) if $value;
+    return ($status, $self->loc('Encrypting disabled'));
+}
+
+sub SubjectTag {
+    my $self = shift;
+    return RT->System->SubjectTag( $self );
+}
+
+sub SetSubjectTag {
+    my $self = shift;
+    my $value = shift;
+
+    return ( 0, $self->loc('Permission Denied') )
+        unless $self->CurrentUserHasRight('AdminQueue');
+
+    my $attr = RT->System->FirstAttribute('BrandedSubjectTag');
+    my $map = $attr ? $attr->Content : {};
+    if ( defined $value && length $value ) {
+        $map->{ $self->id } = $value;
+    } else {
+        delete $map->{ $self->id };
+    }
+
+    my ($status, $msg) = RT->System->SetAttribute(
+        Name        => 'BrandedSubjectTag',
+        Description => 'Queue id => subject tag map',
+        Content     => $map,
+    );
+    return ($status, $msg) unless $status;
+    return ($status, $self->loc(
+        "SubjectTag changed to [_1]", 
+        (defined $value && length $value)? $value : $self->loc("(no value)")
+    ))
+}
+
 # {{{ sub Templates
 
 =head2 Templates
 # {{{ sub Templates
 
 =head2 Templates
@@ -398,7 +608,7 @@ sub Templates {
 
 # {{{  CustomField
 
 
 # {{{  CustomField
 
-=item CustomField NAME
+=head2 CustomField NAME
 
 Load the queue-specific custom field named NAME
 
 
 Load the queue-specific custom field named NAME
 
@@ -413,20 +623,47 @@ sub CustomField {
 }
 
 
 }
 
 
-# {{{ CustomFields
+# {{{ TicketCustomFields
+
+=head2 TicketCustomFields
+
+Returns an L<RT::CustomFields> object containing all global and
+queue-specific B<ticket> custom fields.
+
+=cut
+
+sub TicketCustomFields {
+    my $self = shift;
+
+    my $cfs = RT::CustomFields->new( $self->CurrentUser );
+    if ( $self->CurrentUserHasRight('SeeQueue') ) {
+        $cfs->SetContextObject( $self );
+       $cfs->LimitToGlobalOrObjectId( $self->Id );
+       $cfs->LimitToLookupType( 'RT::Queue-RT::Ticket' );
+        $cfs->ApplySortOrder;
+    }
+    return ($cfs);
+}
+
+# }}}
+
+# {{{ TicketTransactionCustomFields
 
 
-=item CustomFields
+=head2 TicketTransactionCustomFields
 
 
-Returns an RT::CustomFields object containing all global custom fields, as well as those tied to this queue
+Returns an L<RT::CustomFields> object containing all global and
+queue-specific B<transaction> custom fields.
 
 =cut
 
 
 =cut
 
-sub CustomFields {
+sub TicketTransactionCustomFields {
     my $self = shift;
 
     my $cfs = RT::CustomFields->new( $self->CurrentUser );
     if ( $self->CurrentUserHasRight('SeeQueue') ) {
     my $self = shift;
 
     my $cfs = RT::CustomFields->new( $self->CurrentUser );
     if ( $self->CurrentUserHasRight('SeeQueue') ) {
-        $cfs->LimitToGlobalOrQueue( $self->Id );
+       $cfs->LimitToGlobalOrObjectId( $self->Id );
+       $cfs->LimitToLookupType( 'RT::Queue-RT::Ticket-RT::Transaction' );
+        $cfs->ApplySortOrder;
     }
     return ($cfs);
 }
     }
     return ($cfs);
 }
@@ -442,41 +679,13 @@ sub CustomFields {
 
 =head2 _CreateQueueGroups
 
 
 =head2 _CreateQueueGroups
 
-Create the ticket groups and relationships for this ticket. 
+Create the ticket groups and links for this ticket. 
 This routine expects to be called from Ticket->Create _inside of a transaction_
 
 It will create four groups for this ticket: Requestor, Cc, AdminCc and Owner.
 
 It will return true on success and undef on failure.
 
 This routine expects to be called from Ticket->Create _inside of a transaction_
 
 It will create four groups for this ticket: Requestor, Cc, AdminCc and Owner.
 
 It will return true on success and undef on failure.
 
-=begin testing
-
-my $Queue = RT::Queue->new($RT::SystemUser); my ($id, $msg) = $Queue->Create(Name => "Foo",
-                );
-ok ($id, "Foo $id was created");
-ok(my $group = RT::Group->new($RT::SystemUser));
-ok($group->LoadQueueRoleGroup(Queue => $id, Type=> 'Cc'));
-ok ($group->Id, "Found the requestors object for this Queue");
-
-
-ok ((my $add_id, $add_msg) = $Queue->AddWatcher(Type => 'Cc', Email => 'bob@fsck.com'), "Added bob at fsck.com as a requestor");
-ok ($add_id, "Add succeeded: ($add_msg)");
-ok(my $bob = RT::User->new($RT::SystemUser), "Creating a bob rt::user");
-$bob->LoadByEmail('bob@fsck.com');
-ok($bob->Id,  "Found the bob rt user");
-ok ($Queue->IsWatcher(Type => 'Cc', PrincipalId => $bob->PrincipalId), "The Queue actually has bob at fsck.com as a requestor");;
-ok ((my $add_id, $add_msg) = $Queue->DeleteWatcher(Type =>'Cc', Email => 'bob@fsck.com'), "Added bob at fsck.com as a requestor");
-ok (!$Queue->IsWatcher(Type => 'Cc', Principal => $bob->PrincipalId), "The Queue no longer has bob at fsck.com as a requestor");;
-
-
-$group = RT::Group->new($RT::SystemUser);
-ok($group->LoadQueueRoleGroup(Queue => $id, Type=> 'Cc'));
-ok ($group->Id, "Found the cc object for this Queue");
-$group = RT::Group->new($RT::SystemUser);
-ok($group->LoadQueueRoleGroup(Queue => $id, Type=> 'AdminCc'));
-ok ($group->Id, "Found the AdminCc object for this Queue");
-
-=end testing
 
 =cut
 
 
 =cut
 
@@ -516,7 +725,9 @@ PrinicpalId The RT::Principal id of the user or group that's being added as a wa
 Email       The email address of the new watcher. If a user with this 
             email address can't be found, a new nonprivileged user will be created.
 
 Email       The email address of the new watcher. If a user with this 
             email address can't be found, a new nonprivileged user will be created.
 
-If the watcher you\'re trying to set has an RT account, set the Owner paremeter to their User Id. Otherwise, set the Email parameter to their Email address.
+If the watcher you\'re trying to set has an RT account, set the Owner parameter to their User Id. Otherwise, set the Email parameter to their Email address.
+
+Returns a tuple of (status/id, message).
 
 =cut
 
 
 =cut
 
@@ -529,45 +740,41 @@ sub AddWatcher {
         @_
     );
 
         @_
     );
 
+    return ( 0, "No principal specified" )
+        unless $args{'Email'} or $args{'PrincipalId'};
+
+    if ( !$args{'PrincipalId'} && $args{'Email'} ) {
+        my $user = RT::User->new( $self->CurrentUser );
+        $user->LoadByEmail( $args{'Email'} );
+        $args{'PrincipalId'} = $user->PrincipalId if $user->id;
+    }
+
     # {{{ Check ACLS
     # {{{ Check ACLS
+    return ( $self->_AddWatcher(%args) )
+        if $self->CurrentUserHasRight('ModifyQueueWatchers');
+
     #If the watcher we're trying to add is for the current user
     #If the watcher we're trying to add is for the current user
-    if ( $self->CurrentUser->PrincipalId  eq $args{'PrincipalId'}) {
+    if ( defined $args{'PrincipalId'} && $self->CurrentUser->PrincipalId  eq $args{'PrincipalId'}) {
         #  If it's an AdminCc and they don't have 
         #   'WatchAsAdminCc' or 'ModifyTicket', bail
         #  If it's an AdminCc and they don't have 
         #   'WatchAsAdminCc' or 'ModifyTicket', bail
-        if ( $args{'Type'} eq 'AdminCc' ) {
-            unless ( $self->CurrentUserHasRight('ModifyQueueWatchers')
-                or $self->CurrentUserHasRight('WatchAsAdminCc') ) {
-                return ( 0, $self->loc('Permission Denied'))
-            }
+        if ( defined $args{'Type'} && ($args{'Type'} eq 'AdminCc') ) {
+            return ( $self->_AddWatcher(%args) )
+                if $self->CurrentUserHasRight('WatchAsAdminCc');
         }
 
         #  If it's a Requestor or Cc and they don't have
         #   'Watch' or 'ModifyTicket', bail
         }
 
         #  If it's a Requestor or Cc and they don't have
         #   'Watch' or 'ModifyTicket', bail
-        elsif ( ( $args{'Type'} eq 'Cc' ) or ( $args{'Type'} eq 'Requestor' ) ) {
-
-            unless ( $self->CurrentUserHasRight('ModifyQueueWatchers')
-                or $self->CurrentUserHasRight('Watch') ) {
-                return ( 0, $self->loc('Permission Denied'))
-            }
+        elsif ( $args{'Type'} eq 'Cc' or $args{'Type'} eq 'Requestor' ) {
+            return ( $self->_AddWatcher(%args) )
+                if $self->CurrentUserHasRight('Watch');
         }
         }
-     else {
-            $RT::Logger->warn( "$self -> AddWatcher got passed a bogus type");
+        else {
+            $RT::Logger->warning( "$self -> AddWatcher got passed a bogus type");
             return ( 0, $self->loc('Error in parameters to Queue->AddWatcher') );
         }
     }
 
             return ( 0, $self->loc('Error in parameters to Queue->AddWatcher') );
         }
     }
 
-    # If the watcher isn't the current user 
-    # and the current user  doesn't have 'ModifyQueueWatcher'
-    # bail
-    else {
-        unless ( $self->CurrentUserHasRight('ModifyQueueWatchers') ) {
-            return ( 0, $self->loc("Permission Denied") );
-        }
-    }
-
-    # }}}
-
-    return ( $self->_AddWatcher(%args) );
+    return ( 0, $self->loc("Permission Denied") );
 }
 
 #This contains the meat of AddWatcher. but can be called from a routine like
 }
 
 #This contains the meat of AddWatcher. but can be called from a routine like
@@ -583,45 +790,52 @@ sub _AddWatcher {
     );
 
 
     );
 
 
-    my $principal = RT::Principal->new($self->CurrentUser);
-    if ($args{'PrincipalId'}) {
-        $principal->Load($args{'PrincipalId'});
+    my $principal = RT::Principal->new( $self->CurrentUser );
+    if ( $args{'PrincipalId'} ) {
+        $principal->Load( $args{'PrincipalId'} );
+        if ( $principal->id and $principal->IsUser and my $email = $principal->Object->EmailAddress ) {
+            return (0, $self->loc("[_1] is an address RT receives mail at. Adding it as a '[_2]' would create a mail loop", $email, $self->loc($args{'Type'})))
+                if RT::EmailParser->IsRTAddress( $email );
+        }
     }
     }
-    elsif ($args{'Email'}) {
-
+    elsif ( $args{'Email'} ) {
+        if ( RT::EmailParser->IsRTAddress( $args{'Email'} ) ) {
+            return (0, $self->loc("[_1] is an address RT receives mail at. Adding it as a '[_2]' would create a mail loop", $args{'Email'}, $self->loc($args{'Type'})));
+        }
         my $user = RT::User->new($self->CurrentUser);
         my $user = RT::User->new($self->CurrentUser);
-        $user->LoadByEmail($args{'Email'});
+        $user->LoadByEmail( $args{'Email'} );
+        $user->Load( $args{'Email'} )
+            unless $user->id;
 
 
-        unless ($user->Id) {
-            $user->Load($args{'Email'});
-        }
-        if ($user->Id) { # If the user exists
-            $principal->Load($user->PrincipalId);
+        if ( $user->Id ) { # If the user exists
+            $principal->Load( $user->PrincipalId );
         } else {
         } else {
+            # if the user doesn't exist, we need to create a new user
+            my $new_user = RT::User->new($RT::SystemUser);
 
 
-        # if the user doesn't exist, we need to create a new user
-             my $new_user = RT::User->new($RT::SystemUser);
+            my ( $Address, $Name ) =  
+               RT::Interface::Email::ParseAddressFromHeader($args{'Email'});
 
             my ( $Val, $Message ) = $new_user->Create(
 
             my ( $Val, $Message ) = $new_user->Create(
-                Name => $args{'Email'},
-                EmailAddress => $args{'Email'},
-                RealName     => $args{'Email'},
+                Name         => $Address,
+                EmailAddress => $Address,
+                RealName     => $Name,
                 Privileged   => 0,
                 Privileged   => 0,
-                Comments     => 'Autocreated when added as a watcher');
+                Comments     => 'Autocreated when added as a watcher'
+            );
             unless ($Val) {
                 $RT::Logger->error("Failed to create user ".$args{'Email'} .": " .$Message);
                 # Deal with the race condition of two account creations at once
             unless ($Val) {
                 $RT::Logger->error("Failed to create user ".$args{'Email'} .": " .$Message);
                 # Deal with the race condition of two account creations at once
-                $new_user->LoadByEmail($args{'Email'});
+                $new_user->LoadByEmail( $args{'Email'} );
             }
             }
-            $principal->Load($new_user->PrincipalId);
+            $principal->Load( $new_user->PrincipalId );
         }
     }
     # If we can't find this watcher, we need to bail.
         }
     }
     # If we can't find this watcher, we need to bail.
-    unless ($principal->Id) {
+    unless ( $principal->Id ) {
         return(0, $self->loc("Could not find or create that user"));
     }
 
         return(0, $self->loc("Could not find or create that user"));
     }
 
-
     my $group = RT::Group->new($self->CurrentUser);
     $group->LoadQueueRoleGroup(Type => $args{'Type'}, Queue => $self->Id);
     unless ($group->id) {
     my $group = RT::Group->new($self->CurrentUser);
     $group->LoadQueueRoleGroup(Type => $args{'Type'}, Queue => $self->Id);
     unless ($group->id) {
@@ -636,7 +850,7 @@ sub _AddWatcher {
 
     my ($m_id, $m_msg) = $group->_AddMember(PrincipalId => $principal->Id);
     unless ($m_id) {
 
     my ($m_id, $m_msg) = $group->_AddMember(PrincipalId => $principal->Id);
     unless ($m_id) {
-        $RT::Logger->error("Failed to add ".$principal->Id." as a member of group ".$group->Id."\n".$m_msg);
+        $RT::Logger->error("Failed to add ".$principal->Id." as a member of group ".$group->Id."".$m_msg);
 
         return ( 0, $self->loc('Could not make that principal a [_1] for this queue', $args{'Type'}) );
     }
 
         return ( 0, $self->loc('Could not make that principal a [_1] for this queue', $args{'Type'}) );
     }
@@ -669,17 +883,32 @@ sub DeleteWatcher {
 
     my %args = ( Type => undef,
                  PrincipalId => undef,
 
     my %args = ( Type => undef,
                  PrincipalId => undef,
+                 Email => undef,
                  @_ );
 
                  @_ );
 
-    unless ($args{'PrincipalId'} ) {
-        return(0, $self->loc("No principal specified"));
+    unless ( $args{'PrincipalId'} || $args{'Email'} ) {
+        return ( 0, $self->loc("No principal specified") );
+    }
+
+    if ( !$args{PrincipalId} and $args{Email} ) {
+        my $user = RT::User->new( $self->CurrentUser );
+        my ($rv, $msg) = $user->LoadByEmail( $args{Email} );
+        $args{PrincipalId} = $user->PrincipalId if $rv;
+    }
+    
+    my $principal = RT::Principal->new( $self->CurrentUser );
+    if ( $args{'PrincipalId'} ) {
+        $principal->Load( $args{'PrincipalId'} );
+    }
+    else {
+        my $user = RT::User->new( $self->CurrentUser );
+        $user->LoadByEmail( $args{'Email'} );
+        $principal->Load( $user->Id );
     }
     }
-    my $principal = RT::Principal->new($self->CurrentUser);
-    $principal->Load($args{'PrincipalId'});
 
     # If we can't find this watcher, we need to bail.
 
     # If we can't find this watcher, we need to bail.
-    unless ($principal->Id) {
-        return(0, $self->loc("Could not find that principal"));
+    unless ( $principal->Id ) {
+        return ( 0, $self->loc("Could not find that principal") );
     }
 
     my $group = RT::Group->new($self->CurrentUser);
     }
 
     my $group = RT::Group->new($self->CurrentUser);
@@ -688,13 +917,15 @@ sub DeleteWatcher {
         return(0,$self->loc("Group not found"));
     }
 
         return(0,$self->loc("Group not found"));
     }
 
+    my $can_modify_queue = $self->CurrentUserHasRight('ModifyQueueWatchers');
+
     # {{{ Check ACLS
     #If the watcher we're trying to add is for the current user
     # {{{ Check ACLS
     #If the watcher we're trying to add is for the current user
-    if ( $self->CurrentUser->PrincipalId  eq $args{'PrincipalId'}) {
+    if ( defined $args{'PrincipalId'} and $self->CurrentUser->PrincipalId  eq $args{'PrincipalId'}) {
         #  If it's an AdminCc and they don't have 
         #   'WatchAsAdminCc' or 'ModifyQueue', bail
         #  If it's an AdminCc and they don't have 
         #   'WatchAsAdminCc' or 'ModifyQueue', bail
-  if ( $args{'Type'} eq 'AdminCc' ) {
-            unless ( $self->CurrentUserHasRight('ModifyQueueWatchers')
+        if ( $args{'Type'} eq 'AdminCc' ) {
+            unless ( $can_modify_queue
                 or $self->CurrentUserHasRight('WatchAsAdminCc') ) {
                 return ( 0, $self->loc('Permission Denied'))
             }
                 or $self->CurrentUserHasRight('WatchAsAdminCc') ) {
                 return ( 0, $self->loc('Permission Denied'))
             }
@@ -703,21 +934,21 @@ sub DeleteWatcher {
         #  If it's a Requestor or Cc and they don't have
         #   'Watch' or 'ModifyQueue', bail
         elsif ( ( $args{'Type'} eq 'Cc' ) or ( $args{'Type'} eq 'Requestor' ) ) {
         #  If it's a Requestor or Cc and they don't have
         #   'Watch' or 'ModifyQueue', bail
         elsif ( ( $args{'Type'} eq 'Cc' ) or ( $args{'Type'} eq 'Requestor' ) ) {
-            unless ( $self->CurrentUserHasRight('ModifyQueueWatchers')
+            unless ( $can_modify_queue
                 or $self->CurrentUserHasRight('Watch') ) {
                 return ( 0, $self->loc('Permission Denied'))
             }
         }
         else {
                 or $self->CurrentUserHasRight('Watch') ) {
                 return ( 0, $self->loc('Permission Denied'))
             }
         }
         else {
-            $RT::Logger->warn( "$self -> DelWatcher got passed a bogus type");
-            return ( 0, $self->loc('Error in parameters to Queue->DelWatcher') );
+            $RT::Logger->warning( "$self -> DeleteWatcher got passed a bogus type");
+            return ( 0, $self->loc('Error in parameters to Queue->DeleteWatcher') );
         }
     }
 
     # If the watcher isn't the current user 
     # and the current user  doesn't have 'ModifyQueueWathcers' bail
     else {
         }
     }
 
     # If the watcher isn't the current user 
     # and the current user  doesn't have 'ModifyQueueWathcers' bail
     else {
-        unless ( $self->CurrentUserHasRight('ModifyQueueWatchers') ) {
+        unless ( $can_modify_queue ) {
             return ( 0, $self->loc("Permission Denied") );
         }
     }
             return ( 0, $self->loc("Permission Denied") );
         }
     }
@@ -735,7 +966,7 @@ sub DeleteWatcher {
     my ($m_id, $m_msg) = $group->_DeleteMember($principal->Id);
     unless ($m_id) {
         $RT::Logger->error("Failed to delete ".$principal->Id.
     my ($m_id, $m_msg) = $group->_DeleteMember($principal->Id);
     unless ($m_id) {
         $RT::Logger->error("Failed to delete ".$principal->Id.
-                           " as a member of group ".$group->Id."\n".$m_msg);
+                           " as a member of group ".$group->Id."".$m_msg);
 
         return ( 0,    $self->loc('Could not remove that principal as a [_1] for this queue', $args{'Type'}) );
     }
 
         return ( 0,    $self->loc('Could not remove that principal as a [_1] for this queue', $args{'Type'}) );
     }
@@ -880,8 +1111,8 @@ sub IsWatcher {
 
 =head2 IsCc PRINCIPAL_ID
 
 
 =head2 IsCc PRINCIPAL_ID
 
-  Takes an RT::Principal id.
-  Returns true if the principal is a requestor of the current queue.
+Takes an RT::Principal id.
+Returns true if the principal is a requestor of the current queue.
 
 
 =cut
 
 
 =cut
@@ -900,8 +1131,8 @@ sub IsCc {
 
 =head2 IsAdminCc PRINCIPAL_ID
 
 
 =head2 IsAdminCc PRINCIPAL_ID
 
-  Takes an RT::Principal id.
-  Returns true if the principal is a requestor of the current queue.
+Takes an RT::Principal id.
+Returns true if the principal is a requestor of the current queue.
 
 =cut
 
 
 =cut
 
@@ -996,15 +1227,15 @@ sub HasRight {
         Principal => $self->CurrentUser,
         @_
     );
         Principal => $self->CurrentUser,
         @_
     );
-    unless ( defined $args{'Principal'} ) {
-        $RT::Logger->debug("Principal undefined in Queue::HasRight");
-
+    my $principal = delete $args{'Principal'};
+    unless ( $principal ) {
+        $RT::Logger->error("Principal undefined in Queue::HasRight");
+        return undef;
     }
     }
-    return (
-        $args{'Principal'}->HasRight(
-            Object => $self,
-            Right    => $args{'Right'}
-          )
+
+    return $principal->HasRight(
+        %args,
+        Object => ($self->Id ? $self : $RT::System),
     );
 }
 
     );
 }