Code

Opened 5 years ago

Closed 21 months ago

#10057 closed Bug (fixed)

ModelAdmin.render_change_form should not override the 'has_delete' option

Reported by: rajeesh Owned by: nobody
Component: contrib.admin Version: master
Severity: Normal Keywords: ModelAdmin render_change_form
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: yes

Description (last modified by ramiro)

The methods, add_view and change_view in class ModelAdmin in options.py, call another method, render_change_form, to display the add and edit forms respectively. The add_view passes it a context dictionary which includes a 'has_delete' option which in turn makes the 'Delete' button invisible from the add form. When I wanted to make the Delete button invisible from the edit form also (based on specific conditions), I hoped a similar approach in change_view, that is, including 'has_delete' option may help the cause. But render_change_form seems to ignore that option explained below:

def render_change_form(self, request, context, add=False, change=False, form_url='', obj=None): 
    #few lines here
    context.update({
        #few_other_options_here
        ################################################################
        'has_delete_permission': self.has_delete_permission(request, obj)
        ################################################################
        # The above line is what overrides the has_delete option passed!
   #more code here

Changing the above mentioned line to

'has_delete_permission': context.get('show_delete',True) and \
                        self.has_delete_permission(request, obj) 

may solve the problem. By this way, the developer can decide when to make the Delete button visible just by passing an extra context option, 'has_delete' to the change_view.

Looking forward to alternative suggestions and comments

Attachments (2)

show_delete.patch (872 bytes) - added by patcoll 5 years ago.
10057.diff (5.6 KB) - added by dgouldin 21 months ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 5 years ago by ramiro

  • Description modified (diff)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

(edited description to make it legible, next time please use the Preview button)

comment:2 Changed 5 years ago by anonymous

Well, what happened to this?

comment:3 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:4 Changed 5 years ago by jacob

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

A better approach would be to just make a custom template for the model in question; see http://docs.djangoproject.com/en/dev/ref/contrib/admin/#overriding-admin-templates

comment:5 Changed 5 years ago by patcoll

  • Resolution wontfix deleted
  • Status changed from closed to reopened

Just ran into the same problem. The logic that produces a True or False for show_delete_link in django.contrib.admin.templatetags.admin_modify seems a bit flawed because the value of show_delete in the passed context is essentially always ignored because the changed boolean is hard-coded to True by the admin system if you are viewing the change_view` with a bound Form.

I've attached a patch that ensures that show_delete is checked if it exists, otherwise it always displays the delete link.

Changed 5 years ago by patcoll

comment:6 Changed 4 years ago by SmileyChris

  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

From a quick look at the logic changes in the attached patch, it looks like a valid issue.

comment:7 Changed 3 years ago by SmileyChris

  • Severity set to Normal
  • Type set to Bug

comment:8 Changed 3 years ago by julien

  • UI/UX set

comment:9 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:10 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:11 Changed 2 years ago by aaugustin

  • UI/UX set

Revert accidental batch modification.

comment:12 Changed 21 months ago by dgouldin

  • Needs tests unset

Updated patcoll's patch to current trunk and added a test.

Changed 21 months ago by dgouldin

comment:13 Changed 21 months ago by Julien Phalip <jphalip@…>

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

In [01c392623d988d7486bdaa870886df0ea3da5fa7]:

Fixed #10057 -- Ensured that the 'show_delete' context variable in the admin's change view actually controls the display of the delete button. Thanks to rajeesh for the report, to patcoll for the patch, and to David Gouldin for the test.

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.