<html>
 <body>
  <div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
   <table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
    <tr>
     <td>
      This is an automatically generated e-mail. To reply, visit:
      <a href="https://reviewboard.asterisk.org/r/3016/">https://reviewboard.asterisk.org/r/3016/</a>
     </td>
    </tr>
   </table>
   <br />





 <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
 <br />







<div>




<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
 <thead>
  <tr>
   <th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
    <a href="https://reviewboard.asterisk.org/r/3016/diff/1/?file=48256#file48256line369" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/apps/app_directory.c</a>
    <span style="font-weight: normal;">

     (Diff revision 1)

    </span>
   </th>
  </tr>
 </thead>

 <tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
  <tr>

   <td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static int select_item_seq(struct ast_channel *chan, struct directory_item **items, int count, const char *dialcontext, struct ast_flags *flags, char *opts[])</pre></td>

  </tr>
 </tbody>



 
 

 <tbody>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
    <td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
    <th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">369</font></th>
    <td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb">   </span><span class="tb">  </span><span class="tb">  </span><span class="tb">  </span><span class="n">pbx_builtin_setvar_helper</span><span class="p">(</span><span class="n">chan</span><span class="p">,</span> <span class="s">"DIRECTORY_RESULT"</span><span class="p">,</span> <span class="s">"SELECTED"</span><span class="p">);</span></pre></td>
  </tr>

 </tbody>

</table>

<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</div>
<br />



<p>- Mark Michelson</p>


<br />
<p>On November 14th, 2013, 5:45 p.m. UTC, Jonathan Rose wrote:</p>








<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
 <tr>
  <td>

<div>Review request for Asterisk Developers, Mark Michelson and rmudgett.</div>
<div>By Jonathan Rose.</div>


<p style="color: grey;"><i>Updated Nov. 14, 2013, 5:45 p.m.</i></p>









<div style="margin-top: 1.5em;">
 <b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>


<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
 <table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
 <tr>
  <td>
   <pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
  </td>
 </tr>
</table>


<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
 <tr>
  <td>
   <pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
  </td>
 </tr>
</table>


<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">

 <li>/trunk/apps/app_directory.c <span style="color: grey">(402766)</span></li>

 <li>/trunk/CHANGES <span style="color: grey">(402766)</span></li>

</ul>

<p><a href="https://reviewboard.asterisk.org/r/3016/diff/" style="margin-left: 3em;">View Diff</a></p>







  </td>
 </tr>
</table>








  </div>
 </body>
</html>