[Home]

Summary:ASTERISK-11280: [branch] [sound] Ability to mark a voicemail message as URGENT.
Reporter:James Rothenberger (jaroth)Labels:
Date Opened:2008-01-22 13:12:56.000-0600Date Closed:2010-11-18 16:56:25.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Applications/NewFeature
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) jaroth-review-response.txt
( 1) message_marked_urgent_48KHz_english.wav
( 2) message_marked_urgent_48KHz_french.wav
( 3) message_marked_urgent_48KHz_spanish.wav
( 4) message_marked_urgent_english.gsm
( 5) message_marked_urgent_french.gsm
( 6) Press_4_to_mark_this_message_as_urgent_48_KHz.wav
( 7) Press_4_to_mark_this_message_as_urgent_48KHz_french.wav
( 8) Press_4_to_mark_this_message_as_urgent_48KHz_spanish.wav
( 9) press_4_to_mark_this_message_as_urgent_english.gsm
(10) press_4_to_mark_this_message_as_urgent_french.gsm
(11) Press_4_to_remove_the_urgent_status_of_this_message_48KHz_english.wav
(12) Press_4_to_remove_the_urgent_status_of_this_message_48KHz_french.wav
(13) Press_4_to_remove_the_urgent_status_of_this_message_48KHz_spanish.wav
(14) press_4_to_remove_urgent_status_english.gsm
(15) press_4_to_remove_urgent_status_french.gsm
(16) Ugly-looking-review.txt
(17) urgent_48KHz_english.wav
(18) urgent_48KHz_french.wav
(19) urgent_48KHz_spanish.wav
(20) urgent_english.gsm
(21) urgent_french.gsm
(22) urgent_status_removed_48KHz_french.wav
(23) urgent_status_removed_48_KHz_spanish.wav
(24) urgent_status_removed_49KHz_english.wav
(25) urgent_status_removed_english.gsm
(26) urgent_status_removed_french.gsm
(27) vm-marked-urgent.wav
(28) vm-review-urgent.wav
(29) vm-Urgent.wav
(30) vm-urgent-removed.wav
Description:This patch adds the ability to tag a voicemail as urgent.  It supports both IMAP and file-based storage.  After recording a voicemail and pressing "#", an additional option (4) is available to mark the message as urgent.  Pressing the '4' key a second time turns the flag off.   In IMAP, the message is marked as //FLAGGED, as well as the addition of an additional email header "X-Asterisk-VM-Flag".  File-based storage uses an additional attribute in the .txt file called "flag".  The subject line of notification email will state that "you have an URGENT message" as well.  

In IMAP, when retrieving a message, all URGENT messages will be presented first.  In file storage, urgent and new messages will be presented together.  Because the URGENT flag is just that, and not a new directory, urgent and new messages both appear in the INBOX.  

When accessing voicemail, the user will be prompted that they have X urgent messages, Y new messages, and Z old messages.  The number of urgent messages is also sent to extern_notify in case there needs to be special handling of these messages outside of voicemail.

VMCOUNT will accept URGENT as a folder name to return the number of urgent messages.

Some custom sound files were necessary and are included here for testing purposes only.

The branch that contains this code is:

http://svn.digium.com/svn/asterisk/team/jrothenberger
Comments:By: Mark Michelson (mmichelson) 2008-02-01 13:28:58.000-0600

qwell and I have given the branch a review. I will have some sort of text version of the review up as an attachment by the end of the day.
By: Mark Michelson (mmichelson) 2008-02-01 14:02:26.000-0600

I have uploaded Ugly-looking-review.txt. This is a rough translation of what was some AJAXy prettiness into plain text, so sorry if it's aesthetically lacking. I've divided it up into Jason's review and mine. What you'll see is the file name in question, followed by the line number of the change you made, then some of the code you have written, and then a comment regarding the change. In some cases, you will also see what was in the original version of the file if it is relevant to the comment.

I may have messed up some formatting somewhere, and so if anything doesn't make sense or you need clarification regarding a comment, just post a note and I will try to eliminate any confusion.
By: James Rothenberger (jaroth) 2008-02-04 14:06:41.000-0600

I have some questions about the review, I'll post them here unless I hear otherwise.  The question about using the ast_config mechanism instead of reading the file myself - my idea was to stop reading the file as soon as I saw the flag keyword, thus making it more efficient than reading the whole file, since this read would need to be done for every file during voicemail access.  I did not, however, get to move the flag keyword to the beginning of the file as I originally intended (oversight) to make the check as short as possible.  If you want me to use the ast_config method, I would assume that there is no advantage to moving the flag keyword to the front of the file?

Also, how would you suggest that I handle externnotify if you don't suggest changing the parameters that are sent?
By: Mark Michelson (mmichelson) 2008-02-05 10:13:29.000-0600

Regarding externnotify: I actually didn't know if it was problematic or not. It's probably just fine the way you have it now. It's just something that I thought might break whatever scripts or programs people run. We can just put a note in UPGRADE.txt indicating that they will need to handle the extra argument (or ignore it if they don't use urgent messages).

About the ast_config method, you actually can just grab the flag value out of the text file by using ast_variable_retrieve. I recommended using the ast_config API simply because it's a well-tested method. I'm not really sure if there's much of an efficiency gain by doing it the way you were doing or not, and any speed gain made from moving the flag keyword to the beginning of the file would most likely be negligible.
By: James Rothenberger (jaroth) 2008-02-15 10:57:23.000-0600

Thanks guys.  I've made the changes as noted, with explanations of all in the attached review response.  The changes are tested and pushed back into the branch.
By: Mark Michelson (mmichelson) 2008-02-18 16:53:53.000-0600

Great. Thanks for the response. I'll make a second pass at the code ASAP.
By: Digium Subversion (svnbot) 2008-02-18 17:48:52.000-0600

Repository: asterisk
Revision: 103800

U   team/jrothenberger/asterisk-urgent/include/asterisk/app.h
U   team/jrothenberger/asterisk-urgent/main/app.c

------------------------------------------------------------------------
r103800 | mmichelson | 2008-02-18 17:48:51 -0600 (Mon, 18 Feb 2008) | 6 lines

Clear compiler warning due to the fact that the inboxcount function in app_voicemail
now has a different signature.

All changes to this branch are tracked on issue ASTERISK-11280.


------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=103800
By: Mark Michelson (mmichelson) 2008-02-19 15:45:45.000-0600

I'd like to give a preliminary report of my testing of this branch. First, the good:

When I set the 'U' option for voicemail, urgent messages were properly saved to the mailbox. This worked with both file system storage and IMAP. IMAP especially was nice because my mail client marked the message properly.

When I tested using non-urgent messages only, things worked properly.

Now for the problems I encountered:

I could not figure out how to mark a message as urgent when leaving a voicemail. According to the instructions in the description, I should have been able to press 4 to do this, but this did not work. The only way I could find to leave an urgent message was with the 'U' option for Voicemail().

When checking my file-storage voicemails, if I had both urgent and non-urgent new messages, behavior was erratic regarding which messages I heard. For instance, I left 5 messages, 2 urgent, and 3 non-urgent. I left them in the following order
Message 1: Non-urgent
Message 2: Urgent
Message 3: Non-urgent
Message 4: Urgent
Message 5: Non-urgent

I called VoiceMailMain, where I was greeted with the correct count of urgent, new, and old messages (2, 3, 0). I pressed 1 to listen to my messages.
Message 1 played first. I pressed 7 to delete, then 6 for next message.
Message 2 played (it was presented as "last" urgent message). I pressed 7 to delete it. I listened to the options and was not given the '6' option to hear the next message. I pressed '6' anyway.
Message 3 played (it was presented as "first" new message). I pressed 7 to delete it and then 6 to move to the next message.
Message 4 played (it was presented as "last" urgent message). I pressed 7 to delete it. Once more, I was not given the option to press 6 to hear the next message. This time, when I pressed 6, I was told there were no new messages. I tried navigating through all the messages but there was no way for me to hear Message 5. In order to hear it, I had to call into VoiceMailMain a second time.

I unfortunately cannot comment yet about retrieval of urgent messages from IMAP since I'm using a new computer and setting up an IMAP server has not been going smoothly (for the test where I saved voicemails to IMAP it worked just fine to use the company IMAP server :) ).
By: James Rothenberger (jaroth) 2008-02-19 15:52:38.000-0600

To mark a message urgent, you have to press '#' after recording the message (for more options), then press 4.  The recording that I submitted has an updated list of the options available at that menu.
By: Mark Michelson (mmichelson) 2008-02-19 15:53:35.000-0600

But pressing # after you record the message exits voicemail.
By: James Rothenberger (jaroth) 2008-02-19 15:54:48.000-0600

I'll take a look at the message navigation issue.
By: James Rothenberger (jaroth) 2008-02-19 15:57:38.000-0600

Hmmm...  When I press #, I get the option to press 1 to save the message, etc... all the way to press 4 to mark it urgent.
By: Mark Michelson (mmichelson) 2008-02-19 16:13:57.000-0600

Ah, the '#' key problem for me was because I did not have review=yes in voicemail.conf. Setting this gave me the prompt that you had mentioned.
By: James Rothenberger (jaroth) 2008-02-20 08:15:42.000-0600

It might be a benefit for the recording to say "...or press the # key for more options" instead of just "...or press the # key".
By: James Rothenberger (jaroth) 2008-02-21 12:02:52.000-0600

When you created the urgent messages, did you create them all from the phone, or did you mark some //flagged in your email client?
By: Mark Michelson (mmichelson) 2008-02-21 12:05:11.000-0600

I created them all from the phone.
By: James Rothenberger (jaroth) 2008-02-21 13:17:43.000-0600

I created 5 new messages as tested, and here is what I saw.   When you get your "new" messages, you are going to get new "urgent" messages first (I think that this is right behavior since they are urgent).  So when I pressed 1 for my new messages, I got the first urgent message (#2).  After listening and pressing 6 for the next message, I got the last urgent message (#4 - there were only two).  Then pressing 6 took me to the first new message (non-urgent) which was message #1, and then #3, then ASTERISK-1.  I think that the confusion may be from hearing that you are going to be listening to new messages (whether they are urgent or not).  And also after hearing the "last urgent message", hearing that there are still new messages, when it is the case that one would have new urgent and non-urgent messages in their INBOX.  Am I missing a case here?
By: Mark Michelson (mmichelson) 2008-02-21 14:44:32.000-0600

The message order you describe above is exactly what I expected, but it is not what happened. I reran my test a few different ways. First, I ran the test by leaving the non-urgent messages by calling VoiceMail without the 'U' option and leaving the urgent messages with the 'U' option. The next time I ran the test, I didn't delete the messages right after listening to them. Instead I just navigated to the next one using the 6 key. Then I reran the test and marked the messages as urgent by calling VoiceMail without the 'U' option but then pressing 4 to mark them as urgent after recording the message.

In all three scenarios, I experienced the exact same behavior that I experienced before. That is, the urgent messages did not play first, but rather the messages played in chronological order; the message number played before each urgent message was always "last" even if I was listening to the first one; and it was not possible for me to listen to the final message I had left unless I called back after deleting the previous messages.
By: James Rothenberger (jaroth) 2008-02-21 15:15:11.000-0600

Are you using IMAP storage or file-based storage?  I can't reproduce this problem with IMAP storage, which to be honest is where I did most of my testing.
By: Mark Michelson (mmichelson) 2008-02-21 15:19:25.000-0600

I mentioned in note 82603 that due to my new setup here, I don't have an IMAP server which I can retrieve message from, so I've been doing my tests with file-system storage.
By: James Rothenberger (jaroth) 2008-02-21 15:53:09.000-0600

Got it.  When you said that it worked nice with IMAP, I thought that was the storage model you were using.  The file-based access was much more difficult to write from a message-access standpoint.  I'll take another look.
By: James Rothenberger (jaroth) 2008-02-27 12:54:48.000-0600

File-based message access is now working correctly.  I changed the way it works from a file-based urgent flag to the current method of using directories.  There is now an urgent directory under a mailbox for urgent message storage.  I also changed the filename of one of the recordings from vm-Urgent to vm-URGENT to match both the flag and directory names, so be sure to update the name of that file in your sounds directory.
By: Mark Michelson (mmichelson) 2008-02-27 15:39:01.000-0600

Great! I gave things a test and got the asterisk-urgent branch synced back up to trunk now. Things appear to be working now with the file system storage. I'll need to take another look through the code soon to see what has changed since I initially reviewed it to be sure that there aren't problems.
By: Mark Michelson (mmichelson) 2008-03-07 13:52:15.000-0600

I spent yesterday and this morning cleaning up the code and fixing bugs in the asterisk-urgent branch. Here's a list of the revisions and a summary of the changes:

106401: Coding guidelines cleanups
106432: Make buffer size of the urgent flag string consistent
106434: Remove useless variables from functions
106435: Remove an awkward sound file playback
106495: Fixed a bug where it was impossible to mark a message as urgent when originating a voicemail from VoiceMailMain
106498: Corrected a bug where forwarded urgent messages were saved to the INBOX instead of the urgent folder
106500: Constify some strings
106503: Fix the perusal of messages when you delete as you go.
106505: A function was called with the parameters out of order. I swapped them.
106695: Remove an unused function
106702: Cleaned up some logic and made a comment explaining it.
106791: Changed a strncmp to a strcmp

At this point, I'm comfortable with the way that urgent messaging works for IMAP and file-based storage. Now the problem is that there currently is no ODBC storage support. As far as I'm concerned, the only things left to do are:

1. Write ODBC support for urgent messages
2. Test that out thoroughly
3. Get the necessary sounds recorded

Then this will be ready for merge. Could you please get started on the ODBC support? Thanks very much!
By: Criss Keating (crissk) 2008-04-29 11:48:18

Added the english files from Allison
By: Criss Keating (crissk) 2008-04-29 14:42:52

added the french files from June
By: Criss Keating (crissk) 2008-05-01 10:04:50

all the recordings are attached
By: Digium Subversion (svnbot) 2008-05-09 16:16:54

Repository: asterisk
Revision: 115588

U   trunk/CHANGES
U   trunk/UPGRADE.txt
U   trunk/apps/app_voicemail.c
U   trunk/channels/chan_iax2.c
U   trunk/channels/chan_sip.c
U   trunk/channels/chan_skinny.c
U   trunk/channels/chan_unistim.c
U   trunk/include/asterisk/app.h
U   trunk/main/app.c
U   trunk/main/manager.c

------------------------------------------------------------------------
r115588 | mmichelson | 2008-05-09 16:16:53 -0500 (Fri, 09 May 2008) | 19 lines

Adding support for "urgent" voicemail messages. Messages which are
marked "urgent" are considered to be higher priority than other messages
and so they will be played before any other messages in a user's mailbox.

There are two ways to leave an urgent message.
1. send the 'U' option to VoiceMail().
2. Set review=yes in voicemail.conf. This will give instructions for
  a caller to mark a message as urgent after the message has been recorded.

I have tested that this works correctly with file and ODBC storage, and James
Rothenberger (who wrote initial support for this feature) has tested its use
with IMAP storage.

(closes issue ASTERISK-11280)
Reported by: jaroth
Based on branch http://svn.digium.com/svn/asterisk/team/jrothenberger/asterisk-urgent
Tested by: putnopvut, jaroth


------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=115588
By: Digium Subversion (svnbot) 2008-05-09 16:18:20

Repository: asterisk
Revision: 115589

_U  branches/1.6.0/

------------------------------------------------------------------------
r115589 | mmichelson | 2008-05-09 16:18:19 -0500 (Fri, 09 May 2008) | 26 lines

Blocked revisions 115588 via svnmerge

........
r115588 | mmichelson | 2008-05-09 16:22:42 -0500 (Fri, 09 May 2008) | 19 lines

Adding support for "urgent" voicemail messages. Messages which are
marked "urgent" are considered to be higher priority than other messages
and so they will be played before any other messages in a user's mailbox.

There are two ways to leave an urgent message.
1. send the 'U' option to VoiceMail().
2. Set review=yes in voicemail.conf. This will give instructions for
  a caller to mark a message as urgent after the message has been recorded.

I have tested that this works correctly with file and ODBC storage, and James
Rothenberger (who wrote initial support for this feature) has tested its use
with IMAP storage.

(closes issue ASTERISK-11280)
Reported by: jaroth
Based on branch http://svn.digium.com/svn/asterisk/team/jrothenberger/asterisk-urgent
Tested by: putnopvut, jaroth


........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=115589