Summary

Add additional validation to bug tracker URLs

Review Request #3378 — Created Sept. 29, 2012 and submitted Sept. 30, 2012, 7:45 a.m.

Information

Owner

Karl-L

Repository

Review Board

Branch

Bugs

1996

Depends On

Reviewers

Groups

reviewboard

People

Description

When specifying a custom bug tracker URL, the URL is passed through additional validation to ensure that there is a '%s' string formatting element.

Testing Done

Tested on local development server against custom urls and a new GitHub repository's issue tracker. Added unit test for validator.

Issues

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_conley	Sept. 30, 2012, 6:42 a.m.
Would love some input on the message.	KA Karl-L	Sept. 30, 2012, 6:42 a.m.
As before, we tend to do things alphabetically - so django.forms stuff should be imported before django.http stuff.	mike_conley	Sept. 30, 2012, 6:42 a.m.
There should be a docstring here say what this test is doing.	mike_conley	Sept. 30, 2012, 6:42 a.m.
Input on the message here would be appreciated too.	KA Karl-L	Sept. 30, 2012, 6:42 a.m.
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 …	david	Sept. 30, 2012, 7:07 a.m.

reviewboard/scmtools/forms.py (Diff revision 1)
Show all issues
```
Would love some input on the message.
```
reviewboard/site/validation.py (Diff revision 1)
Show all issues
```
Input on the message here would be appreciated too.
```

Hey Karl,

This looks really good! Just a few suggestions - see below.

-Mike

reviewboard/scmtools/forms.py (Diff revision 1)

Show all issues

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.

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.

KA

Karl-L Sept. 30, 2012, 6:45 a.m.
```
I like this better. Thanks.
```

reviewboard/site/tests.py (Diff revision 1)

Show all issues

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

reviewboard/site/tests.py (Diff revision 1)

Show all issues

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

reviewboard/site/validation.py (Diff revision 1)
```
This seems fine.
```

Change Summary:

Fixed a few import ordering issues (made alphabetical), updated some copy and fixed test so that it can fail (instead of just error).

Diff:

Revision 2 (+54 -2)

Show changes

	reviewboard/scmtools/forms.py
	reviewboard/site/tests.py
	reviewboard/site/validation.py

Ship it!

```
Ship It!
```

Oops, nevermind on than ship it. I have a couple comments.

reviewboard/scmtools/forms.py (Diff revision 2)

Show all issues

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 ?

KA

Karl-L Sept. 30, 2012, 7:08 a.m.
```
Moved. Thanks for catching this.
```

Change Summary:

Move validator and test to correct app -- in reviewboard.admin instead of reviewboard.site (and fix paths).

Diff:

Revision 3 (+54 -1)

Show changes

	reviewboard/admin/tests.py
	reviewboard/admin/validation.py
	reviewboard/scmtools/forms.py

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

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