chad at opensourcery
Jan 8, 2009, 8:38 AM
Post #5 of 7
(1124 views)
Permalink
|
|
Re: [PATCH] Specify Queue ACLS at queue declaration on initialdata
[In reply to]
|
|
Thank you for the feedback and tips!
I have made changes based on the feedback that I *think* should take
care of the issues.
As I mentioned in a previous email I was planning to roll all the
Handle.pm enhancements I think people might like into one patch. Here it is.
This covers the patch from the start of this thread, but fixed, the
patch I submitted that is now in the ticket:
http://rt3.fsck.com/Ticket/Display.html?id=13036 , as well as some new
functionality.
I added ExpandItems() and several ExpandXXX() functions that use it.
Essentially there are many cases where it would be nice to specify a
list of items in an initialdata item instead of duplicating the item for
each thing you want to apply it to. But on some things, such as ACL
doing so for every key people might want to provide a list to would
create a huge set of nested loops.
This is an alternative to doing a for loop on each thing that can be a
list. Essentially it is a function to take the items that can be a list,
and duplicate them in advance. It does one list at a time and as such it
is much more maintainable.
This also allows a developer to add new items that can be lists very
easily w/o creating more nested loops. The one caveat I can think of is
that there are some things that can be lists that we do not want to
duplicate the item for. The best example is Queues => in @CustomFields,
we do NOT want to create a new field for each queue, we want the same
field in each. Luckily Queue => already supports a list in CustomFields.
There is one part specifically I suspect you might want to ditch, thats
the ExpandScrips, see the comment in the patch.
-Chad Granum
OpenSourcery (www.opensourcery.com)
Jesse Vincent wrote:
> Reasonable intent, but the code itself is fairly hard to read.
>
> "$return and $ACL" sounds kind of bizarre.
>
> Standard convention is ->id rather than ->Id
>
> I'd shy away from using $ACL and @ACL in the same
>
> chunk of code.
>
> $item->{ ACL } isn't really the same as standard RT style. I'd probably
> do $item->{'ACL'} instead.
>
> I'd move your new if block to inside the else clause running off the end
> of your patch and phrase it as
>
> if (my $acl = delete $item->{'ACL'}) {
>
>
> }
>
> though I suppose you'd then be passing it into the create, which is
> suboptimal.
>
> Well, at least lowercase your variable, since the new allcaps scalar
> variable looks like a global ;)
>
>
>
>
>> -Chad Granum
>>
>> OpenSourcery (www.opensourcery.com)
>>
>
>
>> --- lib/RT/Handle.pm-orig 2009-01-07 14:57:06.000000000 -0800
>> +++ lib/RT/Handle.pm 2009-01-07 15:24:17.000000000 -0800
>> @@ -768,7 +768,14 @@
>> $RT::Logger->debug("Creating queues...");
>> for my $item (@Queues) {
>> my $new_entry = new RT::Queue($RT::SystemUser);
>> + my $ACL = delete $item->{ ACL };
>> my ( $return, $msg ) = $new_entry->Create(%$item);
>> +
>> + if ( $return and $ACL ) {
>> + $_->{ Queue } = $new_entry->Id for @$ACL;
>> + push @ACL => @$ACL;
>> + }
>> +
>> unless ( $return ) {
>> $RT::Logger->error( $msg );
>> } else {
>>
>
>
>
>
>
>> _______________________________________________
>> List info: http://lists.bestpractical.com/cgi-bin/mailman/listinfo/rt-devel
>>
>
>
>
|
rt-QueueACL.patch
signature.asc
