Avoid double values when using horizontal=True

[leibowitz]

For some reasons, when using horizontal=True, the change event is triggered twice, and so is the API request to retrieve the values.

The result is that the _from SelectBox is filled with twice the same values.

Moving the reset of the SelectBox cache from within the API request .then() method works around the issue.

Not entirely sure if it needed to be outside for anything. Please test before merging

[coveralls]

Coverage increased (+7.8%) to 97.576% when pulling 57c3090ea1f6e8511b4311d2a8430191de18f936 on leibowitz:fix_double_values into ca55e0bc8ad378dc7a3d1bb5e68e18e8fc51c546 on digi604:master.

[manelclos]

@leibowitz is this happening with the latest master version?

[leibowitz]

Pretty sure since this part of the code hasn't been changed since this PR was created. I will make a test repo to illustrate when I get the time

[leibowitz]

It is still happening indeed, and to reproduce you simply have to use an inline.

In the test app, if you have group/persons/talents data setup, try adding a membership via groups > add > add another membership, and simply select a person.

Illustrated below:

[leibowitz]

The main issue is not really the duplication of values, but more the double initialisation of the select. This seems to be due to the inline being initialised first with __prefix__, and then again when the formset:added is triggered. The first one shouldn't happen, whereas the second is the correct and expected behaviour.

I can try to create another PR to fix the underlying issue. This one is more like a workaround to avoid the select filling up with duplicate values in the meantime

[leibowitz]

Also worth noting the issue with formset:added not being listened to when using the provided jQuery (2.2.0). To be able to work with inline, currently one has to use django provided jquery by adding these to their settings:

JQUERY_URL = None
USE_DJANGO_JQUERY = True

Don't forget to do this if you want to try it out

[manelclos]

@leibowitz I'm trying to reproduce this without success. Using current master, on the test-app, both Group and Book models in the admin, which are using horizontal filter, just make one single request. I tested USE_DJANGO_JQUERY=False which loads jQuery 2.2.0 from google apis, and USE_DJANGO_JQUERY=True which loads jQuery 3.4.1 included with Django.

Can you reproduce using the test-app?

Thanks!

[leibowitz]

Have you used both?

JQUERY_URL = None
USE_DJANGO_JQUERY = True

https://github.com/jazzband/django-smart-selects/pull/274#issuecomment-620903473 https://github.com/jazzband/django-smart-selects/pull/301#issuecomment-620761048

[leibowitz]

I tried with django 3.0.9 (latest as far as I can tell) https://docs.djangoproject.com/en/3.0/releases/3.0.9/

And some older versions as well, and I cannot reproduce either... not sure how to explain this. This issue was a real issue I was having for a while, which this workaround prevented. But if we can't reproduce I'm gonna have a hard time convincing you to merge it :D

[manelclos]

No worries! Reopen if you hit the problem again. Thanks!!

[leibowitz]

Hi @manelclos – would you consider re-opening/merging this PR?

I still have the issue with the latest changes (master), and would love to get this merged because it would avoid having to maintain a fork just for this simple change

Obviously there's probably more use cases out there than mine, so I would appreciate if this can be merged as soon as most general use cases are not impacted negatively – and just to be clear, I don't think they are

I don't see any breaking side-effects to moving the code, and it seems to solve this issue. In summary, looks pretty safe to merge imho (I have been using this change in production since 2018 BTW)

Let me know what you think

[manelclos]

Sure thing. Will test as per your instructions above.

[codecov[bot]]

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@ca55e0b). Click here to learn what that means. The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #274   +/-   ##
=========================================
  Coverage          ?   80.42%           
=========================================
  Files             ?        8           
  Lines             ?      470           
  Branches          ?       68           
=========================================
  Hits              ?      378           
  Misses            ?       60           
  Partials          ?       32           

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ca55e0b...57c3090. Read the comment docs.

[manelclos]

Just tested this, I only get one request when changing Person and the Talents list is populated.

[leibowitz]

Thanks for looking into it again. We have discussed this before, I couldn't get the repro to do the same as my personal project. That must be because of incompatibilities between different django admin extensions. I use django-nested-admin, which has to trigger the formset:added and formset:removed events (like django admin normally does).

For some reason this extension has the whole formset created on the page, and duplicate them every time a user press on "add another". This procedure replaces all the __prefix__ present in the id string to an actual number (the index, starting at 0)

This works fine for any formset created by django but not for those created by django-nested-inline. The main issue I can see is the onload method in django-smart-selects not filtering the elements with id including __prefix__.

This PR was a workaround to avoid the multiple requests populating the list of available values more than once.

I actually created another PR with the proper fix (exclude any elements where id includes __prefix__): https://github.com/jazzband/django-smart-selects/pull/332

Therefore I would suggest looking at #332 instead. It shouldn't break anything else (famous last words)