3898: "Enable Markdown" checkbox isn't reset when RBTools uploads a new plaintext change description

buck*****@gmai***** (Google Code) (Is this you? Claim this profile.)
chipx86
chipx86
June 26, 2015
What version are you running?
ReviewBoard 2.0.13
RBTools 0.7.4 (also reproduces with RBTools 0.5.2)

What's the URL of the page containing the problem?
https://reviewboard.example.com/api/review-requests/xxxxxx/draft/

What steps will reproduce the problem?
1. In your ReviewBoard account settings, uncheck "Always use Markdown for text fields" and save the settings.
2. Post (but do not publish) a new review using command-line RBTools, including a description.
3. Check the "Enable Markdown" box on the review description. Press "OK".
4. Publish the review.
5. Post a new revision of the review using command-line RBTools, including an updated description.
6. View the review description. Note that even though the description was updated by RBTools, the "Enable Markdown" checkbox is still checked.

What is the expected output? What do you see instead?
The "Enable Markdown" checkbox should have been reset to unchecked to match the user's profile settings and type of the uploaded text.

What operating system are you using? What browser?
Linux. Firefox.

Please provide any additional information below.
"Always use Markdown for text fields" seems to determine the initial state of the "Enable Markdown" checkbox on text fields but doesn't do anything beyond that. In this case, I was attempting to disable it to avoid ReviewBoard's behavior of escaping Markdown found in uploaded change descriptions. It might be better to instead:
"Always use Markdown for text fields": This option should behave as it already does (i.e. set the initial state of the "Enable Markdown" checkbox on text fields).
Add an additional "Convert non-Markdown review descriptions to Markdown" (enable Markdown escaping) sub-option below "Always use Markdown for text fields".
Any time a change description is uploaded by RBTools to the server, do the following:
1) If the change description is Markdown (i.e. `--markdown` was passed to `rbt post`), check the "Enable Markdown" checkbox and update the description with the verbatim text received.
2) If the change description is plain text and the "Convert non-Markdown review descriptions to Markdown" option is enabled, perform Markdown escaping on the change description, check the "Enable Markdown" checkbox and update the description with the escaped text.
3) If the change description is plain text and the "Convert non-Markdown review descriptions to Markdown" option is not enabled, uncheck the "Enable Markdown" checkbox and update the description with the verbatim text received.

This allows authoring Markdown descriptions without having to explicitly specify `--markdown` to `rbt post`, and also allows users to disable Markdown conversion/escaping entirely. It also ensures that the "Enable Markdown" option on the Description field always matches the actual type of the text uploaded by RBTools (or at least what type RBTools thinks it is).

If you don't want to add a new option to toggle conversion, then I'd still like to see the state of the "Enable Markdown" checkbox clobbered by the text type of newly uploaded descriptions. It makes no sense to preserve the existing type when the text is being replaced entirely by `rbt post`. Since RBTools already tells the server the type, it shouldn't be using the user's settings to infer what they want instead of setting the checkbox to the correct value.
chipx86
#1 chipx86
  • +PendingReview
  • +Milestone-Release2.0.x
    +Project-ReviewBoard
    +Component-Reviews
  • +chipx86
chipx86
#2 chipx86
Fixed on release-2.0.x (d2de929). This will make it into 2.0.18.
  • -PendingReview
    +Fixed