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

Mon Jul 8 18:44:32 CDT 2013

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

I made it a little less than halfway through the entire diff, but I figure I'd post what I have before I leave for the evening.

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

    If it's configurable, then rather than placing <literal>beep</literal> here, you should name the configuration option (custom_beep in this case) and place it in a <replaceable></replaceable> tag.

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

    Someone like opticron may want to chime in here, but most AMI events now send entire channel snapshots instead of bits and pieces. So the same approach may be desired here. Getting the strings to send in the AMI event is easy. Documenting is still a bunch of copypasta at this point.

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

    Any particular reason not to give both name and number here?

    'Bob <1234>' instead of just '1234'?

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

    Possibly another instance where an entire channel snapshot could be useful instead of just a single piece of information.

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

    I would suggest a rename of this from "global" to "general" since the actual name of the reserved configuration section in agents.conf is "general". For someone getting CLI help, this will make things more clear.

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

    Consider adding default values to configOptions.

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

    It's worth noting that these options have no effect if ackcall is disabled.

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

    For alignment purposes, these single-bit fields should be at the end of the struct.

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

    It seems wasteful to continue any further if you already know that you are going to return in error. Why try to setup the interval hook if setting up the DTMF hook already failed?

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

    s/manally/manually/

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

    The second line should be agent->caller_bridge = NULL;

    The caller_bridge won't be destroyed because it will always be NULL

- Mark Michelson

On July 5, 2013, 9:12 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2657/
> -----------------------------------------------------------
> 
> (Updated July 5, 2013, 9:12 p.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 393766 
>   /trunk/UPGRADE.txt 393766 
>   /trunk/apps/app_agent_pool.c PRE-CREATION 
>   /trunk/channels/chan_agent.c 393766 
>   /trunk/configs/agents.conf.sample 393766 
>   /trunk/configs/queues.conf.sample 393766 
>   /trunk/include/asterisk/bridging.h 393766 
>   /trunk/include/asterisk/config_options.h 393766 
>   /trunk/include/asterisk/stasis_channels.h 393766 
>   /trunk/main/bridging.c 393766 
>   /trunk/main/stasis_channels.c 393766 
> 
> 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/20130708/6c54335a/attachment-0001.htm>