Summary:	ASTERISK-06414: [patch] Add option to display channel variables in AgentCalled events
Reporter:	Christopher Howard (cchasteria)	Labels:
Date Opened:	2006-02-24 09:48:19.000-0600	Date Closed:	2006-06-22 10:43:34
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Applications/app_queue
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) 20060613__bug6589.diff.txt ( 1) ast_queue_chanvars.patch
Description:	If the eventwhencalled is set to yes in queues.conf so that agent events are enabled and the new option introduced in this patch "channelvarsonevent" is set to yes queues.conf file then the channel variables will be included in the "AgentCalled" event.   In extensions.conf : exten => 1111,1,Answer() exten => 1111,n,Set(foo=12345) exten => 1111,n,Set(bar=abc123) exten => 1111,n,Queue(test) When an agent answers the event will look like the following: Event: AgentCalled Privilege: agent,all AgentCalled: Agent/007 ChannelCalling: IAX2/2888-6 CallerID: unknown CallerIDName: unknown Context: inqueue Extension: 1111 Priority: 4 ChanVars: bar=abc123|foo=12345| If no channel variables are set on the channel then the ChanVars line will read: ChanVars: NONE
Comments:	By: Olle Johansson (oej) 2006-03-09 14:42:02.000-0600 Why do you add context, exten, priority only when chanvarsonevent is set - that is not very logical unless you have a reason. Seems like if they are needed, they should always be there - at least not depend on the chanvars setting... I am also a bit hesitant on the NONE - do we have that syntax anywhere else in manager? Any previous examples on multiple values separated by vertical bar? Manager is complicated enough as it is, so we have to be very careful before we add new syntax constructs to it. The output format for channel variables can later be implemented in the SIP channel, so we need to agree upon something that fits the current way of constructing manager events. By: Christopher Howard (cchasteria) 2006-03-09 15:49:31.000-0600 The context,exten and priority should be there for both cases.  Something must have happend when I created the diff.   Maybe use a blank if there are no channel variables? I'm not sure of multiple values separated bu a verticle bar in an event but it is used to specify channel variables on an Originate Action. Action: Originate Channel: SIP/101test Context: default Exten: 8135551212 Priority: 1 Callerid: 3125551212 Timeout: 30000 Variable: var1=23|var2=24|var3=25 ActionID: ABC4567890123456789 I'm not sure I inderstand the line "output format for channel variables can later be implemented in the SIP channel"  In my case these channel variables are not necessarily on a sip channel. By: Olle Johansson (oej) 2006-03-10 00:40:25.000-0600 Thanks for the example. So we use "Variable" in Action: Originate but "ChanVar" here - not a good solution. We need to continue to use "Variable" even if I feel that ChanVar is a better name. The SIPpeer action does not currently list the channel variables for a SIPpeer - but it can easily be added. That was an example on why we need a syntax that not only works here, but in other events/actions as well. Looking forwarard to a new patch! /O By: Jon Hood (squinky86) 2006-03-14 13:48:24.000-0600 * context, exten, and priority seem to be there every time from what I can see. * oej is correct on the "NONE" usage; by convention (GetVar, etc), it has been blank. * oej is correct on the ChanVars as the tick instead of Variable; however, variable suggests that we will be getting A variable back, not multiple variables. I'd like to propse adding another name to the manager called "Variables" since we will, in many if not most casses, be getting multiple variables back. I also propose that we switch the Originate action to use "Variables" instead of "Variable."  "Variable" should be used when only one variable is expected, such as setvar or getvar. Variables should be used when multiple variables may be used, such as in originate actions and agentcalled events. * updated patch to apply to current svn By: Tilghman Lesher (tilghman) 2006-03-18 11:11:25.000-0600 I would suggest the following changes: 1.  eventwhencalled=vars to turn this on.  It is confusing to have a separate option that does nothing when another option is not turned on.  This also conforms to how other options work in queues.conf and elsewhere. 2.  A separate Variable: line in the output for EACH variable set.  If no variables are set, then there will simply be no Variable: line in the output.  Otherwise, we have to worry about variables which have | or some other delimiter in their value. By: Christopher Howard (cchasteria) 2006-03-29 08:03:31.000-0600 We can modify this patch to use eventwhencalled=vars and that would give us yes/no/vars where vars implies yes.  Also, I considered making the patch use a separate variable line in the output for each variable set however this could cause some issues for libraries that have predifined mapping of these variables.  I know that the Asterisk-Java library would spit out warnings saying that these channel variables are not mapped.  Personally I think it would be cleaner to do one per line but for consistency I think we should do it the same way we set the variables on the Originate action. By: Christopher Howard (cchasteria) 2006-04-28 21:11:20 Any more thought on this issue?  The reason behind this is that we set info about the caller on the channel so we can retreive it when it gets to the agent via the manager.  I think this is very valuable for anyone building call center applications. By: Serge Vecher (serge-v) 2006-05-08 18:53:39 bweschke: could you please comment on this one? By: BJ Weschke (bweschke) 2006-05-08 19:02:19 I agree with Corydon76 on both points. Since we're kicking out all of the variables, we really have no control on would *could* be in them (eg. a delimiter). That's not really the case with Originate and setting them. If an updated patch can be put up to correct these conditions, I'd have no problem getting it into the trunk. By: Jon Hood (squinky86) 2006-05-11 14:28:05 This applies against the current trunk. Each variable gets its own line. The following cases were accounted for: 1) there are no variables: no "Variable: " line is printed 2) buffers (too many variables)- I make sure we don't overflow any character pointers when creating the string of "Variable: " lines. It was considerably more difficult, but at least we're assured we won't overflow any buffers. disclaimer on file By: Jon Hood (squinky86) 2006-05-11 14:32:33 And also a small change to queues.conf.sample for the new variable. By: Serge Vecher (serge-v) 2006-05-11 14:35:42 squinky86: thanks for an updated patch. Can you please formatting in a couple of places: 1. Be generous with 'spaces' in assignments and in if, while statements. 2. The following line doesn't need curly brackets as per coding guidelines. + if (strcmp(varPtr, "\n\0") == 0) { + break; + } 3. Please put all changes in one patch file, instead of two. Thanks. By: Jon Hood (squinky86) 2006-05-11 15:12:31 voilla By: Serge Vecher (serge-v) 2006-05-11 15:35:37 hmm, item 1 is not quite voila :). Let me elaborate: +if(pbx_builtin_serialize_variables(qe->chan,vars,sizeof(vars))) { -> if (pbx_builtin_serialize_variables(qe->chan, vars, sizeof(vars))) { + varPtr=vars; -> varPtr = vars; + /*we don't want to overflow our buffer*/ -> /* we don't want to overflow our buffer */ Please follow through on the code throughout the patch. Thanks. By: Jon Hood (squinky86) 2006-05-11 16:26:49 Ah, that makes much more sense; sorry for not asking for clarification earlier. Let's see if this patch fares better. By: Jon Hood (squinky86) 2006-06-02 16:53:22 The latest attached patch applies to current svn trunk and is cleaned up a little more. By: Jon Hood (squinky86) 2006-06-05 17:42:18 I am unable to monitor this issue (bug in mantis?). Please send me an email manually when something changes. By: Jon Hood (squinky86) 2006-06-12 17:21:17 Please remove the older patches; only the current (and smaller, cleaner) one is needed. What is the status of this and what is keeping it from being included in asterisk? By: Tilghman Lesher (tilghman) 2006-06-13 03:04:27 This would make the patch a little more consistent, as well as adding the parameter that I had suggested before. By: Serge Vecher (serge-v) 2006-06-16 12:03:22 squinky86: does Corydon86's patch match your needs? By: Jon Hood (squinky86) 2006-06-19 09:08:06 It looks like it does. I haven't been able to test it extensively, but it seems to work sufficiently. Thanks Corydon! By: Tilghman Lesher (tilghman) 2006-06-22 10:43:34 Committed to trunk.