Opened 15 months ago

Closed 5 months ago

Last modified 5 months ago

#34789 closed Bug (fixed)

`filter_horizontal` duplicates entries in "Chosen" column after instance is added via in another field using the "plus" JS action

Reported by: Devin Cox Owned by: Devin Cox
Component: contrib.admin Version: dev
Severity: Normal Keywords:
Cc: David Sanders, Marcelo Galigniana Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: yes

Description (last modified by Devin Cox)

Hi there, I noticed a minor frontend bug related to the filter_horizontal tables in the admin console. Here is what I have:

There is a model called Transition with two fields, source and target:

source = models.ManyToManyField(State, related_name="transition_source")
target = models.ForeignKey(State, related_name="transition_target", on_delete=models.CASCADE)

For the admin console, we set filter_horizontal to include 'source'. See "First Screenshot" for an example of a given state of a Transition, with "test" selected as Source and "test2" selected as Target.

Now, if I add a new State by clicking the "+" Icon next to the Target field (let's call it test3), it will add it to both the Target AND the "Chosen Source" (see "Second Screenshot" for an example).

I will note that upon saving the Transition, it will not actually save "test3" to the Chosen Source, meaning that this is an issue only from a UI perspective. After any refresh or save, it will also no longer show in the Chosen Source column. Additionally, there is no issue going the other direction (if I add directly using the "+" next to Chosen Source, it will not appear to set the "Target" field as well, and it will save properly). Finally, if I were to select another available source (ex. "test2") and add it to the Chosen Source, it will successfully add "test2" and remove "test3" without needing a save or refresh.

I believe this is due to the admin conveniently updating all related fields to include the added object, and therefore displaying it as if it was selected temporarily until refreshed

Diving into this, I believe the issue is in this file: django/django/admin/static/admin/js/admin/RelatedObjectLookups.js

In this function: UpdateRelatedSelectOptions, on this query:

const selectsRelated = document.querySelectorAll(`[data-model-ref="${modelName}"] select:not(.admin-autocomplete)`);

My initial guess is that updating the query to not include the "Chosen" column in filter_horizontal fields would solve this issue (something like

const selectsRelated = document.querySelectorAll(`[data-model-ref="${modelName}"] div:not(.selector-chosen) > select:not(.admin-autocomplete)`);

Let me know if more context is needed to help reproduce what is going on here. Thanks!

Attachments (2)

Screenshot 2023-08-21 at 2.09.51 PM.png (45.2 KB ) - added by Devin Cox 15 months ago.
First Screenshot
Screenshot 2023-08-21 at 2.14.24 PM.png (46.7 KB ) - added by Devin Cox 15 months ago.
Second Screenshot

Download all attachments as: .zip

Change History (29)

by Devin Cox, 15 months ago

First Screenshot

by Devin Cox, 15 months ago

Second Screenshot

comment:1 by Devin Cox, 15 months ago

Description: modified (diff)

comment:2 by Natalia Bidart, 15 months ago

Hello! Thank you for your report. Could you please post your admin.py definition as well as your models.py (reduced to this example)? So we can try to reproduce and triage accordingly. Thanks!

in reply to:  2 ; comment:3 by Devin Cox, 15 months ago

Replying to Natalia Bidart:

Hello! Thank you for your report. Could you please post your admin.py definition as well as your models.py (reduced to this example)? So we can try to reproduce and triage accordingly. Thanks!

Of course, here is the model (reduced to the example):

class Workflow(models.Model):
    initial = models.ForeignKey('workflow.State', blank=True, null=True, related_name="workflow_initial", on_delete=models.SET_NULL)

class Transition(models.Model):
    source = models.ManyToManyField(State, related_name="transition_source")
    target = models.ForeignKey(State, related_name="transition_target", on_delete=models.CASCADE)

class State(models.Model):
    label = models.CharField(max_length=255)

And the admin (also reduced):

class WorkflowAdmin(SortableAdminBase, admin.ModelAdmin):
    inlines = [TransitionInline, ]
    fieldsets = (
        (None, {
            'fields': ["initial"],
        }),
    )

class TransitionInline(SortableStackedInline, admin.StackedInline):

    model = Transition
    extra = 0
    filter_horizontal = ['source']

    fieldsets = (
        (' ', {
            'classes': ('collapse',),
            'fields': ('source', 'target')
        }),
    )

class StateAdmin(admin.ModelAdmin):
    fieldsets = (
            (None, {
                'fields': ("label",),
            }),
        )

As you can see, the model of interest is Transition and in the admin console it is represented through as an inline for another model called Workflow. I tried to remove any extra information to avoid confusion (part of this removal was my_order fields in case there is confusion as to why Sortable is being used).

While I don't think that the "Workflow" component (and Inline stacking) are relevant to this bug -- I kept them there to keep it as close to my current flow as possible. This should be able to be reproduced without the workflow element.

Last edited 15 months ago by Devin Cox (previous) (diff)

in reply to:  3 comment:4 by Natalia Bidart, 15 months ago

Triage Stage: UnreviewedAccepted

Replying to devin13cox:

Of course, here is the model (reduced to the example):

Great! Thank you.

While I don't think that the "Workflow" component (and Inline stacking) are relevant to this bug -- I kept them there to keep it as close to my current flow as possible. This should be able to be reproduced without the workflow element.

Confirmed I was able to reproduce with a reduced example like this:

from django.db import models


class State(models.Model):
    label = models.CharField(max_length=255)

    def __str__(self):
        return self.label


class Transition(models.Model):
    source = models.ManyToManyField(State, related_name="transition_source")
    target = models.ForeignKey(State, related_name="transition_target", on_delete=models.CASCADE)
from django.contrib import admin

from .models import State, Transition


class TransitionAdmin(admin.ModelAdmin):
    filter_horizontal = ['source']


admin.site.register(State)
admin.site.register(Transition, TransitionAdmin)

Would you like to prepare a patch?

comment:5 by Natalia Bidart, 15 months ago

Summary: Admin filter_horizontal shows item in "Chosen" column after being added through a Foreign Key field of the same model.`filter_horizontal` duplicates entries in "Chosen" column after instance is added via in another field using the "plus" JS action

comment:6 by Mariusz Felisiak, 15 months ago

Cc: David Sanders Marcelo Galigniana added

Check out related tickets #34025 and #11803.

Last edited 15 months ago by Mariusz Felisiak (previous) (diff)

comment:7 by Devin Cox, 15 months ago

Owner: nobody removed
Status: newassigned

in reply to:  description ; comment:8 by David Sanders, 15 months ago

Replying to devin13cox:

My initial guess is that updating the query to not include the "Chosen" column in filter_horizontal fields would solve this issue (something like

const selectsRelated = document.querySelectorAll(`[data-model-ref="${modelName}"] div:not(.selector-chosen) > select:not(.admin-autocomplete)`);

That could work 👍 I'm just wondering though whether there's a better approach than to continuously blacklist ancestors as we come across these 🤔

in reply to:  8 ; comment:9 by yokeshwaran1, 15 months ago

Hi David,
Im new to this repo, so I appreciate any guidance or feedback u can offer.

Regardin issue with filter_horizontal "Chosen" column. From the discussion, I think about a solution which will not continuously blacklisting ancestors

  1. Adding a data attribute like data-context to select elements during its render phase in the Django admin templates. So each select will have a more specific context about its purpose (eg. source, target, chosen etc.).
  1. Then modifying the query in RelatedObjectLookups.js to use this new data-context attribute, so our selector will become:
const selectsRelated = document.querySelectorAll(`[data-model-ref="${modelName}"][data-context="source"] select:not(.admin-autocomplete)`);

This approach will provide clear context for each select element which makes JavaScript logic more intentional. would this be a feasible, acceptable solution in ur opinion? I'm eager to hear ur thoughts & make any necessary adjustments. Once approach is approved, I'm interested in working on this ticket. Please let me know if that's possible.

Replying to David Sanders:

Replying to devin13cox:

My initial guess is that updating the query to not include the "Chosen" column in filter_horizontal fields would solve this issue (something like

const selectsRelated = document.querySelectorAll(`[data-model-ref="${modelName}"] div:not(.selector-chosen) > select:not(.admin-autocomplete)`);

That could work 👍 I'm just wondering though whether there's a better approach than to continuously blacklist ancestors as we come across these 🤔

in reply to:  9 comment:10 by David Sanders, 15 months ago

Replying to yokeshwaran1:

I think about a solution which will not continuously blacklisting ancestors

Yep that's along the lines of what I was getting at 😊👍

The question is though which approach is clearer/easier to maintain. The benefit of whitelisting is we can remove not(.admin-autocomplete) too I think 🤔

Edit: Felix & Nesita will decide which is better; if you like submit a PR with a fix + some tests.

Last edited 15 months ago by David Sanders (previous) (diff)

comment:11 by yokeshwaran1, 15 months ago

Thanks, David. I’ll make the necessary changes and submit a PR with tests soon.

in reply to:  11 comment:12 by yokeshwaran1, 14 months ago

Hi David,
I have opened a PR to address this issue: https://github.com/django/django/pull/17219

Replying to yokeshwaran1:

Thanks, David. I’ll make the necessary changes and submit a PR with tests soon.

comment:13 by Natalia Bidart, 14 months ago

Has patch: set

Setting the has patch flag.

comment:14 by Mariusz Felisiak, 14 months ago

Owner: set to yokeshwaran1

comment:15 by Natalia Bidart, 14 months ago

Needs tests: set
Patch needs improvement: set

comment:16 by Devin Cox, 9 months ago

Owner: changed from yokeshwaran1 to Devin Cox

Opened new PR on this https://github.com/django/django/pull/17897. Ran into small issue that still needs work, as described in the old PR. Will get it patched soon, but if anyone has direction on it I would appreciate feedback. Thanks!

comment:17 by Devin Cox, 8 months ago

Needs tests: unset
Patch needs improvement: unset

comment:18 by Devin Cox, 8 months ago

Hello, just wanted to check in and see if there is anything else I need to do pending a review for this ticket. No rush on getting this in, I just wanted to make sure you guys aren't waiting on something from me. Thanks!

comment:19 by Tim Graham, 8 months ago

No, the ticket flags are set correctly to put the ticket in the patch review queue. You can confirm this by looking at the yellow box below the ticket description ("According to the ticket's flags...").

comment:20 by Natalia Bidart, 8 months ago

Patch needs improvement: set
Version: 4.2dev

comment:21 by Natalia Bidart, 7 months ago

Easy pickings: unset

comment:22 by Devin Cox, 7 months ago

Patch needs improvement: unset

comment:23 by Sarah Boyce, 6 months ago

Patch needs improvement: set

comment:24 by Devin Cox, 5 months ago

Patch needs improvement: unset

comment:25 by Sarah Boyce, 5 months ago

Triage Stage: AcceptedReady for checkin

comment:26 by Sarah Boyce <42296566+sarahboyce@…>, 5 months ago

Resolution: fixed
Status: assignedclosed

In 719a42b5:

Fixed #34789 -- Prevented updateRelatedSelectsOptions from

adding entries to filter_horizontal chosen box.

Co-authored-by: yokeshwaran1 <yokesh440@…>

comment:27 by Sarah Boyce <42296566+sarahboyce@…>, 5 months ago

In 49a3a8d:

[5.1.x] Fixed #34789 -- Prevented updateRelatedSelectsOptions from

adding entries to filter_horizontal chosen box.

Co-authored-by: yokeshwaran1 <yokesh440@…>

Backport of 719a42b589d7551fc84708044b9e984ce723c8a2 from main.

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