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

[devin13cox]

This is largely based off the work of @yokeshwaran1 and the closed PR https://github.com/django/django/pull/17219.

This PR replaces the Selenium Test with a more active walkthrough to recreate the bug.

Ticket Link: https://code.djangoproject.com/ticket/34789#no1

[devin13cox]

Resolved the issue commented on above. Interestingly, the failure was actually caused when using a model with casing like this:

FooBar

The uppercase results in data-model-ref checking foobar against foo bar, and therefore data-model-ref does not pick up the match.

By changing the model name to this: Foobar, it prevents the added space and therefore data-model-ref is able to make the correct match.

This issue already exists in main so not relevant to this specific fix. I think @yokeshwaran1 already posted a ticket for the adjacent bug, but if not we can create a new one.

Note if trying to reproduce: The model with the casing issue was resolved by turning TransitionState into Transitionstate

[nessita]

Hey @devin13cox, thank you for your work so far!

I have restarted my review in this branch/ticket but I have an issue, I'm not being able to reproduce the original report using latest main (nor Django 5.0.3 nor Django 4.2). Can you confirm if you are still able to reproduce?

I'm using the same models as before:

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)

    def __str__(self):
        return f"Sources: {', '.join(s.label for s in self.source.all())} -- Target: {self.target.label}"

And the same admin:

from django.contrib import admin

from .models import State, Transition, TransitionTriple


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


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

Let me know! Thank you.

[nessita]

@devin13cox I spent quite some time debugging why I wasn't able to reproduce. Finally I realized I was trying to reproduce a slightly different issue :facepalm:. We have two bugs to solve (as we have said in the past), unclear whether we want to solve them in the same PR or not:

1. One issue is the reported one, when adding a new related entry using the "+" sign from the FK field, the M2M with the filter_horizontal gets incorrectly updated. For this, I think your proposed solution could work, I will go back to review this diff with that in mind, will submit updates on this soon.
2. The other issue is when adding a new related entry using the "+" sign from from the M2M field. In this case, no other widget gets updated, but really it should update all other fields listing instances of the related model just added. For this, I tracked down the issue to the selector used in dismissAddRelatedObjectPopup to decide whether to call updateRelatedSelectsOptions or not. Basically the id used in the document.getElementById is wrong for the M2M widget (but correct for the single FK field, so we may need to fix the call site to pass the proper name):

--- a/django/contrib/admin/static/admin/js/admin/RelatedObjectLookups.js
+++ b/django/contrib/admin/static/admin/js/admin/RelatedObjectLookups.js
@@ -119,7 +119,7 @@
 
     function dismissAddRelatedObjectPopup(win, newId, newRepr) {
         const name = removePopupIndex(win.name);
-        const elem = document.getElementById(name);
+        const elem = document.getElementById(name + '_from');
         if (elem) {
             const elemName = elem.nodeName.toUpperCase();
             if (elemName === 'SELECT') {

[nessita]

Resolved the issue commented on above.

:trophy:

Interestingly, the failure was actually caused when using a model with casing like this: FooBar

I think this is in fact another issue, not the one reported in ticket-34789.

The uppercase results in data-model-ref checking foobar against foo bar, and therefore data-model-ref does not pick up the match.

By changing the model name to this: Foobar, it prevents the added space and therefore data-model-ref is able to make the correct match.

This issue already exists in main so not relevant to this specific fix. I think @yokeshwaran1 already posted a ticket for the adjacent bug, but if not we can create a new one.

Note if trying to reproduce: The model with the casing issue was resolved by turning TransitionState into Transitionstate

When I first read this comment, I wasn't sure what you mean. Now, after having been debugging things in this code for a few hours, I understand that we have 3 issues: the two I detailed in my previous comment, and a third one which can be described as:

The update of related objects fails in the admin when the related model has a name in camel case.

Do you agree? I don't think we have tickets for each of the three issues, would you fancy filing ticket for 2 and 3?

[nessita]

The update of related objects fails in the admin when the related model has a name in camel case.

Do you agree? I don't think we have tickets for each of the three issues, would you fancy filing ticket for 2 and 3?

After some more debugging, I believe the fix for the third issue should something along these lines:

diff --git a/django/contrib/admin/templates/admin/widgets/related_widget_wrapper.html b/django/contrib/admin/templates/admin/widgets/related_widget_wrapper.html
index 8e4356a95c..99b20545af 100644
--- a/django/contrib/admin/templates/admin/widgets/related_widget_wrapper.html
+++ b/django/contrib/admin/templates/admin/widgets/related_widget_wrapper.html
@@ -1,5 +1,5 @@
 {% load i18n static %}
-<div class="related-widget-wrapper" {% if not model_has_limit_choices_to %}data-model-ref="{{ model }}"{% endif %}>
+<div class="related-widget-wrapper" {% if not model_has_limit_choices_to %}data-model-ref="{{ model_name }}"{% endif %}>
     {{ rendered_widget }}
     {% block links %}
         {% spaceless %}
diff --git a/django/contrib/admin/widgets.py b/django/contrib/admin/widgets.py
index fc0cd941d1..9633ebb1a1 100644
--- a/django/contrib/admin/widgets.py
+++ b/django/contrib/admin/widgets.py
@@ -328,6 +328,7 @@ class RelatedFieldWidgetWrapper(forms.Widget):
             "name": name,
             "url_params": url_params,
             "model": rel_opts.verbose_name,
+            "model_name": rel_opts.model_name,
             "can_add_related": self.can_add_related,
             "can_change_related": self.can_change_related,
             "can_delete_related": self.can_delete_related,

Would you have some time to create the ticket and propose a patch? :medal_sports:

[devin13cox]

Hey @nessita, thank you for taking a deep dive into this! I was not aware of the m2m issue, but that makes sense. I can set aside some time next week to introduce the patch and some tests.

As for the third issue, that's exactly what I was describing. Simply a naming issue.

Would it make sense to make alterations to the scope of this ticket due to the related nature and push it all together? Or would you prefer separate patches for these items?

Adding to this ticket would likely simplify it, especially for the third issue (could simply alter the existing test to utilize camel casing). The M2M would need a slightly different setup.

[nessita]

Hey @nessita, thank you for taking a deep dive into this! I was not aware of the m2m issue, but that makes sense. I can set aside some time next week to introduce the patch and some tests.

Amazing, thank you!

I had the Selenium tests running for this PR (see actions outcome) and there was a failure, would you also have time to see if that's related to your work?

As for the third issue, that's exactly what I was describing. Simply a naming issue.

Would it make sense to make alterations to the scope of this ticket due to the related nature and push it all together? Or would you prefer separate patches for these items?

I think we should have separated patches, that allows to have a tidier git history and it also helps in the unlikely case that we need to revert a given change due to a regression.

Adding to this ticket would likely simplify it, especially for the third issue (could simply alter the existing test to utilize camel casing). The M2M would need a slightly different setup.

Yeah, I know that merging fixes is simpler in the short term, but in the medium to long term, it really pays off to have a clean git history.

Thank you!!!

/remind me to check on progress in 2 weeks

[github-actions[bot]]

@nessita set a reminder for 4/1/2024

[devin13cox]

Just commenting on this, I've been pretty busy lately so haven't had much time to fix this. I did have a chance to peek at the failed test, and it could be fixed by removing two lines but I'm actually questioning if that would be the functionality we want. Basically, when there are multiple foreign key fields pointing to the same object, like so:

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

class Foo(models.Model):
    first = models.ForeignKey(State)
    second = models.ForeignKey(State)

And we add to one of the first Foreign Key, then currently (in main), the added option would appear under the dropdown for second. However with this fix, that no longer occurs. Not sure immediately where to white-list this to allow for it without impacting the original fix (this was originally @yokeshwaran1 logic so need to take a deeper look, my initial suggestion was just to block the chosen component).

Not sure if I'll be able to resolve it this week so dropping this here in case there is no update by 4/1.

[devin13cox]

Altered the approach to add the context variable for RelatedWidgetWrapper instead of the Select widget based on whether or not allow_multiple_selected was chosen. This still works for the bug in this ticket, and appears to allow previous desired functionality to persist. Might still be sort of a blacklist-type approach, I think it's a little better than the frontend tweak I initially proposed.

Let me know what you think of this new direction, as it does divert from the original fix. Thanks!

[nessita]

Just commenting on this, I've been pretty busy lately so haven't had much time to fix this.

Thanks for sharing that. As mentioned before, we understand that contributors work when they can, so that's fine!

I did have a chance to peek at the failed test, and it could be fixed by removing two lines but I'm actually questioning if that would be the functionality we want. Basically, when there are multiple foreign key fields pointing to the same object, like so:

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

class Foo(models.Model):
    first = models.ForeignKey(State)
    second = models.ForeignKey(State)

And we add to one of the first Foreign Key, then currently (in main), the added option would appear under the dropdown for second. However with this fix, that no longer occurs. Not sure immediately where to white-list this to allow for it without impacting the original fix (this was originally @yokeshwaran1 logic so need to take a deeper look, my initial suggestion was just to block the chosen component).

Let me know what you find, I can help if you have a concrete theory.

Not sure if I'll be able to resolve it this week so dropping this here in case there is no update by 4/1.

No worries, at all! The reminders I set are for me to check on progress, with a focus in knowing whether a contributor has (eventually) the time to continue the work or if we should free the ticket for someone else to work on. So really if you work on this when you can, that helps!

Thank you.

[nessita]

Altered the approach to add the context variable for RelatedWidgetWrapper instead of the Select widget

I like the change of the data-context out of Select and into RelatedFieldWidgetWrapper, though I'm not a fan of the pop in AutocompleteMixin. Could we instead only add the data-context to the RelatedFieldWidgetWrapper when necessary?

based on whether or not allow_multiple_selected was chosen.

I'm a little lost on how the latest proposal matches the above, could you elaborate a bit? Thank you!

[devin13cox]

I'm a little lost on how the latest proposal matches the above, could you elaborate a bit? Thank you!

Wording was a bit awkward, the previous approach added data-context to Select widgets based on if allow_multiple_selected was True. Now we don't need that logic since it's in the wrapper.

I'll take a look at not including the removal during AutocompleteMixin and get back to you.

[devin13cox]

Hey @nessita , altered the approach to only add data-context if the widget is not an instance of AutocompleteMixin. Similar concept but a cleaner approach to not add it when it's not needed.

[github-actions[bot]]

:wave: @nessita, check on progress

[nessita]

@devin13cox As with the other PR, thank you for your patience, I haven't forgotten about this PR either, it's also in my list and I'll get to this ASAP.

[sarahboyce]

@devin13cox we can add asserts to SeleniumTests.test_related_object_update_with_camel_casing rather than creating new models/admin/test for this case

--- a/tests/admin_views/test_related_object_lookups.py
+++ b/tests/admin_views/test_related_object_lookups.py
@@ -117,3 +117,6 @@ class SeleniumTests(AdminSeleniumTestCase):
             <option value="1">{interesting_name}</option>
             """,
         )
+        # Check the newly added instance is not also added in the "to" box.
+        m2m_to = self.selenium.find_element(By.ID, "id_m2m_to")
+        self.assertHTMLEqual(m2m_to.get_attribute("innerHTML"), "")

Then you can delete all the added test Models/admin etc.

Otherwise this looks good to me, when you're done can you reduce this down to a single commit with @yokeshwaran1 as a co-author 👍