• 
      

    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

    brennie brennie

    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() } …

    brennie brennie

    Missing docstring.

    brennie brennie

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

    brennie brennie
    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:
    Completed
    Change Summary:
    Pushed to release-2.5.x (90119a8)