----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3061/#review10358 -----------------------------------------------------------
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 >'[email protected]', '[email protected]', and '[email protected]'. 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. - Matt Jordan On Dec. 9, 2013, 9:51 p.m., rmudgett wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3061/ > ----------------------------------------------------------- > > (Updated Dec. 9, 2013, 9:51 p.m.) > > > Review request for Asterisk Developers. > > > Repository: Asterisk > > > Description > ------- > > 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". > > > Diffs > ----- > > /trunk/res/res_sorcery_astdb.c 403574 > /trunk/res/res_mwi_external_ami.c PRE-CREATION > /trunk/res/res_mwi_external.exports.in PRE-CREATION > /trunk/res/res_mwi_external.c PRE-CREATION > /trunk/main/app.c 403574 > /trunk/include/asterisk/res_mwi_external.h PRE-CREATION > /trunk/include/asterisk/doxyref.h 403574 > /trunk/include/asterisk/app.h 403574 > /trunk/configs/sorcery.conf.sample 403574 > /trunk/apps/app_voicemail.c 403574 > > Diff: https://reviewboard.asterisk.org/r/3061/diff/ > > > Testing > ------- > > Used the CLI "database show" along with the new CLI commands and AMI actions > to test adding/updating, getting, and deleting external MWI counts. > > > Thanks, > > rmudgett > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
