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

Improve consistency of ModelAdmin's view initialisation methods #3626

Closed

Conversation

ababic
Copy link
Contributor

@ababic ababic commented Jun 1, 2017

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.

  • Do the tests still pass? 👍
  • Does the code comply with the style guide? 👍
  • For Python changes: Have you added tests to cover the new/fixed behaviour? N/A
  • For new features: Has the documentation been updated accordingly? N/A

Andy Babic added 7 commits June 1, 2017 18:24
…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
@ababic ababic changed the title Improve consistency of ModelAdmin view initialisation methods Improve consistency of ModelAdmin's view initialisation methods Jun 1, 2017
…nts, adjust the signature on InstanceSpecificView and WMABaseView to accept and pass them through to TemplateView
@ababic
Copy link
Contributor Author

ababic commented Jun 7, 2017

Hi @gasman. I've had some second thoughts about using SingleObjectMixin here. We're really not gaining too much from it in the way of functionality, and the mixing of instance and object terminology on the same class bothers me. If we're going to be making a shift to using objectmore consistently a later date, we can likely apply that here as part of rebasing modeladmin's views on the views in wagtail/wagtailadmin/views/generic.py

- 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
@gasman
Copy link
Collaborator

gasman commented Jun 9, 2017

Hi @ababic,
Really sorry to bounce this back again, but with the SingleObjectMixin change being reverted I'm not able to decipher what the net effect of this PR is meant to be. If you're making changes A, B, C and D and then reverting B, then I need to be able to look at changes A, C and D independently of B.

I've tried to do that by cherry-picking the non-SingleObjectMixin-related changes (which I think are 9eda12c, a27ba7e, 2fed250) onto a new branch https://github.com/gasman/wagtail/commits/cleanup/modeladmin-view-init-consistency, but the tests are now failing, so it looks like these changes are too intertwined for that to work. Please could you create a new PR consisting of just the cleanup steps that remain once SingleObjectMixin is off the table?

(On the subject of instance vs object, you may want to take a look at #3625 which refactors wagtail/wagtailadmin/views/generic.py to use Django's higher-level CBVs, and standardises on object along the way. Not sure if that has a bearing on the direction you want to take things here...?)

@gasman gasman closed this Jun 9, 2017
@ababic
Copy link
Contributor Author

ababic commented Jun 9, 2017

@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 object instead of instance would be a good move. Either way, I'll deal with that separately once you guys are happy with that part of wagtailadmin

@ababic ababic deleted the modeladmin/view-init-consistency-2 branch June 13, 2017 20:06
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.

Improve consistency of how views in ModelAdmin are initialised
2 participants