Add support for limit_choices_to on related fields. #1843
Conversation
tomchristie
changed the title
| @@ -846,6 +843,9 @@ def get_related_field(self, model_field, related_model, to_many): | |||
| if model_field.help_text is not None: | |||
| kwargs['help_text'] = model_field.help_text | |||
|
|
|||
| if model_field.rel.limit_choices_to: | |||
| kwargs['limit_choices_to'] = model_field.rel.limit_choices_to | |||
There was a problem hiding this comment.
I'm not clear about whether it should be related_model instead of model_field.rel here. Thoughts ?
|
@pipermerriam This PR should be rebased. |
| # GenericForeignKey and their reverse relationships don't have | ||
| # a `field` property. | ||
| pass | ||
|
|
There was a problem hiding this comment.
I'm not sure what this code block is for. self.limit_choices_to is only relevant when queryset is not None so why do we need to set it to anything here?
There was a problem hiding this comment.
In the event that the model field is defined as:
class MyModel(models.Model):
user = models.ForeignKey(User, limit_choices_to={'is_active': True})
it seemed appropriate for DRF to respect that value and filter the default queryset.
Do you think it would be better to only do filtering when limit_choices_to is explicitly declared on the serializer field?
|
This PR is not rebasing easily. Sorry for the long turn-around time. |
|
Not a problem, still on my radar so will get to it myself too if you don't first. Dropping the 3.3.0 milestone, but mostly just because there's no reason it can't be before then. |
|
Thanks @tomchristie . I ended up in rebase hell trying to rebase this. A lot of the surrounding code has changed since it was originally written. I think I'm going to step back from this feature for now as I don't have much time to put towards things. Feel free to take it an run with it if you see fit. |
|
@pipermerriam I would like to take charge of this change if you step back. |
|
It's all yours. My focus is currently elsewhere On Sun, Dec 20, 2015, 12:47 AM Asif Saifuddin Auvi notifications@github.com
|
|
@tomchristie is the method & approach ok? I can fix the conflicts if that's all that's needed. |
|
@mishu- Seems broadly right yup, there'll probably need to be some more thorough review following the conflict being resolved, but the general approach is 👌. |
|
oi, things changed.. as in.. completely changed
…On Mon, Mar 6, 2017 at 12:28 PM, Tom Christie ***@***.***> wrote:
@mishu- <https://github.com/mishu-> Seems broadly right yup, there'll
probably need to be some more thorough review following the conflict being
resolved, but the general approach is 👌.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1843 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAlUZe0szTDyv_8JpHrZ0XpPQy1p4nFUks5ri9_BgaJpZM4CfS78>
.
--
Mihai Oprea
University of Pennsylvania, Class of 2009
School of Engineering and Applied Sciences
Masters of Science and Engineering in Computer and Information Science
Bachelor of Science and Engineering in Computer and Information Science
RO: +40 (72) 000 2704
|
|
yes I also got that while tried to takeover |
|
That's likely, however, with #3605 things may be easier. |
|
Closing this off as stale. |
Refs #1811.
What is the problem / feature ?
Serializer fields representing relationships did not support or respect the
limit_choices_tooption as introduced from django'sForeignKeyfield.limit_choices_to#1811How did it get fixed / implemented ?
limit_choices_totorest_framework.relations.RelatedFieldHow can someone test / see it ?
Test coverage, as well as creating a related field using the
limit_choices_toargument and seeing that the resulting queryset used for the field is correctly filtered.Here is a cute animal picture for your troubles...