Display URL support for user profile adaotor

Display URL support for user profile adaotor

[ejona, 2013-08-23 19:58:50]

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

https://codereview.appspot.com/13191044/diff/1/src/com/google/enterprise/adap...
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/adap...
src/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptor.java:310:
UserProfileServiceSoap inUserProfileServiceSoap
This looks unrelated. Important, but unrelated.

Note that the indentation is off.

https://codereview.appspot.com/13191044/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptor.java:545:
String displayUrl = MessageFormat.format(
Why use MessageFormat instead of concatenation? Normally I do think
MessageFormat is more readable, but here I'd just assume to use normal
concatenation. If you like MessageFormat though, then that is fine.

https://codereview.appspot.com/13191044/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptor.java:548:
response.setDisplayUrl(URI.create(displayUrl) );
s/) )/))/

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"});
This is also unrelated. Good, but unrelated.

[Tanmay Vartak, 2013-08-23 20:33:16]

Thanks for review

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

https://codereview.appspot.com/13191044/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptor.java:310:
UserProfileServiceSoap inUserProfileServiceSoap
Yes this is unrelated but important. can be a different CL
On 2013/08/23 19:58:50, ejona wrote:
> This looks unrelated. Important, but unrelated.
> 
> Note that the indentation is off.

https://codereview.appspot.com/13191044/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptor.java:545:
String displayUrl = MessageFormat.format(
It just my .NET practice of using String.Format
On 2013/08/23 19:58:50, ejona wrote:
> Why use MessageFormat instead of concatenation? Normally I do think
> MessageFormat is more readable, but here I'd just assume to use normal
> concatenation. If you like MessageFormat though, then that is fine.

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"});

This is important and not unrelated. With this we are testing that "\" in
username is actually getting decoded as "%5C"

On 2013/08/23 19:58:50, ejona wrote:
> This is also unrelated. Good, but unrelated.

[Tanmay Vartak, 2013-08-23 20:34:15]

On 2013/08/23 20:33:16, Tanmay Vartak wrote:
> Thanks for review
> 
>
https://codereview.appspot.com/13191044/diff/1/src/com/google/enterprise/adap...
> File
> src/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptor.java
> (right):
> 
>
https://codereview.appspot.com/13191044/diff/1/src/com/google/enterprise/adap...
>
src/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptor.java:310:
> UserProfileServiceSoap inUserProfileServiceSoap
> Yes this is unrelated but important. can be a different CL
> On 2013/08/23 19:58:50, ejona wrote:
> > This looks unrelated. Important, but unrelated.
> > 
> > Note that the indentation is off.
> 
>
https://codereview.appspot.com/13191044/diff/1/src/com/google/enterprise/adap...
>
src/com/google/enterprise/adaptor/sharepoint/SharePointUserProfileAdaptor.java:545:
> String displayUrl = MessageFormat.format(
> It just my .NET practice of using String.Format
> On 2013/08/23 19:58:50, ejona wrote:
> > Why use MessageFormat instead of concatenation? Normally I do think
> > MessageFormat is more readable, but here I'd just assume to use normal
> > concatenation. If you like MessageFormat though, then that is fine.
> 
>
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"});
> 
> This is important and not unrelated. With this we are testing that "\" in
> username is actually getting decoded as "%5C"
> 
> On 2013/08/23 19:58:50, ejona wrote:
> > This is also unrelated. Good, but unrelated.
username is actually getting encoded* as "%5C"

[ejona, 2013-08-24 00:05:22]

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.