<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/1568/">https://reviewboard.asterisk.org/r/1568/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 5th, 2011, 8:08 p.m., <b>mjordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On November 6th, 2011, 9:17 a.m., <b>wdoekes</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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 ;)</pre>
</blockquote>
<p>On November 7th, 2011, 9:27 a.m., <b>Tilghman Lesher</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I disagree. In fact, we should allow the 'exists' here and deprecated MAILBOX_EXISTS.</pre>
</blockquote>
<p>On November 7th, 2011, 9:41 a.m., <b>wdoekes</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On November 7th, 2011, 9:48 a.m., <b>Tilghman Lesher</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On November 7th, 2011, 2:17 p.m., <b>shawkris</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">'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@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.
</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<p>- mjordan</p>
<br />
<p>On November 5th, 2011, 10:56 a.m., shawkris wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By shawkris.</div>
<p style="color: grey;"><i>Updated Nov. 5, 2011, 10:56 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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@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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-18634">ASTERISK-18634</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>trunk/apps/app_voicemail.c <span style="color: grey">(343488)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1568/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>