[asterisk-dev] [Code Review] 3984: Module to add devstate for MWI.

[Matt Jordan]

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


This looks really good. Nice job!

It'd be good to get a test for this - not because the module itself has any problems, but to prevent it getting broken from a bad change to the APIs that it depends on (Stasis, device state, mailbox core, etc.) Having a test puts the burden of fixing inadvertent problems on the initial committer, as opposed to discovering it when FreePBX breaks and having the issues get pushed to you - only to discover that someone else broke the module.

You could write a test for this very easily using the external MWI functionality. A collection of tests that are based on that functionality can be seen here:

https://bamboo.asterisk.org/bamboo/browse/AST-ATSEM-C632TE-210/test

Essentially, you could use a small Python script to update the mailbox count on a mailbox, then check that the appropriate device state changed. If you need a hand writing the test, let me know and I can pitch in on it.


branches/12/res/res_mwi_devstate.c
<https://reviewboard.asterisk.org/r/3984/#comment23745>

    extended :-)



branches/12/res/res_mwi_devstate.c
<https://reviewboard.asterisk.org/r/3984/#comment23746>

    I don't think you need this include, since you're managing the message routing yourself.


- Matt Jordan


On Sept. 8, 2014, 1:21 p.m., Jason Parker wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3984/
> -----------------------------------------------------------
> 
> (Updated Sept. 8, 2014, 1:21 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Add a device state for MWI.  This allows for a blinky light when a mailbox has at least 1 new message.
> 
> 
> Diffs
> -----
> 
>   branches/12/res/res_mwi_devstate.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/3984/diff/
> 
> 
> Testing
> -------
> 
> Leaving a voicemail then deleting that voicemail causes the hint to go to IN_USE then to NOT_INUSE.  Polled mailboxes do not appear to update, but I can't tell whether that's a failing of pollmailboxes or not.
> 
> Patch also applies to and functions on branches/13/.
> 
> 
> Thanks,
> 
> Jason Parker
> 
>

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