Fix issues related to text type metadata for custom fields.

Review Request #8263 — Created June 30, 2016 and submitted

Information

Review Board
release-2.5.x
f4273b9...

Reviewers

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.

  1. Added Python unit tests which failed before the back end fixes.
    Successfully executed all unit tests.
  2. Added Javascript unit tests which failed before the front end fixes.
    Successfully executed all unit tests.
  3. 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.
Description From Last Updated

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

brenniebrennie

Col: 38 E222 multiple spaces after operator

reviewbotreviewbot

"Propagate"

brenniebrennie

:py:fn:`load_data`

brenniebrennie

:py:fn:`save_data`

brenniebrennie

"This function"

brenniebrennie

:py:attr:`review_request_details`

brenniebrennie

Put this on a single line. Its OK if it goes over 79 characters.

brenniebrennie

Same here.

brenniebrennie

"Any custom fields IDs" (or similar).

brenniebrennie

"Any custom fields IDs" (or similar).

brenniebrennie

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 …

chipx86chipx86

Col: 80 E501 line too long (89 > 79 characters)

reviewbotreviewbot

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

chipx86chipx86

This can be one line.

chipx86chipx86

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

chipx86chipx86

"Return"

chipx86chipx86

"Propagate"

chipx86chipx86

Col: 80 E501 line too long (89 > 79 characters)

reviewbotreviewbot

Same as above about the type.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Only one blank line.

chipx86chipx86

Let's fetch extraData just once.

chipx86chipx86

For consistency, let's wrap useExtraData: true to the next line, like the call below.

chipx86chipx86

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

chipx86chipx86

Same as above with parens.

chipx86chipx86

"ordinary" can mean a lot of things. I think it's fine to say custom fields, as it still applies. The …

chipx86chipx86

Col: 80 E501 line too long (92 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (92 > 79 characters)

reviewbotreviewbot
reviewbot
  1. 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
    
    
  2. reviewboard/reviews/fields.py (Diff revision 1)
     
     
    Show all issues
    Col: 38
     E222 multiple spaces after operator
    
  3. 
      
gmyers
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/reviews/fields.py (Diff revision 2)
     
     
    Show all issues

    "Propagate"

  3. reviewboard/reviews/fields.py (Diff revision 2)
     
     
    Show all issues

    :py:fn:`load_data`
    
  4. reviewboard/reviews/fields.py (Diff revision 2)
     
     
    Show all issues

    :py:fn:`save_data`
    
  5. reviewboard/reviews/fields.py (Diff revision 2)
     
     
    Show all issues

    "This function"

  6. reviewboard/reviews/fields.py (Diff revision 2)
     
     
    Show all issues

    :py:attr:`review_request_details`
    
  7. reviewboard/reviews/fields.py (Diff revision 2)
     
     
     
    Show all issues

    Put this on a single line. Its OK if it goes over 79 characters.

  8. reviewboard/reviews/fields.py (Diff revision 2)
     
     
     
    Show all issues

    Same here.

  9. Show all issues

    "Any custom fields IDs" (or similar).

  10. Show all issues

    "Any custom fields IDs" (or similar).

  11. 
      
gmyers
reviewbot
  1. 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
    
    
  2. reviewboard/reviews/fields.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (89 > 79 characters)
    
  3. reviewboard/reviews/fields.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (89 > 79 characters)
    
  4. 
      
brennie
  1. 
      
  2. Show all issues

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

  3. 
      
chipx86
  1. 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.

  2. reviewboard/reviews/fields.py (Diff revision 3)
     
     
     
    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.
    
  3. reviewboard/reviews/fields.py (Diff revision 3)
     
     
    Show all issues

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

  4. reviewboard/reviews/fields.py (Diff revision 3)
     
     
     
    Show all issues

    This can be one line.

  5. reviewboard/reviews/fields.py (Diff revision 3)
     
     
     
     
     
    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.

  6. reviewboard/reviews/fields.py (Diff revision 3)
     
     
    Show all issues

    "Return"

  7. reviewboard/reviews/fields.py (Diff revision 3)
     
     
    Show all issues

    "Propagate"

  8. reviewboard/reviews/fields.py (Diff revision 3)
     
     
    Show all issues

    Same as above about the type.

  9. reviewboard/reviews/tests.py (Diff revision 3)
     
     
    Show all issues

    No period.

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

    1. 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.
    2. 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"

  10. reviewboard/reviews/tests.py (Diff revision 3)
     
     
     
     
     
    Show all issues

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

    1. 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.
    2. 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.

  11. reviewboard/reviews/tests.py (Diff revision 3)
     
     
     
     
     
    Show all issues

    Only one blank line.

  12. Show all issues

    Let's fetch extraData just once.

  13. Show all issues

    For consistency, let's wrap useExtraData: true to the next line, like the call below.

  14. 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.

  15. Show all issues

    Same as above with parens.

  16. 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.

  17. 
      
gmyers
gmyers
reviewbot
  1. 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
    
    
  2. reviewboard/reviews/fields.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (92 > 79 characters)
    
  3. reviewboard/reviews/fields.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (92 > 79 characters)
    
  4. 
      
chipx86
  1. Ship It!
  2. 
      
gmyers
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.5.x (9d0eaa8)