[asterisk-dev] [Code Review] 2657: Replace chan_agent with app_agent_pool.

Thu Jul 4 11:27:06 CDT 2013

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


/trunk/UPGRADE.txt
<https://reviewboard.asterisk.org/r/2657/#comment17832>

    Since the application was removed, we can remove the other upgrade notice that the 'c' option isn't supported.


/trunk/apps/app_agent_pool.c
<https://reviewboard.asterisk.org/r/2657/#comment17827>

    I can see needing two other statuses here:
    
    * LOGGED_OUT - agent was logged out of the pool
    * HUNGUP - agent hung up
    
    More on that when the login app exits.
    
    
/trunk/apps/app_agent_pool.c
<https://reviewboard.asterisk.org/r/2657/#comment17825>

    Write a wiki page in the Developer section of the Asterisk wiki on the "proper" way to write comparison functions for AO2 containers, both for hash table as well as RB trees.


/trunk/apps/app_agent_pool.c
<https://reviewboard.asterisk.org/r/2657/#comment17826>

    Just curious, why a RB tree?


/trunk/apps/app_agent_pool.c
<https://reviewboard.asterisk.org/r/2657/#comment17829>

    In this function you note that the locking order is logged in channel, then agent. Go ahead and promote that as a doxygen \note on this function declaration - that's useful information for people looking at how to log an agent.


/trunk/apps/app_agent_pool.c
<https://reviewboard.asterisk.org/r/2657/#comment17820>

    Consider reducing indentation here by bailing early and/or jumping to a cleanup point if the bridge features cannot be initialized.


/trunk/apps/app_agent_pool.c
<https://reviewboard.asterisk.org/r/2657/#comment17821>

    This kind of off nominal path could probably use an ERROR message (or at least a debug so we can figure out why none of the agents are working)


/trunk/apps/app_agent_pool.c
<https://reviewboard.asterisk.org/r/2657/#comment17823>

    It's a subtle point that the ast_bridge_join returns when the agent either:
    
    (a) Leaves the holding bridge because they've hung up or otherwise moved on
    
    (b) Was removed from the holding bridge, did a call with a caller, and the bridging system returned through the stack of both bridges to this point
    
    A comment or something that describes this somewhere might be appropriate.


/trunk/apps/app_agent_pool.c
<https://reviewboard.asterisk.org/r/2657/#comment17822>

    You could probably just bail out here immediately, moving your logic from of the end of the function up to here.


/trunk/apps/app_agent_pool.c
<https://reviewboard.asterisk.org/r/2657/#comment17835>

    Should this failure path reset the agent->devstate as well? caller_abort_agent doesn't appear to do this and probably should.


/trunk/apps/app_agent_pool.c
<https://reviewboard.asterisk.org/r/2657/#comment17833>

    Should this hang up the calling channel on exit?
    
    I would think we'd return 0 here and let the dialplan decide what to do.


/trunk/apps/app_agent_pool.c
<https://reviewboard.asterisk.org/r/2657/#comment17831>

    As noted previously, I'd set a channel variable here that noted whether or not the channel was hung up or was logged off.
    
    Since a log off doesn't necessarily hangup the channel, shouldn't this return 0?


/trunk/apps/app_agent_pool.c
<https://reviewboard.asterisk.org/r/2657/#comment17828>

    Document both of these events at the top of the file. Reference the events from the Agents action.


/trunk/channels/chan_agent.c
<https://reviewboard.asterisk.org/r/2657/#comment17824>

    Yay! It's dead!


- Matt Jordan


On July 4, 2013, 1:14 a.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2657/
> -----------------------------------------------------------
> 
> (Updated July 4, 2013, 1:14 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-21554
>     https://issues.asterisk.org/jira/browse/ASTERISK-21554
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> The ill conceived chan_agent is no more.  It is now replaced by app_agent_pool.
> 
> Agents login using the AgentLogin() application as before.  The AgentLogin() application no longer does any authentication.  Authentication is now the responsibility of the dialplan.  (Besides, the authentication done by chan_agent did not match what the voice prompts asked for.)
> 
> Sample extensions.conf
> [login]
> ; Sample agent 1001 login
> ; Set COLP for in between calls so the agent does not see the last caller COLP.
> exten => 1001,1,Set(CONNECTEDLINE(all)="Agent Waiting" <1001>)
> ; Give the agent DTMF transfer and disconnect features when connected to a caller.
> same => n,Set(CHANNEL(dtmf-features)=TX)
> same => n,AgentLogin(1001)
> same => n,NoOp(AGENT_STATUS is ${AGENT_STATUS})
> same => n,Hangup()
> 
> [caller]
> ; Sample caller direct connect to agent 1001
> exten => 800,1,AgentRequest(1001)
> same => n,NoOp(AGENT_STATUS is ${AGENT_STATUS})
> same => n,Hangup()
> 
> ; Sample caller going through a Queue to agent 1001
> exten => 900,1,Queue(agent_q)
> same => n,Hangup()
> 
> Sample queues.conf
> [agent_q]
> member => Local/800 at caller,,SuperAgent,Agent:1001
> 
> Under the hood operation overview:
> 1) Logged in agents wait for callers in an agents holding bridge.
> 2) Caller requests an agent using AgentRequest()
> 3) A basic bridge is created, the agent is notified, and caller joins the basic bridge to wait for the agent.
> 4) The agent is either automatically connected to the caller or must ack the call to connect.
> 5) The agent is moved from the agents holding bridge to the basic bridge.
> 6) The agent and caller talk.
> 7) The connection is ended by either party.
> 8) The agent goes back to the agents holding bridge.
> 
> To avoid some locking issues with the agent holding bridge, I needed to make some changes to the after bridge callback support.  The after bridge callback is now a list of requested callbacks with the last to be added the only active callback.  The after bridge callback for failed callbacks will always happen in the channel thread when the channel leaves the bridging system or is destroyed.
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 393634 
>   /trunk/UPGRADE.txt 393634 
>   /trunk/apps/app_agent_pool.c PRE-CREATION 
>   /trunk/channels/chan_agent.c 393634 
>   /trunk/configs/agents.conf.sample 393634 
>   /trunk/configs/queues.conf.sample 393634 
>   /trunk/include/asterisk/bridging.h 393634 
>   /trunk/include/asterisk/config_options.h 393634 
>   /trunk/include/asterisk/stasis_channels.h 393634 
>   /trunk/main/bridging.c 393634 
>   /trunk/main/stasis_channels.c 393634 
> 
> Diff: https://reviewboard.asterisk.org/r/2657/diff/
> 
> 
> Testing
> -------
> 
> Tested the features of agents and they work as expected.
> 1) Agents login and get MOH and any COLP set by the dialplan before AgentLogin().
> 2) Agents logging in with a local channel chain wait for the local channels to optimize out.
> 3) Caller channels directly running AgentRequest() are able to connect to the agent.
> 4) Callers going through Queue() can connect to agents via local channels.  The local channels can optimize themselves out.
> 5) Tested recording agent calls.  Note: Agent calls cannot be recorded currently if the caller came in on an optimizing local channel because MixMonitor audio hooks are not being handled by the optimization.
> 6) Caller COLP is shown to the agent before the agent accepts the call.
> 7) The original COLP when the agent logged in is restored while the agent is between calls.
> 8) Tested agent wrapup time.
> 9) Tested agent not acknowledging the call before autologoff time.
> 
> Many more.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130704/5e1af6b3/attachment-0001.htm>