[dev] [commits] Horde branch Horde_LoginTasks-Refactor created. b70b84343d05538bad914a0738ae25e63ab31806

[Gunnar Wrobel]

Hi Micheal,

Quoting Michael M Slusarz <slusarz at horde.org>:

> Quoting Gunnar Wrobel <p at rdus.de>:
>
>>> For example, if you have the following tasks:
>>>
>>> DISPLAY_CONFIRM_NO
>>> DISPLAY_CONFIRM_YES
>>> DISPLAY_NONE
>>> DISPLAY_NOTICE
>>> DISPLAY_CONFIRM_YES
>>> DISPLAY_NONE
>>>
>>> There should be 3 confirmation screens generated: 1 with  
>>> CONFIRM_NO and CONFIRM_YES tasks,
>>
>> As CONFIRM_NO != CONFIRM_YES the check "$v['task']->display !=  
>> $display" in needDisplay() should disallow displaying both on the  
>> same screen or do I misunderstand that section?
>
> I think the code is correct.  When adding the task with addTask(),  
> CONFIRM_NO will have 'display' set to true and $_addFlag will be  
> false.  CONFIRM _YES will also have 'display set to true since it is  
> not DISPLAY_NONE and $_addFlag is not set.  Therefore, when you run  
> needDisplay(), both tasks will skip the break statement (in our  
> example, CONFIRM_NO will skip because 'displa'y is true and $display  
> is null; CONFIRM_YES will skip because 'display' is true and

Everything correct ...

> $v['task']->display == $display).

... except for this statement. In the first round when handling  
CONFIRM_NO $display will be set to CONFIRM_NO while  
$v['task']->display is CONFIRM_YES in the second round.

Just to be certain I tested this outside of my test suite by adding  
two dummy tasks to a horde base installation. I do indeed only get one  
confirmation.

After confirming this first task I get redirected to the portal  
instead of getting the remaining login tasks displayed. This is  
something I also see in my unit tests. The redirection will transport  
me to the URL initially provided to runTasks() instead of moving to  
the login tasks page again.

I'm going to provide suggested fixes in the branch.

>
>> I'm done with the core part of the refactoring. Having the abstract  
>> Horde_LoginTasks_Backend class should be sufficient for me to plug  
>> the system into something other than Horde. Can you give me some  
>> feedback if you think the modifications are okay and if I can merge?
>
> I will take a look.  It would be best to move all Horde-specific  
> backend stuff to Core - like I did with the Notification driver.   
> That would remove all horde core-specific code from LoginTasks.

This would mean moving Horde_LoginTasks_Backend_Horde into  
Horde_Core_LoginTasks_Backend, right? That would make sense to me. I'd  
convert Horde_LoginTasks_Backend to an interface in that case (rather  
than the abstract class it is now).

>
>> Two additional items...
>>
>> One question concerning ONCE tasks: Why is the _once value set when  
>> the task is being added to the tasklist? Shouldn't it be set once  
>> the task was actually executed? I didn't test this but I assume the  
>> user could login, visit the confirmation page, kill his session,  
>> login again and thus avoid execution of the task.
>
> That's fine.  To me this is an implicit decision by the user that  
> they didn't want the task to be run.

Ah. I was thinking of tasks such as the TOS agreement that are  
required to be confirmed. I'll have to think about this again.

>
>> The Horde_Util::getFormData() in runTasks() is still inconvenient  
>> as it accesses $_POST directly so I guess I will add a workaround  
>> there later.
>
> You could use Horde_Variables instead (from the Util package).  The  
> Util package is already marked as a dependency of LoginTasks.

Thanks for the hint, I'll look into that.

I noticed some additional things when I started using the package  
outside of Horde now. I can't place the redirection feature into the  
backend as I'm in a Zend MVC context. And redirection needs to be done  
by the controller. So runTasks() must return the redirection address  
so that the controller can perform the actual redirection. This is  
something already supported as runTasks() returns the result of the  
redirect call to the backend. Hope that is okay.

And I need to consider if it is possible to have tasks that redirect  
to their own specific URLs rather than a global login task URL.

Cheers,

Gunnar

>
> michael
>
> -- 
> ___________________________________
> Michael Slusarz [slusarz at horde.org]
>
>
> -- 
> Horde developers mailing list - Join the hunt: http://horde.org/bounties/
> Frequently Asked Questions: http://horde.org/faq/
> To unsubscribe, mail: dev-unsubscribe at lists.horde.org
>




-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digitale PGP-Unterschrift
URL: <http://lists.horde.org/archives/dev/attachments/20100309/b778fd44/attachment.bin>