Inconsistent treatment of 0 by ModelAdmin.delete_confirmation_max_display attribute

[Jacob Walls]

ModelAdmin.delete_confirmation_max_display was added first in #10919 (6.1), and it treated zero as "show no objects" about to be deleted.

It was then adapted for a very similar use for inlines in #36984 (6.1), but the treatment of 0 was flipped from "show none" to "show all".

We had an explicit test over the template response for the former case confirming "no objects shown", but the flipped logic still caused some over-processing under the hood. Then, for inlines, 0 now means the opposite ("show all").

Opening a PR with a unit test shortly.

[Jacob Walls]

​PR

[Natalia Bidart]

Reproduced while doing the PR review.

[Natalia Bidart]

Jacob and I agreed to add a new minimal string to avoid having a sentence finished with a colon.

[Jacob Walls]

Indeed, we did agree on that earlier today, however, I'm back to my original hesitation. The release-process.txt says:

The release candidate marks the string freeze ... After this point, new translatable strings must not be added.

During the 6.0.x series, we fixed a bug in a string in 05a5244b8924c47cc5b1332dd15aaa1be6abbe1e, but was a bug in a string that manifested 100% of the time, not a new string. Here we are contemplating a new string just to fix a dangling colon in an edge case. I don't think that qualifies for a backport. I don't think it's worth announcing a call for translations on the forum.

When targeting main only, we could also take the opportunity to address the dangling colon for top-level deletes as well. I'm pretty sure we have the same problem in that template:

django/contrib/admin/templates/admin/delete_selected_confirmation.html

@@ -23 +23 @@
     <p>{% blocktranslate %}Deleting the selected {{ objects_name }} would result in deleting related objects, but your account doesn't have permission to delete the following types of objects:{% endblocktranslate %}</p>
     <ul>{{ perms_lacking|truncated_unordered_list:delete_confirmation_max_display }}</ul>
 {% elif protected %}
-    <p>{% blocktranslate %}Deleting the selected {{ objects_name }} would require deleting the following protected related objects:{% endblocktranslate %}</p>
-    <ul>{{ protected|truncated_unordered_list:delete_confirmation_max_display }}</ul>
+    {% if delete_confirmation_max_display == 0 %}
+        <p>{% blocktranslate %}Deleting the selected {{ objects_name }} would require deleting some protected related objects.{% endblocktranslate %}</p>
+    {% else %}
+        <p>{% blocktranslate %}Deleting the selected {{ objects_name }} would require deleting the following protected related objects:{% endblocktranslate %}</p>
+        <ul>{{ protected|truncated_unordered_list:delete_confirmation_max_display }}</ul>
+    {% endif %}
 {% else %}
     <p>{% blocktranslate %}Are you sure you want to delete the selected {{ objects_name }}? All of the following objects and their related items will be deleted:{% endblocktranslate %}</p>
     {% include "admin/includes/object_delete_summary.html" %}

And the {% if delete_confirmation_max_display is None or delete_confirmation_max_display > 0 %} I asked for in ​review only made it into one of the two relevant templates, so that also needs a fix, but again, IMO it's a "cleanup", not a "backportable bug in a new feature":

django/contrib/admin/templates/admin/delete_selected_confirmation.html

@@ -28 +28 @@
 {% else %}
     <p>{% blocktranslate %}Are you sure you want to delete the selected {{ objects_name }}? All of the following objects and their related items will be deleted:{% endblocktranslate %}</p>
     {% include "admin/includes/object_delete_summary.html" %}
+    {% if delete_confirmation_max_display is None or delete_confirmation_max_display > 0 %}
     <h2>{% translate "Objects" %}</h2>
     {% for deletable_object in deletable_objects %}
         <ul>{{ deletable_object|truncated_unordered_list:delete_confirmation_max_display }}</ul>
     {% endfor %}
+    {% endif %}
     <form method="post">{% csrf_token %}
     <div>
     {% for obj in queryset %}

How does that full picture strike you?

[Natalia Bidart]

Replying to Jacob Walls:

How does that full picture strike you?

Thanks for the detailed write up, I don't object to the plan for 6.1 (except a note in the next sentence), but I don't see why this can't all happen in the PR for this ticket though, as two commits: the first one with the current fix (and I'd argue also adding the missing guard in delete_selected_confirmation.html too, since it adds no strings and is a visible bug in a new 6.1 feature), which gets backported; and a second "Refs" commit with the new strings for the inline and both templates, since delete_confirmation.html has the same dangling colon, main only.

Locally I get:

<div id="content" class="colM">
          <div class="titles-and-tools">
            <div class="titles">
              <h1>Cannot delete loooooooooooooooooooooooooooooooooooooooooooooooooooooong</h1>
            </div>
          </div>
    <p>Deleting the loooooooooooooooooooooooooooooooooooooooooooooooooooooong “Repro 37386” would require deleting the following protected related objects:</p>
    <ul id="deleted-objects"></ul>
          <br class="clear">
        </div>

Thoughts?

[Jacob Walls]

Spot on. Sounds like a plan!

[nessita <124304+nessita@…>]

In 7d94ee2:

Fixed #37386 -- Observed delete_confirmation_max_display=0 for inlines.

0 was being treated equivalently to None, which didn't agree with the
behavior on ModelAdmin for the delete actions.

Thanks Natalia Bidart for the review.

Bug in 57c8c8b107248a3358dd26276ac497c577454011.

[nessita <124304+nessita@…>]

In 8abacfc:

Refs #37386 -- Tailored error messages for the delete_confirmation_max_display=0 case.

[Natalia <124304+nessita@…>]

In e34d7763:

[6.1.x] Fixed #37386 -- Observed delete_confirmation_max_display=0 for inlines.

0 was being treated equivalently to None, which didn't agree with the
behavior on ModelAdmin for the delete actions.

Thanks Natalia Bidart for the review.

Bug in 57c8c8b107248a3358dd26276ac497c577454011.

Backport of 7d94ee295df9b2503135158a7f2b55fb686785fc from main.