-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Fix: Modeladmin doesn't discover proxy models #4952
Fix: Modeladmin doesn't discover proxy models #4952
Conversation
…ieve the type for the current model, and using that in get_all_model_permissions() instead to query for Permission objects (simplifying the query in the process)
@ababic this works well, thanks. I did encounter some unexpected behavior, which is perhaps what you are referring to by "there's more work to be done in supporting proxy models". Specifically, given the test example you provide here, with Author and LegendaryAuthor, and ModelAdmins defined for both:
Granted, the use case of having ModelAdmins for both models could be rare. If there's only a single ModelAdmin defined for the proxy model, everything works fine, although there's an unexpected "Can view author" permission option in the Groups screen that doesn't seem to do anything. A bigger problem comes if there's only a single ModelAdmin defined for the base model. The base model appears in the sidebar, but add/change/delete permissions don't seem to work (and again, there's an unexpected "Can view legendary author" permission option that doesn't do anything). This all can perhaps be summed up as: if you want to create a proxy model on a model that's being exposed via ModelAdmin, the ModelAdmin has to be defined for the proxy model, and not the base model. Does this match with your expectations? I'm not that familiar with Django ModelAdmin and if it has any similar constraints with proxy models. If you think it makes sense it might be good to add a note in the ModelAdmin documentation about use of proxy models if it seems likely that users might encounter these issues. |
Hi @chosak. Thanks so much for taking the time to review this, and for concise feedback. A lot of these peculiarities are expected, but there are a couple of things that are not. Specifically these two...
I'll investigate these and see if anything can be done without too much scope creep, but my gut feeling is that separate issues/PRs will be required. I'm a touch short on time right now, so sorry for not addressing all of your points here. I will find the time to reply more thoroughly soon. Thanks again :) |
Hi @chosak, I wanted to start by outlining some of the current behaviour pertaining to proxy models, which I think will aid in the review of these changes: You may or may not know this, but since proxy models became a thing in Django, Django hasn't ever created a new With the above in mind, If you register a
AND any proxy models that subclass it, e.g:
This might not be apparent if you're using the Group permission UI to gauge which permissions are registered, because that view itself has some behaviour that prevents most of those permissions from appearing (with the exception of the 'Can view' permissions for each model - which Django started adding from version 2.1) - which should probably be the subject of a separate issue / PR. If you register a With all this in mind, I guess the ideal scenario is this:
This first one seems achievable, and is perhaps something this PR should cover to help reduce potential confusion? Though, the only way I can think to do this currently would be to filter by The second one is difficult for a couple of reasons:
What are your thoughts? |
@chosak I think a lot of the very weird behaviour you reported is actually more to do with what's happening in the Group permissions UI/view. I think it could be unexpectedly showing you a set of checkboxes for |
Hi @chosak. I've come to realise there are two ways to resolve this problem... One that aims to support Django's default proxy model/ContentType/Permission setup (the approach taken by this PR). And another that focuses on ensuring proxy models have their own unique Personally, I think the latter is the better approach, which would make this PR redundant. However, I'll await feedback on that issue before closing this. |
@ababic thank you very much for the detailed explanation! You've taught me a thing or two about proxy models. My apologies for the delay in responding (holidays). That 10-year old Django ticket has quite an interesting history. I agree with your definition of the ideal scenario, and also with the pain that would be required to implement it, given how Django proxy models don't have their own
I think you're right, based on this code that creates a set of permissions based on content type. I suppose this could be improved to somehow distinguish proxy models, but you'd have to use some kind of codename-based logic (which also wouldn't properly handle custom permissions). It feels like this would just be better fixed if Django gave proxy models their own I agree that a |
No worries :) It took doing this work to really get my head around everything, so it was all useful. |
Am closing this, as the solution relies on Django behaviour that will be changing in 2.2. |
Resolves wagtail-nest/wagtail-modeladmin#16.
PermissionHelper.get_all_model_permissions() currently only works for concrete models, because the cross-table query created by the method presumes the model has its own content type. Although there's more work to be done in supporting proxy models more fully throughout Wagtail, there's no reason why
PermissionHelper
should get in the way here.The fix utilises the
ContentTypeManager
'sget_for_model()
method to fetch aContentType
object for the model - which, when provided with a proxy model, returns the type for the 'concrete' model instead.Not only does this fix the issue, but it's also more consistent with how content types are used throughout Wagtail. It also simplifies the query made by
get_all_model_permissions()
slightly, and benefits from theContentTypeManager
's built-in cache.