[asterisk-dev] [Code Review]: Added VM_INFO dialplan function to return voicemail user information

[mjordan]

> On Nov. 5, 2011, 8:08 p.m., mjordan wrote:
> > The only real comment I have with this patch is that you duplicate the MAILBOX_EXISTS function.  Since there's already a dedicated function for testing if a mailbox exists, I wouldn't duplicate it here.  Also, if you get some time, it'd be great if this had a unit test associated with it - see the test_voicemail_msgcount unit test for an example of this (although I imagine a test for VM_INFO could be a bit less involved).  You could certainly submit that as a separate patch if you'd like.  Overall, looks like this would be a useful addition to get user information from the dialplan.
> 
> wdoekes wrote:
>     May I add that you have 'mailbox_folder', 'mbox' and 'mbxfolder' variables. I suggest you choose one name for 'mailbox' and stick with it.
>     
>     The language, tz (timezone) and locale attributes might be worthy to add, although I don't have a strong opinion about that.
>     
>     As far as the exists check, I second removing it. Having more ways than one to do things should be restricted to the perl language ;)
> 
> Tilghman Lesher wrote:
>     I disagree.  In fact, we should allow the 'exists' here and deprecated MAILBOX_EXISTS.
> 
> wdoekes wrote:
>     Fine by me too. (Even better.. but I wasn't sure how lightly deprecation is taken.) As long as the end result is that there is only one canonical way to do things.
> 
> Tilghman Lesher wrote:
>     If past history is any guide, look at how the CHANNEL() dialplan function was created and what functions were deprecated as a result.  We like dialplan functions that do multiple things, as long as the multiple things are all  related in function.
> 
> shawkris wrote:
>     'mailbox_context' (I assume you mean that and not mailbox_folder), 'mbox' and 'mbxfolder' mean different things in the code.
>     
>     mailbox_context is the raw argument from the dialplan, such as 1234 at default.
>     mbox is the parsed mailbox, such as 1234.
>     mbxfolder is the folder name, after the original folder name was checked for null. This would something like Inbox.
>     
>     Are there standard names for these variables? I did look through app_voicemail.c and chose variable names that seemed similar to ones already in use in other functions for the same purpose.
>     
>

It'd be nice if there were standard names, but as such, no, there aren't.  In general, 'mailbox' and 'folder' are used to represent what you've chosen here, but you will see various abbreviations for them.  I think wdoekes point wasn't that the variables were all the same, but rather that your nomenclature was different.  For example, the following is consistent:
mbox_context
mbox
mbox_folder

As is:
mailbox_context
mailbox
mailbox_folder

But mailbox_context, mbox, and mbxfolder isn't.  I prefer things to be spelled out, but that's just me.

Also: please update the CHANGES.txt with your new function, and UPGRADE.txt with the deprecation of MAILBOX_EXISTS (I agree with Tilghman, its probably better to deprecate that function).  You'll also need to update the XML documentation for MAILBOX_EXISTS to note that its deprecated.


- mjordan


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


On Nov. 5, 2011, 10:56 a.m., shawkris wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1568/
> -----------------------------------------------------------
> 
> (Updated Nov. 5, 2011, 10:56 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Creted VM_INFO dialplan function which can return information about a user defined in the standard voicemail application.
> 
> Supported attributes are: email, exists, fullname, pager, password, count.
> 
> e.g. from the dialplan, ${VM_INFO(1234 at test,email)} would return the email address for voicemail user 1234 in the test context.
> 
> The 'count' attribute can also take a folder parameter and will return the number of messages in that folder, or by default the Inbox.
> 
> 
> This addresses bug ASTERISK-18634.
>     https://issues.asterisk.org/jira/browse/ASTERISK-18634
> 
> 
> Diffs
> -----
> 
>   trunk/apps/app_voicemail.c 343488 
> 
> Diff: https://reviewboard.asterisk.org/r/1568/diff
> 
> 
> Testing
> -------
> 
> Tested with a local dialplan calling the VM_INFO function with each parameter to make sure the expected result was returned. Tested that calling with an invalid attribute parameter produced an error message.
> 
> 
> Thanks,
> 
> shawkris
> 
>

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