Fix openIssue checkbox set to default when reloading the page with explicitly set value
Review Request #6867 — Created Feb. 1, 2015 and submitted
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.
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.
| 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
|
|
|
We might want to check for the presence of issue_opened and issue_status. We seem to do the same for text, … |
mike_conley
|
|
|
Can you remove the trailing whitespace on this line? (Any trailing whitespace is always highlighted in red in the diff … |
 anselina
|
|
|
If w're modifying this function signature, maybe instead we want to just pass a dictionary of attributes to set. We … |
 chipx86
|
|
|
Blank line between these. |
chipx86
|
|
|
Blank line between these. |
chipx86
|
|
|
In JavaScript, you actually want to use ===. That is a strict comparison, whereas == will try casting to equivalent … |
chipx86
|
|
|
var statements should go at the top of hte function. |
chipx86
|
|
|
This should happen at the top of the function (before the // We load in ... comment). We define all … |
 david
|
MI
Description: |
|
|---|
-
-
reviewboard/static/rb/js/models/abstractCommentBlockModel.js (Diff revision 1) 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) 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.
-
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/
MI
Change Summary:
- Break up the calling long line
- Add if condition statement
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+30 -5) |
-
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) Can you remove the trailing whitespace on this line? (Any trailing whitespace is always highlighted in red in the diff viewer.)
MI
Change Summary:
Remove extra blanks.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+28 -5) |
-
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 3) 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) Blank line between these.
-
reviewboard/static/rb/js/models/abstractCommentBlockModel.js (Diff revision 3) Blank line between these.
-
reviewboard/static/rb/js/models/commentEditorModel.js (Diff revision 3) 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) varstatements should go at the top of hte function.
MI
Change Summary:
- Change func ensureDraftComment header, pass dict instead
- '===' means equals to
- Move var define to the top
Bugs: |
|
||||
|---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 4 (+29 -9) |
-
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) 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.
MI
Change Summary:
Move defination place.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 5 (+30 -10) |
-
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
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