Summary:	ASTERISK-06980: VMCOUNT() does not work with ODBC_STORAGE.
Reporter:	Will McCown (flynwill)	Labels:
Date Opened:	2006-05-15 15:00:46	Date Closed:	2006-05-18 13:29:20
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Applications/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) 20060515__bug7167.diff.txt ( 1) 20060518__bug7167.diff.txt ( 2) vmcountfix.patch
Description:	When using ODBC voicemail storage the dialplan function VMCOUNT()does not work. It usually returns zero as it is still counting files.  (The HasNewVoicemail() function also fails as it relies on the same code.) This code also overlaps the functionality of the routine app_voicemail.c:messagecount, which does have an ODBC_STORAGE version. Therefore IMHO the proper fix involves some code cleanup here.  What I would recommend is to create a new routine in app_voicemail that checks a particular box, context & folder much like the routine app_hasnewvoicemail:hasvoicemail_internal, adding the necessary conditional compiles for ODBC_STORAGE.  Then app_voicemail.c:messagecount should be replace with a much simpler function that calls the new code twice, one for "INBOX" and once for "Old". I'm willing to write this cleanup and submit a patch, but I would like to make sure the administrators believe that this is a reasonable thing to do.
Comments:	By: Tilghman Lesher (tilghman) 2006-05-15 16:21:10 Actually, the way this should go is that the routines needed to be accessed in more than one place should be moved to app.c and become public functions.  If you do this in a minimal way (i.e. without a complete redesign of ODBC_STORAGE), then this can go into 1.2.  Otherwise, it will need to go into trunk and become part of 1.4, when that version is released in about a month or so. By: Will McCown (flynwill) 2006-05-15 17:15:27 Hmm.  I see your point.  That does make it more complicated. It would have made more sense (at least to me) if the dialplan functions provided by hasnewvoicemail.c had been app_voicemail.c in the first place. The new routine would still want to live in app_voicemail (so the conditional compile for ODBC doesn't have to extend beyond that module), but then could be exported to a wrapper in app.c in the same way as app_voicemail.c:messagecount() is wrapped by app.c:ast_app_messagecount. Alternately app_voicemail.c:has_voicemail (and therefore the wrapper app.c:ast_app_has_voicemail) could be modified to return a count rather than just 0 or 1.  But that would require a lot of looking to make sure it doesn't break anything. By: Tilghman Lesher (tilghman) 2006-05-15 18:29:11 This patch isn't out of the woods yet, because this changes the core API, so for it to go in, it needs an exception from Russell, the 1.2 maintainer. By: Will McCown (flynwill) 2006-05-16 09:34:24 Well there's always the cheap way out... app_hasnewvoicemail.c:hasvoicemail_internal could be modified directly to know how to query the ODBC.  Largely a cut & paste job from app_voicemail.c, just makes that code that much bigger. This issue is not a "show stopper" for our needs -- I only stumbled on it trying to find a way to light message lights on a connected legacy phone switch.  With the patch in issue 7133 the "externnotify" option works for needs just fine. By: Tilghman Lesher (tilghman) 2006-05-16 21:41:10 Word from another senior developer is that we can fix this in trunk, but it's too late in the 1.2 cycle to change the API.  You may query russellb, the 1.2 maintainer to appeal this decision, but it's improbable that he'll allow this change to 1.2. I'll let this stay open for a few more days, if Russell cares to make a final determination, then it will be committed to trunk alone. By: Tilghman Lesher (tilghman) 2006-05-18 09:46:57 flynwill:  your patch suffers from the same problem as the previous one; namely, it changes the public API. By: Will McCown (flynwill) 2006-05-18 10:02:25 Afraid I didn't even notice that you had uploaded a patch with your message of 5/15, and proceeded to write it myself.  I've uploaded my work for your consideration, but won't be offended if you move it to /dev/null. I did a bit more cleanup, there is now only one conditionally compiled routine -- a new one I called voicemail_count -- functionally the same as your messagecount2. In my submission: the other routines -- messagecount & has_voicemail are now outside of the conditional compile.  (Which I believe will keep the code easier to maintain). One additional detail.  It appears that the original non-ODBC versions of messagecount and has_voicemail were designed to accept a comma-separated list of  mailboxes.  The ODBC versions did not have this feature.  In my submission the comma-separated list feature now works in both ODBC and non-ODBC compiles.  I have no idea if anyone anywhere uses this so it may not matter. By: Will McCown (flynwill) 2006-05-18 10:08:01 Understood. However, the only other practical fix I can see is for app_voicemail.c to directly export your messagecount2 (or my voicemail_count), and for app_hasvoicemail.c to call it. By: Tilghman Lesher (tilghman) 2006-05-18 12:16:06 Okay, the cut'n'paste approach was approved by Russell for 1.2.  The better solution that doesn't involve code duplication will go into trunk. By: Tilghman Lesher (tilghman) 2006-05-18 13:04:41 First fix committed to 1.2. By: Tilghman Lesher (tilghman) 2006-05-18 13:29:20 Also fixed in trunk.