[asterisk-dev] [Code Review] 1152: Access to any Exchange 2007 and 2010 calendar.

[Jonathan Rose]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1152/#review13094
-----------------------------------------------------------



/trunk/res/res_calendar_ews.c
<https://reviewboard.asterisk.org/r/1152/#comment23529>

    Clean up the red space here.



/trunk/res/res_calendar_ews.c
<https://reviewboard.asterisk.org/r/1152/#comment23545>

    This function seems significant enough that it could use a little XML documentation.
    
    I know little else in this module is providing XML documentation so far, but we like to be a little more on top of that sort of thing lately.



/trunk/res/res_calendar_ews.c
<https://reviewboard.asterisk.org/r/1152/#comment23530>

    trailing red space here. Plus we usually close function calls on the last name containing arguments.



/trunk/res/res_calendar_ews.c
<https://reviewboard.asterisk.org/r/1152/#comment23546>

    XML Documentation
    
    Also this function is really large. Consider refactoring it and breaking it into smaller chunks.  That might help dealing with all the excessive cleanup steps too.



/trunk/res/res_calendar_ews.c
<https://reviewboard.asterisk.org/r/1152/#comment23539>

    I'm not sure if this is a useful debug message. It should probably be removed since it isn't pointing anything specifically.



/trunk/res/res_calendar_ews.c
<https://reviewboard.asterisk.org/r/1152/#comment23531>

    red space here.



/trunk/res/res_calendar_ews.c
<https://reviewboard.asterisk.org/r/1152/#comment23532>

    and here.



/trunk/res/res_calendar_ews.c
<https://reviewboard.asterisk.org/r/1152/#comment23533>

    Mixed tabs and spaces.



/trunk/res/res_calendar_ews.c
<https://reviewboard.asterisk.org/r/1152/#comment23534>

    trailing whitespace



/trunk/res/res_calendar_ews.c
<https://reviewboard.asterisk.org/r/1152/#comment23540>

    I don't love the huge stacks of cleanup going on here at every failure step.  Perhaps you could just have a single place where you free everything and use a goto to skip to it on failure conditions.
    
    Plus at this point it looks like you are going to be freeing everything immediately afterwards anyway, so you could just track your return from send_ews_request_and_parse and then return 0 early if that ends up returning non-zero.



/trunk/res/res_calendar_ews.c
<https://reviewboard.asterisk.org/r/1152/#comment23535>

    trailing whitespace.



/trunk/res/res_calendar_ews.c
<https://reviewboard.asterisk.org/r/1152/#comment23536>

    Trailing whitespace



/trunk/res/res_calendar_ews.c
<https://reviewboard.asterisk.org/r/1152/#comment23537>

    Trailing whitespace, plus we usually close function calls on the last line with an argument rather than like we do with braces.



/trunk/res/res_calendar_ews.c
<https://reviewboard.asterisk.org/r/1152/#comment23541>

    I don't think there is any need to have both the debug log message and the warning log message. If this is something the user needs to be informed of, get rid of the debug.  If not, get rid of the warning.



/trunk/res/res_calendar_ews.c
<https://reviewboard.asterisk.org/r/1152/#comment23542>

    As above



/trunk/res/res_calendar_ews.c
<https://reviewboard.asterisk.org/r/1152/#comment23543>

    As above



/trunk/res/res_calendar_ews.c
<https://reviewboard.asterisk.org/r/1152/#comment23544>

    As above



/trunk/res/res_calendar_ews.c
<https://reviewboard.asterisk.org/r/1152/#comment23538>

    whitespace


I'm not seeing any obvious technical problems, but I did notice a little that could be refactored. Let me know if you get around to that cleanup patch.

Also, it might be a good idea to go ahead and file an issue report for this on JIRA if we are going to get started back up on this.

- Jonathan Rose


On May 22, 2011, 1:59 a.m., astmiv wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1152/
> -----------------------------------------------------------
> 
> (Updated May 22, 2011, 1:59 a.m.)
> 
> 
> Review request for Asterisk Developers and pitlicek at gmail.com.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch will add access to any calendar folder within Exchange 2007 and 2010. 
> 
> The current resource only gives access to the default calendar folder of the specified user. With this patch it is possible to access any calendar folder within the system as long as the user has read rights to the folder and its complete folder path.
> 
> For example:
> - Calendar folders below the publicfoldersroot.
> - Calendar folders below the user's mailbox outside of his default calendar.
> - Calendar folders below the user's default calendar.
> - etc....
> 
> Also did some cleanup for XML schema labeling. They are now all the same.
> 
> 
> Diffs
> -----
> 
>   /trunk/res/res_calendar_ews.c 311843 
>   /trunk/configs/calendar.conf.sample 311843 
> 
> Diff: https://reviewboard.asterisk.org/r/1152/diff/
> 
> 
> Testing
> -------
> 
> Tested the following scenario's:
> - Access to default calendar of specified user. (folderbase not specified or folderbase=calendar)
> - Access to shared default calendar of other person. (mailbox=emailother at company.com and folderbase not specified or folderbase=calendar)
> - Access to calendar folder, named testfolder1, below default Calendar. (folderbase=calendar and folderpath=/testfolder1)
> - Access to calendar folder, named testfolder2, below a subfolder, named testfolder3, of the default Calendar. (folderbase=calendar and folderpath=/testfolder3/testfolder2)
> - Access to calendar folder in Public Folders. (folderbase=publicfoldersroot and folderpath=/meetingroom1)
> - Access to calendar folder below a subfolder in Public Folders. (folderbase=publicfoldersroot and folderpath=/meetingrooms/meetingroom1)
> - Access to calendar folder below mailbox of specified user. (folderbase=msgfolderroot and folderpath=/calendar2)
> 
> 
> Thanks,
> 
> astmiv
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140815/59202656/attachment-0001.html>