Re: [Django] #36182: querystring templatetag should render empty querystring as "?" not "" when it has removed items from the querydict (was: querystring templatetag should render empty querystring as "?" not "")

[Django]

#36182: querystring templatetag should render empty querystring as "?" not "" when
it has removed items from the querydict
-------------------------------------+-------------------------------------
Reporter: David Feeley | Owner: (none)
Type: Bug | Status: new
Component: Template system | Version: 5.1
Severity: Release blocker | Resolution:
Keywords: querystring | Triage Stage: Accepted
templatetag |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Sarah Boyce):

* cc: Tom Carrick, Natalia Bidart (added)
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
* 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

Comment:

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:
{{{#!diff
--- a/django/template/defaulttags.py
+++ b/django/template/defaulttags.py
@@ -1195,16 +1195,18 @@ def querystring(context, query_dict=None,
**kwargs):
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}"

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/tests/template_tests/syntax_tests/test_querystring.py
+++ b/tests/template_tests/syntax_tests/test_querystring.py
@@ -20,6 +20,13 @@ class QueryStringTagTests(SimpleTestCase):
"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
--
Ticket URL: <https://code.djangoproject.com/ticket/36182#comment:1>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.