Summary

Fix issues related to text type metadata for custom fields.

Review Request #8263 — Created June 30, 2016 and submitted Aug. 22, 2016, 2:53 p.m.

Information

Owner

gmyers

Repository

Review Board

Branch

release-2.5.x

Bugs

3963, 4110

Depends On

Commit

f4273b9...

Reviewers

Groups

reviewboard

People

Description

There were a couple of problems, on both the front and back ends, that
prevented text types from being set and handled correctly for custom
fields.

The primary issue was that the <filename>_text_type metadata was not
being stored into the extra_data dictionary on the review request.
Investigation revealed that this item was being set correctly on the
review request draft, but was not being propagated upon publishing. The
root cause of this lies in ReviewRequestDraft.copy_fields_to_request
where changed fields are identified and their value's are copied
from draft to request. Nominally, fields only contain values and this
copying process is fine, but fields derived from BaseTextAreaField
have both a value and an associated text type metadata element. This
issue is resolved by adding a new propagate_data method to the
BaseReviewRequestField base class, which is now called in
copy_fields_to_request. This method assumes the responsibility for
copying value's from draft to review request. In BaseTextAreaField the
method is extended to also copy text type metadata.

Additionally there were a couple of problems on the front end which
prevented the Enable Markdown checkbox from always being set correctly
for custom fields. The getDraftField method in ReviewRequestEditor
has logic for handing the useExtraData case and was always getting
options.fieldID from extraData, while ignoring the fieldName
parameter passed into the function. This had the effect of always
returning the value associated with a custom field, regardless of what
data element the caller may have actually been asking for. Specifically,
this prevented the retrieval of the rich text attribute associated with
the field. Additionally in ReviewRequestEditorView.render the name of
the rich text attribute was being incorrectly set to
<fieldname>_rich_text rather than <fieldname>RichText. This was
problematic because the later naming syntax is assumed in other places
for accessing the rich text attribute. Finally, in
ReviewRequestEditorView.registerField the naming convention of the
text type field for the special case field name of 'text' was
incorrectly using text_text_type rather than text_type. All of these
problems have been resolved.

Testing Done


Added Python unit tests which failed before the back end fixes. 

Successfully executed all unit tests. 
Added Javascript unit tests which failed before the front end fixes. 

Successfully executed all unit tests. 
Lots of manual testing with Note to Reviewers extension. Observed 

that beanbag_notefield_notes_text_type key-value pair is now correctly 

serialized to the database in extra_data, markdown text is now 

correctly rendered, and markdown checkbox state is sensible.

Issues

Description	From	Last Updated
Would you mind wrapping your description & testing done at 72 characters?	brennie	July 17, 2016, 10:38 a.m.
Col: 38 E222 multiple spaces after operator	reviewbot	June 30, 2016, 6:40 p.m.
"Propagate"	brennie	July 2, 2016, 5:59 a.m.
:py:fn:`load_data`	brennie	July 2, 2016, 5:59 a.m.
:py:fn:`save_data`	brennie	July 2, 2016, 5:59 a.m.
"This function"	brennie	July 2, 2016, 5:59 a.m.
:py:attr:`review_request_details`	brennie	July 2, 2016, 7:03 a.m.
Put this on a single line. Its OK if it goes over 79 characters.	brennie	July 2, 2016, 6:57 a.m.
Same here.	brennie	July 2, 2016, 6:58 a.m.
"Any custom fields IDs" (or similar).	brennie	July 2, 2016, 7:04 a.m.
"Any custom fields IDs" (or similar).	brennie	July 2, 2016, 7:04 a.m.
This is confusing at first glance. Let's explicitly say: ... use the ``review_request_details`` parameter instead of the :py:attr:`review_request_details` attribute on …	chipx86	July 17, 2016, 3:05 p.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	July 2, 2016, 7:16 a.m.
While annoyingly long, this should be reviewboard.reviews.models.base_review_request_details. (Other places may have this wrong.)	chipx86	July 17, 2016, 3:06 p.m.
This can be one line.	chipx86	July 17, 2016, 3:12 p.m.
Instead of doing the caching here, you can use Django's @cached_property and just return the value. @cached_property will take care …	chipx86	July 17, 2016, 5:43 p.m.
"Return"	chipx86	July 17, 2016, 3:07 p.m.
"Propagate"	chipx86	July 17, 2016, 3:07 p.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	July 2, 2016, 7:16 a.m.
Same as above about the type.	chipx86	July 17, 2016, 5:56 p.m.
No period. This shoud also list the class type/function being tested.	chipx86	July 18, 2016, 5:47 p.m.
Rather than doing all this and then removing the fields, how about just creating a new fieldset instance to work …	chipx86	July 18, 2016, 1:53 a.m.
Only one blank line.	chipx86	July 17, 2016, 5:55 p.m.
Let's fetch extraData just once.	chipx86	July 17, 2016, 3:26 p.m.
For consistency, let's wrap useExtraData: true to the next line, like the call below.	chipx86	July 17, 2016, 3:22 p.m.
Can you put this in parens? That way the === comparison is visually separated more from the assignment. Code above …	chipx86	July 17, 2016, 3:28 p.m.
Same as above with parens.	chipx86	July 17, 2016, 5:55 p.m.
"ordinary" can mean a lot of things. I think it's fine to say custom fields, as it still applies. The …	chipx86	July 17, 2016, 6:02 p.m.
Col: 80 E501 line too long (92 > 79 characters)	reviewbot	July 18, 2016, 6:03 p.m.
Col: 80 E501 line too long (92 > 79 characters)	reviewbot	July 18, 2016, 6:03 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/tests.py
    reviewboard/reviews/fields.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py

Ignored Files:
    reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
    reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/tests.py
    reviewboard/reviews/fields.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py

Ignored Files:
    reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
    reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js

reviewboard/reviews/fields.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 38
 E222 multiple spaces after operator
```

Commit:

-	2b184344b284dd9986063a0f403c84a169b77852
+	f5574e294881eea186d7be6ac60ac34b962c0664

Diff:

Revision 2 (+265 -38)

Show changes

	reviewboard/reviews/fields.py
	reviewboard/reviews/tests.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/static/rb/js/models/reviewRequestEditorModel.js
	reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
	reviewboard/static/rb/js/views/reviewRequestEditorView.js
	reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
	reviewboard/webapi/tests/test_review_request_draft.py

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/tests.py
    reviewboard/reviews/fields.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py

Ignored Files:
    reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
    reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/tests.py
    reviewboard/reviews/fields.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py

Ignored Files:
    reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
    reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js

reviewboard/reviews/fields.py (Diff revision 2)
The issue has been resolved. Show all issues
```
"Propagate"
```
reviewboard/reviews/fields.py (Diff revision 2)
The issue has been resolved. Show all issues
```
:py:fn:`load_data`
```
reviewboard/reviews/fields.py (Diff revision 2)
The issue has been resolved. Show all issues
```
:py:fn:`save_data`
```
reviewboard/reviews/fields.py (Diff revision 2)
The issue has been resolved. Show all issues
```
"This function"
```

reviewboard/reviews/fields.py (Diff revision 2)

The issue has been resolved. Show all issues

:py:attr:`review_request_details`

reviewboard/reviews/fields.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Put this on a single line. Its OK if it goes over 79 characters.
```
reviewboard/reviews/fields.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 2)
The issue has been resolved. Show all issues
```
"Any custom fields IDs" (or similar).
```
reviewboard/reviews/models/review_request_draft.py (Diff revision 2)
The issue has been resolved. Show all issues
```
"Any custom fields IDs" (or similar).
```

Change Summary:

Documentation cleanup.

Commit:

-	f5574e294881eea186d7be6ac60ac34b962c0664
+	8d4e5b3520f7a249c0c461ac34d3c175e7a60097

Diff:

Revision 3 (+263 -38)

Show changes

	reviewboard/reviews/fields.py
	reviewboard/reviews/tests.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/static/rb/js/models/reviewRequestEditorModel.js
	reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
	reviewboard/static/rb/js/views/reviewRequestEditorView.js
	reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
	reviewboard/webapi/tests/test_review_request_draft.py

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/tests.py
    reviewboard/reviews/fields.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py

Ignored Files:
    reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
    reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/tests.py
    reviewboard/reviews/fields.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py

Ignored Files:
    reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
    reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js

reviewboard/reviews/fields.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```
reviewboard/reviews/fields.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```

Ship it!

The issue has been resolved. Show all issues

Would you mind wrapping your description & testing done at 72 characters?

Great change! Thank for the hard work on this :)

I have some things I'd like to see tweaked before this goes in. All small things, and many are doc-related.

reviewboard/reviews/fields.py (Diff revision 3)

The issue has been resolved. Show all issues

This is confusing at first glance. Let's explicitly say:

... use the ``review_request_details`` parameter instead of the :py:attr:`review_request_details` attribute on the field.

reviewboard/reviews/fields.py (Diff revision 3)

The issue has been resolved. Show all issues

While annoyingly long, this should be reviewboard.reviews.models.base_review_request_details. (Other places may have this wrong.)

reviewboard/reviews/fields.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This can be one line.
```

reviewboard/reviews/fields.py (Diff revision 3)

The issue has been resolved. Show all issues

Instead of doing the caching here, you can use Django's @cached_property and just return the value. @cached_property will take care of all the local caching logic.

reviewboard/reviews/fields.py (Diff revision 3)
The issue has been resolved. Show all issues
```
"Return"
```
reviewboard/reviews/fields.py (Diff revision 3)
The issue has been resolved. Show all issues
```
"Propagate"
```
reviewboard/reviews/fields.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Same as above about the type.
```

reviewboard/reviews/tests.py (Diff revision 3)

The issue has been resolved. Show all issues

No period.
This shoud also list the class type/function being tested.

gmyers July 17, 2016, 6:40 p.m.

To clarify, you mean that detail should be listed within the one sentence docstring?  So something like "Testing ReviewRequestDraft.publish with custom fields".  I looked around and didn't notice any other standard special section for this for tests, but wanted to make sure I'm not overlooking something.

chipx86 July 18, 2016, 1:53 a.m.

Some of these tests are a bit older and didn't have much of a formal way of structuring the test docstring, but newer ones follow a form like that. The "Testing ReviewRequestDraft.publish with custom fields" would be the right way to go. It's also okay for these docstrings to wrap, if you need to provide more information. Maybe "Testing ReviewRequestDraft.publish with custom fields propagating from draft to review request"

reviewboard/reviews/tests.py (Diff revision 3)

The issue has been dropped. Show all issues

Rather than doing all this and then removing the fields, how about just creating a new fieldset instance to work on?

gmyers July 17, 2016, 6:40 p.m.

I'm not sure I follow.  Are you suggesting I register a custom fieldset with the review request and drop the try/finally?  FWIW, the add and remove approach is what I found for existing tests that are working with custom fields.

chipx86 July 18, 2016, 1:53 a.m.

Yeah. What you have works. It just might be a little easier to create a new fieldset instead of using an existing one, and then you'd only have to remove the old fieldset at the end. That said, you can ignore this suggestion. What you have works fine.

reviewboard/reviews/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Only one blank line.
```
reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Let's fetch extraData just once.
```
reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js (Diff revision 3)
The issue has been resolved. Show all issues
```
For consistency, let's wrap useExtraData: true to the next line, like the call below.
```

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

The issue has been resolved. Show all issues

Can you put this in parens? That way the === comparison is visually separated more from the assignment.

Code above should have had that as well.

reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Same as above with parens.
```

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

The issue has been resolved. Show all issues

"ordinary" can mean a lot of things. I think it's fine to say custom fields, as it still applies. The The "rich-text field" part elaborates for an additional condition.

Could maybe move the rich-text ones into here as well, since it'd then be a subset.

Change Summary:

Wrap Description and Testing Done at 72 characters.  No change to content other than formatting.

Description:

~		There were a couple of problems, on both the front and back ends, that prevented text types from being set and handled correctly for custom fields.
	~	There were a couple of problems, on both the front and back ends, that
	+	prevented text types from being set and handled correctly for custom
	+	fields.

~		The primary issue was that the `<filename>_text_type` metadata was not being stored into the `extra_data` dictionary on the review request. Investigation revealed that this item was being set correctly on the review request draft, but was not being propagated upon publishing. The root cause of this lies in `ReviewRequestDraft.copy_fields_to_request` where changed fields are identified and their value's are copied from draft to request. Nominally, fields only contain values and this copying process is fine, but fields derived from `BaseTextAreaField` have both a value and an associated text type metadata element. This issue is resolved by adding a new `propagate_data` method to the `BaseReviewRequestField` base class, which is now called in `copy_fields_to_request`. This method assumes the responsibility for copying value's from draft to review request. In `BaseTextAreaField` the method is extended to also copy text type metadata.
	~	The primary issue was that the `<filename>_text_type` metadata was not
	+	being stored into the `extra_data` dictionary on the review request.
	+	Investigation revealed that this item was being set correctly on the
	+	review request draft, but was not being propagated upon publishing. The
	+	root cause of this lies in `ReviewRequestDraft.copy_fields_to_request`
	+	where changed fields are identified and their value's are copied
	+	from draft to request. Nominally, fields only contain values and this
	+	copying process is fine, but fields derived from `BaseTextAreaField`
	+	have both a value and an associated text type metadata element. This
	+	issue is resolved by adding a new `propagate_data` method to the
	+	`BaseReviewRequestField` base class, which is now called in
	+	`copy_fields_to_request`. This method assumes the responsibility for
	+	copying value's from draft to review request. In `BaseTextAreaField` the
	+	method is extended to also copy text type metadata.

~		Additionally there were a couple of problems on the front end which prevented the Enable Markdown checkbox from always being set correctly for custom fields. The `getDraftField` method in `ReviewRequestEditor` has logic for handing the `useExtraData` case and was always getting `options.fieldID` from `extraData`, while ignoring the `fieldName` parameter passed into the function. This had the effect of always returning the value associated with a custom field, regardless of what data element the caller may have actually been asking for. Specifically, this prevented the retrieval of the rich text attribute associated with the field. Additionally in `ReviewRequestEditorView.render` the name of the rich text attribute was being incorrectly set to `<fieldname>_rich_text` rather than `<fieldname>RichText`. This was problematic because the later naming syntax is assumed in other places for accessing the rich text attribute. Finally, in `ReviewRequestEditorView.registerField` the naming convention of the text type field for the special case field name of 'text' was incorrectly using `text_text_type` rather than `text_type`. All of these problems have been resolved.
	~	Additionally there were a couple of problems on the front end which
	+	prevented the Enable Markdown checkbox from always being set correctly
	+	for custom fields. The `getDraftField` method in `ReviewRequestEditor`
	+	has logic for handing the `useExtraData` case and was always getting
	+	`options.fieldID` from `extraData`, while ignoring the `fieldName`
	+	parameter passed into the function. This had the effect of always
	+	returning the value associated with a custom field, regardless of what
	+	data element the caller may have actually been asking for. Specifically,
	+	this prevented the retrieval of the rich text attribute associated with
	+	the field. Additionally in `ReviewRequestEditorView.render` the name of
	+	the rich text attribute was being incorrectly set to
	+	`<fieldname>_rich_text` rather than `<fieldname>RichText`. This was
	+	problematic because the later naming syntax is assumed in other places
	+	for accessing the rich text attribute. Finally, in
	+	`ReviewRequestEditorView.registerField` the naming convention of the
	+	text type field for the special case field name of 'text' was
	+	incorrectly using `text_text_type` rather than `text_type`. All of these
	+	problems have been resolved.

Testing Done:

~		Added Python unit tests which failed before the back end fixes. Successfully executed all unit tests.
~		Added Javascript unit tests which failed before the front end fixes. Successfully executed all unit tests.
~		Lots of manual testing with Note to Reviewers extension. Observed that `beanbag_notefield_notes_text_type` key-value pair is now correctly serialized to the database in `extra_data`, markdown text is now correctly rendered, and markdown checkbox state is sensible.
	~	Added Python unit tests which failed before the back end fixes. Successfully executed all unit tests.
	~	Added Javascript unit tests which failed before the front end fixes. Successfully executed all unit tests.
	~	Lots of manual testing with Note to Reviewers extension. Observed that `beanbag_notefield_notes_text_type` key-value pair is now correctly serialized to the database in `extra_data`, markdown text is now correctly rendered, and markdown checkbox state is sensible.

Change Summary:

Address feedback from Christian.

Commit:

-	8d4e5b3520f7a249c0c461ac34d3c175e7a60097
+	f4273b9d2f38acd1ca56280beee1aca35cd1132a

Diff:

Revision 4 (+265 -39)

Show changes

	reviewboard/reviews/fields.py
	reviewboard/reviews/tests.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/static/rb/js/models/reviewRequestEditorModel.js
	reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
	reviewboard/static/rb/js/views/reviewRequestEditorView.js
	reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
	reviewboard/webapi/tests/test_review_request_draft.py

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/tests.py
    reviewboard/reviews/fields.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py

Ignored Files:
    reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
    reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/tests.py
    reviewboard/reviews/fields.py
    reviewboard/reviews/models/review_request_draft.py
    reviewboard/webapi/tests/test_review_request_draft.py

Ignored Files:
    reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js
    reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js

reviewboard/reviews/fields.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (92 > 79 characters)
```
reviewboard/reviews/fields.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (92 > 79 characters)
```

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (9d0eaa8)