[asterisk-dev] [Code Review] 3016: app_directory: Set DIRECTORY_RESULT variable to indicate why Directory application finished

[Mark Michelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3016/#review10207
-----------------------------------------------------------


I have made a similar set of changes for app_confbridge, and in that set of changes, I opted for the string "ERROR" where you used "FAILED". I think we need to decide on one of these two strings and use it for both applications. To me, it doesn't really matter which one gets used, it just needs to be consistent.

On a side note, after inspecting app_directory's code, I'm shocked that there are not more exit points for timeouts.


/trunk/apps/app_directory.c
<https://reviewboard.asterisk.org/r/3016/#comment19536>

    Instead of setting the variable to "SELECTED" here, I suggest doing it in select_entry() in the case where it is going to return successfully. Otherwise, in the failure scenario, the channel variable will be "SELECTED" temporarily and then switch over to "FAILED" once control returns to directory_exec(). Making the suggested change will result in the channel being set to "FAILED" only.


- Mark Michelson


On Nov. 14, 2013, 5:45 p.m., Jonathan Rose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3016/
> -----------------------------------------------------------
> 
> (Updated Nov. 14, 2013, 5:45 p.m.)
> 
> 
> Review request for Asterisk Developers, Mark Michelson and rmudgett.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Directory is a bit of a mess stylistically... so I tried to manage this as best as I could. The premise is simple enough. Directory should set a reason for exit when it is finished. The reasons include:
> 
>      OPERATOR    user requested operator by pressing '0' for operator
>      ASSISTANT   user requested assistant by pressing '*' for assistant
>      TIMEOUT     user pressed nothing and Directory stopped waiting
>      HANGUP      user's channel hung up
>      SELECTED    user selected a user from the directory and is routed
>      USEREXIT    user pressed '#' from the selection prompt to exit
>      FAILED      directory failed in a way that wasn't accounted for.
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_directory.c 402766 
>   /trunk/CHANGES 402766 
> 
> Diff: https://reviewboard.asterisk.org/r/3016/diff/
> 
> 
> Testing
> -------
> 
> I tested that each of the values would be received from their respective actions with the exception of FAILED... which I put in there to account for exit conditions that I wasn't aware of anyway.
> 
> I'm currently writing a testsuite test to verify the behavior of this feature.
> 
> 
> Thanks,
> 
> Jonathan Rose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20131118/02da3ecc/attachment.html>