-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Improve consistency of ModelAdmin's view initialisation methods #3626
Improve consistency of ModelAdmin's view initialisation methods #3626
Conversation
…accepting kwargs - Call super().__init__() in WMABaseView to maintain the __init__ behaviour of the View class (see: https://github.com/django/django/blob/master/django/views/generic/base.py#L36)
- Remove the code from __init__ for identifying the instance, and raise a warning there in `instance_pk` is provided - Because instance-specific views commonly look for `self.instance` (previously populated on __init__), a cached property method with that name is added to return the same value - Because the pk of the instance can be passed in 2 different ways, get_object() needs to be overridden to put the value somewhere where SingleObjectMixin’s get_object method will pick it up and identify the correct instance (the pk is also ‘unquoted’ as it was before) - Because instance specific views commonly look for `self.pk_quoted` (previously populated on __init__), a property method with that name is added to return the same value - Breaks EditView, but will address that in next commit
…s[‘instance_pk’] isn’t set
…admin’s page edit view
…nts, adjust the signature on InstanceSpecificView and WMABaseView to accept and pass them through to TemplateView
Hi @gasman. I've had some second thoughts about using |
- Rename InstanceSpecific’s ‘get_object’ method to ‘get_model_instance’ and have it do the work that SingleObjectMixin was doing (using ‘get_instance’ would be preferable, but that causes a RuntimeError in EditView’s get_instance method) - Have InstanceSpecific’s add ‘instance’ to the context
Hi @ababic, I've tried to do that by cherry-picking the non- (On the subject of |
@gasman Okay. Not to worry. I can do that. I have been keeping track of #3626 - but I was waiting to see what the feedback was there RE renaming. I'm in agreement that a shift to |
Fixes wagtail-nest/wagtail-modeladmin#12. Partly replaces PR #3542
@gasman I have tried to do the very least here to achieve what I wanted with the init functions. If these changes can be reviewed, I'll work on making the other consistency updates in another PR at a later date.