DEV Community

Cover image for Djangonaut diaries, week 2: working on the first issue
rodbv
rodbv

Posted on

Djangonaut diaries, week 2: working on the first issue

#django #djangonaut

Hi there!

OK, so in parts 1 and 2 we prepared our development environment to contribute with Django. I guess now it's time to actually get stuff done!

The issue I picked up is 17 years old, which I'd love to say "nearly my age", but unfortunately not :P

In summary: when you select one or more items to be deleted in Django Admin, it rightly shows a confirmation page, like this

Delete confirmation page with a list of objects to be deleted, and a red button at the bottom with text

So, it shows the object(s) you're about to delete, and their children, recursively, which is great to avoid regrets.

But what if there are thousand of children objects? Suddenly the list may get reeeeeeaaaaaally long, taking several seconds to load, potentially slowing down the browser due to memory usage, while forcing the user to scroll a lot to reach the "Yes, I'm sure" button.

Very long list with hundreds of objects and red button all the way down

While the original request was to add the ability to hide the list of objects altogether, I really like a suggestion (comment #13) by user terminator14, to let one define the maximum number of items to show (say, 100) and then something like "...and 821 more", to give a sense of how much was truncated - note that there's also a summary with the total number on top.

Fine, now that the ideas are out there, let's get to the code. In part two of this series I already showed how I found the related code - there's no magic really, I just searched for a specific string, "Are you sure you want to delete".

1. Make it work

This is where everything starts, in the delete confirmation template:

<p>{% blocktranslate %}Are you sure you want to delete the {{ object_name }} “{{ escaped_object }}”? All of the following related items will be deleted:{% endblocktranslate %}</p>
{% include "admin/includes/object_delete_summary.html" %}
<h2>{% translate "Objects" %}</h2>
<ul id="deleted-objects">{{ deleted_objects|unordered_list }}</ul>
Enter fullscreen mode Exit fullscreen mode

From this, I can see that the list with <li>s is rendered by

<ul id="deleted-objects">{{ deleted_objects|unordered_list }}</ul>
Enter fullscreen mode Exit fullscreen mode

Curious. I was expecting to find a for loop with deleted_objects, but somehow[1] deleted_objects|unordered_list is producing the <li>s in it, so let's have a look.

This is what we see in the deleted_objects variable (thanks, awesome debugger from the previous post!)

[
    'Blog: <a href="/admin/blog/blog/.../">First Blog</a>', 
    [
        'Post: <a href="/admin/blog/post/54db9806.../">Consectetur labore...</a>', 
        ['Comment: Nice post!', 'Comment: Thanks for sharing'], # These are the [...]

        'Post: <a href="/admin/blog/post/dcbba30f.../">Porro dolorem...</a>', 
        ['Comment: Very informative'],

        'Post: <a href="/admin/blog/post/f80ecf48.../">Numquam voluptatem...</a>', 
        ['Comment: I disagree with this point'],

        'Post: <a href="/admin/blog/post/84438d87.../">Sed modi labore...</a>', 
        ['Comment: Great read'],

        'Post: <a href="/admin/blog/post/38505bb6.../">Dolor est dolorem...</a>', 
        ['Comment: Love the style'],

        'Post: <a href="/admin/blog/post/5dbb276c.../">Dolore amet...</a>', 
        ['Comment: This was helpful'],

        'Post: <a href="/admin/blog/post/b0eb6a30.../">Sed dolorem...</a>', 
        ['Comment: Can you explain more?'],

        'Post: <a href="/admin/blog/post/9153de52.../">Quaerat magnam...</a>', 
        ['Comment: +1'],

        'Post: <a href="/admin/blog/post/03c071e1.../">Quiquia quisquam...</a>', 
        ['Comment: Excellent summary'],

        'Post: <a href="/admin/blog/post/e4b7fcd0.../">Porro voluptatem...</a>', 
        ['Comment: Looking forward to the next one']
    ]
]
Enter fullscreen mode Exit fullscreen mode

So, there are no <li>s here, just a recursive data structure. My first idea was to actually truncate the items at this level, which would have to be a recursive function too (to limit the number of comments for each post as well), but whenever we're getting into a very complex code change, it's good to take a step back and think..is it really the simplest thing I could do?

The answer is no. If there's some other code traversing this data structure to produce nested <li>s, that's probably where I can have some sort of counter, while the list items are being produced, and truncate them.

That's on the filter receiving deleted_objects, called unordered_list.

The documentation for unordered_list provides a great explanation of what it does, and how:

Recursively take a self-nested list and return an HTML 
unordered list -- WITHOUT opening and closing <ul> tags.
Enter fullscreen mode Exit fullscreen mode

Inside this function there's another function which actually builds the list items, in the last line:

def list_formatter(item_list, tabs=1):
  indent = "\t" * tabs
  output = []
  for item, children in walk_items(item_list):
     sublist = ""
     if children:
       sublist = "\n%s<ul>\n%s\n%s</ul>\n%s" % (
          indent,
          list_formatter(children, tabs + 1),
          indent,indent)

       output.append("%s<li>%s%s</li>" % (indent, escaper(item), sublist))
Enter fullscreen mode Exit fullscreen mode

Ok so I want to test a concept: we already have an empty list called output (line 3); can I just create a counter and have an if statement checking the length of output and breaking the loop when we reach the limit? Let's check!

def list_formatter(item_list, tabs=1):
    indent = "\t" * tabs
    output = []
    max_items = 3 # changed here...
    for item, children in walk_items(item_list):
        sublist = ""
        if children:
            sublist = "\n%s<ul>\n%s\n%s</ul>\n%s" % (
                indent,
                list_formatter(children, tabs + 1),
                indent,
                indent,
             )

        if len(output) >= max_items: # ...and here
            output.append("%s<li>...and N more.</li>" % indent)
            break
        output.append("%s<li>%s%s</li>" % (indent, escaper(item), sublist))

    return "\n".join(output)
Enter fullscreen mode Exit fullscreen mode

Reloading the page...

...it worked!!!!!11

Now, I need "N" to be an actual number. The trouble is that this list is built iteratively (it's a recursive function), so I can't break the loop when I reach the limit, I gotta let it continue and keep counting how many were truncated.


def list_formatter(item_list, tabs=1):
    indent = "\t" * tabs
    output = []
    max_items = 3
    truncated_count = 0 # added a counter for truncated items
    for item, children in walk_items(item_list):
        sublist = ""
        if children:
            sublist = "\n%s<ul>\n%s\n%s</ul>\n%s" % (
                indent,
                list_formatter(children, tabs + 1),
                indent,
                indent,
            )

        if len(output) >= max_items: # changed a bit here...
            truncated_count += 1
        else:
            output.append("%s<li>%s%s</li>" % (indent, escaper(item), sublist))

    if truncated_count > 0: # and moved the truncated message outside the loop
        output.append("%s<li>...and %s more.</li>" % (indent, truncated_count))

    return "\n".join(output)
Enter fullscreen mode Exit fullscreen mode

Drumroll...

Woohoo! We're basically there. 90% done (yeah not really, but that's how we work, right? always 90% done!)

To see if this works recursively, I can change max_items to 1 and see if they're applied on the comments too.

As an Australian friend of mine would say... "that's a beauty, mate!"

Now that it works, it's time for ....

2. Make it right

Now that we made it work, we have to make it right, which means we need to get rid of that hard-coded max_items=3, it should be promoted to a parameter for the filter:

<ul id="deleted-objects">{{ deleted_objects|unordered_list:max_items }}</ul>
Enter fullscreen mode Exit fullscreen mode

...where max_items is a context variable like 100.

To achieve this I just added a parameter on unordered_list. Since I want to keep this function backwards-compatible, it's a nullable param, with default None.

def unordered_list(value, autoescape=True, max_items: int | None = None):
    # ... previous code ...

    def list_formatter(item_list, tabs=1):
            # ... previous code ...

            # ...tweaked a bit here to handle None
            if max_items is not None and len(output) >= max_items:
                truncated_count += 1
            else:
                output.append("%s<li>%s%s</li>" % (indent, escaper(item), sublist))

        if truncated_count > 0:
            output.append("%s<li>...and %s more.</li>" % (indent, truncated_count))

        return "\n".join(output)

    return mark_safe(list_formatter(value))
Enter fullscreen mode Exit fullscreen mode

Now, let's try with a value on the filter...

<ul id="deleted-objects">{{ deleted_objects|unordered_list:2 }}</ul>
Enter fullscreen mode Exit fullscreen mode

Uh-oh, the template engine is not happy.

Template crashed with a syntax error

Basically, since unordered_filter already has a parameter autoescape, I can't add a second one, and pass a value for it. Bummer. I can't force max_items to be in front of autoescape as well, this would be a change of interface.

The simplest solution I could think of is to create a new filter, truncated_unordered_list, which will have the parameter max_items as its first parameter:

@register.filter(is_safe=True, needs_autoescape=True)
def truncated_unordered_list(value, max_items: int | None = None, autoescape=True):
    return unordered_list(value, max_items=max_items)
Enter fullscreen mode Exit fullscreen mode

Maybe one of the reviewers will suggest a better way? we'll see :)

So now let's try our new filter:

<ul id="deleted-objects">{{ deleted_objects|truncated_unordered_list:2 }}</ul>
Enter fullscreen mode Exit fullscreen mode

...the moment of truth...

We're back!

Let's try some variations. First, I don't pass any param, truncated_unordered_list should not break.

<ul id="deleted-objects">{{ deleted_objects|truncated_unordered_list }}</ul>
Enter fullscreen mode Exit fullscreen mode

I won't put the screenshot here, but suffice to say, the whole list was rendered, un-truncated (is that a word? no) so we're good.

Next test, is if I pass a value of zero.

<ul id="deleted-objects">{{ deleted_objects|truncated_unordered_list:0 }}</ul>
Enter fullscreen mode Exit fullscreen mode

It works, although it's a bit weird. We can do something better with it later.

To wrap up this, I just want to have a context variable, not a hard-coded number on the template, since this is a template that ships with Django. Something like this

<ul id="deleted-objects">{{ deleted_objects|truncated_unordered_list:delete_confirmation_max_objects }}</ul>
Enter fullscreen mode Exit fullscreen mode

To get there, let's go from the bottom up and find our way.

What I want to do, is to let a person configuring a given admin by giving it a value for the variable

@admin.register(Blog)
class BlogAdmin(admin.ModelAdmin):
    delete_confirmation_max_objects = 100
    # ...other code for this admin
Enter fullscreen mode Exit fullscreen mode

This is cool because it's a backwards-compatible change: if a value for delete_confirmation_max_objects is not set, the delete confirmation page works just like it always worked.

Aside: This also means that maybe this could be a change added to Django 6.1 or 6.2 (as of the time of writing, Django is on 6.0), since this change does not break current interfaces - but honestly I don't know how this policy for backwards-compatible new features work..we'll see.

Anyway, having this variable there won't do anything by itself, I need to add it to ModelAdmin

class ModelAdmin(BaseModelAdmin):
    # ..lots of code...

    # Custom templates (designed to be over-ridden in subclasses)

    # ...other constants
    delete_confirmation_max_objects = None
Enter fullscreen mode Exit fullscreen mode

Note that there's already a section for custom template varibles, so it looks right to have our new constant there.

Finally, I need to pass this constant from admin to the template, via context. I searched for how and where other constants were passed, and found the place

# django/contrib/admin/actions.py
@action(
    permissions=["delete"],
    description=gettext_lazy("Delete selected %(verbose_name_plural)s"),
)
def delete_selected(modeladmin, request, queryset):
    # ... lots of code

    context = {
        **modeladmin.admin_site.each_context(request),
        # ... other values
        "delete_confirmation_max_objects": modeladmin.delete_confirmation_max_objects,
    }
Enter fullscreen mode Exit fullscreen mode

Nice! We closed the loop.

Now when I do this...

<ul id="deleted-objects">{{ deleted_objects|truncated_unordered_list:delete_confirmation_max_objects }}</ul>
Enter fullscreen mode Exit fullscreen mode

...and this

@admin.register(Blog)
class BlogAdmin(admin.ModelAdmin):
    delete_confirmation_max_objects = 2
    # ...other code for this admin
Enter fullscreen mode Exit fullscreen mode

I see this

Mission accomplished! Time to prepare the PR!

For the PR, there was quite a lot I had to do. The most important bits being

  • Add tests for "happy path" and edge cases;
  • Add validation and error handling for invalid values like truncated_unordered_list:invalid_number

..and also some instructions from the contributor's guide, like documentation and add my name to AUTHORS (which feels very..entitled? early? weird.)

Anyway.... Here is my first PR and I couldn't be more proud

I do expect this PR to require a few iterations before it's approved (if ever), but that's part of the process, and part of the experience I want to have as a Djangonaut.

Hope you enjoyed! Cheers from Floripa, Brazil!

[1] I know you thought "Palpatine returned" after "Somehow", if you're a Star Wars fan too.

Top comments (0)