Opened 5 years ago

Closed 4 years ago

#17856 closed New feature (fixed)

Pass "obj" parameter to get_inline_instances

Reported by: ybon Owned by: Stephan Jaensch
Component: contrib.admin Version: 1.4
Severity: Normal Keywords:
Cc: kmike84@…, Twidi, dguardiola@…, django@…, sj@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

In the 1.4 rc1 release, a get_inline_instances method has been added in [source:/django/trunk/django/contrib/admin/options.py#L348 contrib.admin].

This method does not accept the "obj" parameter.

This is not consistent with the other methods of ModelAdmin:

  • get_formset
  • get_form
  • get_fieldsets
  • get_prepopulated_fields
  • get_readonly_fields

My opinion is that the faster we make this change, the less painful it will be in the future.

A patch is provided.

(Discussion in this thread.)

Attachments (2)

adding_obj_parameter_to_get_inline_instances.diff (1.4 KB) - added by ybon 5 years ago.
Adding "obj" parameter to get_inline_instances method in contrib.admin
ticket17856.diff (1.7 KB) - added by domguard 5 years ago.

Download all attachments as: .zip

Change History (21)

Changed 5 years ago by ybon

Adding "obj" parameter to get_inline_instances method in contrib.admin

comment:1 Changed 5 years ago by Mikhail Korobov

Cc: kmike84@… added

comment:2 Changed 5 years ago by Mikhail Korobov

Needs tests: set

comment:3 Changed 5 years ago by Aymeric Augustin

Severity: NormalRelease blocker
Triage Stage: UnreviewedDesign decision needed

First, get_inline_instances wasn't introduced as a new API, but to fix a permission-checking bug on inlines. See r16934. Viewing it a a hook for admin customization is interesting :)

Currently, it isn't documented, which makes it an internal API. And we don't commit to keeping backwards compatibility for internal APIs. If you use or override them and they change in a later version of Django, you're on your own -- and in this case it will be trivial to fix. That means we could make that change after 1.4.

Alternatively, if we decide it's a public of "semi-public" API, and try not to break backwards compatibility, we need the API to be correct and consistent with surrounding code before it's set in stone. If we're choosing this path, this bug is a release blocker.

I'm marking this as a release blocker because we must decide between (a) accepting to change the API after 1.4 (b) fix it now. If we choose (a) then it's a normal ticket, if we choose (b) then it's a release blocker.

Given this situation, the patch being as harmless as they come, and the convincing (IMO) arguments of Yohan on the mailing list, I'm seriously tempted to make the change.

comment:4 Changed 5 years ago by Julien Phalip

Easy pickings: set
Needs documentation: set
Severity: Release blockerNormal
Triage Stage: Design decision neededAccepted
Type: UncategorizedNew feature

It's a shame that this feature is missing, however it is too late to be considered for 1.4. The admin doesn't really have a strict backwards-compatibility policy, so we can consider fixing this in 1.5.

comment:5 Changed 5 years ago by Twidi

Cc: Twidi added

comment:6 Changed 5 years ago by domguard

Cc: dguardiola@… added

What can be done to see this happen ?

comment:7 Changed 5 years ago by Aymeric Augustin

The idea is accepted, but the current patch lacks tests and docs. You can add them and upload a new patch.

comment:8 Changed 5 years ago by Mathieu Pillard

Cc: django@… added

comment:9 Changed 5 years ago by domguard

@aaugustin : Ok, I'm looking for examples of regression tests on the other related methods mentioned in the ticket, I can't find any...
Apart of passing another object, that will be ignored by past code, this code is quite simple (and works)
Please aaugustin can you just give me an idea of what tests could be made here ?

Changed 5 years ago by domguard

Attachment: ticket17856.diff added

comment:10 Changed 5 years ago by domguard

Needs tests: unset
Version: 1.4-beta-11.4

Updated and tested patch (the add_view case had been omitted)
The patch is really simple : it adds the obj parameter to the method, and passes it in the three only places where the get_inline_instances is called. And this makes it behave just like its cousins ModelAdmin methods.

More can be said about the need of an entry point here.
For example, this is the only clean way I found (https://groups.google.com/forum/?fromgroups#!topic/django-users/l_nsr0_ea0o) to pass the parent object instances to inlines (Inline classes have to be modified to get the arg, but this is another possible enhancement once this one exists)
You need to know which object you're working on when you need to filter an inline form field queryset based on the parent object, a rather common pattern...

Last edited 5 years ago by domguard (previous) (diff)

comment:11 Changed 4 years ago by Aymeric Augustin

#18619 was a duplicate with a pull request: https://github.com/django/django/pull/202

comment:12 Changed 4 years ago by Stephan Jaensch

Cc: sj@… added

As the author of the pull request (and the one that's to blame for the omission in the definition of get_inline_instances()) I can verify that the patch in this bug fixes the issue without introducing unwanted side effects. In fact it's almost identical to my independently written pull request. The method still works when called without the obj parameter and behaves exactly the same as in Django 1.4.

comment:13 Changed 4 years ago by Stephan Jaensch

On second thought: I do actually like my pull request a bit better since it passes the obj parameter along to the permission checking functions instead of just ignoring it.

comment:14 Changed 4 years ago by domguard

Last version of the pull request https://github.com/django/django/pull/202 supersedes the above patch , do we need to create a new patch here, or does github pull requests are now accepted in the django project workflow ?

comment:15 Changed 4 years ago by Aymeric Augustin

Pull requests are a accepted as well as patches.

comment:16 Changed 4 years ago by Stephan Jaensch

Owner: changed from nobody to Stephan Jaensch

Hey quinode, could you (or someone else) review this small patch and mark the ticket as "Ready for checkin" if applicable? It would be really nice to fix this oversight for 1.5.

comment:17 Changed 4 years ago by domguard

Hey @sjaensch I just pushed a documentation update to complete your patch branch
Please merge it and mark this as "Ready for checkin", we'll see what happens :)

comment:18 Changed 4 years ago by Stephan Jaensch

Triage Stage: AcceptedReady for checkin

Done.

comment:19 Changed 4 years ago by Aymeric Augustin <aymeric.augustin@…>

Resolution: fixed
Status: newclosed

In c2e19e26bc33d34eff57079bd1a6838ff64d9e81:

Fixed #17856 -- Passed obj to get_inline_instances

Thanks ybon, quinode and sjaensch for the patch, and Tim Graham
for the review.

Note: See TracTickets for help on using tickets.
Back to Top