Bug 465336

Summary:	comments posted on left side show up on right side
Product:	z_Archived	Reporter:	Sam Davis <sam.davis>
Component:	Mylyn	Assignee:	Chris Poon <chris.poon>
Status:	RESOLVED FIXED	QA Contact:
Severity:	major
Priority:	P3	Keywords:	contributed
Version:	unspecified
Target Milestone:	2.7
Hardware:	PC
OS:	Windows 7
Whiteboard:

Description Sam Davis

2015-04-23 14:03:32 EDT

Comments posted on the left side of the compare editor show up on right side after submission. E.g. if I post a comment on line 100 of the base revision, it shows up on line 100 of the right.

Comment 1 Chris Poon

2015-05-22 17:47:03 EDT

Comment drafts that are submitted to Gerrit have a "side" attribute associated with them.  In Mylyn, this side attribute is only set to the "PARENT" or left side in a comment for the base revision.

So the way Gerrit deals with comment publishing is that it gets all of the drafts first and then makes a POST to set the review.  In our code, the draft getter is able to retrieve all of the info about the comment drafts (represented as CommentInfo) from Gerrit (including the "side" attribute).  However, the set review POST call requires CommentInputs, so we convert those CommentInfos.  In our conversion, we never set the "side" attribute, so it is omitted.  Gerrit assumes that all comments without a side attribute are comments for the revision side (https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#comment-input).  This explains why it was always posting base left side comments to the right side.

Comment 2 Sam Davis

2015-05-22 18:06:28 EDT

Thanks Chris! I've pushed a fix to https://git.eclipse.org/r/#/c/48503/. I don't know why we were converting from CommentInfos to CommentInputs since a CommentInfo is already a CommentInput, but I've just removed the CommentInfo class entirely since it seems unneeded.

Can you add a test in a separate review? There are tests of inline comments in PatchSetRemoteFactoryTest and it's easy enough to publish comments on the left side (even if the file doesn't exist in teh base), but I'm not sure how to retrieve them and check that they're on the correct side.

Comment 3 Chris Poon

2015-05-25 19:38:54 EDT

Added some tests in this review - https://git.eclipse.org/r/#/c/48577/.  This should ensure that the comments are posted on the right revision (they fail in a build before the change, and pass after).

Comment 4 Sam Davis

2015-05-26 14:38:48 EDT

Thanks. The changes have been merged.