Code

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#12962 closed (fixed)

Broken Admin Delete Action With Confirmation

Reported by: leveille Owned by: mlavin
Component: contrib.admin Version: 1.2-beta
Severity: Keywords: admin delete
Cc: preston@…, adam.skevy@…, i@…, drmeers@…, aarond10@…, Maniac@…, jedie, aaloy, sir.Gollum@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: UI/UX:

Description

After updating to head this morning I noticed that I was unable to delete items in the administrator via the delete dropdown action (and confirmation). I was able to track the issue back to [12525]. Specifically, the introduced check looks for 'index', which will not be present when coming from the delete confirmation page:

pprint of request.POST in response_action after clicking GO

<QueryDict: {u'action': [u'delete_selected'], u'select_across': [u'0'], u'csrfmiddlewaretoken': [u'9e1ca9bc4deb4f7750b1ade512afdf8d'], u'_selected_action': [u'12'], u'index': [u'0']}>

pprint of request.POST in response_action after confirming I want to delete the record

<QueryDict: {u'action': [u'delete_selected'], u'_selected_action': [u'12'], u'csrfmiddlewaretoken': [u'9e1ca9bc4deb4f7750b1ade512afdf8d'], u'post': [u'yes']}>

To be sure my code wasn't introducing any issues, I testing with a fresh django project/app and the bug was still present.

Attachments (3)

modeladmin-12595.diff (1.4 KB) - added by ptone 4 years ago.
with_test_change-12812.diff (2.0 KB) - added by mlavin 4 years ago.
ptone's patch with updated test
modeladmin-12962.diff (3.5 KB) - added by blinkylights 4 years ago.
Ambiguous form actions

Download all attachments as: .zip

Change History (25)

comment:1 Changed 4 years ago by jezdez

  • milestone set to 1.2
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 4 years ago by kmtracey

#12973 reported this again and has a patch.

Changed 4 years ago by ptone

comment:3 Changed 4 years ago by ptone

  • Cc preston@… added
  • Has patch set
  • Needs tests set

Moving my patch from #12973 to the open ticket for this issue

comment:4 Changed 4 years ago by skevy

  • Cc adam.skevy@… added

I've done a bit more research into this ticket.

The patch does indeed work, and all currently written tests (including those written in changeset 12525) pass correctly.

However, what I found interesting was that in the admin_view test suite, in the following test:

def test_model_admin_default_delete_action(self):
        "Tests the default delete action defined as a ModelAdmin method"
        action_data = {
            ACTION_CHECKBOX_NAME: [1, 2],
            'action' : 'delete_selected',
            'index': 0,
        }
        delete_confirmation_data = {
            ACTION_CHECKBOX_NAME: [1, 2],
            'action' : 'delete_selected',
            'index': 0,
            'post': 'yes',
        }
        confirmation = self.client.post('/test_admin/admin/admin_views/subscriber/', action_data)
        self.assertContains(confirmation, "Are you sure you want to delete the selected subscriber objects")
        self.failUnless(confirmation.content.count(ACTION_CHECKBOX_NAME) == 2)
        response = self.client.post('/test_admin/admin/admin_views/subscriber/', delete_confirmation_data)
        self.failUnlessEqual(Subscriber.objects.count(), 0)

Notice that it sets 'index' equal to 0 in delete_confirmation_data. This is not what happens for real. In fact, when you confirm you want to delete objects, 'index' is never set in the post data. The fact that 'index' is set to 0 in this test is what caused this test to pass way back when 12525 actually happened.

So, I don't know if index SHOULD be set in the post data when the objects are actually deleted, or if the test is incorrect. Some more info on how to proceed from someone who knows WAY more about this code than me would be awesome :-)

Thanks,
-Adam

comment:5 Changed 4 years ago by skevy

  • Version changed from SVN to 1.2-beta

comment:6 Changed 4 years ago by blinkylights

  • Patch needs improvement set

With the patch applied, deleting is working. However, we still have a few issues:

  1. If you hit 'Go' while an action is selected and no list_editable changes are made, the action is applied but no, "# items have changed" message is sent.
  2. If you hit 'Go' while an action is selected, and list_editable changes have been made, both changes are applied (not what we want)
  3. If you hit the list_editable save button with items selected and an action chosen, you get the, "No action selected" message.

I'm working on an additional patch.

comment:7 Changed 4 years ago by skevy

@blinkylights

does this patch cause this error? or was the error already there before the patch is applied?

comment:8 Changed 4 years ago by blinkylights

@skevy

Oh, no... there was definitely a problem before the patch. The patch makes how we're differentiating between action POSTs and list_editable POSTs more explicit (and it works). In doing so, it also reveals why we actually need to be even more explicit to cover cases where users might intend to launch an action, but do a list_editable submit, or vice-versa. So no: the patch is fine, it just needs to do a little more.

Think I have something that works, but you're absolutely right - we need to get some up-to-date tests written so this doesn't come back up.

comment:9 Changed 4 years ago by azf

  • Cc i@… added

comment:10 follow-up: Changed 4 years ago by DrMeers

  • Cc drmeers@… added

comment:11 in reply to: ↑ 10 Changed 4 years ago by aarond10

  • Cc aarond10@… added

comment:12 Changed 4 years ago by isagalaev

  • Cc Maniac@… added

comment:13 Changed 4 years ago by jedie

  • Cc jedie added

comment:14 Changed 4 years ago by aaloy

  • Cc aaloy added

I can also reproduce the bug. It affects any model, even the most basic ones (User).

comment:15 Changed 4 years ago by sir_Gollum

  • Cc sir.Gollum@… added

comment:16 Changed 4 years ago by mlavin

  • Owner changed from nobody to mlavin

comment:17 Changed 4 years ago by mlavin

Removing 'index' from delete_confirmation_data in test_model_admin_default_delete_action fails on the current revision [12812] and passes with the patch. I started a blank project which included the admin. I created a new user and then selected it to be deleted with the admin action. Inspecting what is actually posted in each step in Firebug I see:

_selected_action:	2
action:	delete_selected
csrfmiddlewaretoken:	ec77fa407235521666739722702efee1
index:	0
select_across:	0

followed by the confirmation:

_selected_action:	2
action:	delete_selected
csrfmiddlewaretoken:	ec77fa407235521666739722702efee1
post:	yes

I think this confirms that 'index' is not included in the confirmation post and should be removed from test_model_admin_default_delete_action.

Changed 4 years ago by mlavin

ptone's patch with updated test

comment:18 Changed 4 years ago by kmtracey

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

(In [12813]) Fixed #12962: Made admin delete action work again. Thanks ptone, skevy, mlavin and anyone else I've missed.

comment:19 Changed 4 years ago by kmtracey

(In [12815]) [1.1.X] Fixed #12707. Admin action messages are no longer displayed when submitting list_editable content. Thanks, copelco.

and

Fixed #12962: Made admin delete action work again. Thanks ptone, skevy, mlavin and anyone else I've missed.

r12525 and r12813 from trunk, together to avoid the regression introduced by r12525 alone.

comment:20 Changed 4 years ago by blinkylights

  • Resolution fixed deleted
  • Status changed from closed to reopened

I have another patch that covers cases where the user might change list_editable elements and hit the action 'Go' button, or select an action to execute and hit 'Save' (which will be there for 'list_editable' fields). In our current state BOTH interactions are handled in the same form, and both sets of changes get handled - an ambiguous and confusing situation that this patch fixes.

Changed 4 years ago by blinkylights

Ambiguous form actions

comment:21 Changed 4 years ago by blinkylights

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

Closing this ticket and opening #13166 to handle action/list_editable ambiguity.

comment:22 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

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.