-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes #2953 - Url resolved with special characters #3725
Conversation
Any updates on this PR ? |
Thanks for the patch! This could use a test confirming that it does indeed work for projects with |
Thanks for the review @ericholscher! I will add a test for this! |
@ericholscher Can you please guide about where these tests should be added? |
I linked the tests file above: https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/rtd_tests/tests/test_views.py#L115-L124 You could also test the regex itself, outside of a view, and just confirm that it's matching what we expect. |
@ericholscher I have added the tests! |
|
||
def test_project_details_resolve(self): | ||
self.project.users.add(self.user) | ||
response = self.client.get('/projects/my-mainproject/') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should test for the user profile /profiles/{user}/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have fixed that as well as added some new users for testing!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution!
This solves #3890
@@ -7,7 +7,7 @@ | |||
|
|||
|
|||
urlpatterns = [ | |||
url(r'^(?P<username>[\w@.-]+)/$', | |||
url(r'^(?P<username>[+\w@.-]+)/$', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the correct regex here is:
[\w@\.\+-]+
which follows the same as the django validation field from the username
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forget about this comment, @stsewd is right here: #3890 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@humitos no need to -1 yourself haha
|
||
def test_profile_details_page(self): | ||
|
||
response = self.client.get('/profiles/foo+bar/') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of creating these users and accessing the page itself, isn't it better to use the resolver and check that it matches?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @humitos, Sorry for the delay on this. I was busy with class tests in my college.
Can you please elaborate on this as I am a bit confused about how to proceed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant the reverse
function from django:
In [1]: from django.core.urlresolvers import reverse
In [3]: reverse('profiles_profile_detail', kwargs={'username': 'foo+bar'})
Out[3]: '/profiles/foobar/'
and check the URL returned with assertEqual
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation @humitos, I have made the changes!
Merging, Thanks! |
This PR Fixes #2953. I have added the
+
character to the username set. Rest all charactersletters
,numbers
,-
and_
were already doing fine with the URL.