[Home]

Summary:ASTERISK-03858: [patch] adds hidefromdir option to app_directory
Reporter:mh720 (mh720)Labels:
Date Opened:2005-04-04 13:15:08Date Closed:2008-01-15 15:31:06.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Applications/app_directory
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) bug_3950_rev3.diff.txt
( 1) bug_3950_rev4.diff.txt
Description:This patch adds a "hide from directory" option to app_directory.  With this patch you can hide voicemail boxes that you do not want to appear in the company directory, such as the CEO or a conference room's voicemail box can be hidden.

The hidefromdir option was originally included in a patch submitted in bug 2475, but was removed from subsequent patches before it made it to CVS, apparently because it only supported ARA.  I have written this patch so that it works with both voicemail.conf and realtime ARA, simultaneously.  

One caveat: if ARA is used, a hidefromdir column _MUST_ exist in the database, otherwise no ARA entries are ever seen by app_directory (because haveopts never gets set).   I am not quite proficient enough with C to implement a better solution, so I ask that someone here take a look at my patch and have a go at removing this caveat.  I'd prefer the caveat be removed before this is added to CVS.

The patch as-is will work fine for anyone that has the need to hide voicemail entries - if you either use voicemail.conf, or use ARA and include add a hidefromdir column to your voicemail_users schema (I have updated the wiki with the schema).

****** ADDITIONAL INFORMATION ******

Disclaimer on file
Comments:By: Kevin P. Fleming (kpfleming) 2005-04-04 14:07:40

Code review notes:

1 - Don't change strncpy's length argument to not subtract 1 from the size of the buffer, it's mandatory.

2 - Make hidefromdir a 'char *', not a buffer, initialized to an empty string. Don't copy data into it, just set it to a reference to either a ",hidefromdir=yes" string or a ",hidefromdir=no" string. Doing that will eliminate the need for haveopts, you can just include the whole hidefromdir string into the snprintf.

3 - Why did you remove the comment about app_directory only needing certain fields to be filled in?
By: mh720 (mh720) 2005-04-04 14:45:56

Thanks for looking at this Kevin.


>> 1 - Don't change strncpy's length argument to not subtract 1 from the size of the buffer, it's mandatory.

I'm clueless here, wouldn't that return "ye" instead of "yes" for rtvar->value?


>>2 - Make hidefromdir a 'char *', not a buffer, initialized to an empty string.

I need some help here, I do not know enough about C to cast hidefromdir the way you are asking.  Can someone provide me an example?


>>Don't copy data into it, just set it to a reference to either a ",hidefromdir=yes" string or a ",hidefromdir=no" string. Doing that will eliminate the need for haveopts, you can just include the whole hidefromdir string into the snprintf.

I don't beleive that would work in the current logic, because we can't call

ast_variable_append(cat, ast_variable_new(mailbox, tmp));

until we know if we need to add hidefromdir=yes to the string or not.  That loop parses one rtvar->name,value pair at a time, I'm sure there is a much better way to do it, but I'm a C dummy :(


>>3 - Why did you remove the comment about app_directory only needing certain fields to be filled in?

The comment was unnecessary, as was tacking on email@email.com to the string, it is ignored by the do_directory function anyway.  I guess maybe a comment could go there saying "we really only need the mailbox number and name, and hidefromdir option if you use ARA".

edited on: 04-04-05 14:46
By: Kevin P. Fleming (kpfleming) 2005-04-04 19:05:02

OK, I'll re-work your patch and post it here so you can see what I mean :-)
By: mh720 (mh720) 2005-04-04 21:11:45

Kevin, you are too busy of a guy to be working on my small feature request, let me keep at it for a couple days, I'll read some more * code and see if I can pick up on the paradigm you guys are using.


Edit:
I just noticed that you uploaded a patch, this actually breaks the hidefromdir RT functionality (tested) because

     ast_variable_append(cat, ast_variable_new(mailbox, tmp));

gets called as soon as the condition

     if (havemailbox && havename) {

is met.  the hidefromdir option may not be present for several more iterations of the while (rtvar) { loop.  In any case, you showed me how to correctly cast hidefromdir, so I'll incorporate that and hopefully release another patch once I can figure out the logic of removing the hidefromdir DB requirement.


-mike

edited on: 04-04-05 21:44
By: Kevin P. Fleming (kpfleming) 2005-04-04 23:09:01

OK, I understand your point now. Yes, that will be a little tricky to implement, but I'm sure you can figure out a way. Thanks for sticking with it :-)
By: mh720 (mh720) 2005-04-05 16:36:03

I've uploaded bug_3950_rev3.diff.txt which incorporates the changes discussed so far.  It does not yet remove the 'hidefromdir' field requirement of voicemail_users table, but I have tested this patch thoroughly and it performs as expected.  I consider this patch ready for production machines, but only for those either using voicemail.conf, or ARA and a voicemail_users table modified with the hidefromdir option column as described on the voip-info tiki.

I was hoping that this feature would also be compatible with the old way of storing voicemail users in a database (before ARA), but it appears that app_directory never supported loading these users.
By: Kevin P. Fleming (kpfleming) 2005-04-05 16:43:23

But there is still no need for the haveopts variable, as (haveopts != 0) == (!ast_strlen_zero(hidefromdir)). In other words, if the hidefromdir is pointing to a non-empty string, you have seen the hidefromdir variable pass by.

Also, please put the ',' back into the hidefromdir string, instead of the printf format string, so that multiple options (if needed) can be easily strung together in the future.
By: mh720 (mh720) 2005-04-05 18:01:19

Uploaded.
By: Kevin P. Fleming (kpfleming) 2005-04-05 18:18:21

Last thought: if this is supporting 'hidefromdir' in voicemail.conf as well, then it should be using strcasestr instead of strstr, so as not to make the configuration file case-sensitive (sorry I missed that one before).

However, I can make that change at commit time, you don't need to upload a new patch.
By: Kevin P. Fleming (kpfleming) 2005-04-06 13:26:54

As it turns out, the implementation of ARA in app_directory is not quite right, and I'm going to reimplement it. Once that is done the requirement for a 'hidefromdir' column in the ARA backend will be eliminated as well. I'll incorporate your work into the changes and you'll still get your karma :-)
By: Kevin P. Fleming (kpfleming) 2005-04-06 14:03:14

New version committed to CVS, please test it... Thanks!
By: Kevin P. Fleming (kpfleming) 2005-04-06 14:03:44

Not in stable
By: Digium Subversion (svnbot) 2008-01-15 15:31:06.000-0600

Repository: asterisk
Revision: 5429

U   trunk/apps/app_directory.c
U   trunk/configs/voicemail.conf.sample

------------------------------------------------------------------------
r5429 | kpfleming | 2008-01-15 15:31:06 -0600 (Tue, 15 Jan 2008) | 3 lines

re-implement realtime support in app_directory
add support for hiding entries from app_directory using new hidefromdir= option (bug ASTERISK-3858)

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

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