[asterisk-dev] [Code Review] 3117: ARI: mailboxes resource - use external MWI with ARI

[Mark Michelson]

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



/branches/12/res/ari/resource_mailboxes.c
<https://reviewboard.asterisk.org/r/3117/#comment20031>

    It's a bit pedantic, but if I were faced with an error response that said "Mailbox is missing", I would interpret that to mean that I omitted the required mailbox in my HTTP request, not that the mailbox did not exist on the server. The fact that I'm getting a 404 should be a hint as to what the actual problem is, but I still think the response text could be changed to be more clear. Something like "Mailbox does not exist" would work better.



/branches/12/res/ari/resource_mailboxes.c
<https://reviewboard.asterisk.org/r/3117/#comment20032>

    Same "Mailbox is missing" nit here.



/branches/12/res/res_stasis_mailbox.c
<https://reviewboard.asterisk.org/r/3117/#comment20033>

    mailbox_to_json() may return NULL if there was an error creating the JSON. Also, ast_json_array_append() can return an error.
    
    With this sort of operation, I think you should either return all or nothing. If an error occurs while creating the array, you should free everything and return NULL.


- Mark Michelson


On Jan. 9, 2014, 11:38 p.m., Jonathan Rose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3117/
> -----------------------------------------------------------
> 
> (Updated Jan. 9, 2014, 11:38 p.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch adds the ability to interface with res_external_mwi via AMI.  The following commands are implemented:
> 
> PUT mailboxes/mailboxName
>     modifies mailbox state and implicitly creates new mailboxes
> 
> GET mailboxes/mailboxName
>     retrieves a JSON representation of a single mailbox if it exists
> 
> GET mailboxes
>     retrieves a JSON array of all mailboxes
> 
> DELETE mailbox/mailboxName
>     deletes a mailbox
> 
> Thanks to Richard's external MWI work, this is actually a very simple system.
> 
> 
> Diffs
> -----
> 
>   /branches/12/rest-api/resources.json 405265 
>   /branches/12/rest-api/api-docs/mailboxes.json PRE-CREATION 
>   /branches/12/res/res_stasis_mailbox.exports.in PRE-CREATION 
>   /branches/12/res/res_stasis_mailbox.c PRE-CREATION 
>   /branches/12/res/res_ari_mailboxes.c PRE-CREATION 
>   /branches/12/res/ari/resource_mailboxes.c PRE-CREATION 
>   /branches/12/res/ari/resource_mailboxes.h PRE-CREATION 
>   /branches/12/res/ari/ari_model_validators.c 405265 
>   /branches/12/res/ari/ari_model_validators.h 405265 
>   /branches/12/res/ari.make 405265 
>   /branches/12/include/asterisk/stasis_app_mailbox.h PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/3117/diff/
> 
> 
> Testing
> -------
> 
> Tested unloads and reloads of the res_stasis_mailboxes module with res_mwi_external loaded and unloaded
> Tested how commands respond from resource_mailboxes when res_stasis_mailboxes isn't loaded
> Tested each of the commands with numerous parameters
> Created testsuite test in https://reviewboard.asterisk.org/r/3118/ which confirms basic operation of all the new ARI functions.
> 
> 
> Thanks,
> 
> Jonathan Rose
> 
>

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