[Home]

Summary:ASTERISK-01670: [patch] chan_agent.c: adds new options for CLI Functions AgentLogin() and AgentCallbackLogin()
Reporter:vulture (vulture)Labels:
Date Opened:2004-05-21 05:56:11Date Closed:2008-01-15 15:11:31.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Applications/app_queue
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) chan_agent_20040628_diff_u.patch
( 1) chan_agent_20040628.patch
( 2) chan_agent_20041014_diff_u.patch
( 3) chan_agent_20041014b_diff_u.patch
( 4) chan_agent_new_options_20040523_agents_conf.patch
( 5) chan_agent_new_options_20040523_chan_agent_c.patch
( 6) chan_agent-new_callback_options.patch
( 7) chan_agent-new_options20040522.patch
Description:This patch adds quite a few new features into __login_exec () of channels/chan_agent.c for AgentLogin() and AgentCallbackLogin(). Only one option ('s') existed previously.

Syntax/Documentation:

AgentLogin([AgentNo][|options]):
Asks the agent to login to the system.  Always returns -1.  While
logged in, the agent can receive calls and will hear a 'beep'
when a new call comes in.  The agent can dump the call by pressing
the star key.
The option string may contain zero or more of the following characters:
'a-f' - Max Login Attemps (1-6)
's' -- silent login - do not announce the login ok segment


AgentCallbackLogin([AgentNo][|[options][exten]@context]):
Asks the agent to login to the system with callback.
The agent's callback extension is called (optionally with the specified
context.
Options: (defaults are 3 login attempts, play announcement, hangup, and return -1)
a-f - Max Login Attemps (1-6)
g - play goodbye only on login failure
G - never play goodbye
h - hangup only on login failure (returns -1 on fail or 0 on success)
H - never hangug (returns 0)
l- hangup when agent logs off overriding option h/H (returns -1)
L - hangup when agent logs off overriding option h/H and g/G to play goodbye (returns -1)
s - do not play the announcement
v - Set Local Variables AGENTNUMBER, AGENTPASS, AGENTEXTEN on successful login
V - Same as v - but set the variables globally

****** ADDITIONAL INFORMATION ******

These new options can allow the use of a menu driven system to adjust queue membership after logging on/off.

Patches most useful modes of operation:
AgentCallbackLogin(|ghv)
AgentCallbackLogin(|ghLv)
AgentCallbackLogin(|GHv)
AgentCallbackLogin(|GHLv)

Patch is against
Repository revision:
1.72    /usr/cvsroot/asterisk/channels/chan_agent.c,v
Comments:By: Mark Spencer (markster) 2004-05-21 10:53:33

This is a nice start.  I've got a few suggestions though:

1) How about adjusting the max login attempts in the [general] section of agents.conf.  Is that sufficient?  If you really want it in the agentlogin/agentcallbacklogin line, you should do it like m(4) or m(6) but definitely don't have 6 different options for each length.

2) I'm a little concerned about four different values for hangup. Let me make a suggestion:

a) In general, if priority n + 1 exists, return 0 instead of -1

b) If priority n + 101 exists and there is a failed login, go to that priority, otherwise if step n + 1 exists, continue there.

c) If you're populating the variables agent number, agent exten, etc. then you can use extension logic to determine a logon or logoff and there is no need for an additional option.

3) We can simply allow "s" to run silently (including goodbye messages).  If you really want a "goodbye" in a particular circumstance you'll be able to do it with extension logic.

4) You don't really need the "v" option, you can just set them -- although i'm a little leary of putting the password in a variable.  Is there some reason you think that's necessary or adds any value?

5) Setting global variables is a bad idea in general.  Besides, one could use SetGlobalVar in their extensions.conf if they really wanted that.

If you like all these suggestions, that gets us down to just one option "s" and still retains backwards compatibility with the rest of the system.
By: Mark Spencer (markster) 2004-05-21 10:53:49

Oh, and one more thing, please use "cvs diff -u" thanks!
By: vulture (vulture) 2004-05-21 14:29:58

Well - to satisfy my enviroment, I will be wanting more than one mode of operation for different call in extensions as we are a company that does customer service for other companies, thus my abundance of options. However I do realize that most enviorments will probably just have a single mode, most desirably set thru agents.conf. I have tried to make sure that all changes will be backwards compatible as currently any option not defined will be ignored.

Let me address each of your points in reverse order:
I'll make sure to use the correct diff options for patch on the next one!

5. I didn't know there was a SetGlobalVar as I am new to * within the last month but rapidly learning.

4. I am currious as to the exact operation of local variables, If I set a variable inside say exten => 100,1,AgentCallbackLogin(), will that var stay within ext 100 or be available to the entire context as in someone reading the var in ext 101?

3. I will need to look at opt s in more detail to see what it really does.

2. I think that that solution will suffice.

1. I agree that 6 different options are not the best way to do it and I shall change that. My solution to this problem is to use the default option of 3 hard coded, override that with a setting in agents.conf, then override again for a local variable AGENTMAXTRIES on any uses where you want to use something other than the global default/agents.conf setting. using a # in the options for the function will not work as it will break the exten setting.

I will work on condensing the settings into the agents.conf file, and setup override variables that can be used to change the mode on-the-fly.

Look forward to a new patch sometime this weekend - hopefully!

Thanks markster for your input!
By: Mark Spencer (markster) 2004-05-22 10:58:43

Okay I'll try to address the things you've asked and i'll address them in the same order as you have them listed:

5) No problem.

4) Local variables are stored within the channel structure.  As such they are available from anything that runs on that channel including other applications.

3) The idea is just to have one "s" or silent option, no need for lots of them right?

2) Excellent.

1) Perfect.
By: vulture (vulture) 2004-05-22 13:42:21

OK- Got the second version of this done up! Haven't fully tested it, but it's here for someone to go over with a fine tooth comb.

This patch returns the functions back to a single option 's' for no announcements.

New settings in agents.conf - defaults hard coded are listed
maxtries=integer (<1 will disable checking, default is 3)
playannouncement=boolean (sets option 's' globally, default is 1/yes)
playgoodbye=boolean (default is 1/yes)
setvars=boolean (default is 0/no)
hangupcall=always|never|fail or use boolean for always/never (default is always)
hanguplogff=boolean (has no effect if hangupcall=always, default is 1/yes)

New Channel Scope Variables settable in extensions.conf
These variables will override the defaults and agents.conf settings for the call only.
AGENTMAXTRIES=integer
AGENTPLAYANNOUNCEMENT=boolean
AGENTPLAYGOODBYE=boolean
AGENTSETVARS=boolean
AGENTUPDATECDR=boolean
AGENTHANGUPCALL=always|never|fail or use boolean for always/never
AGENTHANGUPLOGOFF=boolean (has no effect if hangupcall=always)
AGENTACKCALL=always + boolean
AGENTAUTOLOGOFF=integer
AGENTWRAPUPTIME=integer

When these variables are set, a verbose message will be displayed like below:
Saw variable AGENTHANGUPCALL=false, setting hangup_call to: 0 on Channel 'SIP/130-52bc'.
Saw variable AGENTMAXTRIES=2, setting max_tries to: 2 on Channel 'SIP/130-0eae'.

The setvars feature will set the following Channel scope variables when not hanging up the call on exiting AgentCallbackLogin
AGENTNUMBER
AGENTPASS
AGENTEXTEN

The AgentLogin() function will always exit with -1
The AgentCallbackLogin() function will exit with -1 if it is hanging up the call or 0 if it is not.
If not hanging up and login was successful, chan->priority is untocuhed sending the call to n+1.
If login failed due to max_tries being exceededm then priority will be bumped up  to n+101 to follow suit with other apps/functions on failure of some sort.
By: vulture (vulture) 2004-05-22 13:48:00

Do not use original patch 'new_callback_options.patch' from 5/21

Note: Current chan_agent.c in cvs does not acutally have a working max_tries for login and will act as if max_tries=0. The code was not completly finished as there was no try counter and no way to set the max to something other than 3 if it did have that counter.

New options in agents.conf should be backwards compatible as they will simply be ignored. Override channel scope variables will also be ignored.
By: Mark Spencer (markster) 2004-05-22 17:52:20

This is a definite improvement.  A few more things I'd like to mention:

1) Is atoi(NULL) universally okay?  Might be worthwhile to check for NULL first before calling it.

2) Is there value in storing the AGENTPASS?  This makes me a little nervous.

3) The agent.conf needs to be patched to explain the new settings in that file.

4) Setting variables is okay, there is no need for that to be an option.

5) This patch seems to still use lots of environment variables instead of the handling I suggested in #2 of my original commentary.

6) What happens to AGENTEXTEN in a non-callback agent login?

In summary, I believe the following to be completely unnecessary:

AGENTPLAYANNOUNCMENT (You can just use "s" if you don't want it)
AGENTPLAYGOODBYE (If you want just the goodbye, use "s" and then Playback(vm-goodbye)
AGENTSETVARS (No reason not to set them)
AGENTHANGUPCALL (Can just fall through if n + 1 exists, see #2 in original post)
AGENTHANGUPLOGOFF (ditto)

As for the "AGENTUPDATECDR" it's a little hard to imagine why this option would be anything but global for the most part.  Do you have a reasonable reason why you think it ought to be there?  If so, maybe a "u" option would be more concise, but I'm not even sure that's necessary.

AGENTMAXTRIES, AGENTNUMBER, and AGENTEXTEN should all be documented in the doc/README.variables file.

Thanks again!
By: vulture (vulture) 2004-05-22 20:06:29

1. Good Point - No sense in crashing * when the user puts in bogus data! - I'll add null checking into the atoi calls and see if there are any other things needing data validity.

2. This patch is tweaked to allow maximum flexibilty for my needs, as I will be having over 10 different departments with sometimes 2+ queues each, and each requiring different settings, based upon their unique enviroment. My company does customer support for other companies, thus the overkill.

3. Documentation is not my strong suit, but I'll get those done once we have a patch that is a little more solid and settings/vars aren't changing as much.

4. This ties in with #2 of setting the AGENTPASS variable. Explicitly requiring setvars to be turned on will keep other apps from being able to access that password, agent # and callback exten. Since setvars will set the variables on the channel and will make that agent actived/deactivated upon the exit of AgentCallbackLogin, the agent will not be keeping the channel open for more that 2 minutes in most cases.

5. Simply exiting to n+1 or n+101 does not take into account that the agent was logging off, not on. The function will basically have 3 outputs, login ok, login fail, and logged off (which includes a login ok).
The other thing to take into account is the current operation of AgentCallbackLogin is to always hangup. This patch is meant to be completly backwards compatible with older config files. If for example a user has one of n+1 or n+101 in their config from prior setups or they have been using those prioritys in a goto for example, then they will all of a sudden have a changed mode of operation, thus requiring changes to their config to return that behavior of supposedly better code to it's previous ways. Thus my sticking with 2 settings to determine hangup behavior.
hangupcall to state if we want it to always hangup, hangup on fail, or not hangup.
hanguplogoff to hangup if logging off, overriding hangupcall=never/no.
Both of these settings combined can yeild you 3 ways of exiting, hangup, n+1 and n+101. In some instances you may want to hangup if logging off, because the next step wouldn't make much sense, such as making a custom log entry that the user has logged on or maybe taking the AGENTNUMBER variable to lookup what other agent is to be automatically logged off, or removed from a queue. Once again this patch is written to give me and others the maximum flexibilty when exiting AgentCallbackLogin without breaking older configs.

6. Vars are only set when the channel is going to be kept open in callback mode. So what do you want to see if login failed or agent logged off in reguards to this variable? Set it to -1 if logging out and don't set it if login failed?


AGENTPLAYANNOUNCEMENT and agents.conf:playannouncement allows us to eventually fade out option 's' all togetther.
AGENTPLAYGOODBYE is there because I want to have the announcemnet played of agent logged in/off, but might not want it to say goodbye. I could remove playback of vm-goodbye alltogether from the function so I can do a playback(), but that wouldn't be backwards compatible.
SETVARS - User may want those variables not accessible from other apps working with the channel on purpose and keeps the security of the password a little more secure. An example of this is to use callerid to direct calls into one way that it will be captured (such as external caller), and another that they won't (internal caller).

AGENTUPDATECDR is there because I was making all variables in agents.conf that could be changed for a paticular agent adjustable on-the-fly. This gets back to me having 10 different departments that correspond to different client companies in which I may want to have it tracked for some, but not others.

And I said documentation is not my strong suit - you would never guess from this posting.

I think I've addressed all of your issues above, and have the following tasks to complete:
1. Data validity for user settings in agents.conf channel scope variables.
2. Determining if AGENTPASS will ever be useful to me.
3. Setting for AGENTEXTEN when user logs off or fails to login.
4. Documentation change of agents.conf and doc/README.variables.
By: vulture (vulture) 2004-05-22 21:45:20

I had a vision in the shower!
User will still need to use hangupcall=yes/always|no/never|loginok/ok|loginfail/fail
Caller will be hungup on matching case and stay online for non-matching case. -Adding "loginfail" as long version of "fail"
-Adding "loginok" or "ok" to do opposite of loginfail
This covers all possible states of login for hangup

AGENTLOGINSTATE=on|off|fail
User will still need to use "hangupcall" != yes to ever see this variable
This removes the option "hanguplogoff" from agents.conf and override variable

Users of the new functionality can do logic inside extensions.conf to send caller to a, b, or c based upon that variable.

channel priority will still goto n+101 on fail

Hope this solves some option confusion and simplifies it for everyone!

Thanks for your comments on this markster!
By: Mark Spencer (markster) 2004-05-23 01:55:14

Perhaps a conversation on IRC would be more helpful, but lets go back over this and I'll address your comments to mine.

1. (we were in agreement)

2. The question is specific to AGENTPASS -- I don't see any practical use, and I'm searching for whether you have one.

3. Okay when we've settled on the patch, we'll do the documentation, but documentation has to come in the same time the patche does.

4. If we decide not to have AGENTPASS, there is no reason not to populate the variables.

5.  AGENTLOGINSTATE is perfect for all that.

6.  I'm not sure what's most logical maybe we should talk about it.

why get rid of the "s" option?  it seems reasonable right?  much more efficient and consise than the two other options.
By: vulture (vulture) 2004-05-23 07:16:13

option 's' can stay - thats fine with me - i'm just giving the user the option via a variable because everything else is available like that.

AGENTPASS has passed away - I don't think I can come up with a use for it either at this time. I know where to put it if i do need it so I'll leave it out.

setvars has also gone to a higher plane of existence as it was chained to AGENTPASS when AGENTPASS got hit by a semi!

Is this an acceptable way to check for nulls?
agents.conf version:
if (!strcasecmp(v->name, "optionnamehere") && strlen(v->value))
channel variable version:
if (pbx_builtin_getvar_helper(chan, "AGENTOPTIONNAMEHERE") && strlen(pbx_builtin_getvar_helper(chan, "AGENTOPTIONNAMEHERE")))

AGENTEXTEN will only be set if agent is logging in, not on fail or logoff

Inconsitient maxtries/maxlogintries has been fixed to be maxlogintries on both agents.conf and override variable.

Override variable naming rule: agents.conf name of option in caps prefixed with AGENT - same values for both .conf and variables.

agents.conf file patch is posted and contains the proper documentation for these new options.

README.variables did not look like the correct place to talk about the on-the-fly override variables, so I put it in agents.conf

All comments are welcome!
By: vulture (vulture) 2004-05-23 07:19:49

Null checking as described above seems to be working for me and is in the patch that I just posted. If there is a better way to do multiple files for cvs diff -u to combine the patch into one file, please let me know!
Thanks
By: Mark Spencer (markster) 2004-05-23 19:55:37

Making progress, but:

1) Unless I'm missing something, this patch still has the million variables at its inception including the ones to handle the hangup stuff.  

2) "cvs diff -u" should give you a big patch with everything in it if you have modified more than one file, or you can explicitly list all the files on the command line, e.g. (cvs diff -u channels/chan_agent.c docs/README.variables).

3) The checks for strlen(v->value) aren't really necessary, i meant NULL return values from the getvar_helper which I believe *can* return NULL.  strlen does NOT check for NULL and will most assuredly segfault on a NULL pointer.

4) README.variables is the right place for documenting special purpose variables, and you'll notice there are relatively few of them.  For that reason, I'm not excited about adding a bunch of variables which duplicate functionality that can otherwise be done with extension logic fairly easily, and which will be very rarely if ever used.

5) As a continuation of item 4, combining a check for the existance of priority n + 1, combined with the use of AGENTLOGINSTATE can be used to eliminate AGENTPLAYGOODBYE and  AGENTHANGUPCALL.  Those variables can be dropped in their entirity with no ill effects whatsoever.

6) Again a continuation of item 4, AGENTPLAYANNOUNCEMENT is also useless by the availability of the "s" option, which could even be represented by a non-special variable which would simply be in the command line, e.g. exten => 100,1,AgentLogin(${AGENTFLAGS}).  

7) Again a continuation of item 4, the options AGENTACKCALL, AGENTWRAPUPTIME, AGENTACKCALL, AGENTUPDATECDR can all already be specified on a per-agent or global basis.  Do you see any practical application in which they would need to be overridden on a case-by-case basis at the time of login?  i.e. Can you envision a time when an agent might need to be able to change that principle when they're logging in as the *same* agent?  Clearly, you could emulate this functionality by having someone login as agent 2200 for one set of features and 3200 for another.  I'm not sure how you're selecting that feature set now if you are.

In general: I'm all for making sure the agent stuff is extensible and flexible, but I also place an extremely high value on simplicity and as such am very nervous about adding "clutter" to the code -- that is, fields, options, variables that are extremely rarely or never used and can be worked around fairly straight forwardly in those cases.

I truly cannot express to you the extreme importance of simplicity.
By: Mark Spencer (markster) 2004-05-23 20:05:12

Since this has gone back and forth a number of times, I think it might be worthwhile to say a few words here that really apply to all contributions but are perhaps clearly demonstrated here in this bug report.

As a a contributer, your goal is to get the functionality you need into the base Asterisk core, so that it is no longer your responsibility to keep merging those changes, and so that others can benefit from and improve upon your work.

As Asterisk's author and maintainer, it is my goal to be a good custodian of the Asterisk code base, to allow it to expand and become a more powerful piece of code, while maintaining its cleanliness, ease of maintenance, and keeping it true to my vision -- all in a timely fashion and all the while working to meet the needs of the developers that are key to Asterisk's success as an Open Source project (not to mention trying to run Digium, too).

If I merge changes that I feel unnecessarily complicate or risk the integrity of the Asterisk code base, clearly I am doing a disservice to the community at large.  If I simply add the functionality "the way I think it should go off the top of my head" then I risk adding the functionality in a way that doesn't serve your purpose and thus I still leave you having to maintain separate code changes.

One way or another, we will reach the medium in which these two goals intersect (hopefully sooner rather than later) and I don't want you to feel like your changes are being made in vain, but instead I want you to know I appreciate your patience in helping improve Asterisk.
By: vulture (vulture) 2004-05-24 01:26:08

I think we should get in IRC and try to resolve the misunderstandings between us on how these items should be accomplished.

1. Well, you didn't like going with new options in addition to "s" and you don't like the override variables.

7. I don't see how you can change those values on-the-fly. Giving my users more than 1 agent number is not an acceptable way of giving them the flexability to have different values.

2. Ok - thanks

3. That still does not tell me what code I should use - I'm nowhere near an expert in C. I've worked with quite a few programing languages, but not enough to even begin making something as complex as *.

4. I didn't see that those variables would feel at home there, but next time, depending how many are left, I get them in there somewhere.

5+6. We'll hash these out in irc

IRC is definitly where we need to go! I'll try to catch you in there sometime soon.

Thanks

edited on: 05-24-04 01:08
By: Mark Spencer (markster) 2004-06-21 14:47:22

Where do we stand with this one?
By: vulture (vulture) 2004-06-21 15:52:54

Sorry - been busy on other projects

The code to exit with setting a chan var is done, but we still need to discuss allowing changes to the default settings on the agents before launching the AgentCallbackLogin().

I will also need to make sure that I merge the new FreeBSD changes that have been added recently.

Will try to catch you in irc this week
By: jrollyson (jrollyson) 2004-06-23 14:27:30

One thing that would be very useful, on AgentCallBackLogin, a flag is needed to disable the prompt when called with a null extension - this allows silent logout.

Other than that, this looks very promising.


Oh, and the obligitory question... Have you sent in a disclaimer ? :)
By: vulture (vulture) 2004-06-29 01:13:44

The patch chan_agent_20040628_diff_u.patch contains the changes for:
chan_agent.c
agents.conf.sample
README.variables

The final results of this patch:
1. new config file option maxlogintries to set maximum number of login attempt allowed per call.
2. new config file option goodbye to set sound to play for goodbye message.
3. new channel variable AGENTSTATUS=on|off|fail - set upon exiting AgentCallbackLogin().
4. new channel variable AGENTEXTEN=callback extension - set upon exiting AgentCallbackLogin() when agent is logging in.
5. AgentCallbackLogin() will exit at priority n+1 if it exists, otherwise hangup.
6. config file options are checked to make sure they are not a strlen of 0.

This patch is ready for final testing, before being committed.

I have sent in the disclaimer!
By: Olle Johansson (oej) 2004-07-16 11:34:11

Sent test request to mailing list.
By: twisted (twisted) 2004-07-23 20:58:47

No replies from mailing list yet... Houskeeping notice - need input. :)
By: Olle Johansson (oej) 2004-08-14 15:45:58

Does this patch apply to current CVS?
By: twisted (twisted) 2004-08-25 18:28:54

since markster has owned this one, i'm not going to close it, just simply nudge both him and vulture to get a status update. ;)
By: Mark Spencer (markster) 2004-09-16 14:49:22

This patch still has some potential crashes in it since pbx_builtin_getvar_helper can return NULL, other than that I think it's about ready.
By: bobman (bobman) 2004-09-18 21:26:29

What happened to the AGENTNUMBER variable set at login?  To me, it's one of the most useful, as I can then use that and AddQueueMember(support|Agent/${AGENTNUMBER}).  That'll get me around a behaviour of Queue when none of my agents are logged in.
By: vulture (vulture) 2004-09-20 04:32:50

${AGENTNUMBER} is still in the patch - I must have missed it when stating final adjustments. AGENTNUMBER should be set along with AGENTSTATUS and AGENTEXTEN
By: bobman (bobman) 2004-09-20 12:41:29

Echoing oej, I attempted to apply this patch to current CVS-HEAD (chan_agent.c 1.81) as of 20/9/2004 and it failed to apply chunks 5 and 6.  I'm not familiar with why patch would reject specific bits, but it looks like a simple problem of it being offset too much.

edited on: 09-20-04 13:39
By: Mark Spencer (markster) 2004-10-06 19:49:51

Any chance of getting this updated for CVS head so we can get it merged in?
By: umarsear (umarsear) 2004-10-07 13:49:06

I hope I'm not missing the obvious.

Is there option for silent agentcallbacklogout ?. If not that would be nice to add
By: vulture (vulture) 2004-10-14 08:15:39

New patch has been uploaded and is awaiting testing against HEAD. I have dropped out some of the additional checking I added before on reading the ini file. Should work fine with out it.

Use chan_agent_20041014b_diff_u.patch
The first one had a couple lines that were missed in updating the patch.
By: Mark Spencer (markster) 2004-10-24 09:20:45

Added to CVS, thanks!
By: Russell Bryant (russell) 2004-10-24 20:52:49

not included in 1.0
By: Digium Subversion (svnbot) 2008-01-15 15:11:31.000-0600

Repository: asterisk
Revision: 4083

U   trunk/channels/chan_agent.c

------------------------------------------------------------------------
r4083 | markster | 2008-01-15 15:11:30 -0600 (Tue, 15 Jan 2008) | 2 lines

Add new features to agent stuff (bug ASTERISK-1670)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=4083