[asterisk-dev] [Code Review] Calendaring API for Asterisk

[Mark Michelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/58/#review139
-----------------------------------------------------------



/trunk/main/calendar.c
<http://reviewboard.digium.com/r/58/#comment249>

    Be extra careful here, because calendar finding is not going to be as case-insensitive as you want. The best way to explain this is with an example.
    
    Say that someone sets up a calendar called "MyCalendar" in calendar.conf. Then, they execute some dialplan query on the calendar "mycalendar." It is apparent from your code here that you intend for this query to match "MyCalendar." The problem is that despite the case-insensitive comparison used in calendar_cmp_fn, it is very unlikely that you will find a match.
    
    This is due to the fact that "MyCalendar" and "mycalendar" hash to different values when put through the calendar_hash_fn. What's needed is to hash an all-uppercase or all-lowercase version of the name instead of the name as-is. I actually have a commit that's due into Asterisk trunk which will provide a version of ast_str_hash which converts all characters to lowercase for calculating the hash.
    
    Also, while I'm here, I suggest having your comparison function return CMP_MATCH | CMP_STOP in the case that a match is found.



/trunk/main/calendar.c
<http://reviewboard.digium.com/r/58/#comment250>

    Everything I said about calendar names applies to events, too.


- Mark


On 2008-11-12 19:04:58, Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/58/
> -----------------------------------------------------------
> 
> (Updated 2008-11-12 19:04:58)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Calendaring integration for Asterisk--currently supporting iCalendar files (hosted over HTTP/HTTPS), CalDAV, and Microsoft Exchange.  Features available: dialplan functions for querying and writing (if the calendar technology supports it like CalDAV and Exchange do) calendar event information, a device state provider for phone BLF support, and calendar event notification through executing dialplan apps or context/exten.
> 
> The overall structure is that the res_* calendar modules register as a calendar tech to main/calendar.c, and after all modules are loaded, main/calendar.c parses calendar.conf and calls the calendar_load callback for each register calendar tech based on the "type" field for that calendar in calendars.conf.  Currently each calendar runs in its own thread downloading and refreshing the remote calendar data as necessary as to avoid serializing downloads.  
> 
> Eventually it would probably be a good idea for me to implement a thread pool and honor a maximum number of downloads per host:port as well as a global maximum number of simultaneous downloads--perhaps with a min/max refresh value so I could randomize the time for refreshing a bit so as not to be refreshing all of the calendars at once.  Also, there was a request to add support for the dialplan functions to query calendars that aren't in calendar.conf i.e. CALENDAR_QUERY(ical/http://www.google.com/calendar/ical/nkt5atdq7cdbes3ehdfpendpnc%40group.calendar.google.com/public/basic.ics,${EPOCH},$[${EPOCH} + 3600]), etc.
> 
> Currently I don't have any support for querying/setting attendees, mostly because it is a list of results whereas all of the others are individual fields.  Writing gets especially ugly for them because of the current format of CALENDAR_WRITE(calendar,${HASHKEYS(calendar)})=${HASH(calendar)}.  I suppose I could add a ...,${HAHSKEYS(attendees)})=...,${HASH(attendees)} to the end...it is just getting a little ugly.
> 
> 
> Diffs
> -----
> 
>   /trunk/build_tools/menuselect-deps.in 156482 
>   /trunk/configs/calendar.conf.sample PRE-CREATION 
>   /trunk/configure.ac 156482 
>   /trunk/include/asterisk/_private.h 156482 
>   /trunk/include/asterisk/autoconfig.h.in 156482 
>   /trunk/include/asterisk/calendar.h PRE-CREATION 
>   /trunk/main/Makefile 156482 
>   /trunk/main/asterisk.c 156482 
>   /trunk/main/calendar.c PRE-CREATION 
>   /trunk/main/loader.c 156482 
>   /trunk/makeopts.in 156482 
>   /trunk/res/res_caldav.c PRE-CREATION 
>   /trunk/res/res_exchangecal.c PRE-CREATION 
>   /trunk/res/res_icalendar.c PRE-CREATION 
> 
> Diff: http://reviewboard.digium.com/r/58/diff
> 
> 
> Testing
> -------
> 
> I have tested all three calendar modules by adding, deleting, and moving events and verifying that notifications occur.  I have tested writing to Exchange, and Zimbra and Google Calendar through CalDAV.  I have run with MALOC_DEBUG looking for leaks and dropped references to astobj2 objects. 
> 
> 
> Thanks,
> 
> Terry
> 
>