Summary:	ASTERISK-11298: [patch] [sound] Ability to send to multiple recipients.
Reporter:	Travis Hein (travishein)	Labels:
Date Opened:	2008-01-24 11:47:01.000-0600	Date Closed:	2011-06-07 14:03:06
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Applications/app_voicemail/NewFeature
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) 0011837_1.4-trunk-137054.patch ( 1) 0011837_177349.diff ( 2) 0011837_trunk-136885.patch ( 3) app_voicemail_1.4.17.diff ( 4) app_voicemail_multi_recipient_trunk_20080228.diff
Description:	for the menu option 3 (advanced), 5 (leave voice message to a recipient) we came across the need to allow a user to be able to enter more than one extension to send a message to (instead of the leave a message for just one extension at a time as the current implementation was). To make this work the logic is now 1- user presses 3, 5 to get to the leave voicemail prompt. 2- the existing prompt to enter the extension is played. 3- the user enters the extension for which to send the voicemail to 4- (NEW): the system play a message :press 1 to send message, press 2 to add another recipient.  if user presses 2  on this new prompt, the system returns to the step 3, to prompt and process of entering  the extension.  if the user presses 1 on this new prompt, the system continues to the "please say your message" as it used to before. 5- system plays the please say your message when finished hang up. prompt. 6- user sais their message, and hangs up, or preses pound key. In order to realize this, we changed input gathering function so that the char[] buffer that stores the target recipient is larger, as long as the max receiver string. The operation of adding more recipients now builds the delimited recipient string that leave_voicemail() understands and expects, mainly extension@context&extension@context There is also boundary checking so that if the user tries to enter more exensions than is room in the char[] array, the system will play a (new) vm-forward-multiple-too-many message to indicate the user is attempting to forward the message to too many recipients. So in addition to the attached patch file, there are two new sound audio prompts that the system must find in the sound prompts folder, so that it can of course play these when needed. vm-forward-multiple : "Press 1 to enter an send message, press 2 add another recipient" vm-forward-multiple-too-many  : when the user has attempted to enter too many exensions to leave a message to.
Comments:	By: Ronald Chan (loloski) 2008-01-25 23:12:46.000-0600 travishein, If this is a new feature, then it should be applied to SVN trunk not on 1.4 release branch Thanks By: Travis Hein (travishein) 2008-01-26 03:00:20.000-0600 sorry, it was applied against the svn trunk, at the time 1.4.17 just came out. By: Mark Michelson (mmichelson) 2008-01-28 19:43:06.000-0600 Thanks for the patch. The ability to send messages to multiple recipients is a great idea. The patch you have provided has some problems, though. 1. There is some code in this patch for something completely unrelated (VM_MOVEHEARD). 2. The patch was not made against trunk, but was made against the 1.4 branch. 3. When compiling, there are two warnings generated. One for mixed declarations and code, and one for passing an int where a size_t is expected. 4. There is a very arbitrary check to see if the current length of the recipients buffer + 40 is greater than its maximum length. Using 40 is not a good idea since the majority of mailbox names are going to be shorter than this. Even worse is the fact that you set aside space for 162 characters for the mailbox + @ + extension. Think of a case where you want to add a 160 character mailbox and extension to the buffer, but it only has 50 characters of space left. Your patch would allow for a buffer overflow. What you should do is build the mailbox string and then see if it will fit inside the buffer. 5. Contexts are not handled gracefully in this implementation. The context argument for forward_message could be NULL, but you append it to the mailbox no matter what. Luckily, this is handled by the rest of the code, but you should probably check to be sure if the context is non-NULL before appending to the mailbox. 6. You went to extra effort to be sure that forwarding a message would not allow specifying multiple recipients. Why? 7. There are a lot of debug messages in the patch that should either be removed or at least only be displayed at a high debug level. Furthermore, most of the code that you commented out should instead just be removed. 8. Certain variables can be optimized away. num_receivers, for instance, is used only for debugging purposes and can be removed. 9. Regarding coding guidelines: there are several places here where you violate coding guidelines (for instance, an else should be on the same line as a } of a preceding if) but also there are many places in the code where you changed something that was within the coding guidelines to something that does not follow the guidelines (for instance adding {} to a single-line if statement). Please review the CODING-GUIDELINES document in the doc/ directory. I encourage you to make the above changes. There may be other problems, too, but the ones above were the most glaring problems. Thanks for contributing, and I look forward to seeing your next patch! By: Mark Michelson (mmichelson) 2008-02-04 15:25:26.000-0600 I've looked a bit further at the existing code, and it appears that this could actually be implemented a lot more easily than it is done here. forward_message already supports leaving a voicemail for multiple recipients by separating the extensions with a * key. Right now, the code explicitly handles the case of forwarding a message but does not allow for leaving a message for multiple recipients in the same way via VoiceMailMain. I was looking at SVN revision 102354 earlier and found the section beginning on line 4401 (line numbers may be slightly different in different revisions). The line is "if(flag==1) {". Inside this block of code, a single mailbox string is made and then leave_voicemail is called. This could be modified to remove strings off the "extensions" list that was created earlier in the function and then call leave_voicemail with all these extensions with '&' placed between them. By: jmls (jmls) 2008-02-17 12:58:51.000-0600 travishein, any comments to the above comment ? By: Travis Hein (travishein) 2008-02-29 02:00:11.000-0600 Sorry to be so long in replying. I have done some reworking of this and attached a new patch file (app_voicemail_multi_recipient_trunk_20080228.diff), and addresses the following items to  putnopvut's feedback items: 1. We had the MOVEHEARD patch back applied for our 1.4 series we were running, and it seems to have snuck in there. This diff no longer has that MOVEHEARD stuff. 2. Thats true, we were on the 1.4 branch at the time. The patch today is against the trunk. 3. I dont seem to get warnings anymore; warnings were in the moveheard stuff that is now removed. 4. Yes, thats a very good point. The way it is now done is the currently entered user is computed into a mailbox string, and then it is evaluated if this mailbox string would fit into the recipients string, and if not play the error message. (no more arbitrary +40 chars hack thing). 5. I think I am now handling the contexts gracefully, in that i do the test if context is not null and sprintf it into mailbox, otherwise copy just the mailbox. (like the 1.4 and trunk codes do for this now): /* Make sure that context doesn't get set as a literal "(null)" (or else find_user won't find it) */ if (context)  snprintf(mailbox, sizeof(mailbox), "%s@%s", username, context); else  ast_copy_string(mailbox, username, sizeof(mailbox)); 6. Not sure why the extra effort to make sure forwarding message would not allow multiple recipients. I guess in our user base it just has not come up yet where they would want to have to do that. Perhaps they did not want to have to be prompted with the additional "press 1 to enter message, press 2 to add another recipient" for forwarding messages mode.   I can't see a technical reason why we would not be able to just do the allow multiple recipients in the both forward and leave message modes, and it would likely be less confusing if it was consistent right. Well, what do you think ?  For now, I have removed the checking / restriction to only do the multi-recipient in the leave meessage mode, so forwarding a message would receive the same feature now. 7. Yes, good point. I have cleaned the extra debug messages out, and removed the commented out dead code parts. 8. Removed the num_receivers variable. This was following the cleaning up of the unnecessary debug outputs from 7. 9. Had a read over coding guidelines, and now following the conventions of same line and use of braces for single-line if statements. So thats the going over the initial glaring problems checklist. For your second comment, on the possible easier way to do this in the forward_message section, I think they actually wanted to hear the prompt telling them to go ahead and enter another or start leaving message now.   So what you are saying, is the user experience would be instead of (my patch way of) 1 1234 1 2345 1 3456 2 <leave message> it would be 1234*2345*3456 <leave message> I guess one advantage to my patch implementation is  each extension is validated as it is entered, instead of getting an invalid extension message at the end of entering some really long one. ? By: Tilghman Lesher (tilghman) 2008-04-02 13:22:19 Could you please summarize, in a separate bugnote, EACH of the new prompts that you'll need for this? By: Travis Hein (travishein) 2008-04-02 14:18:51 This patch would require two (2) new prompts.  The prompts below are the names as required by the code (i.e. "vm-forward-multiple", which could be vm-forward-multiple.gsm, etc). (1) vm-forward-multiple would say: "press 1 to leave message, press 2 add another recipient" This is played as the option before the prompt to "say your message after the tone" is played, to give the user the option to return to the adding a recipient  . (2) vm-forward-multiple-too-many would say something like: "unable to add another recipient". This is played when the the number of extensions entered exceeds the maximum number of allowed extensions to forward copies to  (limited by the internal string length of the leave_voicemail function). By: Travis Hein (travishein) 2008-04-04 12:14:09 we had managed to have these voice prompts recorded for our use. would it be helpful / good for me to post those sound files here as well then ? By: Tilghman Lesher (tilghman) 2008-04-04 13:32:52 travishein:  no, because they'll need to be recorded in all 3 of our standard languages. By: Tilghman Lesher (tilghman) 2008-04-15 21:39:21 I'm looking over this, and it occurs to me that instead of using a static buffer for the recipient mailboxes, we should instead use the ast_str subsystem to allow as many recipients as the sender would like. I can convert this, when we have the additional sounds recorded, or you may find it instructive to alter your patch yourself to use ast_str for the input string.  It's your choice. By: Greg Merriweather (shido6) 2008-07-14 14:53:01 After entering 1 extension, the greeting of that recipient should play so you know that you have entered the correct extension and you recognize the name/voice of the recipient. Now you can either add another recipient or leave that person a message. Same for forwarding to multiple recipients. By: Travis Hein (travishein) 2008-08-09 22:22:20 Ok, I added the playing of the greeting or the extension of the person after their extension is entered. I looked into making this use the ast_str api, but the 1.4 series does not seem to have this dynamic string api yet, right? the (0011837_1.4-trunk-137054.patch) is against the current 1.4 trunk. and the (0011837_trunk-136885.patch) is against the current (1.6) trunk. (but neigher  have the ast_strings in it for the recipients yet) I don't have a 1.6 executable test set up at the moment, so this 1.6 patch compiles for me, but I have not been able to boot it up to test it. though it is a transposition from the 1.4 patch that i was able to test, so it most likely will work By: Tilghman Lesher (tilghman) 2008-08-10 00:04:54 Correct, 1.4 does not have it, but as a new feature, it is ineligible for inclusion in 1.4.  Assuming we have the sounds recorded, the earliest release it could be in is 1.6.1. By: Leif Madsen (lmadsen) 2008-10-22 10:53:12 OK, so this looks like it is *real close now(tm)* to possibly being merged. I guess we are just waiting on prompts to be recorded? If that is the case, can we do one last audit of the code to make sure that is all sane, and determine what we need to do to get prompts? By: Tilghman Lesher (tilghman) 2008-12-18 14:07:44.000-0600 A couple of things more: 1) You stated that you were validating users as they come in, but your code doesn't actually this.  It just blindly copies every user into the string and lets another routine deal with the invalid users. 2) Please use the ast_str dynamic string routines for building the list of users, rather than a static buffer. 3) Use ast_debug() instead of ast_log(LOG_DEBUG...). 4) You should use Directory as a possible method of adding extensions, just as is done above. 5) The method above yours builds a list in extensions, but you aren't using it.  Ideally both methods should be consolidated into one, so there is one extension input loop, not two. 6) Your playback includes a "patches" prefix, which should go away.  By this time, all additional requested sounds should already be in the current -sounds release. By: Tilghman Lesher (tilghman) 2009-01-15 12:54:50.000-0600 travishein:  I need action on all of the points I posted above before this patch can be merged. By: Travis Hein (travishein) 2009-01-19 06:41:09.000-0600 Ok, Ill see where I can get in this list this week. By: Tilghman Lesher (tilghman) 2009-01-21 16:37:16.000-0600 Setting to feedback to denote waiting for reporter input. By: Tilghman Lesher (tilghman) 2009-02-09 14:35:50.000-0600 travishein:  it has been more than a week since your last reply.  Have you made any progress? By: Travis Hein (travishein) 2009-02-11 13:22:11.000-0600 Yes, from the most recent comments points, 3, 6 easy ones to take care of) Working to resolve points(1, 4, 5) by refactoring the logic, so that all the input of extensions uses the same (existing) loop control, and thus the same validations and directory support. and #2 needs to be done still I need to get a good current test environment up now to test it a bit. By: Travis Hein (travishein) 2009-02-18 23:51:38.000-0600 Ok, attached new patch ( 0011837_177349.diff ), against current svn trunk (r177349). - uses the same prompting loop for reading in user extensions as the original leave voicemail, so it now allows for directory input, and has what ever validation that it used to have  (addresses previous comments: 1, 4, 5) - now using ast_str dynamic string to build list of recipients. This required small change to the leave_voicemail() function, which was used in about three  places, so adjusted the code to use ast_str and clean up after for these other spots as well. (addresses previous comments: 2) - uses ast_debug() now instead ast_log(LOG_DEBUG...). (address previous comments: 3) - playback sounds no longer include "patches" prefix (addresses previous comments :6) New notes and comments: 1) now that the recipients list is theoretically dynamic and unlimited, there is no need for the prompt vm-forward-multiple-too-many, so the logic surrounding this has been removed, we just have the prompt vm-forward-multiple  now. 2) i have left in the commented out code lines for where I have converted the static char[] stuff into ast_str, in the hope that someone can validate that I have captured all of the spots that needed changing and didn't miss anything. 3) added support for this forward multiple feature to be turned on in the voicemail.conf option. before it was always there kind of thing. Why not right. 4) Now that we use ast_str API, the branch of the forward_message() function for leave a message (is_new_message = 1) is enabled for the forward multiple, owever I don't currently understand what or how to make the multiple recipients  work for the forward message mode (is_new_message = 0). A lot of this part has changed in 1.6, and I don't even see where "username" entered in the input loop is used for this part ? I would recommend putting back the if(is_new_message) { } wrapper around the  prompt and loop for reading another extension for the forward multiple ? (that was what I originally had there anyway). 4) so far, this compiles now without warnings, and it looks good on paper to me now, but I _still_ don't have my asterisk 1.6 test env up here. so this has not been road tested yet. and in the mean time, I felt I owed some kind of progress update,and I was hoping that posting the current work to capture many of these improvements would open further discussion. By: Tilghman Lesher (tilghman) 2009-02-19 15:57:20.000-0600 travishein:  please post this patch on http://reviewboard.digium.com By: Travis Hein (travishein) 2009-02-19 16:13:16.000-0600 Ok, http://reviewboard.digium.com/r/169/ By: Tilghman Lesher (tilghman) 2009-02-19 16:50:14.000-0600 It's not viewable, as is.  Please change the branch field to the string "/trunk" and add "asterisk-dev" to the reviewer group field. By: Travis Hein (travishein) 2009-02-20 06:52:08.000-0600 oops, ok, fixed. By: Tilghman Lesher (tilghman) 2009-05-04 17:51:07 Given the lack of response on reviewboard, I am suspending this issue.  Please reopen when you have addressed those comments.