Add API support for the rich_text field on several resources.

Review Request #4801 — Created Oct. 18, 2013 and submitted

Information

Review Board
master

Reviewers

Add API support for the rich_text field on several resources.

rich_text was previously the default for any new objects, and clients
were expected to send text in valid Markdown. This posed a problem for
Review Bot, RBTools, and probably a lot of scripts used in production,
which would suddenly have to know about Markdown.

This change gives clients more control. Now, by default, rich text is an
opt-in thing. It's disabled on new objects by default, but callers can
pass rich_text=true in API calls to indicate that the text fields for
that object should be rich text.

For resources with multiple affected fields, setting rich_text to true
will set the provided text as-is, and escape any text fields not set
during that API call, ensuring their contents won't accidentally be
interpreted as Markdown.

Likewise, if setting rich_text=false, any unspecified text fields will
have any escaping removed, leaving just the text content (meaning that
**This** is a \*test\* will turn into **This** is a *test*).

The web UI always sets rich_text=true in calls. It also ensures that the
text coming in is Markdown, even if it's not stored as Markdown on the
server. That is, if rbt post provides a description for a review
request, then server-side, rich_text will be false, but the web UI will
then escape it when displaying. This ensures that the user will see
Markdown-formatted text and can act appropriately when they go to save
(which will formally set rich_text=true on the server).

Unit tests for the API and UI have been added.

Python and JavaScript unit tests pass.

Tested the following scenarios:

Review requests

  • Posted a change for review, with a description that would be otherwise interpreted as Markdown.
    Saw that it escaped properly for display, but was internally stored with the original content
    and rich_text=false.
  • Edited the Description and Testing Done fields individually when a review request had
    rich_text=false. The other fields properly escaped, but the new content went through.
    The server had rich_text=true after the save.
  • Did the same test with a change description.

Reviews

  • Tested creating a review and saw that it was in Markdown by default.
  • Took an old review with rich_text=false and edited it. The original text was properly escaped.
    Saving it set rich_text=true.

Comments

  • Created a comment for a new review, and saw that it was Markdown by default. Repeated with a new
    comment on an existing review. Published these and saw my Markdown was rendered.
  • Took an old comment with rich_text=false and edited it. The original text was properly escaped.
    Saving it set rich_text=true.

Replies

  • Made a new reply to a comment and added Markdown. It properly rendered, before and after a reload.
    Saw in the database that rich_text=true.
  • Edited an older, unpublished reply with rich_text=false. The text was escaped in the input box.
    Saved and it saw that rich_text=true, and saw it render properly.
  • Repeated the tests with a reply to a review's body text.
Description From Last Updated

We repeat (basically) this block in many places. Can it be refactored out?

daviddavid
david
  1. 
      
  2. reviewboard/webapi/resources/review_request_draft.py (Diff revision 1)
     
     
     
     
     
     
    Show all issues

    We repeat (basically) this block in many places. Can it be refactored out?

    1. I'm adding a markdown_set_field_escaped function, which does the if/else part of this.

      I started on one that takes a list of fields and loops, but decided that it was too weird
      and specific an API to have in markdown_utils.py, and there wasn't a great place to put it
      otherwise.

      Short of having each of these do some extra statements to filter a list before passing it in,
      or an awkward API that takes a list of possible fields and a list of ones to ignore from that
      first list, I didn't have a great solution. Plus, there's not always a 1:1 mapping of actual
      fields and kwargs.

      markdown_set_field_escaped, at least, shortens these a little without being too specialized.

  3. 
      
chipx86
david
  1. Ship It!

  2. 
      
chipx86
Review request changed
Status:
Completed