- Change Summary:
-
Added screenshot
- Added Files:
Fix error with repo settings caused by hosting services with extra %s
Review Request #5810 — Created May 12, 2014 and submitted
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 … |
 david
|
|
|
I think we should test this using the actual format operation instead of checking whether the string contains '%s', because … |
david
|
|
|
This string is a bit dense, and not really in character with the rest of our validation errors. How about … |
david
|
|
|
The first part of a docstring should fit on one line. In this cane you could just move "Note that … |
david
|
|
|
I'm not sure what this means. |
david
|
|
|
typo ('th') |
david
|
|
|
"have error present" read clumsy. |
david
|
|
|
I think you should assign the field to a variable. You also don't need to prefix strings with 'u' because … |
david
|
|
|
The text should be shortened a tad and this should be all one line. """Check that hosting service bug URLs … |
david
|
|
|
This is no longer necessary (the format operation will handle escaped % sequences itself). |
david
|
|
|
Blank line between these two. |
david
|
|
|
Period at the end of the sentence. |
david
|
VO
- 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 %sFix 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.
-
-
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.
-
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(...) -
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_urlline is also misaligned. -
The first part of a docstring should fit on one line. In this cane you could just move "Note that ..." into another paragraph.
-
-
-
-
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)
-
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
- 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: