Issue 317850043: Use FileRef to identify Files (Closed)
|
Created:
7 years, 5 months ago by lchandramouli Modified:
7 years, 5 months ago CC:
connector-cr_google.com Visibility:
Public. |
DescriptionUse FileRef to identify Files
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address review comments #
Total comments: 6
Patch Set 3 : Removed double verification of golden content type in testGetDocContentInList. #
Total comments: 2
Patch Set 4 : Added indentation at L:3036 #
MessagesTotal messages: 9
https://codereview.appspot.com/317850043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java (right): https://codereview.appspot.com/317850043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:3035: && contentTypeId.startsWith(CONTENTTYPEID_DOCUMENT_PREFIX)) || Move || to the beginning of the next line. More generally, I don't think isFile really adds much here, especially given the comment inside. if ((contentTypeId != null && contentTypeId.startsWith(CONTENTTYPEID_DOCUMENT_PREFIX)) || fileRef != null && !fileRef.endsWith("_.000")) { There's some asymmetry between CONTENTTYPEID_DOCUMENT_PREFIX, which is a single-use constant, and "_.000". Maybe add a FILEREF_LISTITEM_SUFFIX constant above? I am mildly nervous about the apparent lack of documentation for _.000, but OK with it. https://codereview.appspot.com/317850043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java (right): https://codereview.appspot.com/317850043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java:2697: @Test Fix indentation. https://codereview.appspot.com/317850043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java:2765: assertEquals("application/pdf", response.getContentType()); There is some inconsistencies here with other tests. The msg test looks like this, but the attachment test also includes: String responseString = new String(baos.toByteArray(), charset); assertEquals(goldenContents, responseString); and also verifies the content and content type before the ACL, etc. Think about adding this to the Outlook msg test as well.
Sign in to reply to this message.
https://codereview.appspot.com/317850043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java (right): https://codereview.appspot.com/317850043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:3035: && contentTypeId.startsWith(CONTENTTYPEID_DOCUMENT_PREFIX)) || On 2016/10/28 02:29:38, JohnL wrote: > Move || to the beginning of the next line. > > More generally, I don't think isFile really adds much here, especially given the > comment inside. > > if ((contentTypeId != null > && contentTypeId.startsWith(CONTENTTYPEID_DOCUMENT_PREFIX)) > || fileRef != null && !fileRef.endsWith("_.000")) { > > There's some asymmetry between CONTENTTYPEID_DOCUMENT_PREFIX, which is a > single-use constant, and "_.000". Maybe add a FILEREF_LISTITEM_SUFFIX constant > above? I am mildly nervous about the apparent lack of documentation for _.000, > but OK with it. Done. https://codereview.appspot.com/317850043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java (right): https://codereview.appspot.com/317850043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java:2697: @Test On 2016/10/28 02:29:38, JohnL wrote: > Fix indentation. Done. https://codereview.appspot.com/317850043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java:2765: assertEquals("application/pdf", response.getContentType()); On 2016/10/28 02:29:38, JohnL wrote: > There is some inconsistencies here with other tests. The msg test looks like > this, but the attachment test also includes: > > String responseString = new String(baos.toByteArray(), charset); > assertEquals(goldenContents, responseString); > > and also verifies the content and content type before the ACL, etc. > > Think about adding this to the Outlook msg test as well. Done. Added assert verification for content response, whereas response.getContentType() is already being verified.
Sign in to reply to this message.
There's something wrong with patch set 2, like you have 2 commits in your local branch and the second patch set is just the second commit, instead of including both commits. If you have 2 commits, you'll have to squash them before submitting, so you can do that before the next patch set as well. John L https://codereview.appspot.com/317850043/diff/20001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java (right): https://codereview.appspot.com/317850043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:3036: && contentTypeId.startsWith(CONTENTTYPEID_DOCUMENT_PREFIX)) Indent +4 and delete trailing space. https://codereview.appspot.com/317850043/diff/20001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java (right): https://codereview.appspot.com/317850043/diff/20001/test/com/google/enterpris... test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java:2685: .getDocContent(request, response); Add back two trailing spaces (not because we want them, but because we don't make whitespace only changes to otherwise unmodified lines). https://codereview.appspot.com/317850043/diff/20001/test/com/google/enterpris... test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java:2761: assertEquals(goldenContentType, response.getContentType()); There's no need to check the content type twice, and please be consistent about where we are checking the content and content type. I prefer them together, and you need to just pick before the ACL check or at the end, you can't be consistent with existing practice, which did both.
Sign in to reply to this message.
https://codereview.appspot.com/317850043/diff/20001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java (right): https://codereview.appspot.com/317850043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:3036: && contentTypeId.startsWith(CONTENTTYPEID_DOCUMENT_PREFIX)) On 2016/11/01 22:30:37, JohnL wrote: > Indent +4 and delete trailing space. Done. https://codereview.appspot.com/317850043/diff/20001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java (right): https://codereview.appspot.com/317850043/diff/20001/test/com/google/enterpris... test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java:2685: .getDocContent(request, response); On 2016/11/01 22:30:37, JohnL wrote: > Add back two trailing spaces (not because we want them, but because we don't > make whitespace only changes to otherwise unmodified lines). Done. https://codereview.appspot.com/317850043/diff/20001/test/com/google/enterpris... test/com/google/enterprise/adaptor/sharepoint/SharePointAdaptorTest.java:2761: assertEquals(goldenContentType, response.getContentType()); On 2016/11/01 22:30:37, JohnL wrote: > There's no need to check the content type twice, and please be consistent about > where we are checking the content and content type. I prefer them together, and > you need to just pick before the ACL check or at the end, you can't be > consistent with existing practice, which did both. Done.
Sign in to reply to this message.
By mistake I had 2 commits on the same CL. I have corrected the patch. Thank you John.
Sign in to reply to this message.
LGTM++ (++ is sticky; please make the noted change, but no need to review that again.) John L https://codereview.appspot.com/317850043/diff/40001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java (right): https://codereview.appspot.com/317850043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:3036: && contentTypeId.startsWith(CONTENTTYPEID_DOCUMENT_PREFIX)) Please indent this line 4 more spaces, to make the logical expression more clear (&& is not a peer of || here).
Sign in to reply to this message.
https://codereview.appspot.com/317850043/diff/40001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java (right): https://codereview.appspot.com/317850043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/sharepoint/SharePointAdaptor.java:3036: && contentTypeId.startsWith(CONTENTTYPEID_DOCUMENT_PREFIX)) On 2016/11/02 22:48:02, JohnL wrote: > Please indent this line 4 more spaces, to make the logical expression more clear > (&& is not a peer of || here). Done.
Sign in to reply to this message.
Committed: https://github.com/googlegsa/sharepoint/commit/319eeab3b88abbeff9763582c052b7... Please close this review. John L
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
