Code

Opened 2 years ago

Closed 18 months ago

#17856 closed New feature (fixed)

Pass "obj" parameter to get_inline_instances

Reported by: ybon Owned by: sjaensch
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 2 years ago.
Adding "obj" parameter to get_inline_instances method in contrib.admin
ticket17856.diff (1.7 KB) - added by quinode 23 months ago.

Download all attachments as: .zip

Change History (21)

Changed 2 years ago by ybon

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

comment:1 Changed 2 years ago by kmike

  • Cc kmike84@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 2 years ago by kmike

  • Needs tests set

comment:3 Changed 2 years ago by aaugustin

  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Design 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 2 years ago by julien

  • Easy pickings set
  • Needs documentation set
  • Severity changed from Release blocker to Normal
  • Triage Stage changed from Design decision needed to Accepted
  • Type changed from Uncategorized to New 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 2 years ago by Twidi

  • Cc Twidi added

comment:6 Changed 2 years ago by quinode

  • Cc dguardiola@… added

What can be done to see this happen ?

comment:7 Changed 2 years ago by aaugustin

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

comment:8 Changed 2 years ago by mat

  • Cc django@… added

comment:9 Changed 2 years ago by quinode

@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 23 months ago by quinode

comment:10 Changed 23 months ago by quinode

  • Needs tests unset
  • Version changed from 1.4-beta-1 to 1.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 23 months ago by quinode (previous) (diff)

comment:11 Changed 22 months ago by aaugustin

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

comment:12 Changed 22 months ago by sjaensch

  • 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 22 months ago by sjaensch

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 21 months ago by quinode

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 21 months ago by aaugustin

Pull requests are a accepted as well as patches.

comment:16 Changed 21 months ago by sjaensch

  • Owner changed from nobody to sjaensch

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 21 months ago by quinode

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 21 months ago by sjaensch

  • Triage Stage changed from Accepted to Ready for checkin

Done.

comment:19 Changed 18 months ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from new to closed

In c2e19e26bc33d34eff57079bd1a6838ff64d9e81:

Fixed #17856 -- Passed obj to get_inline_instances

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.