Summary:	ASTERISK-10685: New application app_pickupextn.c
Reporter:	Andrew Smith (eurodrew555)	Labels:
Date Opened:	2007-11-05 06:56:26.000-0600	Date Closed:	2007-12-24 22:07:35.000-0600
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Applications/NewFeature
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) app_pickupchan_LATEST.c ( 1) app_pickupchan.c
Description:	We attempted to implement call pickup using the cmd Pickup(extension[@context]) but found it was limited to only being able to pickup a channel that is in the DIAL state. This application app_pickupextn.c extends the Asterisk pickup application to be able to do the following: - will pickup any ringing CHANNEL (whether it be SIP, IAX, or ZAP). - will pickup any ringing CHANNEL which can be from a DIAL or QUEUE I believe the command Pickup() is restricted in that it only picks up from a DIAL. Examples of using the command: PickupExtn(SIP/usera) PickupExtn(SIP/usera&SIP/userb) PickupExtn(SIP/usera&SIP/userb&SIP/userc) You could also pickup SIP/usera&SIP/userb&SIP/userc by simply PickupExtn(SIP/user) which is useful but represents a possibly issue at the same time (see additional information). ****** ADDITIONAL INFORMATION ****** To implement call pickup group restrictions we do this in MySQL - we basically look at the user who is making the dial and check which pickupgroup they are in, and then do a search for any users in that callgroup. We then use this variable to call the command PickupExtn(${EXTENS}) There is a potential issue with this application in that the channel search will search for part of the name, so that if we had SIP/brad and SIP/bradley if we do PickupExtn(SIP/brad) it will attempt to pickup SIP/brad or SIP/bradley regardless of the call pickup restrictions. We are now using this application in production to allow us to pickup from queues, and while we know about this issue we haven't had any problems arising from it. It would clearly be preferable to check for the exact channel name to ensure we enforce the call pickup restrictions.
Comments:	By: Jason Parker (jparker) 2007-11-05 10:59:28.000-0600 Rather than create a new application, why not just add options to the existing app_pickup? By: Andrew Smith (eurodrew555) 2007-11-05 11:41:17.000-0600 I believe the application app_pickup is used to pickup an extension that has been dialled rather than a channel that is ringing. Pickup(extension[@context]) We wanted to be just able to pickup SIP/user if it is ringing by just using PickupExtn(SIP/user). I suppose this is what BristuffPickUpChan does, although we have not yet looked at Bristuff or any of the source code (I've only noticed it on voip-info.org - http://www.voip-info.org/wiki/index.php?page=Asterisk+cmd+BristuffPickUpChan). Perhaps PickupExtn would also be better named PickupChan considering that is what it is doing... By: Jason Parker (jparker) 2007-11-12 17:17:47.000-0600 That makes sense.  I would agree with the name PickupChan. By: Tilghman Lesher (tilghman) 2007-11-13 16:21:52.000-0600 Okay, so I think what we're waiting for is for you to upload an updated patch.  Also, please rename the fields that you're using with context and extension to just channel, since that is what it's doing.  Otherwise, the code is very confusing. By: Andrew Smith (eurodrew555) 2007-11-20 03:37:26.000-0600 We are currently working on modifying this code, will upload the new code this week. By: Andrew Smith (eurodrew555) 2007-11-26 06:37:13.000-0600 New code has been uploaded. Command is now named PickupChan and is used in the following format: PickupChan(Technology/extension) For example if we have a SIP user called 2821, we would use PickupChan(SIP/2821) And this will pickup the first ringing channel for that user regardless of whether the call is from a QUEUE or DIAL. Have made further modifications so that it matches on the exact channel, rather than a part of the name, as this could of potentially picked up the wrong channel. Have also followed Corydon76's note to change the variable names - hope this clears up any confusion! By: Andrew Smith (eurodrew555) 2007-11-26 06:58:24.000-0600 Sorry just realised the description was still set as PickupExtn Also the correct description for using this app is therefore: PickupChan(channel[&channel...] e.g. Where channel can be SIP/2821, ZAP/2 etc I have also uploaded a revised patch called app_pickupchan_LATEST.c to replace app_pickupchan.c as I can't upload with the same name. By: Tilghman Lesher (tilghman) 2007-11-26 10:22:07.000-0600 Okay, a couple more items, really just syntax issues: 1) Don't use C++ style comments (//).  Always use C-style comments (/* */) in all code. 2) Don't comment out code if you don't want to run it.  Remove it entirely.  If you want to temporarily remove some code, use "#if 0" preprocessor logic. 3) There are good number of lines indented with spaces, while they should be indented only with tab characters. 4) There's also a good number of spacing issues I'm not going to enumerate.  Look in doc/CODING-GUIDELINES for a complete list. If you'd prefer, I can fix these at commit, but many people prefer to fix up files themselves, so they can get into practice. By: Andrew Smith (eurodrew555) 2007-11-26 13:05:54.000-0600 OK thanks for the advice Corydon76. We will clean up the code as best we can ourselves and resubmit the patch. Will upload the new code in the next few days. By: Andrew Smith (eurodrew555) 2007-11-28 03:09:57.000-0600 Revised app_pickupchan.c has been uploaded with the following changes: 1)       Have changed the comments 2)       Have removed the commented code 3)       Have checked that all lines are indented with TABS 4)       Have changed the Functions and all IF statements to confirm to the coding guidelines (I think) By: Digium Subversion (svnbot) 2007-12-24 22:07:35.000-0600 Repository: asterisk Revision: 94773 A   trunk/apps/app_pickupchan.c ------------------------------------------------------------------------ r94773 | tilghman | 2007-12-24 22:07:34 -0600 (Mon, 24 Dec 2007) | 3 lines Add pickup by channel (Closes issue ASTERISK-10685) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=94773