Opened 3 days ago

Closed 7 hours ago

Last modified 7 hours ago

#36182 closed Bug (fixed)

querystring templatetag should render empty querystring as "?" not "" when it has removed items from the querydict

Reported by: David Feeley Owned by: Sarah Boyce
Component: Template system Version: 5.1
Severity: Release blocker Keywords: querystring templatetag
Cc: David Feeley, Tom Carrick, Natalia Bidart Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no
Pull Requests:19167 merged

Description

Hello,
Consider 2 anchors that link to a filtered and unfiltered version of the current page via a GET param foo.

<a href="{% querystring foo=None %}">All Records</a>
<a href="{% querystring foo=bar %}">Bar Records Only</a>

If the user is currently on the filtered page https://example.com/?foo=bar and clicks the first link, the querystring template tag, being empty, will render as "", which means href="" which the browser will interpret as reload the current (filtered) page. I believe instead the templatetag should render this case as "?"

https://github.com/dfeeley/django/compare/main..empty-querystring

thanks
Dave

Change History (9)

comment:1 by Sarah Boyce, 3 days ago

Cc: Tom Carrick Natalia Bidart added
Severity: NormalRelease blocker
Summary: querystring templatetag should render empty querystring as "?" not ""querystring templatetag should render empty querystring as "?" not "" when it has removed items from the querydict
Triage Stage: UnreviewedAccepted

Hi David, thank you for the ticket!
I think I agree with a slight caveat that I feel this should only be the case when the query dict has changed

A test and a very rough solution just to illustrate what I mean:

  • TabularUnified django/template/defaulttags.py

    a b def querystring(context, query_dict=None, **kwargs):  
    11951195    if query_dict is None:
    11961196        query_dict = context.request.GET
    11971197    query_dict = query_dict.copy()
     1198    has_removed = False
    11981199    for key, value in kwargs.items():
    11991200        if value is None:
    12001201            if key in query_dict:
    12011202                del query_dict[key]
     1203                has_removed = True
    12021204        elif isinstance(value, Iterable) and not isinstance(value, str):
    12031205            query_dict.setlist(key, value)
    12041206        else:
    12051207            query_dict[key] = value
    12061208    if not query_dict:
    1207         return ""
     1209        return "?" if has_removed else ""
    12081210    query_string = query_dict.urlencode()
    12091211    return f"?{query_string}"
    12101212
  • TabularUnified tests/template_tests/syntax_tests/test_querystring.py

    diff --git a/tests/template_tests/syntax_tests/test_querystring.py b/tests/template_tests/syntax_tests/test_querystring.py
    index dea8ee0142..fba0fe22d7 100644
    a b class QueryStringTagTests(SimpleTestCase):  
    2020            "test_querystring_empty_get_params", context, expected=""
    2121        )
    2222
     23    @setup({"test_querystring_remove_all_params": "{% querystring a=None %}"})
     24    def test_querystring_remove_all_params(self):
     25        context = RequestContext(self.request_factory.get("/?a=b"))
     26        self.assertRenderEqual(
     27            "test_querystring_remove_all_params", context, expected="?"
     28        )
     29
    2330    @setup({"test_querystring_non_empty_get_params": "{% querystring %}"})
    2431    def test_querystring_non_empty_get_params(self):
    2532        request = self.request_factory.get("/", {"a": "b"})

I am cc-ing some other folks to see what they think as well

comment:2 by Tom Carrick, 3 days ago

Seems like we should do something here.

So for only when it's changed, because if nothing has changed, you would expect it to reload? I think I agree with that line of reasoning though I haven't spent any time thinking about possible use cases and edge cases.

comment:3 by Sarah Boyce, 3 days ago

{% querystring %}

Outputs the current query string verbatim. So if the query string is ?color=green, the output would be ?color=green.

I guess specifically, I did not expect the test template_tests.syntax_tests.test_querystring.QueryStringTagTests.test_querystring_empty_get_params to now return "?" instead of "". But perhaps there's no real advantage of keeping the url the same?

in reply to:  3 comment:4 by David Feeley, 2 days ago

Replying to Sarah Boyce:

{% querystring %}

Outputs the current query string verbatim. So if the query string is ?color=green, the output would be ?color=green.

I guess specifically, I did not expect the test template_tests.syntax_tests.test_querystring.QueryStringTagTests.test_querystring_empty_get_params to now return "?" instead of "". But perhaps there's no real advantage of keeping the url the same?

I like your addition of the has_removed condition as I think rewriting an empty query string ("") to "?" more generally probably violates the principle of least surprise -- ie. why rewrite it when there is no need to.

comment:5 by Sarah Boyce, 29 hours ago

Has patch: set
Owner: set to Sarah Boyce
Status: newassigned

comment:6 by Natalia Bidart, 8 hours ago

Triage Stage: AcceptedReady for checkin

comment:7 by Sarah Boyce <42296566+sarahboyce@…>, 7 hours ago

Resolution: fixed
Status: assignedclosed

In 05002c15:

Fixed #36182 -- Returned "?" if all parameters are removed in querystring template tag.

Thank you to David Feeley for the report and Natalia Bidart for the review.

comment:8 by Sarah Boyce <42296566+sarahboyce@…>, 7 hours ago

In 92d5b2f3:

[5.2.x] Fixed #36182 -- Returned "?" if all parameters are removed in querystring template tag.

Thank you to David Feeley for the report and Natalia Bidart for the review.

Backport of 05002c153c5018e4429a326a6699c7c45e5ea957 from main.

comment:9 by Sarah Boyce <42296566+sarahboyce@…>, 7 hours ago

In 6511340:

[5.1.x] Fixed #36182 -- Returned "?" if all parameters are removed in querystring template tag.

Thank you to David Feeley for the report and Natalia Bidart for the review.

Backport of 05002c153c5018e4429a326a6699c7c45e5ea957 from main.

Note: See TracTickets for help on using tickets.
Back to Top