Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for to_field_name to SortedMultipleChoiceField #76

Merged
merged 1 commit into from
May 24, 2016

Conversation

conradev
Copy link
Contributor

The current implementation depends on in_bulk, which requires value to be the pk, when it can actually be any unique field thanks to the to_field_name attribute.

The super implementation handles filtering the queryset, so clean only needs to order the objects. This implementation is partially inspired by how the ModelMultipleChoiceField implements it.

@gregmuellegger
Copy link
Collaborator

Cool that makes sense. Thanks for taking the effort to contribute this.

I see two things missing here before I can merge this:

  • The tests are failing for Python 2.6, could you change the dict comprehension to use the old style dict((key, value) ...) one?
  • There are unittests missing for this feature. It would be nice to have some tests here so that can be sure enough that we don't break the support for to_field_name in the future.

@conradev
Copy link
Contributor Author

I went ahead and made those two changes, let me know if I need to make any more! 👍

@gregmuellegger gregmuellegger merged commit 930548d into jazzband:master May 24, 2016
@gregmuellegger
Copy link
Collaborator

👍 great! Thanks for the contribution, it's merged.

gregmuellegger added a commit that referenced this pull request May 24, 2016
@gregmuellegger
Copy link
Collaborator

I just released 1.3.0, containing your changes: https://pypi.python.org/pypi/django-sortedm2m/1.3.0

@conradev
Copy link
Contributor Author

Awesome, thanks! 🎉

@vqw
Copy link

vqw commented May 29, 2016

@conradev @gregmuellegger
I have the same issue as @czpython in DjangoCMS which drives me crazy. Reverting to previous version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants