Summary

Fix rich text defaults when loading newly-saved comments.

Review Request #7606 — Created Aug. 23, 2015 and submitted Aug. 23, 2015, 11:13 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-2.0.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

When saving a comment in the diff viewer and turning off rich text
(while having the "Always use Markdown by default" option checked), the
comment state would appear incorrect. The Markdown checkbox would be
unchecked, and the text would style the intended plain text as Markdown.
Both were clearly wrong.

The comment dialog and underlying editor model now follow the behavior
of the other editors in the UI. If the user's default is to use rich
text, then any loaded comment will be set for rich text editing (escaped
if needed and with the Markdown checkbox checked). If the default is to
use the comment's original mode, then the editor will fully reflect that
mode.

Testing Done

Unit tests pass.

Tested the following conditions manually, both with and without "Always
use Markdown for text fields" set, and in both diffs and file attachments:

Created a plain text comment, and verified its state when immediately
loading after save.
Created a rich text comment, and verified its state when immediately
loading after save.
Viewed exising plain text comment, and verified its state.
Viewed existing rich text comment, and verified its state.
Viewed tooltips for both plain text and rich text comments.

This was also tested on both Review Board 2.0 and 2.5.

Issues

Description	From	Last Updated
What about the case where defaultRichText = True and comment.attributes.richText = False ? Wouldn't we want this still to be …	brennie	Aug. 23, 2015, 10:11 p.m.
This should mention that the provided data will override these.	brennie	Aug. 23, 2015, 10:20 p.m.

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/tests/commentDialogViewTests.js
    reviewboard/static/rb/js/models/tests/commentEditorModelTests.js
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/draftResourceModelMixin.js
    reviewboard/static/rb/js/resources/models/baseResourceModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/js/views/commentDialogView.js
    reviewboard/static/rb/js/models/commentEditorModel.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/tests/commentDialogViewTests.js
    reviewboard/static/rb/js/models/tests/commentEditorModelTests.js
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/draftResourceModelMixin.js
    reviewboard/static/rb/js/resources/models/baseResourceModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/js/views/commentDialogView.js
    reviewboard/static/rb/js/models/commentEditorModel.js

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

The issue has been dropped. Show all issues

What about the case where defaultRichText = True and comment.attributes.richText = False ? Wouldn't we want this still to be false (as we'd prefer whatever the comment has set).

chipx86 Aug. 23, 2015, 10:13 p.m.

That variable isn't just the default for new comments. It's the "always use Markdown for text fields" setting, which means that no matter what the comment is, it'll be edited in rich text. The text value will reflect this. If defaultRichText == true, then the text will be Markdown-escaped.

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

The issue has been resolved. Show all issues

This should mention that the provided data will override these.

chipx86 Aug. 23, 2015, 10:13 p.m.

These aren't model attributes, so that's not necessarily the case. Unless I misunderstood.

brennie Aug. 23, 2015, 10:16 p.m.

I'm referring to the behaviour on lines 143--145. If someone wants to pass a query param that has the same name as a model attribute, it will be overriden by the model attribute.

chipx86 Aug. 23, 2015, 10:19 p.m.
```
Ohh I see, right. I'll update.
```

Change Summary:

Updated the comment for extraQueryArgs mentioning that the values can be overridden.

Diff:

Revision 2 (+267 -41)

Show changes

	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/resources/models/baseCommentModel.js
	reviewboard/static/rb/js/resources/models/baseResourceModel.js
	reviewboard/static/rb/js/resources/models/draftResourceModelMixin.js
	reviewboard/static/rb/js/views/commentDialogView.js
	reviewboard/static/rb/js/views/tests/commentDialogViewTests.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/tests/commentDialogViewTests.js
    reviewboard/static/rb/js/models/tests/commentEditorModelTests.js
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/draftResourceModelMixin.js
    reviewboard/static/rb/js/resources/models/baseResourceModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/js/views/commentDialogView.js
    reviewboard/static/rb/js/models/commentEditorModel.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/tests/commentDialogViewTests.js
    reviewboard/static/rb/js/models/tests/commentEditorModelTests.js
    reviewboard/static/rb/js/models/abstractCommentBlockModel.js
    reviewboard/static/rb/js/resources/models/draftResourceModelMixin.js
    reviewboard/static/rb/js/resources/models/baseResourceModel.js
    reviewboard/static/rb/js/resources/models/baseCommentModel.js
    reviewboard/static/rb/js/views/commentDialogView.js
    reviewboard/static/rb/js/models/commentEditorModel.js

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.0.x (7a69d32)