Fix error with repo settings caused by hosting services with extra %s

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

Information

Review Board

Reviewers

When adding a new repository, having %s in the base URL of a non-custom bug tracker breaks things. That's because we don't have an error checking for that, and once the complete URL gets generated out of it, the problem is detected, but that leaves the form in a broken state, where you cannot save the form anymore.

We decided to address this by letting the user know of the error and tell him/her that % character can be escaped if there is a need. The appropriate error message is presented if this error happens.

Tested locally: * Entering invalid URL for bugzilla, trac or redmine will result in an error that you must fix before submitting. * Other things work as expected.

Description From Last Updated

For docstrings, the first line should a short summary on the same line as the initial """, and should be …

daviddavid

I think we should test this using the actual format operation instead of checking whether the string contains '%s', because …

daviddavid

This string is a bit dense, and not really in character with the rest of our validation errors. How about …

daviddavid

The first part of a docstring should fit on one line. In this cane you could just move "Note that …

daviddavid

I'm not sure what this means.

daviddavid

typo ('th')

daviddavid

"have error present" read clumsy.

daviddavid

I think you should assign the field to a variable. You also don't need to prefix strings with 'u' because …

daviddavid

The text should be shortened a tad and this should be all one line. """Check that hosting service bug URLs …

daviddavid

This is no longer necessary (the format operation will handle escaped % sequences itself).

daviddavid

Blank line between these two.

daviddavid

Period at the end of the sentence.

daviddavid
VO
VO
david
  1. I like approach #2, and this change looks like it's on the right track.

  2. reviewboard/admin/validation.py (Diff revision 2)
     
     
     
     
     
    Show all issues

    For docstrings, the first line should a short summary on the same line as the initial """, and should be < 80 columns total.

    There should then be a blank line, followed by a longer explanation (if appropriate).

    This is because the first line is often pulled out for documentation or help() purposes. In this case, because the """ is followed by a newline, the first line would be blank.

    1. Ok, I'll follow this rule from now on. However, the docstring from the function at the top of this file doesn't follow it :)

  3. reviewboard/admin/validation.py (Diff revision 2)
     
     
     
     
     
     
    Show all issues

    I think we should test this using the actual format operation instead of checking whether the string contains '%s', because there are other format characters which could cause the same issue.

    # Try formatting the URL using an empty tuple to verify that it
    # doesn't contain any format characters.
    try:
        input_url % ()
    except TypeError:
        raise ValidationError(...)
    
  4. reviewboard/admin/validation.py (Diff revision 2)
     
     
     
     
     
     
    Show all issues

    This string is a bit dense, and not really in character with the rest of our validation errors. How about something like this?

    The URL "%s" is not valid because it contains a format character. For bug trackers other than "Custom Bug Tracker", use the base URL of the server. If you need a "%" character, escape it using "%%".

    The input_url line is also misaligned.

  5. reviewboard/scmtools/forms.py (Diff revision 2)
     
     
     
     
    Show all issues

    The first part of a docstring should fit on one line. In this cane you could just move "Note that ..." into another paragraph.

  6. reviewboard/scmtools/forms.py (Diff revision 2)
     
     
     
    Show all issues

    I'm not sure what this means.

    1. I expanded the explanation.

  7. reviewboard/scmtools/forms.py (Diff revision 2)
     
     
    Show all issues

    typo ('th')

  8. reviewboard/scmtools/forms.py (Diff revision 2)
     
     
    Show all issues

    "have error present" read clumsy.

  9. reviewboard/scmtools/forms.py (Diff revision 2)
     
     
     
     
     
     
    Show all issues

    I think you should assign the field to a variable. You also don't need to prefix strings with 'u' because the first line of this file uses the unicode_literals future, which makes every string literal a unicode object.

    field = self.bug_tracker_forms[bug_tracker_type]['default']
    self.bug_tracker_host_error = (
        hasattr(field, 'errors') and
        len(field.errors) > 0)
    
  10. 
      
VO
david
  1. 
      
  2. reviewboard/admin/validation.py (Diff revision 3)
     
     
     
     
    Show all issues

    The text should be shortened a tad and this should be all one line.

    """Check that hosting service bug URLs don't contain %s."""

  3. reviewboard/admin/validation.py (Diff revision 3)
     
     
     
    Show all issues

    This is no longer necessary (the format operation will handle escaped % sequences itself).

  4. reviewboard/scmtools/forms.py (Diff revision 3)
     
     
     
    Show all issues

    Blank line between these two.

  5. reviewboard/scmtools/forms.py (Diff revision 3)
     
     
    Show all issues

    Period at the end of the sentence.

  6. 
      
VO
david
  1. The code looks good now, but I'd like you to rewrite the description to represent the more final state of the change (so it's more appropriate as a commit message).

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

  2. 
      
VO
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (e0a8fef)
Loading...