Add additional validation to bug tracker URLs

Review Request #3378 — Created Sept. 29, 2012 and submitted

Karl-L
Review Board
1996
reviewboard
When specifying a custom bug tracker URL, the URL is passed through additional validation to ensure that there is a '%s' string formatting element.
Tested on local development server against custom urls and a new GitHub repository's issue tracker. Added unit test for validator. 
Description From Last Updated

We tend to do a lot of stuff alphabetically, so these should be imported in this order: validate_bug_tracker, validate_review_groups, validate_users.

mike_conleymike_conley

Would love some input on the message.

KA Karl-L

As before, we tend to do things alphabetically - so django.forms stuff should be imported before django.http stuff.

mike_conleymike_conley

There should be a docstring here say what this test is doing.

mike_conleymike_conley

Input on the message here would be appreciated too.

KA Karl-L

reviewboard.site isn't the right place for validate_bug_tracker. Can you move it to reviewboard/admin/validation.py (which doesn't exist yet) and move the ...

daviddavid
KA
  1. 
      
  2. reviewboard/scmtools/forms.py (Diff revision 1)
     
     
    Would love some input on the message.
  3. reviewboard/site/validation.py (Diff revision 1)
     
     
    Input on the message here would be appreciated too.
  4. 
      
mike_conley
  1. Hey Karl,
    
    This looks really good! Just a few suggestions - see below.
    
    -Mike
  2. reviewboard/scmtools/forms.py (Diff revision 1)
     
     
     
    We tend to do a lot of stuff alphabetically, so these should be imported in this order:
    
    validate_bug_tracker, validate_review_groups, validate_users.
  3. reviewboard/scmtools/forms.py (Diff revision 1)
     
     
    How about "The path should resemble:
    
    http://www.example.com/issues?id=%s
    
    where %s will be the bug number.
    1. I like this better. Thanks.
  4. reviewboard/site/tests.py (Diff revision 1)
     
     
    As before, we tend to do things alphabetically - so django.forms stuff should be imported before django.http stuff.
  5. reviewboard/site/tests.py (Diff revision 1)
     
     
    There should be a docstring here say what this test is doing.
  6. reviewboard/site/validation.py (Diff revision 1)
     
     
    This seems fine.
  7. 
      
KA
david
  1. Ship It!
  2. 
      
david
  1. Oops, nevermind on than ship it. I have a couple comments.
  2. 
      
david
  1. 
      
  2. reviewboard/scmtools/forms.py (Diff revision 2)
     
     
     
    reviewboard.site isn't the right place for validate_bug_tracker. Can you move it to reviewboard/admin/validation.py (which doesn't exist yet) and move the test into reviewboard/admin/tests.py ?
    1. Moved. Thanks for catching this.
  3. 
      
KA
david
  1. Ship It!
  2. 
      
KA
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-1.6.x (b64dd03). Thanks!
Loading...