----------------------------------------------------------- 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 pitli...@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=emailot...@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 > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
