<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/3061/">https://reviewboard.asterisk.org/r/3061/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 10th, 2013, 5:01 p.m. UTC, <b>Matt Jordan</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 fundamentally disagree with a design decision made in this patch.
The concept of a 'context' is an Asterisk thing - specifically, a concept that is derived from static .conf files. It has no semantic meaning outside of Asterisk, and certainly has no semantic meaning in voicemail, databases, e-mail servers, or any other thing. From the perspective of defining a structure to its static configuration files, a "context" is a perfectly fine and legitimate concept - but that doesn't mean it should be applied elsewhere.
Asterisk's app_voicemail, rightly or wrongly, allowed users to group mailboxes into compartments delineated by the 'context' concept. This had the side effect of requiring the unique identifier for a mailbox to be 'mailbox@context'. This limitation is entirely an app_voicemail concept and is not needed here.
>From the perspective of an external system looking to use Asterisk as an MWI proxy, enforcing 'context' here is limiting and completely unnecessary. If I want to supply 'alice', 'bob', and 'jim' as mailboxes I should be allowed to do so. I should not be forced to use 'default' or something else that has no semantic meaning to my system. At the same time, if I want to provide my own domains (which is really what this is from the perspective of an external user), I should be allowed to do so - but that merely means adding 'alice@foo.net', 'bob@foo.net', and 'jim@bar.com'. The AMI actions, ARI REST calls, and the entire MWI proxy shouldn't care if someone appends a domain to the unique ID comprising the mailbox. It should still just be a string unique ID.
This design flaw has a ripple effect through this entire patch - but until we resolve this fundamental problem, I'm not sure I'd look at this patch any further.</pre>
</blockquote>
<p>On December 10th, 2013, 5:45 p.m. UTC, <b>rmudgett</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 mailbox@context concept is completely entrenched in the Asterisk code base and cannot be removed without reworking the entire MWI system. Existing MWI events, channel drivers, and the stasis messages all assume this format. Trying to ignore this fact in the external MWI core and external MWI AMI interface is more limiting.
</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;">I disagree.
First, imposing limitations on an external system because something that we're trying to move away from was written in a way that is different than how external systems would want to consume it defeats the purpose of what we're trying to do here. I'd rather spend time getting Asterisk to be a communications engine that external systems want to consume - and do it correctly - rather than enforce some silly limitation for the next 10 years "because that's the way Asterisk has always done it".
Looking at what actually has to be changed:
(1) Raising a Stasis message for MWI requires a mailbox and context. However, it really doesn't - it actually uses a unique ID (single string) which is a concatanation of mailbox/context. This could easily be modified to accept an empty string for context and, if an empty string is provided, it uses just the mailbox as the unique ID.
(2) chan_sip/chan_iax2/chan_dahdi: Subscribing for MWI. This currently will auto-append a 'default' to what a peer subscribes for doesn't contain a context. This behavior could be kept by default. Optionally, we could choose to not append 'default' via some configuration option. Or we could make a breaking change and no longer auto-append 'default'. Either way, this is not a large amount of code and could be handled.
(3) chan_pjsip: Subscribing for MWI here passes the value in as-is. There's nothing here that needs to be done, as it subscribes for whatever you told it to subscribe for.
I don't see any technical limitation here that couldn't be easily overcome.</pre>
<br />
<p>- Matt</p>
<br />
<p>On December 9th, 2013, 9:51 p.m. UTC, rmudgett wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By rmudgett.</div>
<p style="color: grey;"><i>Updated Dec. 9, 2013, 9:51 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<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;">This patch can be broken into four main components:
1) Voicemail callback registration/unregistration function improvements are in the following files:
include/asterisk/doxyref.h,
include/asterisk/app.h,
main/app.c, and
apps/app_voicemail.c.
* The voicemail registration/unregistration functions now take a struct of callbacks instead of a lengthy parameter list of callbacks.
* The voicemail registration/unregistration functions now prevent a competing module from interfering with an already registered callback supplying module.
2) The core resource support for external MWI providers are in the following files:
include/asterisk/res_mwi_external.h,
configs/sorcery.conf.sample,
res/res_mwi_external.c, and
res/res_mwi_external.exports.in.
* The core external MWI resource provides for MWI message counts persistence using sorcery. With sorcery, the user is able to configure which sorcery wizzard backend to use if the default astdb is not desired.
* The core external MWI resoruce provides some CLI commands to manually manage the MWI counts if needed.
The new CLI commands are:
"mwi external delete all",
"mwi external delete like <regex>",
"mwi external delete mailbox <box[@[context]]>",
"mwi external list all",
"mwi external list like <regex>",
"mwi external show mailbox <box[@[context]]>", and
"mwi external update mailbox <box[@[context]]>".
3) The AMI component of external MWI is in:
res/res_mwi_external_ami.c
* The external MWI AMI interface provides a thin wrapper around the core external MWI resource.
The resource adds the following AMI actions:
MWIExternalGet,
MWIExternalDelete, and
MWIExternalUpdate.
4) A consistency fix to sorcery astdb regex record retrieval is in:
res/res_sorcery_astdb.c
* The AstDB wizzard is inconsistent with the other sorcery wizzards with its regex record selection. Any pattern that uses the '^' regex character to anchor the pattern to the beginning of the key *must* be a simple prefix. Any wildcard matching used with the '^' anchor will fail. As a result, a regex of "^.*@context$" will not return anything even if there are mailboxes in "context".</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;">Used the CLI "database show" along with the new CLI commands and AMI actions to test adding/updating, getting, and deleting external MWI counts.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/res/res_sorcery_astdb.c <span style="color: grey">(403574)</span></li>
<li>/trunk/res/res_mwi_external_ami.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/res/res_mwi_external.exports.in <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/res/res_mwi_external.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/main/app.c <span style="color: grey">(403574)</span></li>
<li>/trunk/include/asterisk/res_mwi_external.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/include/asterisk/doxyref.h <span style="color: grey">(403574)</span></li>
<li>/trunk/include/asterisk/app.h <span style="color: grey">(403574)</span></li>
<li>/trunk/configs/sorcery.conf.sample <span style="color: grey">(403574)</span></li>
<li>/trunk/apps/app_voicemail.c <span style="color: grey">(403574)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3061/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>