Fix stripping of XML-like syntax in comments

Review Request #5796 — Created May 12, 2014 and submitted

Information

Review Board
release-1.7.x
aa44134...

Reviewers

If a comment consisted of only XML-like syntax (such as "<test>"), it would be treated as if the comment was completely empty and the comment would be canceled. The issue here is that reviewCommentFormIsEmpty would strip out tags before testing whether the comment was empty or not. Removing the call to stripTags() fixes the bug.

I manually tried to recreate the bug with different ways.
By submitting '<sim realtime="true>' or 'something <sim realtime="true>' in the comments you can verify that the bug is no longer exists.

SA
SA
david
  1. The change looks OK, but I'd like to see a bit more explanation in your description.

    Your testing done is also formatted strangely and has a typo. Since we use the text in the review request for the commit message, please fix this up.

  2. 
      
SA
SA
david
  1. I'd like to make some suggestions about the review request text. We use this text as the commit message when pushing changes, so it's important that it be well-formed and easy to read. Getting in this practice will be good for the future, too :)

    The summary should be less about the implementation and more about the higher-level functionality. Something like "Fix stripping of XML-like syntax in comments"

    The description is currently OK at explaining what the bug was (on a technical level) and why your change fixes it, but it's not very easy to understand. I would do something more like this:

    "If a comment consisted of only XML-like syntax (such as "<test>"), it would be treated as if the comment was completely empty and the comment would be canceled. The issue here is that reviewCommentFormIsEmpty would strip out tags before testing whether the comment was empty or not. Removing the call to stripTags() fixes the bug."

    Please also be careful--your linked bug number is incorrect.

  2. 
      
SA
david
  1. This looks good. I'm going to hold off on pushing because we're frozen for release.

  2. 
      
SA
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-1.7.x (8e6adee)
Loading...