[asterisk-dev] [Code Review] app_queue per member ringinuse [ignorebusy] / set chan group on answer / change behaviour of neg penalty

[jrose]

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

Ship it!



/trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/1119/#comment7584>

    small typo on [general].



/trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/1119/#comment7581>

    I understand that you are changing this to be more stylistically appropriate with the current guidelines, but you aren't really touching anything in this function in a significant way.  Stuff like this should probably be broken apart from this patch and submitted in a separate commit marked as just changing code to meet current coding guidelines.



/trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/1119/#comment7582>

    If by 'returned value', you mean the buffer string mentioned right below the comment here, this is not making sure the returned value is NULL, it's making sure the returned value is a zero length string.  It's obvious what you mean in this case, but it is a somewhat important distinction.  A string with no termination character can be pretty dangerous, even though it isn't NULL.



/trunk/configs/queues.conf.sample
<https://reviewboard.asterisk.org/r/1119/#comment7580>

    Got a little sloppy with the spelling here in the sample.  Of course, the spelling should be 'negative' (it's right everywhere else), but also make sure that if you use the variable name verbatim you spell it the same was as the variable does it.  Mixed capitalization is fine, but using spaces or changing from underscores to camel case or something... that makes it less immediately obvious that you are referring to the variable name itself.
    
    ; Negative_penalty_invalid will treat members with a negative penalty as logged off
    ;
    ;negative_penalty_invalid = no



/trunk/configs/queues.conf.sample
<https://reviewboard.asterisk.org/r/1119/#comment7583>

    You are probably seeing a recurring theme by now... just about everything is a comment nitpick or something else that's mostly minor.  This is probably the worst for overall nitpickyness.
    
    In 'for autopause delay seconds', autopausedelay should be one word.  It's purely a matter of consistency.


All in all, this is looking good.  I've played around with these settings a bit just using dialplan stuff (haven't gotten fancy with postgres realtime on it yet) and it all seems to be in working order. Feel free to ship it once you've taken care of the junk I mentioned.  I mean, unless I come back here in an hour screaming about segfaults or something.

- jrose


On 2011-06-07 05:11:11, irroot wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1119/
> -----------------------------------------------------------
> 
> (Updated 2011-06-07 05:11:11)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch is a combination of 3 changes id like to submit.
> 
> 1)allow a per member ringinuse setting called ignorebusy this was a requirement of a customer and and has been most useful
> 2)when a call is answered set a channel group this can be used to count active calls as well as be used to not send calls to users flaged above in the dialplan
> 3)when a member penalty is negative see this as a logged off agent this is a behavior change that allows "removing" realtime agents as well as making IMHO more logic sense if the penalty is neg its not valid 
> 
> NB: this is a global config option now as not to change behavior.
> 
> 4)allow a "grace" period for autopause queue option "autopausedelay" [as requested by a customer we support]
> 
> Enjoy the fish
> 
> 
> This addresses bugs 18794, 18825 and 18826.
>     https://issues.asterisk.org/jira/browse/18794
>     https://issues.asterisk.org/jira/browse/18825
>     https://issues.asterisk.org/jira/browse/18826
> 
> 
> Diffs
> -----
> 
>   /trunk/configs/queues.conf.sample 322174 
>   /trunk/apps/app_queue.c 322174 
>   /trunk/CHANGES 322174 
>   /trunk/UPGRADE.txt 322174 
> 
> Diff: https://reviewboard.asterisk.org/r/1119/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> irroot
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110628/dd77d0d4/attachment.htm>