Fix issues related to text type metadata for custom fields.
Review Request #8263 — Created June 30, 2016 and submitted
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_typemetadata was not
being stored into theextra_datadictionary 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 inReviewRequestDraft.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 fromBaseTextAreaField
have both a value and an associated text type metadata element. This
issue is resolved by adding a newpropagate_datamethod to the
BaseReviewRequestFieldbase class, which is now called in
copy_fields_to_request. This method assumes the responsibility for
copying value's from draft to review request. InBaseTextAreaFieldthe
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. ThegetDraftFieldmethod inReviewRequestEditor
has logic for handing theuseExtraDatacase and was always getting
options.fieldIDfromextraData, while ignoring thefieldName
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 inReviewRequestEditorView.renderthe name of
the rich text attribute was being incorrectly set to
<fieldname>_rich_textrather 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.registerFieldthe naming convention of the
text type field for the special case field name of 'text' was
incorrectly usingtext_text_typerather thantext_type. All of these
problems have been resolved.
- 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
thatbeanbag_notefield_notes_text_typekey-value pair is now correctly
serialized to the database inextra_data, markdown text is now
correctly rendered, and markdown checkbox state is sensible.
| Description | From | Last Updated |
|---|---|---|
|
Would you mind wrapping your description & testing done at 72 characters? |
brennie
|
|
|
Col: 38 E222 multiple spaces after operator |
reviewbot
|
|
|
"Propagate" |
brennie
|
|
|
:py:fn:`load_data` |
brennie
|
|
|
:py:fn:`save_data` |
brennie
|
|
|
"This function" |
brennie
|
|
|
:py:attr:`review_request_details` |
brennie
|
|
|
Put this on a single line. Its OK if it goes over 79 characters. |
brennie
|
|
|
Same here. |
brennie
|
|
|
"Any custom fields IDs" (or similar). |
brennie
|
|
|
"Any custom fields IDs" (or similar). |
brennie
|
|
|
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
|
|
|
Col: 80 E501 line too long (89 > 79 characters) |
reviewbot
|
|
|
While annoyingly long, this should be reviewboard.reviews.models.base_review_request_details. (Other places may have this wrong.) |
chipx86
|
|
|
This can be one line. |
chipx86
|
|
|
Instead of doing the caching here, you can use Django's @cached_property and just return the value. @cached_property will take care … |
chipx86
|
|
|
"Return" |
chipx86
|
|
|
"Propagate" |
chipx86
|
|
|
Col: 80 E501 line too long (89 > 79 characters) |
reviewbot
|
|
|
Same as above about the type. |
chipx86
|
|
|
No period. This shoud also list the class type/function being tested. |
chipx86
|
|
|
Rather than doing all this and then removing the fields, how about just creating a new fieldset instance to work … |
chipx86
|
|
|
Only one blank line. |
chipx86
|
|
|
Let's fetch extraData just once. |
chipx86
|
|
|
For consistency, let's wrap useExtraData: true to the next line, like the call below. |
chipx86
|
|
|
Can you put this in parens? That way the === comparison is visually separated more from the assignment. Code above … |
chipx86
|
|
|
Same as above with parens. |
chipx86
|
|
|
"ordinary" can mean a lot of things. I think it's fine to say custom fields, as it still applies. The … |
chipx86
|
|
|
Col: 80 E501 line too long (92 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (92 > 79 characters) |
reviewbot
|
- Commit:
-
2b184344b284dd9986063a0f403c84a169b77852f5574e294881eea186d7be6ac60ac34b962c0664
- Diff:
-
Revision 2 (+265 -38)
-
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
- Change Summary:
-
Documentation cleanup.
- Commit:
-
f5574e294881eea186d7be6ac60ac34b962c06648d4e5b3520f7a249c0c461ac34d3c175e7a60097
- Diff:
-
Revision 3 (+263 -38)
-
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 -
-
-
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.
-
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. -
While annoyingly long, this should be
reviewboard.reviews.models.base_review_request_details. (Other places may have this wrong.) -
-
Instead of doing the caching here, you can use Django's
@cached_propertyand just return the value.@cached_propertywill take care of all the local caching logic. -
-
-
-
-
Rather than doing all this and then removing the fields, how about just creating a new fieldset instance to work on?
-
-
-
-
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.
-
-
"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_typemetadata was not being stored into theextra_datadictionary 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 inReviewRequestDraft.copy_fields_to_requestwhere 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 fromBaseTextAreaFieldhave both a value and an associated text type metadata element. This issue is resolved by adding a newpropagate_datamethod to theBaseReviewRequestFieldbase class, which is now called incopy_fields_to_request. This method assumes the responsibility for copying value's from draft to review request. InBaseTextAreaFieldthe method is extended to also copy text type metadata.~ The primary issue was that the
<filename>_text_typemetadata was not+ being stored into the extra_datadictionary 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_datamethod to the+ BaseReviewRequestFieldbase 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 BaseTextAreaFieldthe+ 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
getDraftFieldmethod inReviewRequestEditorhas logic for handing theuseExtraDatacase and was always gettingoptions.fieldIDfromextraData, while ignoring thefieldNameparameter 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 inReviewRequestEditorView.renderthe name of the rich text attribute was being incorrectly set to<fieldname>_rich_textrather than<fieldname>RichText. This was problematic because the later naming syntax is assumed in other places for accessing the rich text attribute. Finally, inReviewRequestEditorView.registerFieldthe naming convention of the text type field for the special case field name of 'text' was incorrectly usingtext_text_typerather thantext_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 getDraftFieldmethod inReviewRequestEditor+ has logic for handing the useExtraDatacase and was always getting+ options.fieldIDfromextraData, while ignoring thefieldName+ 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.renderthe name of+ the rich text attribute was being incorrectly set to + <fieldname>_rich_textrather 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.registerFieldthe naming convention of the+ text type field for the special case field name of 'text' was + incorrectly using text_text_typerather thantext_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_typekey-value pair is now correctly serialized to the database inextra_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
thatbeanbag_notefield_notes_text_typekey-value pair is now correctly
serialized to the database inextra_data, markdown text is now
correctly rendered, and markdown checkbox state is sensible.
- Change Summary:
-
Address feedback from Christian.
- Commit:
-
8d4e5b3520f7a249c0c461ac34d3c175e7a60097f4273b9d2f38acd1ca56280beee1aca35cd1132a
- Diff:
-
Revision 4 (+265 -39)
-
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 -
-
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