Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(6)

Issue 13191044: Display URL support for user profile adaotor (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 8 months ago by Tanmay Vartak
Modified:
7 years, 8 months ago
Reviewers:
ejona
CC:
connector-cr_google.com
Visibility:
Public.

Description

Display URL support for user profile adaotor

Patch Set 1 #

Total comments: 9

Patch Set 2 : with code review comments #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+41 lines, -10 lines) Patch
M src/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptor.java View 1 6 chunks +31 lines, -6 lines 2 comments Download
M test/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptorTest.java View 6 chunks +10 lines, -4 lines 0 comments Download

Messages

Total messages: 5
Tanmay Vartak
7 years, 8 months ago (2013-08-23 17:35:57 UTC) #1
ejona
https://codereview.appspot.com/13191044/diff/1/src/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptor.java File src/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptor.java (right): https://codereview.appspot.com/13191044/diff/1/src/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptor.java#newcode201 src/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptor.java:201: Log at Level.CONFIG the mysite host? https://codereview.appspot.com/13191044/diff/1/src/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptor.java#newcode310 src/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptor.java:310: UserProfileServiceSoap ...
7 years, 8 months ago (2013-08-23 19:58:50 UTC) #2
Tanmay Vartak
Thanks for review https://codereview.appspot.com/13191044/diff/1/src/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptor.java File src/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptor.java (right): https://codereview.appspot.com/13191044/diff/1/src/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptor.java#newcode310 src/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptor.java:310: UserProfileServiceSoap inUserProfileServiceSoap Yes this is unrelated ...
7 years, 8 months ago (2013-08-23 20:33:16 UTC) #3
Tanmay Vartak
On 2013/08/23 20:33:16, Tanmay Vartak wrote: > Thanks for review > > https://codereview.appspot.com/13191044/diff/1/src/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptor.java > File ...
7 years, 8 months ago (2013-08-23 20:34:15 UTC) #4
ejona
7 years, 8 months ago (2013-08-24 00:05:22 UTC) #5
LGTM

https://codereview.appspot.com/13191044/diff/1/test/com/google/enterprise/ada...
File
test/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptorTest.java
(right):

https://codereview.appspot.com/13191044/diff/1/test/com/google/enterprise/ada...
test/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptorTest.java:150:
new String[] {"domain\\user1"});
On 2013/08/23 20:33:16, Tanmay Vartak wrote:
> This is important and not unrelated. With this we are testing that "\" in
> username is actually getting decoded as "%5C"

Ah, I see it now.

https://codereview.appspot.com/13191044/diff/7001/src/com/google/enterprise/a...
File
src/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptor.java
(right):

https://codereview.appspot.com/13191044/diff/7001/src/com/google/enterprise/a...
src/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptor.java:190:
log.log(Level.CONFIG, "setAcl: {0}", setAcl);
This got duplicated. Remove duplicate.

https://codereview.appspot.com/13191044/diff/7001/src/com/google/enterprise/a...
src/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptor.java:311:
UserProfileServiceSoap inUserProfileServiceSoap
I'd prefer it to be separate CL if reasonable to do so. It makes looking at the
history more obvious.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b