[asterisk-dev] [Code Review] Add voicemail message IDs where needed on startup; Add message IDs to ODBC voicemail

[Mark Michelson]

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

(Updated May 22, 2012, 2:46 p.m.)


Review request for Asterisk Developers.


Changes
-------

This takes a different approach to adding message IDs where they are missing.

The initial approach was to check all messages of a voicemail user when created. This approach is to only add a missing voicemail message ID when a voicemail message snapshot is created. I came to this method based on the following logic: The message ID is only useful for the operations ast_vm_msg_move(), ast_vm_msg_remove(), ast_vm_msg_forward(), and ast_vm_msg_play(). In order to call these methods, one must know the ID of the message(s) to manipulate. How does someone get the ID of the message(s)? This is done by getting a mailbox snapshot, of course!

This new diff removes quite a bit of the changes from the old diff since they are no longer necessary. Instead, ast_vm_msg_snapshot_create() has been modified to add a message ID to a message if one is not present already.

A couple of notes:

* There is also a bit of cruft in this diff from a memory leak fix that Matt Jordan made in trunk. This was something I potentially could have avoided, but it's easier just to point it out so you can ignore it instead.
* IMAP is still not touched in this update. As far as I'm concerned, IMAP is a whole other can of worms from this and adding its changes would compromise the clarity of these initial changes. Once this method has been approved for adding missing message IDs, I will focus on the necessary IMAP changes and post those as a separate review.


Summary
-------

There are two main things added here.

1. Voicemail APIs added in trunk rely on voicemail messages possessing a "msg_id" in their metadata. This is how messages are looked up when they are to be moved, played, removed, and forwarded. The issue is that msg_id is a new piece of data and is not present on voicemails saved in older versions of Asterisk. This patch aims to fix that. When voicemail is loaded, each user's voicemail box is searched and each message is checked for msg_id. If it is not present, one is generated and added to that message's metadata. For ODBC, I added a method that specifically is used to issue a SQL UPDATE command to set the msg_id for the message. IMAP does not have this ability because I have no idea how to alter/update stored voicemails in IMAP.

2. I noticed that the ODBC function for storing voicemails did not bother to store the msg_id. I have updated the code to do this properly.


Diffs (updated)
-----

  /team/mmichelson/trunk-digiumphones/apps/app_voicemail.c 366917 

Diff: https://reviewboard.asterisk.org/r/1898/diff


Testing
-------

For 1, I have manually tested by leaving a voicemail using Asterisk 1.8 and then loading app_voicemail.so in trunk-digiumphones. The metadata file for the message had a msg_id added to it as expected.

For 2, I attempted to test leaving messages via ODBC, but I ran into failures to execute the SQL INSERT statement when storing the voicemail. I attempted on a clean trunk checkout and on a clean 1.8 checkout and got the same error, so I figure it's not necessarily a coding issue but a setup issue on my end. I figured that's not enough to stop me from posting the review now and getting my setup fixed up after. If it turns out I find any glaring bugs in the code, I'll update the diff here.


Thanks,

Mark

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120522/64c072ae/attachment.htm>