Summary

Fix openIssue checkbox set to default when reloading the page with explicitly set value

Review Request #6867 — Created Feb. 1, 2015 and submitted Feb. 5, 2015, 11:41 a.m.

Information

Owner

mingyi

Repository

Review Board

Branch

release-2.0.x

Bugs

3580

Depends On

Commit

29720ca...

Reviewers

Groups

reviewboard, students

People

Description

In the "View Diff" page, click one line in diff content to open one comment dialog
and create one comment WITHOUT issue opened. And reload that view diff page, click
the flag of that comment we just created, the "IssueOpened" checkbox is checked in
the dialog. Close it and reopen it again, the checked status is OK.

Since the status setting is missed, we set the issueOpened status and issued status
when creating the draftcomment in the comment block.

Testing Done

Added a unit test, which passes along with all other tests.

Tested with a commentEditor which contains a comment whose 'loaded' attribute and
'issueOpened' attribute are false and the default setting is true. Previously, the
editor's 'openIssue' attribute is true. After, the attribute will set to user's setting
with the patch.

Test for some cases, all passed.

Issues

Description	From	Last Updated
Let's break up this long line - maybe after comment.text,, like: this.ensureDraftComment(comment.comment_id, comment.text, comment.issue_opened, comment.issue_status);	mike_conley	Feb. 1, 2015, 11:55 p.m.
We might want to check for the presence of issue_opened and issue_status. We seem to do the same for text, …	mike_conley	Feb. 1, 2015, 11:55 p.m.
Can you remove the trailing whitespace on this line? (Any trailing whitespace is always highlighted in red in the diff …	anselina	Feb. 3, 2015, 6:15 p.m.
If w're modifying this function signature, maybe instead we want to just pass a dictionary of attributes to set. We …	chipx86	Feb. 5, 2015, 10:38 a.m.
Blank line between these.	chipx86	Feb. 5, 2015, 10:38 a.m.
Blank line between these.	chipx86	Feb. 5, 2015, 10:38 a.m.
In JavaScript, you actually want to use ===. That is a strict comparison, whereas == will try casting to equivalent …	chipx86	Feb. 5, 2015, 10:38 a.m.
var statements should go at the top of hte function.	chipx86	Feb. 5, 2015, 10:38 a.m.
This should happen at the top of the function (before the // We load in ... comment). We define all …	david	Feb. 5, 2015, 11:10 a.m.

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/models/commentEditorModel.js
    reviewboard/static/rb/js/models/tests/commentEditorModelTests.js



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/models/commentEditorModel.js
    reviewboard/static/rb/js/models/tests/commentEditorModelTests.js

Description:: Fix Bug #3580
Add openIssue part Javascript Test for bug #3580

Bug Description

In the "View Diff" page, click one line in diff content to open one comment dialog

Create one comment WITHOUT issue opened

Reload that view diff page

Click the flag of that comment we just created, you could see the "IssueOpened" checkbox is checked in the dialog

Close the dialog and reopen it again

This time the checked status is OK

~
Fixtion Detail
~
Fixation Detail

Modify the time setting the openIssue to default when the attribute openIssue is null not loaded is false

Modify the function ensureDraftComment header

reviewboard/static/rb/js/models/abstractCommentBlockModel.js (Diff revision 1)

The issue has been resolved. Show all issues

Let's break up this long line - maybe after comment.text,, like:

this.ensureDraftComment(comment.comment_id, comment.text,
                        comment.issue_opened, comment.issue_status);

reviewboard/static/rb/js/models/abstractCommentBlockModel.js (Diff revision 1)

The issue has been resolved. Show all issues

We might want to check for the presence of issue_opened and issue_status. We seem to do the same for text, so might as well follow the pattern.

mingyi Feb. 1, 2015, 11:44 p.m.

I think issueOpended may not need if statement since it can be null. Whether set it or not, it has no influence.

Groups:: students

Hey Mingyi, just a reminder to address the issues that I opened. Also, please mark them "Fixed" as you address them. Thanks!

Make sure to read through the guide on writing summaries and descriptions :) - https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/

Change Summary:


Break up the calling long line
Add if condition statement

Summary:

Bug Fix #3580: IssueOpen Problem

Fix openIssue checkbox set to default when reloading the page with explicitly set value

Description:

~		Fix Bug #3580
~		Add openIssue part Javascript Test for bug #3580
	~	In the "View Diff" page, click one line in diff content to open one comment dialog
	~	and create one comment WITHOUT issue opened. And reload that view diff page, click
	+	the flag of that comment we just created, the "IssueOpened" checkbox is checked in
	+	the dialog. Close it and reopen it again, the checked status is OK.

~		Bug Description
~
	~	Since the status setting is missed, we set the issueOpened status and issued status
	~	when creating the draftcomment in the comment block.
-		In the "View Diff" page, click one line in diff content to open one comment dialog
-		Create one comment WITHOUT issue opened
-		Reload that view diff page
-		Click the flag of that comment we just created, you could see the "IssueOpened" checkbox is checked in the dialog
-		Close the dialog and reopen it again
-		This time the checked status is OK
-
-		Fixation Detail
-
-		Modify the time setting the openIssue to default when the attribute openIssue is null not loaded is false
-		Modify the function ensureDraftComment header

Testing Done:

~		Add an js test in the openIssue part
	~	Added a unit test, which passes along with all other tests.

~		All js-test passed
	~	Tested with a commentEditor which contains a comment whose 'loaded' attribute and
	+	'issueOpened' attribute are false and the default setting is true. Previously, the
	+	editor's 'openIssue' attribute is true. After, the attribute will set to user's setting
	+	with the patch.
	+
	+	Test for some cases, all passed.

Commit:

3066f4225233e3da8cf42c6cdbc394e0c9aafdb6

345b5b60df34b3ddc4c92f7d3b9f54472875846e

Diff:

Revision 2 (+30 -5)

Show changes

	reviewboard/static/rb/js/models/abstractCommentBlockModel.js
	reviewboard/static/rb/js/models/commentEditorModel.js
	reviewboard/static/rb/js/models/tests/commentEditorModelTests.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/models/commentEditorModel.js
    reviewboard/static/rb/js/models/tests/commentEditorModelTests.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/models/commentEditorModel.js
    reviewboard/static/rb/js/models/tests/commentEditorModelTests.js

reviewboard/static/rb/js/models/abstractCommentBlockModel.js (Diff revision 2)

The issue has been resolved. Show all issues

Can you remove the trailing whitespace on this line? (Any trailing whitespace is always highlighted in red in the diff viewer.)

Change Summary:

Remove extra blanks.

Commit:

345b5b60df34b3ddc4c92f7d3b9f54472875846e

cdd36d292d07d6bfbb5ae13ec4a7732d157ad7ca

Diff:

Revision 3 (+28 -5)

Show changes

	reviewboard/static/rb/js/models/abstractCommentBlockModel.js
	reviewboard/static/rb/js/models/commentEditorModel.js
	reviewboard/static/rb/js/models/tests/commentEditorModelTests.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/models/commentEditorModel.js
    reviewboard/static/rb/js/models/tests/commentEditorModelTests.js



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/models/commentEditorModel.js
    reviewboard/static/rb/js/models/tests/commentEditorModelTests.js

Can you put the bug number into the "Bugs" field on the right-hand side?

reviewboard/static/rb/js/models/abstractCommentBlockModel.js (Diff revision 3)

The issue has been resolved. Show all issues

If w're modifying this function signature, maybe instead we want to just pass a dictionary of attributes to set. We can then just call comment.set(attrs); later.

reviewboard/static/rb/js/models/abstractCommentBlockModel.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Blank line between these.
```
reviewboard/static/rb/js/models/abstractCommentBlockModel.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Blank line between these.
```

reviewboard/static/rb/js/models/commentEditorModel.js (Diff revision 3)

The issue has been resolved. Show all issues

In JavaScript, you actually want to use ===. That is a strict comparison, whereas == will try casting to equivalent types.

reviewboard/static/rb/js/models/tests/commentEditorModelTests.js (Diff revision 3)
The issue has been resolved. Show all issues
```
var statements should go at the top of hte function.
```

Change Summary:


Change func ensureDraftComment header, pass dict instead
'===' means equals to
Move var define to the top

Bugs:

3580

Commit:

cdd36d292d07d6bfbb5ae13ec4a7732d157ad7ca

6530a66069d905944c1b0c749cb840d0ebdd98bb

Diff:

Revision 4 (+29 -9)

Show changes

	reviewboard/static/rb/js/models/abstractCommentBlockModel.js
	reviewboard/static/rb/js/models/commentEditorModel.js
	reviewboard/static/rb/js/models/tests/commentEditorModelTests.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/models/commentEditorModel.js
    reviewboard/static/rb/js/models/tests/commentEditorModelTests.js



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/models/commentEditorModel.js
    reviewboard/static/rb/js/models/tests/commentEditorModelTests.js

reviewboard/static/rb/js/models/abstractCommentBlockModel.js (Diff revision 4)

The issue has been resolved. Show all issues

This should happen at the top of the function (before the // We load in ... comment). We define all variables at the top because javascript will hoist their definitions up there anyway, so it makes it less error-prone to read that way.

mingyi Feb. 5, 2015, 11:12 a.m.

Is there any docuement about JavaScript code conventions in rb?

david Feb. 5, 2015, 11:18 a.m.

Not explicitly, but if you install the "jshint" tool, you can run it on your code and it will catch most of these.

There's some work to get jshint running on Review Bot but it's not quite there yet.

MI

mingyi Feb. 5, 2015, 11:20 a.m.
```
Got it. Thanks :)
```

Change Summary:

Move defination place.

Commit:

6530a66069d905944c1b0c749cb840d0ebdd98bb

29720ca74d3e9299dbcca63d36d108cd3c4621eb

Diff:

Revision 5 (+30 -10)

Show changes

	reviewboard/static/rb/js/models/abstractCommentBlockModel.js
	reviewboard/static/rb/js/models/commentEditorModel.js
	reviewboard/static/rb/js/models/tests/commentEditorModelTests.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/models/commentEditorModel.js
    reviewboard/static/rb/js/models/tests/commentEditorModelTests.js



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/models/commentEditorModel.js
    reviewboard/static/rb/js/models/tests/commentEditorModelTests.js

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.0.x (8a4bc09)

Fix openIssue checkbox set to default when reloading the page with explicitly set value

Information

Reviewers

Bug Description

Fixtion Detail

Fixation Detail

Bug Description

Fixation Detail