Re: Proposal for speeding up TicketAcls

Moritz Lenz <[email protected]> Sat, 14 Dec 2013 08:02:06 +0100
Newsgroups gmane.comp.otrs.devel
Message-ID <[email protected]>
Hi Carlos,

On 12/14/2013 01:45 AM, Carlos Rodríguez wrote:
> On Dec 12, 2013, at 11:12 AM, Moritz Lenz <[email protected]
> <mailto:[email protected]>> wrote:
>
>> Hi,
>>
>> On 12/12/2013 09:18 AM, Martin Gruner wrote:
>>> Hi Moritz,
>>>
>>> this sounds great to me.
>>
>> and it turns out it's not too hard to implement either :-)
>> Seehttps://github.com/OTRS/otrs/pull/180
>>
>>> Am 10.12.13 13:58, schrieb Moritz Lenz:
>>>> As a further note, most information seems to show up twice in the
>>>> %Checks argument to the Acl module, for example both as $Checks{Owner}
>>>> and $Checks{Ticket}{Owner}. To get the desired speedup, a module which
>>>> only lists the check 'Ticket' would get whatever data TicketGet returns,
>>>> not the additional data that is currently added in method TicketAcl.
>>>
>>> I'm not sure about this one. Let's hear what Carlos Rodriguez  has to
>>> say about it.
>>
>> I have mitigated possible fallout from this approach by looking a bit
>> deeper into the ACL. If for example an ACL has
>>
>> Properties => {
>>   Ticket => {
>>      Queue => ...
>>   }
>> }
>>
>> then both the Ticket and the Queue data is fetched. Which makes all
>> the available ACL unit tests pass.
>>
> The problem I see here is that Properties hash does not mandatory needs
> a Ticket hash, but it could be for example:
>
> Properties => {
>      Queue => {
> Calendar => [‘MyCalendar’],
>      },
> },
>
> Then if we get the parameter ‘QueueID’ we needed also to do a QueueGet()
> in this case (in order to match the Calendar name with the ACL).

I've stumbled onto this problem during the implementation, and the 
solution is simple: TicketAcl also calls TicketGet and pre-fills the 
Ticket hash, because pretty much everything else depends on it.

There are lots of unit tests like this, for example 
https://github.com/moritz/otrs/blob/feature-ticketacl-master/scripts/test/Ticket/TicketACL.t#L363 
and the branch doesn't regress on any tests (and adds a few, fwiw).

Cheers,
Moritz
_______________________________________________
OTRS mailing list: dev - Webpage: http://otrs.org/
Archive: http://lists.otrs.org/pipermail/dev
To unsubscribe: http://lists.otrs.org/cgi-bin/listinfo/dev