[asterisk-dev] [Code Review] 3008: app_confbridge: Add an option 'announce_join_leave_review' for users that adds recording review to the 'announce_join_leave' name recording feature

[Matt Jordan]

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



/trunk/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/3008/#comment19432>

    The way you've implemented this, where announce_join_leave_review 'augments' another option, is odd.
    
    The idea of having one option augment another sounds interesting, but in practice, is probably going to be more frustrating than useful.
    
    For example, the following won't work:
    
    [my_user]
    type=user
    announce_join_leave_review=Yes
    
    And yet it is a valid user profile that will be loaded. I just won't get the behavior I want.
    
    These two options should be mutually exclusive of each other. You should only be able to specify one of them, but you should only have to specify one of them to get the behavior you want.
    
    Alternatively, you could go with a single option with multiple values.


- Matt Jordan


On Nov. 11, 2013, 8:08 p.m., Jonathan Rose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3008/
> -----------------------------------------------------------
> 
> (Updated Nov. 11, 2013, 8:08 p.m.)
> 
> 
> Review request for Asterisk Developers, Mark Michelson and rmudgett.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Pretty straight forward really.  Adds the same basic functionality provided by the 'i' option to MeetMe into Confbridge (which already implemented functionality similar to the 'I' option through announce_join_leave)
> 
> 
> Diffs
> -----
> 
>   /trunk/configs/confbridge.conf.sample 402603 
>   /trunk/apps/confbridge/include/confbridge.h 402603 
>   /trunk/apps/confbridge/conf_config_parser.c 402603 
>   /trunk/apps/app_confbridge.c 402603 
>   /trunk/CHANGES 402603 
> 
> Diff: https://reviewboard.asterisk.org/r/3008/diff/
> 
> 
> Testing
> -------
> 
> Made sure the recording feature worked with and without the review option in the expected way (with and without recording review). It's a pretty straight forward patch though.
> 
> 
> Thanks,
> 
> Jonathan Rose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20131111/b85b5074/attachment.html>