jazzband / django-smart-selects

chained and grouped selects for django forms
https://django-smart-selects.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
1.11k stars 348 forks source link

Empty ChainedManyToManyField #212

Open silentjay opened 7 years ago

silentjay commented 7 years ago

Using Django 1.11 and django-smart-selects 1.5.2. There are no console errors in admin and only one jquery file is present. I assume smart-selects is what I'm after and im not trying to make it match a use case its not designed for: Limiting choices on field (Site.theme) based on the selections of a ManyToMany field in another model (SiteType.themes)?

Anway if that's fine, for some reason I can't get it to display any options in the Site.theme admin widget.

USE_DJANGO_JQUERY = True
JQUERY_URL = False

class Theme(models.Model):
    name = models.CharField(max_length=50, blank=False, null=False, unique=True)

    def __unicode__(self):
        return str(self.name)

class SiteType(models.Model):
    name = models.CharField(max_length=50, blank=False, null=False, unique=True)
    themes = models.ManyToManyField(Theme, blank=True, null=True)

    def __unicode__(self):
        return str(self.name)

class Site(models.Model):
    user = models.ForeignKey(User)
    site_type = models.ForeignKey(SiteType, related_name="st")
    theme = ChainedManyToManyField(
        SiteType,
        horizontal=True,
        verbose_name='Pick a theme',
        chained_field="site_type",
        chained_model_field="themes")
blag commented 7 years ago

Those models look good to me.

What does the AJAX request look like in your browser developer tools? What do the request and response look like?

Also, you can get GitHub to syntax highlight your code blocks by adding the language after the backticks:

```python
import antigravity

class Astronaut(object):
    def blast_off(self):
        pass
```

will render as:

import antigravity

class Astronaut(object):
    def blast_off(self):
        pass

which makes code much easier to parse.

I'm on mobile but I should be able to get back to this late tonight or tomorrow.

silentjay commented 7 years ago

Hi sorry for the delay in getting back on this. There's no AJAX response/request

blag commented 7 years ago

No worries. Are there any errors in your browser console? Is the smart selects javascript being loaded? Is it targeting the elements you think it should be targeting?

Please include any and all potentially relevant troubleshooting information. It's nearly impossible and incredibly slow to debug your issue when I have next to no information.

silentjay commented 7 years ago

There's no console errors, jquery is being loaded once, all smarts selects js is being loaded. I see the admin widget but it's empty.

Could you give me some more information on what I need to look for regarding the targeted elements?

Should mention this is the second time I've tried smart-selects, but I've given up both times and implemented filtering manually. However I'm happy to help you try and debug or you can close this as some sort of weird unknown edge case.

blag commented 7 years ago

You should have some elements with a chained classes. You should also have something like smart-selects/admin/js/chainedm2m.js and smart-selects/admin/js/bindfields.js loaded.

There is a possibility that 7a5327ed4e1c4981f1e1be2d5452b427e5e09724 fixed it, do you mind trying the code in the master branch?

oceannw commented 7 years ago

I meet the same, all the js file is loaded. When horizontal=True, the ChainedManyToManyField Field is empty. But, when horizontal=False, all is ok.

blag commented 7 years ago

@oceannw How many times are admin/js/core.js, admin/js/SelectBox.js, and admin/js/SelectFilter2.js loaded when horizontal=True?

erozqba commented 7 years ago

@blag I was able to reproduce the issue using the test_app, I have only changed the Book model to use horizontal = True:

diff --git a/test_app/models.py b/test_app/models.py
index a6900af..9d2abff 100644
--- a/test_app/models.py
+++ b/test_app/models.py
@@ -70,6 +70,7 @@ class Book(models.Model):
         Writer,
         chained_field="publication",
         chained_model_field="publications",
+        horizontal=True,
         )
     name = models.CharField(max_length=255)

I think the problem is that admin/js/SelectFilter2.js overwrite the class ".chained" that is used for initializing the element in bindfields.js I have made a patch and open the PR #223

blag commented 7 years ago

@erozqba Thanks for the PRs, do you mind testing the master branch now that it has your merges?

erozqba commented 7 years ago

@blag Thanks for your quick response I will test it right a way.

erozqba commented 7 years ago

@blag I have tested the master branch and it works for me now.

jbnance commented 6 years ago

@erozqba I'm running the current master branch with Django 1.11.6 and this issue appears to have cropped back up. I'd be more than happy to help troubleshoot this if you have any time to work on the project.

erozqba commented 6 years ago

@jbnance I would be happy to help you fix this if the bug is present again. Could you give more information, some example of your setup, or maybe a working example app with the bug?

jbnance commented 6 years ago

@erozqba Thank you very much for your time. This issue occurs in the provided test app (using the Publication, Writer, and Book models).

It looks like there are actually two separate issues. I'm more than happy to open up new items if you'd like.

Basically, without horizontal=True the JavaScript console shows:

Uncaught ReferenceError: SelectBox is not defined
    at Object.fill_field (chainedm2m.js:48)
    at Object.init (chainedm2m.js:132)
    at initItem (bindfields.js:14)
    at HTMLSelectElement.<anonymous> (bindfields.js:20)
    at Function.each (jquery.js:365)
    at bindfields.js:19
    at dispatch (jquery.js:4737)
    at elemData.handle (jquery.js:4549)

chainedm2m.js around line 48 is:

                // SelectBox is a global var from djangojs "admin/js/SelectBox.js"
                // Clear cache to avoid the elements duplication
                if (typeof SelectBox.cache[cache_to] !== 'undefined') {
                    SelectBox.cache[cache_to].splice(0);
                }
                if (typeof SelectBox.cache[cache_from] !== 'undefined') {
                    SelectBox.cache[cache_from].splice(0);
                }

SelectBox.js isn't loaded when horizontal=True is omitted. If you have a non-chained many to many in the model SelectBox.js is loaded and all is well.

With horizontal=True in the ChainedMtoM field things work fine unless you have a non-chained MtoM using filter_horizontal. The order of the JavaScript files changes from this:

<script type="text/javascript" src="/test_app/static/smart-selects/admin/js/chainedm2m.js"></script>
<script type="text/javascript" src="/test_app/static/smart-selects/admin/js/bindfields.js"></script>
<script type="text/javascript" src="/test_app/static/admin/js/SelectBox.js"></script>
<script type="text/javascript" src="/test_app/static/admin/js/SelectFilter2.js"></script>

(which works) ...to this:

<script type="text/javascript" src="/test_app/static/admin/js/SelectBox.js"></script>
<script type="text/javascript" src="/test_app/static/admin/js/SelectFilter2.js"></script>
<script type="text/javascript" src="/test_app/static/smart-selects/admin/js/chainedm2m.js"></script>
<script type="text/javascript" src="/test_app/static/smart-selects/admin/js/bindfields.js"></script>

(which doesn't work)

No messages appear in the JavaScript console.

Thanks for writing such a useful app.

jbnance commented 6 years ago

In the second example I should have included that the order of the fields matters. If the non-chained MtoM comes before the chained MtoM, stuff breaks. If the chained MtoM is first, stuff works. It seems to be tied to the load order of the JavaScript files.

erozqba commented 6 years ago

Hi @jbnance, Thanks a lot for your detailed information, I'm not the maintainer of this app so the thanks go to @digi604, @blag and all the others contributors.

I have reproduced successfully the two bugs that you have pointed. For the first one, I just open another PR the #239 in order to fix this.

For the second issue, looks like a bug in django itself and not in "django-smart-selects", the bug is already reported https://code.djangoproject.com/ticket/28377 and fixed for django 2. https://github.com/django/django/commit/c19b56f633e172b3c02094cbe12d28865ee57772 So my suggestion for django 1.11 is to make sure that the chained MtoM always come first in your Model fields definitions so the media javascript files are loaded in the "working" order.

jbnance commented 6 years ago

@erozqba Thanks for your work on this and the reference to the Django issue. I have applied the pull locally, ran a collectstatic, cleared browser cache, and tested.

Without the non-chained fields in filter_horizontal and with horizontal=False on the chained fields there are no console errors and everything works as expected.

Without the non-chained fields in filter_horizontal and with horizontal=True on the chained fields the first chained field works as expected (in my case this field's chained_field is a foreign key), but subsequent chained fields (whose chained_fields are chained MtoMs) do not populate (they do populate in the previous test). If I remove horizontal=True from any of the chained_fields (the parents) the children work as expected.

With the non-chained fields in filter_horizontal and with horizontal=False on the chained fields there are no console errors and everything works as expected.

With the non-chained fields in filter_horizontal and with horizontal=False on the chained fields the Django media ordering bug is hit.

erozqba commented 6 years ago

@jbnance You could give me an example of modifications to make in the models used in the test_app to reproduce your problems? There are many possible combinations, and an example of code will make easier to test/reproduce/fix the issues.

jbnance commented 6 years ago

@erozqba I'll put a full example together mid next week. Thanks again for your help thus far.

jbnance commented 6 years ago

@erozqba here are the modifications to test app that demonstrate the second scenario (no filter_horizontal and horizontal=True).

Make the following modifications to models.py:

# Imagine that illustrators only work with specific writers
class Illustrator(models.Model):
    name = models.CharField(max_length=255)
    writers = models.ManyToManyField('Writer', blank=True)

    def __str__(self):
        return "%s" % self.name

class Book(models.Model):
    publication = models.ForeignKey(Publication)
    writers = ChainedManyToManyField(
        Writer,
        chained_field="publication",
        chained_model_field="publications",
        horizontal=True,
    )
    illustrators = ChainedManyToManyField(
        Illustrator,
        chained_field="writers",
        chained_model_field="writers",
        horizontal=True,
    )
    name = models.CharField(max_length=255)

And then add some test data.

When you select a publication the available writers panel will populate. However, when you select a writer the illustrators panel will not populate.

If you comment out or set horizontal=False all will populate as expected.

erozqba commented 6 years ago

@jbnance I have tested your changes and reproduced the bug. I have not found a solution yet, the main problem is the collision between chainedm2m.js in django-smart-select and SelectBox.js in django-admin. I think the problems is that even if in chainedm2m.js the events and handlers are initialized, the DOM is changed by SelectBox.js and the new elements are not bonded anymore. I will try to look into this when I have some more free time. Thanks for finding and reporting this bug!

manelclos commented 3 years ago

Please test with latest release and report back if this is still an issue, thanks!