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

[David Feeley]

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

[Sarah Boyce]

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:

django/template/defaulttags.py

@@ -1195 +1195 @@
     if query_dict is None:
         query_dict = context.request.GET
     query_dict = query_dict.copy()
+    has_removed = False
     for key, value in kwargs.items():
         if value is None:
             if key in query_dict:
                 del query_dict[key]
+                has_removed = True
         elif isinstance(value, Iterable) and not isinstance(value, str):
             query_dict.setlist(key, value)
         else:
             query_dict[key] = value
     if not query_dict:
-        return ""
+        return "?" if has_removed else ""
     query_string = query_dict.urlencode()
     return f"?{query_string}"
 

tests/template_tests/syntax_tests/test_querystring.py

@@ -20 +20 @@
             "test_querystring_empty_get_params", context, expected=""
         )
 
+    @setup({"test_querystring_remove_all_params": "{% querystring a=None %}"})
+    def test_querystring_remove_all_params(self):
+        context = RequestContext(self.request_factory.get("/?a=b"))
+        self.assertRenderEqual(
+            "test_querystring_remove_all_params", context, expected="?"
+        )
+
     @setup({"test_querystring_non_empty_get_params": "{% querystring %}"})
     def test_querystring_non_empty_get_params(self):
         request = self.request_factory.get("/", {"a": "b"})

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

[Tom Carrick]

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.

[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?