Fix API ?include-text-types= not serializing custom text field data.

Review Request #8218 — Created June 4, 2016 and submitted

Information

Review Board
release-2.5.x
8bb57e5...

Reviewers

In the API when including multiple text types via ?include-text-types=,
custom text fields stored in extra_data are supposed to be returned
in an extra_data dictionary within the respective <type>_text_fields.
However, these custom text fields were not being serialized at all.

Within the serialize_object() function an empty dictionary,
all_text_types_extra_data, is established to hold any custom text data
and map it with the requisite extra include types. This is passed to
_serialize_text_info() which is responsible for populating the dictionary.
However, there is a requirement in this lower level function that the
dictionary already has keys corresponding to the requisite types. Since the
dictionary is empty, and thus does not satisfy this condition, no population
occurs.

The solution is to re-initialize all_text_types_extra_data to contain the
requisite types as keys upon encountering the first custom text field that
supports markdown. It is critical to perform this step after this initial
encounter, rather than at original initialization time, so as not to end up
with an empty extra_data dictionary under <type>_text_fields in the case
where no custom text fields actually support markdown.

Original and modified JSON output from a sample GET request
(http://127.0.0.1:8080/api/review-requests/83/?api_format=json&force-text-type=html&include-text-types=raw)
is attached to illustrate the effects of the change. Notice the addition of
the "extra_data" field under "raw_text_fields".

Added new tests to exercise this functionality. Fundamentally these are
concerned with ensuring the extra_data dictionary appears within the respective
<type>_text_fields. But, I also took this opportunity to test some other items
that didn't necessarily seem to be covered by other tests. These include:

  1. Custom fields that both do and don't enable/support markdown.
  2. Use of field name "text" for custom fields to cover special case.
  3. Multiple include-text-types types via CSV list.

Description From Last Updated

Single space between sentences

brenniebrennie

RB 3 is python 2.7 only so you can do: all_text_type_extra_data = { k: {} for k in extra_text_type_fields.keys() } …

brenniebrennie

Missing docstring.

brenniebrennie

This doesn't need to be wrapped in a list.

brenniebrennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/mixins.py
        reviewboard/webapi/tests/test_review_request_draft.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/mixins.py
        reviewboard/webapi/tests/test_review_request_draft.py
    
    
  2. 
      
gmyers
brennie
  1. 
      
    1. Thanks for the review, Barret.  I have a couple quick questions below and then I'll get everything fixed up later.
  2. reviewboard/webapi/mixins.py (Diff revision 1)
     
     
    Show all issues

    Single space between sentences

  3. reviewboard/webapi/mixins.py (Diff revision 1)
     
     
     
     
    Show all issues

    RB 3 is python 2.7 only so you can do:

    all_text_type_extra_data = {
        k: {}
        for k in extra_text_type_fields.keys()
    }
    

    In addition, this should use six.iterkeys(dict) instead of dict.keys().

    1. Wasn't sure if this would go in RB 2.5 so that's why I avoided the dictionary comprehension. Also, there is code on line 75 within this same function that uses the old school dict-from-list approach. For the sake of consistency, is that a reason to stick with the code I originally had? Either way, I'll switch to six.iterkeys(dict).

    2. This was branched off master, so I assumed it was for 3.0. If it is for 2.5.x, can you rebase the patch off of the release-2.5.x branch and set the branch: field above to release-2.5.x ?

      If it is indeed for 2.5.x, yes, you'll have to stick with the old dict-from-items approach. For 3.0+, we're not worried about the inconsistency in things that are introduced in 3.0+

  4. Show all issues

    Missing docstring.

    1. I borrowed liberally from _test_get_with_force_text_type() which immediately precedes this function. It doesn't have a docstring. I don't mind adding one here, but just wondering if there is a rule of thumb I should follow so I get this right in the future?

    2. We've started to get pretty strict with respect to undocumented functions and classes. Once upon a time, we weren't as strict and its bit us a few times already. Going forward, all non-trivial functions should be documented.

  5. 
      
gmyers
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/mixins.py
        reviewboard/webapi/tests/test_review_request_draft.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/mixins.py
        reviewboard/webapi/tests/test_review_request_draft.py
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/webapi/mixins.py (Diff revision 2)
     
     
     
     
    Show all issues

    This doesn't need to be wrapped in a list.

    1. For consistency I also updated another instance within this function that was wrapped in a list.
  3. 
      
gmyers
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/mixins.py
        reviewboard/webapi/tests/test_review_request_draft.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/mixins.py
        reviewboard/webapi/tests/test_review_request_draft.py
    
    
  2. 
      
gmyers
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (90119a8)
Loading...