[asterisk-dev] [Code Review] New dialplan application VMSayname with unit test

[Mark Michelson]

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



/trunk/apps/app_voicemail.c
<https://reviewboard.asterisk.org/r/530/#comment3544>

    This warning is a bit odd since it implies that there are optional extra arguments for the application beyond the mailbox at context. 



/trunk/apps/app_voicemail.c
<https://reviewboard.asterisk.org/r/530/#comment3542>

    Argh! My eyes!
    
    static const char TEST_CONTEXT[] = ...
    static const char TEST_EXTENSION[] = ...



/trunk/apps/app_voicemail.c
<https://reviewboard.asterisk.org/r/530/#comment3543>

    Would a call to ast_dummy_channel_alloc work here?



/trunk/apps/app_voicemail.c
<https://reviewboard.asterisk.org/r/530/#comment3545>

    I think the use of the RETRIEVE and DISPOSE macros are unnecessary here since you are not expecting to actually find the greeting for this extension and context in ODBC or IMAP storage.
    
    I suppose the dynamic of this test could change a bit though if the manipulation you do on the file system down below were instead done in such a way that required storing and retrieving the files from IMAP or ODBC. I think that should be a separate unit test though...



/trunk/apps/app_voicemail.c
<https://reviewboard.asterisk.org/r/530/#comment3546>

    res is used somewhat inconsistently in this test. In most cases, it's used to hold the return of various functions, and its value is either treated as 0 (success) or non-zero(failure). Here, it's being set to an enumerated constant that doesn't fit well with its other uses. I think that setting res to -1 here is more in parallel with the rest of its uses in this test.



/trunk/apps/app_voicemail.c
<https://reviewboard.asterisk.org/r/530/#comment3549>

    Have you tried using:
    
    snprintf(dir2, sizeof(dir2), "%s%s", VM_SPOOL_DIR, TEST_CONTEXT);
    remove(dir2);
    
    I'm not sure if remove(3), when used on a directory, will complain about the directory having contents within.


- Mark


On 2010-02-24 16:19:09, Jeff Peeler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/530/
> -----------------------------------------------------------
> 
> (Updated 2010-02-24 16:19:09)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch adds a new application, VMSayName, which plays the greeting for a given voicemail user specified by extension and optionally context. If the greeting does not exist, currently the extension is played back.
> 
> 
> This addresses bug 14973.
>     https://issues.asterisk.org/view.php?id=14973
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_voicemail.c 248659 
> 
> Diff: https://reviewboard.asterisk.org/r/530/diff
> 
> 
> Testing
> -------
> 
> A unit test has been created to test the code paths used for playback in the case when the greeting is present and when it is not. Because the application is really just a wrapper function, the unit test is what needs to be examined more so than anything else.
> 
> 
> Thanks,
> 
> Jeff
> 
>