Skip to content

Commit

Permalink
Fix how URL with query string is formed.
Browse files Browse the repository at this point in the history 
dict() applied to QueryDict produces a dictionary whose values are
string representations of lists, like "[u'44']". This is because
QueryDict associates each key with a list of values even if there is
only one value. QueryDict's dict() method translates each list of
values into a string repr of *one* value from the original list. This
means data is thrown away for any keys with multiple values in the
QueryDict. This is a problem for certain form controls.
  • Loading branch information
@mjzffr
mjzffr committed Jun 4, 2014
1 parent 4ebe9ed commit 3efb06c
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion oneanddone/tasks/helpers.py
View file
Expand Up @@ -4,6 +4,6 @@

@register.function
def page_url(request, page):
params = dict(request.GET)
params = request.GET.dict()
params['page'] = page
return urlparams('', **params)

This comment has been minimized.

Sign in to view

Copy link
@mjzffr

mjzffr Jun 4, 2014

Author Owner

(Copying comments from my broken pull-request to continue discussion here.)

FWIW, see https://github.com/mozilla/funfactory/blob/master/funfactory/helpers.py#L48 and note that "New query params will be appended to exising parameters, except duplicate names, which will be replaced." Since urlparams('', QueryDict('foo=bar&baz=5&foo=ok')) yields a URL that ends with "foo=[u'bar',+u'ok']&baz=[u'5']", I think urlparams is expecting a dictionary where each key has a single value (not a list of values).

"@Osmose can we get your thoughts on this, as you wrote the original code? Is the above change OK/needed, or do we need to keep the code as is and find another way of dealing with the fact that we are having issues with the URL that is generated when doing pagination and filtering at the same time?"

This comment has been minimized.

Sign in to view

Copy link
@Osmose

Osmose Jun 4, 2014

request.GET.dict() seems fine to me.

If you're really concerned about multiple values for a key (which, if we're not currently using, seems fine to ignore until it becomes an issue), then I think the real fix is a patch to funfactory that fixes urlparams to work with QueryDicts, or to just not use urlparams and find another way of building the query string.

This comment has been minimized.

Sign in to view

Copy link
@bobsilverberg

bobsilverberg Jun 4, 2014

Thanks @Osmose. I agree that if we don't currently need support for duplicate url params then we don't really need to worry about it for now. Hopefully if we do in the future my tired old brain will remember this conversation. :)

@mjzffr I'll do another review of the current PR.

1 comment on commit 3efb06c

@mjzffr
Copy link
Owner Author

@mjzffr mjzffr commented on 3efb06c Jun 4, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(To address your previous question, @bobsilverberg.)

There are two problems in master right now:

  1. generating URLs with brackets around GET parameter values (which this commit fixes);
  2. losing some information about GET parameters that are associated with multiple values.

I propose to open a different bug for the second problem as it is significant and unrelated to the profile page.

I would also add the following test to address the issue:

    def test_repeats(self):
        """
        GET parameters with multiple values should have all their
        values preserved
        """
        request = Mock(GET=QueryDict('foo=bar&baz=5&foo=ok'))
        url = urlparse.urlsplit(page_url(request, 4))
        args = urlparse.parse_qs(url.query)
        eq_(args, {'foo': ['bar', 'ok'], 'baz': ['5'], 'page': ['4']})

Please sign in to comment.