[asterisk-dev] [Code Review] 4488: Super Awesome Company: Phase 1 - Patch 2 - Outside Connectivity!

[Matt Jordan]

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



/branches/13/configs/basic-pbx/extensions.conf
<https://reviewboard.asterisk.org/r/4488/#comment25250>

    Does this really need to be a separate context?
    
    I'm all for having contexts break up logical groupings of subroutines, but the dialplan here feels like it is getting a bit out of control. Subroutines can already be named via an actual extension name - when you have 'catch alls' in the various contexts, that feels like a sign that things aren't being set up at the right granularity.
    
    For example, this could just be in your [Internal] context:
    
    [Internal]
    
    exten = internal_setup,1,NoOp()
     same = n,Set(CDR_PROP(disable)=1)
     same = n,Return()
    
    
    Instead of invoking this with an explicit Goto, use GoSub. That way it can be called from anywhere, and doesn't require a lot of Gotos to jump around. Generally, you should always prefer Goto over GoSub, unless you *really* want them to leave the context they are in.



/branches/13/configs/basic-pbx/extensions.conf
<https://reviewboard.asterisk.org/r/4488/#comment25251>

    You have a Gosub here without a Return. That will unbalance the stack.



/branches/13/configs/basic-pbx/extensions.conf
<https://reviewboard.asterisk.org/r/4488/#comment25252>

    If this is a subroutine, it needs to Return().



/branches/13/configs/basic-pbx/extensions.conf
<https://reviewboard.asterisk.org/r/4488/#comment25253>

    Since your 'dialed-${DIALSTATUS}' extensions are subroutines, this needs to be invoked as a subroutine.



/branches/13/configs/basic-pbx/extensions.conf
<https://reviewboard.asterisk.org/r/4488/#comment25254>

    Put the comment that explains the extension above the extension.



/branches/13/configs/basic-pbx/extensions.conf
<https://reviewboard.asterisk.org/r/4488/#comment25257>

    Since this does more than just check for restricted numbers (it also does the actual outbound dial), I'd rename this context to something like [Outbound-Dial] or something along those lines.
    
    The fact that it also blocks restricted numbers is just a good thing.



/branches/13/configs/basic-pbx/extensions.conf
<https://reviewboard.asterisk.org/r/4488/#comment25255>

    I think you may want to think through how you want to comment your contexts/extensions/subroutines.
    
    Generally, comment blocks go *before* things they document, not after. Since there will be times you want to document both contexts and extensions - and comment blocks after the first line of an extension would look odd - I'd place this before the context name.
    
    As an ancillary comment, for subroutines, you may also want to document any parameters that are passed to the return, what it can return, and any channel variables that get set.



/branches/13/configs/basic-pbx/extensions.conf
<https://reviewboard.asterisk.org/r/4488/#comment25256>

    I don't think should includ Pre-Internal, which masks the "Internal" context. Instead, this should include Internal, and Internal should call the sub-routine to hide the CDR if necessary.



/branches/13/configs/basic-pbx/extensions.conf
<https://reviewboard.asterisk.org/r/4488/#comment25258>

    I'm assuming we're going to replace the prompts eventually? :-)



/branches/13/configs/basic-pbx/modules.conf
<https://reviewboard.asterisk.org/r/4488/#comment25259>

    Don't you need a config file for this?


- Matt Jordan


On March 13, 2015, 9:32 a.m., rnewton wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4488/
> -----------------------------------------------------------
> 
> (Updated March 13, 2015, 9:32 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Howdy, here is another patch for the Super Awesome Company configuration. We are still in phase 1. The general requirements are posted on the wiki: https://wiki.asterisk.org/wiki/display/AST/Super+Awesome+Company
> 
> The specific requirements this patch meets are below:
> 
> pjsip.conf
> 
>  * SIP ITSP configuration example and have place holders for the required authentication bits.
>  ** Assume that Asterisk does not have a public IP address, and sits behind a NAT with its desk phones.
>  * Have outbound registration to the SIP trunk, and an endpoint that represents the SIP trunk.
>  * Inbound calls received from the SIP trunk should go into their own context.
> 
> extensions.conf
> 
>  * Match the outbound dial request so that it can only dial US area codes.
>  ** Don't let people dial 900 numbers, international numbers, or any other numbers that could result in a charge
>  * Inbound calls from the SIP trunk should hit a basic Auto Attendant that prompts them for the extension to dial, after greeting them to SAC.
>  * If an inbound call matches a DID that maps to a specific extension/device, dial that extension/device directly.
> 
> Billing
> 
>  * Make sure CDRs output all calls that are from/to the SIP trunk. These should be logged to a CSV.
>  * For intra-office calls, kill the CDRs.
> 
> Additional Requirements Noted:
> 
>  * For outbound calls, each SAC employee’s 10-digit DID number is provided as their Caller ID.
>  * Voicemail may be accessed remotely by employees who dial 256-555-1234. When employees dial voicemail remotely, they must input both their mailbox number and their pin code.
>  * 7, 10 and 10+1 digit dialing for local and long distance calls.
>  * Internal dialing of otherwise inbound features, 
>  ** 1100 to reach the main IVR.
>  * The IVR options possible without getting into Phase 2.
> 
> 
> Diffs
> -----
> 
>   /branches/13/configs/basic-pbx/pjsip.conf 432866 
>   /branches/13/configs/basic-pbx/modules.conf 432866 
>   /branches/13/configs/basic-pbx/logger.conf 432866 
>   /branches/13/configs/basic-pbx/extensions.conf 432866 
> 
> Diff: https://reviewboard.asterisk.org/r/4488/diff/
> 
> 
> Testing
> -------
> 
> Setup with a Digium Cloud Services trunk and a few internal phones.
> Internal to Internal calls.
> Calls Internal to voicemail and other features.
> External to internal DID calls.
> External to internal feature calls.
> 
> Basically tried to call as many ways as I could through all the various features. Everything seemed to work.
> 
> 
> Thanks,
> 
> rnewton
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150316/55c268e4/attachment-0001.html>