Summary

Fix up special casing for closing review requests.

Review Request #9150 — Created Aug. 25, 2017 and submitted Sept. 26, 2017, 10:34 a.m.

Information

Owner

david

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Commit

f68e3d2...

Reviewers

Groups

reviewboard

People

Description

In the case of the close description field, the way that the review request
editor would call reviewRequest.close instead of setting a field on the model
itself was pretty hacky. The implementation of setDraftField would look at
whether it was setting the closeDescription and then special-case its
behavior.

Now that we have JavaScript-side implementations for the built-in fields, we
don't need this anymore. I've moved the special casing into the
CloseDescriptionFieldView.

This refactor will also make things easier down the road for new field types.

Testing Done


Tested setting the close description with both regular and Markdown text.
Ran js-tests.

Issues

Description	From	Last Updated
"javascript" -> "JavaScript" in the description.	chipx86	Sept. 5, 2017, 10:24 a.m.
Do our unit tests fully cover this change? Is there any way our coverage could be improved based on changes …	chipx86	Sept. 5, 2017, 4:22 p.m.
There's an extra blank line here.	chipx86	Sept. 8, 2017, 10:38 a.m.
Here too.	chipx86	Sept. 8, 2017, 10:38 a.m.

flake8 passed.

JSHint passed.

Change Summary:

Even better implementation.

Commit:

70a27d6ecf6136e90fed790b388e1d5e5c67e66a

bed3affb15df2fcace6f2853855d09d5cfa523c1

Diff:

Revision 2 (+57 -42)

Show changes

	reviewboard/static/rb/js/models/reviewRequestEditorModel.es6.js
	reviewboard/static/rb/js/views/reviewRequestFieldViews.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

"javascript" -> "JavaScript" in the description.

The issue has been resolved. Show all issues

Do our unit tests fully cover this change? Is there any way our coverage could be improved based on changes made here?

david Sept. 5, 2017, 10:24 a.m.

Yeah, this behavior is covered by tests. I'm not sure there's anything I can do to substantively improve coverage.

david Sept. 5, 2017, 10:25 a.m.

Err, scratch that, I can do some test work. Hold tight.

Description:

		In the case of the close description field, the way that the review request
		editor would call `reviewRequest.close` instead of setting a field on the model
		itself was pretty hacky. The implementation of `setDraftField` would look at
		whether it was setting the `closeDescription` and then special-case its
		behavior.

~		Now that we have javascript-side implementations for the built-in fields, we
	~	Now that we have JavaScript-side implementations for the built-in fields, we
		don't need this anymore. I've moved the special casing into the
		`CloseDescriptionFieldView`.

		This refactor will also make things easier down the road for new field types.

Commit:

bed3affb15df2fcace6f2853855d09d5cfa523c1

f68e3d25d4ac37a6d6dddac5c340efd6b3caba45

Diff:

Revision 3 (+163 -90)

Show changes

	reviewboard/static/rb/js/models/reviewRequestEditorModel.es6.js
	reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
	reviewboard/static/rb/js/views/reviewRequestFieldViews.es6.js
	reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

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

The issue has been resolved. Show all issues

There's an extra blank line here.

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

The issue has been resolved. Show all issues

Here too.

Status:: Completed
Change Summary:: Pushed to release-3.0.x (6867d6c)