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

Bug in M2M Filter #41

Closed
wants to merge 12 commits into from
Closed

Bug in M2M Filter #41

wants to merge 12 commits into from

Conversation

yeago
Copy link

@yeago yeago commented Feb 16, 2011

If for some reason an M2M field only has one option, this line will display no options whatsoever.

This line was originally written to preclude people selecting all options in M2M blindly

@shelldweller
Copy link

I'm not sure about this patch. What if there are two options and people want to select both? Or three options? Or four? I think it would rather make sense to somehow optimize the query in case all options are selected rather then to drop m2m relationships completely.

@yeago
Copy link
Author

yeago commented Feb 28, 2011

That's a separate issue that this line of code was originally intended to solve. All this patch addresses is a smaller but more immediate bug.

@shelldweller
Copy link

It addresses your immediate problem because you have one record in your db, but it doesn't address mine because I happen to have two. IMO the best and the easiest way to address it would be to drop checking the number of rows selected at all. Just trust the user. If he wants to select all related records so be it!

@yeago
Copy link
Author

yeago commented Feb 28, 2011

At this point I'm open to that as well. We had originally intended to have a way to stop cases where users were selecting all as a means of expressing that they don't want anything excluded but this solution proved to be too simplistic.

@shelldweller
Copy link

I see. From the user perspective it does make sense. I think there are two distinct cases when selecting all m2m records:

  1. If m2m is required it really means "show me everything" and m2m field selection can be safely ignored (current behaviour).
  2. If m2m field is optional then it really means "show all records with relationships and ignore those that don't any defined". This is what your patch does for a special case when the number of m2m relationships is one.

Did I get it right?

@yeago
Copy link
Author

yeago commented Feb 28, 2011

you get it right and I agree but I'd still find my query logs were full of incidents where people would just CTRL+A in the second case.

@apollo13
Copy link
Contributor

Can you update the patch to merge cleanly and add tests?

@nkryptic nkryptic mentioned this pull request Feb 2, 2013
@apollo13
Copy link
Contributor

Closing due to no reaction.

@apollo13 apollo13 closed this Mar 28, 2013
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.

None yet

3 participants