Summary:

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

Review Request #5810 — Created May 12, 2014 and submitted May 20, 2014, 1:54 p.m.

Information
Owner:	volodymyr
Repository:	Review Board
Branch:
Bugs:	3033
Depends On:
Reviewers
Groups:	reviewboard, students
People:

Description

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.

Testing Done

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.

Issues

Description	From	Last Updated
For docstrings, the first line should a short summary on the same line as the initial """, and should be …	david	May 14, 2014, 2:09 p.m.
I think we should test this using the actual format operation instead of checking whether the string contains '%s', because …	david	May 14, 2014, 2:09 p.m.
This string is a bit dense, and not really in character with the rest of our validation errors. How about …	david	May 14, 2014, 2:09 p.m.
The first part of a docstring should fit on one line. In this cane you could just move "Note that …	david	May 14, 2014, 2:10 p.m.
I'm not sure what this means.	david	May 14, 2014, 2:10 p.m.
typo ('th')	david	May 14, 2014, 2:11 p.m.
"have error present" read clumsy.	david	May 14, 2014, 2:11 p.m.
I think you should assign the field to a variable. You also don't need to prefix strings with 'u' because …	david	May 14, 2014, 2:11 p.m.
The text should be shortened a tad and this should be all one line. """Check that hosting service bug URLs …	david	May 14, 2014, 4:22 p.m.
This is no longer necessary (the format operation will handle escaped % sequences itself).	david	May 14, 2014, 4:22 p.m.
Blank line between these two.	david	May 14, 2014, 4:22 p.m.
Period at the end of the sentence.	david	May 14, 2014, 4:22 p.m.

Change Summary:

Added screenshot

Added Files:

Change Summary:

Full implementation of the approach where user will get notified about using %s for the base URL. Error message might br modified to mention that they can escape '%s' in order to still have it. Either way, this solves the problem with broken state and inability to make further changes.

Summary:

-	[WIP] Fix error with repo settings caused by hosting services with extra %s
+	Fix error with repo settings caused by hosting services with extra %s

Description:

~		This is partial solution - I need guidance on what needs to be done. Read the bug description and thread to see what's going on. Basically having %s in a non-custom URL breaks things.
	~	Read the bug description and thread to see what's going on. Basically having %s in a non-custom URL breaks things.
		Options as I see them are:

		1) Silently change %s to %%s (excaped) in non-custom fields. Once the URL is constrcuted, there will be only one %s.
		2) Let the user know of the error and tell him/her that %s can be changed to %%s. I was told that this is better that fixing stuff silently, and agree with it.
		3) Allow having multiple %s, each of which will eventually be replaced to ID.

~		If we go for number 2, there are lots of fields that can lead to the same problem - pretty much every field of every bug tracker (URL, username, etc.). The way I changing them now might not be the best - I will essentially have to add this validator to every field. Any suggestions on how this can be improved?
	~	If we go for number 2, there are lots of fields that can lead to the same problem - pretty much every field of every bug tracker (URL, username, etc.).

~
	~	I went for option number 2, though there are 2 lines that should be added to every bug hoster class in questions.

~		Again, this isn't a full solution, the problem is still there. I just need advice on what to do so that I don't go the wrong way.
	~	Please provide feedback.

Testing Done:

~		This isn't the full solution, and will be modified.
	~	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.

Diff:

Revision 2 (+53 -3)

Show changes

	reviewboard/admin/validation.py
	reviewboard/hostingsvcs/bugzilla.py
	reviewboard/hostingsvcs/redmine.py
	reviewboard/hostingsvcs/trac.py
	reviewboard/scmtools/forms.py

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

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.

volodymyr May 14, 2014, 2:12 p.m.

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 :)

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(...)

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.

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.

reviewboard/scmtools/forms.py (Diff revision 2)
Show all issues
```
I'm not sure what this means.
```
1. VO
  
  volodymyr May 14, 2014, 2:12 p.m.
  I expanded the explanation.
reviewboard/scmtools/forms.py (Diff revision 2)
Show all issues
```
typo ('th')
```
reviewboard/scmtools/forms.py (Diff revision 2)
Show all issues
```
"have error present" read clumsy.
```

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)

Change Summary:

Adressed review comments, retested after making changes.

Diff:

Revision 3 (+55 -3)

Show changes

	reviewboard/admin/validation.py
	reviewboard/hostingsvcs/bugzilla.py
	reviewboard/hostingsvcs/redmine.py
	reviewboard/hostingsvcs/trac.py
	reviewboard/scmtools/forms.py

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."""

reviewboard/admin/validation.py (Diff revision 3)

Show all issues

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

reviewboard/scmtools/forms.py (Diff revision 3)
Show all issues
```
Blank line between these two.
```
reviewboard/scmtools/forms.py (Diff revision 3)
Show all issues
```
Period at the end of the sentence.
```

Change Summary:

Adressed review feedback, retested.

Diff:

Revision 4 (+51 -3)

Show changes

	reviewboard/admin/validation.py
	reviewboard/hostingsvcs/bugzilla.py
	reviewboard/hostingsvcs/redmine.py
	reviewboard/hostingsvcs/trac.py
	reviewboard/scmtools/forms.py

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).

VO

volodymyr May 14, 2014, 6 p.m.
```
Updated.
```

Description:

~		Read the bug description and thread to see what's going on. Basically having %s in a non-custom URL breaks things.
	~	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.
-		Options as I see them are:

~		1) Silently change %s to %%s (excaped) in non-custom fields. Once the URL is constrcuted, there will be only one %s.
	~	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.
-		2) Let the user know of the error and tell him/her that %s can be changed to %%s. I was told that this is better that fixing stuff silently, and agree with it.
-		3) Allow having multiple %s, each of which will eventually be replaced to ID.
-
-		If we go for number 2, there are lots of fields that can lead to the same problem - pretty much every field of every bug tracker (URL, username, etc.).
-
-		I went for option number 2, though there are 2 lines that should be added to every bug hoster class in questions.
-
-		Please provide feedback.

Removed Files:

Ship it!

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

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (e0a8fef)

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

Change Summary:

Added Files:

Change Summary:

Summary:

Description:

Testing Done:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Description:

Removed Files:

Status: Closed (submitted)

Change Summary: