Summary

Fixed link to replying to other replies in the diff viewer

Review Request #7926 — Created Jan. 30, 2016 and submitted March 31, 2016, 1:29 p.m.

Information

Owner

imadueme

Repository

Review Board

Branch

release-2.5.x

Bugs

4049

Depends On

Reviewers

Groups

reviewboard, students

People

Description

On the diff page the list of comments doesn't differenciate between
top-level comments and replies to those comments. Because reviewboard
doesn't support replying to a reply of a comment, the "Reply" link on
those reply comments would not work properly. This fix solves that problem
by changing those reply links to reply to their parent comment.

Testing Done

Manual Test:
-Create a comment on a line on the diff viewer page for a review request.

-Reply to that comment.

-Go back to the diff viewer page and click the reply link of
that reply comment.

-Expected result is that it should go to the review page, scroll to,
and open the reply editor for the parent comment of that reply.

-Jasmine js test suite passed
-Reviewboard unit test suite passed
-2 new tests added to the js test suit

Issues

Description	From	Last Updated
Needs a trailing comma after the last entry (we do this so that if we end up adding another entry …	mike_conley	Jan. 30, 2016, 12:43 p.m.
Let's call this reply_to_id, for consistency.	chipx86	Jan. 30, 2016, 3:19 p.m.
This one should be comment-list-reply-action. That will make your queries easier, too.	chipx86	Jan. 30, 2016, 3:19 p.m.
In our codebase, we only ever have one var statement per function, at the very top, one variable per line. …	chipx86	Jan. 30, 2016, 3:19 p.m.
This made me notice a typo in the original object. Should be "Test User".	chipx86	Jan. 30, 2016, 3:19 p.m.
As above, the var should go at the top of this function (with a blank line after it).	chipx86	Jan. 30, 2016, 3:19 p.m.
Same here.	chipx86	Jan. 30, 2016, 3:19 p.m.
This should be formatted as: expect(/.../) .toContain(/.../)	brennie	Jan. 31, 2016, 8:27 a.m.
Single quotes. Also, this should be formatted as: expect(/.../) .toContain(/.../);	brennie	Jan. 31, 2016, 8:27 a.m.
Same here.	brennie	Jan. 31, 2016, 8:27 a.m.
We don't usually put trailing comments.	brennie	Jan. 31, 2016, 8:27 a.m.
Since you're doing $commentsList.find(selector) multiple times, it would be better to do: var $replies; editor.set(/.../); dlg.open(); $replies = $commentList.find(selector); expect($replies[0].href).toContain(/.../); …	brennie	Jan. 31, 2016, 8:27 a.m.
We use single quotes.	brennie	Jan. 31, 2016, 8:27 a.m.
Same here.	brennie	Jan. 31, 2016, 8:27 a.m.
Can also probably build the target string ahead of time, too.	brennie	Jan. 31, 2016, 8:27 a.m.

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/commentDialogView.js



Tool: Pyflakes
Processed Files:
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/commentDialogView.js

reviewboard/reviews/context.py (Diff revision 1)

This is a test message. Please ignore.

imadueme Jan. 30, 2016, 11:01 a.m.

This is a test reply message. Please ignore.

mike_conley Jan. 30, 2016, 11:11 a.m.

(Gotcha - though, in the future, if you want to do testing stuff, check out http://demo.reviewboard.org)

Something that's important when writing commit/review request descriptions is to tell the reviewer the story of your change. Assume that the person reading it may be reading it 3 years from now, and may have no knowledge of what you did or why. Then go from there.

The summary should be a clear, concise thing about what you did: "Fixed the link for replying to other replies in the diff viewer."

Then tell the story in the description of what was wrong before and what you've done to address it. Not all the technical details, but the higher-level stuff.

See https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/ for examples.

imadueme Jan. 30, 2016, 11:17 a.m.
```
Will do
```

Looking good! Can you please update the bug number in this review request?

reviewboard/reviews/context.py (Diff revision 1)

Show all issues

Needs a trailing comma after the last entry (we do this so that if we end up adding another entry to the end of this list, we don't have to add the comma to the previous entry which updates blame on that line.

Also, I'm not 100% sure, but doesn't this default to None already if no relationship is set? If so, we can probably just rewrite this as:

...
'reply_to': comment.reply_to_id,

chipx86 Jan. 30, 2016, 11:23 a.m.

That's correct, it will default to None.

This is also going to need unit tests that check for the two possibilities: A reply to ID, or a review ID.

This would live in js/views/tests/commentDialogViewTests.js. You can see where we have a describe('Other published comments list', ...) section, which already does a few things. You'd add new tests that verify the links for both types of comments.

By having this, we ensure that this can't break down the road without us noticing.

imadueme Jan. 30, 2016, 11:44 a.m.
```
On it
```

Summary:

-	Reply link on diff page conditonally links to parent comment
+	Fixed bug #4049 which fixes replying to other replies in the diff viewer

Description:

~		Reply link on diff page conditonally links to parent comment
	~	This is a fix for bug #4049. On the diff page the list of comments doesn't differenciate between top-level comments and replies to those comments. Because reviewboard doesn't support replying to a reply of a comment, the "Reply" link on those reply comments would not work properly. This fix solves that problem by changing those reply links to reply to their parent comment.

Testing Done:

~		Only manual testing was done.
	~	Only manual testing was done.
	+	Create a comment on a line on the diff viewer page for a review request.
	+	Reply to that comment.
	+	Go back to the diff viewer page and click the reply link of that reply comment.
	+	Expected result is that it should go to the review page, scroll to, and open the reply editor for the parent comment of that reply.

Change Summary:

Added the trailing comma on the newest attribute to keep git blame consistent. Also removed extraneous "or None".

Diff:

Revision 2 (+2 -1)

Show changes

	reviewboard/reviews/context.py
	reviewboard/static/rb/js/views/commentDialogView.js

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/commentDialogView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/commentDialogView.js

Almost! The bug number actually isn't useful in either the summary or the description. It should go in the bugs field. That will automatically be placed in the commit message when landing.

The description should also wrap at 80 characters, like our source code. It's best to compose this in your commit message, using your editor to ensure the proper width, and use that for the review request description. The summary/description in the review request will be used as-is in the commit message.

mike_conley Jan. 30, 2016, 11:57 a.m.

Also, please don't forget to close the issue I opened in my earlier review - that way. Open issues make reviewers antsy. :)

Bugs:

+	4049

Summary:

-	Fixed bug #4049 which fixes replying to other replies in the diff viewer
+	Fixed link to replying to other replies in the diff viewer

Description:

~		This is a fix for bug #4049. On the diff page the list of comments doesn't differenciate between top-level comments and replies to those comments. Because reviewboard doesn't support replying to a reply of a comment, the "Reply" link on those reply comments would not work properly. This fix solves that problem by changing those reply links to reply to their parent comment.
	~	On the diff page the list of comments doesn't differenciate between top-level comments and replies to those comments. Because reviewboard doesn't support replying to a reply of a comment, the "Reply" link on those reply comments would not work properly. This fix solves that problem by changing those reply links to reply to their parent comment.

Change Summary:

Added unit tests for this fix to make sure it doesn't happen again.

Diff:

Revision 3 (+32 -3)

Show changes

	reviewboard/reviews/context.py
	reviewboard/static/rb/js/views/commentDialogView.js
	reviewboard/static/rb/js/views/tests/commentDialogViewTests.js

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/tests/commentDialogViewTests.js
    reviewboard/static/rb/js/views/commentDialogView.js



Tool: Pyflakes
Processed Files:
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/tests/commentDialogViewTests.js
    reviewboard/static/rb/js/views/commentDialogView.js

reviewboard/reviews/context.py (Diff revision 3)
Show all issues
```
Let's call this reply_to_id, for consistency.
```

reviewboard/static/rb/js/views/commentDialogView.js (Diff revision 3)

Show all issues

This one should be comment-list-reply-action. That will make your queries easier, too.

reviewboard/static/rb/js/views/tests/commentDialogViewTests.js (Diff revision 3)

Show all issues

In our codebase, we only ever have one var statement per function, at the very top, one variable per line. So:

var comment,
    commentReply;

reviewboard/static/rb/js/views/tests/commentDialogViewTests.js (Diff revision 3)
Show all issues
```
This made me notice a typo in the original object. Should be "Test User".
```

reviewboard/static/rb/js/views/tests/commentDialogViewTests.js (Diff revision 3)

Show all issues

As above, the var should go at the top of this function (with a blank line after it).

reviewboard/static/rb/js/views/tests/commentDialogViewTests.js (Diff revision 3)
Show all issues
```
Same here.
```

Change Summary:

Wrapped description to 80 characters.

Description:

~		On the diff page the list of comments doesn't differenciate between top-level comments and replies to those comments. Because reviewboard doesn't support replying to a reply of a comment, the "Reply" link on those reply comments would not work properly. This fix solves that problem by changing those reply links to reply to their parent comment.
	~	On the diff page the list of comments doesn't differenciate between
	+	top-level comments and replies to those comments. Because reviewboard
	+	doesn't support replying to a reply of a comment, the "Reply" link on
	+	those reply comments would not work properly. This fix solves that problem
	+	by changing those reply links to reply to their parent comment.

Testing Done:

~		Only manual testing was done.
~		Create a comment on a line on the diff viewer page for a review request.
~		Reply to that comment.
~		Go back to the diff viewer page and click the reply link of that reply comment.
~		Expected result is that it should go to the review page, scroll to, and open the reply editor for the parent comment of that reply.
	~	Only manual testing was done.
	~
	~	-Create a comment on a line on the diff viewer page for a review request.
	~
	~	-Reply to that comment.
	+
	+	-Go back to the diff viewer page and click the reply link of
	+	that reply comment.
	+
	+	-Expected result is that it should go to the review page, scroll to,
	+	and open the reply editor for the parent comment of that reply.

Change Summary:

Updates in response to review.

Diff:

Revision 4 (+34 -6)

Show changes

	reviewboard/reviews/context.py
	reviewboard/static/rb/js/views/commentDialogView.js
	reviewboard/static/rb/js/views/tests/commentDialogViewTests.js

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/tests/commentDialogViewTests.js
    reviewboard/static/rb/js/views/commentDialogView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/tests/commentDialogViewTests.js
    reviewboard/static/rb/js/views/commentDialogView.js

Testing Done:

		Only manual testing was done.

		-Create a comment on a line on the diff viewer page for a review request.

		-Reply to that comment.

		-Go back to the diff viewer page and click the reply link of
		that reply comment.

		-Expected result is that it should go to the review page, scroll to,
		and open the reply editor for the parent comment of that reply.
	+
	+	-Jasmine js test passed
	+	-Reviewboard unit test passed

Testing Done:

~		Only manual testing was done.
~
	~	Manual Test:
	~	-Create a comment on a line on the diff viewer page for a review request.
-		-Create a comment on a line on the diff viewer page for a review request.

		-Reply to that comment.

		-Go back to the diff viewer page and click the reply link of
		that reply comment.

		-Expected result is that it should go to the review page, scroll to,
		and open the reply editor for the parent comment of that reply.

~		-Jasmine js test passed
~		-Reviewboard unit test passed
	~	-Jasmine js test suite passed
	~	-Reviewboard unit test suite passed
	+	-2 new tests added to the js test suit

reviewboard/static/rb/js/views/tests/commentDialogViewTests.js (Diff revision 4)

Show all issues

This should be formatted as:

expect(/*...*/)
    .toContain(/*...*/)

reviewboard/static/rb/js/views/tests/commentDialogViewTests.js (Diff revision 4)

Show all issues

Single quotes.
Also, this should be formatted as:

expect(/*...*/)
    .toContain(/*...*/);

reviewboard/static/rb/js/views/tests/commentDialogViewTests.js (Diff revision 4)
Show all issues
```
Same here.
```
reviewboard/static/rb/js/views/tests/commentDialogViewTests.js (Diff revision 4)
Show all issues
```
We don't usually put trailing comments.
```

reviewboard/static/rb/js/views/tests/commentDialogViewTests.js (Diff revision 4)

Show all issues

Since you're doing $commentsList.find(selector) multiple times, it would be better to do:

var $replies;

editor.set(/*...*/);
dlg.open();

$replies = $commentList.find(selector);

expect($replies[0].href).toContain(/*...*/);
expect($replies[1].href).toContain(/*...*/);


We should also probably do:

expect($replies.length).toEqual(2);

reviewboard/static/rb/js/views/tests/commentDialogViewTests.js (Diff revision 4)
Show all issues
```
We use single quotes.
```
reviewboard/static/rb/js/views/tests/commentDialogViewTests.js (Diff revision 4)
Show all issues
```
Same here.
```
reviewboard/static/rb/js/views/tests/commentDialogViewTests.js (Diff revision 4)
Show all issues
```
Can also probably build the target string ahead of time, too.
```

Change Summary:

Updated unit tests to match code style.

Diff:

Revision 5 (+38 -6)

Show changes

	reviewboard/reviews/context.py
	reviewboard/static/rb/js/views/commentDialogView.js
	reviewboard/static/rb/js/views/tests/commentDialogViewTests.js

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/tests/commentDialogViewTests.js
    reviewboard/static/rb/js/views/commentDialogView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/context.py

Ignored Files:
    reviewboard/static/rb/js/views/tests/commentDialogViewTests.js
    reviewboard/static/rb/js/views/commentDialogView.js

Groups:

+	students

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (2224eff)