WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 10 years ago

#11673 closed defect (bug) (fixed)

Need confirmation/undo/something for media delete

Reported by: rob1n Owned by:
Milestone: 2.9.1 Priority: normal
Severity: normal Version: 2.9
Component: Administration Keywords:
Focuses: Cc:

Description

In other places in the admin UI, there is a fallback for deleting stuff. Whether it's a "trash," a confirmation dialog, or whatever.

However, in the media section, if you hit Delete Permanently, none of the above happen. All it says is "Media permanently deleted."

Whether for consistency or to give the user one last warning, a fallback should probably be added. A confirmation box would be easiest.

Experienced in the latest 2.9 branch, rather certain it affects trunk as well.

Attachments (3)

11673-against-2.9-branch.diff (2.9 KB) - added by nacin 10 years ago.
Quick patch against 2.9 branch.
11673.2.against-2.9.diff (3.0 KB) - added by nacin 10 years ago.
Better patch against 2.9 branch also checks for EMPTY_TRASH_DAYS.
11673.2.against-trunk.diff (1.9 KB) - added by nacin 10 years ago.
Patch against trunk to add EMPTY_TRASH_DAYS check.

Download all attachments as: .zip

Change History (24)

#1 @rob1n
10 years ago

  • Cc robin.adr@… added

#2 @nacin
10 years ago

I thought we added AYS dialogs back in for when trash was disabled? If so, we're also missing a MEDIA_TRASH check...

#3 @azaozz
10 years ago

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

(In [12591]) Add AYS popup when deleting media, fixes #11673

#4 @nacin
10 years ago

  • Milestone changed from Unassigned to 2.9.1

azaozz:

We don't have AYS when trash is disabled elsewhere. I agree since we don't have media trash in core that we need AYS here. Do we want to add AYS back as well when trash is disabled across the board?

Also, I think this should be done in 2.9.1, at least the committed fix, as it is technically a regression.

#5 @nacin
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#6 follow-up: @azaozz
10 years ago

Not sure what the consensus is on having AYS when trash is disabled, seems most people reject it. Added here to restore the functionality since we decided to disable trash for attachments. Would probably have to go in 2.9.1 too as it may be considered a regression (we still have AYS for deleting tags and categories).

#7 in reply to: ↑ 6 @nacin
10 years ago

  • Priority changed from low to high
  • Severity changed from minor to normal
  • Type changed from enhancement to defect (bug)
  • Version set to 2.9

Replying to azaozz:

Not sure what the consensus is on having AYS when trash is disabled, seems most people reject it.

I couldn't find any discussion in #4529 -- I'll check the IRC logs. As trash was to prevent the need for AYS, then my thought is that if someone doesn't want to use trash -- or if a site admin doesn't want users to use trash -- then AYS should definitely be enabled. Otherwise, one click (either on accident, or without being sure) and you're destroying content forever. If we allow trash to be disabled, then we shouldn't cripple the fallback delete functionality to something it wasn't before.

Changing to a defect to clarify that for media specifically, this is a regression. I would like to see AYS throughout core when trash is disabled, though. That's something can wait until 3.0 I suppose.

@nacin
10 years ago

Quick patch against 2.9 branch.

#8 follow-up: @nacin
10 years ago

I'm noticing that we need to check for EMPTY_TRASH_DAYS as well here, otherwise if trash is disabled but media trash is enabled, it deletes without an AYS. Could be considered an edge case, but if someone enables MEDIA_TRASH then decides to turn off trash all together, EMPTY_TRASH_DAYS = 0 should override everywhere. (And it does in all MEDIA_TRASH checks except here.)

@nacin
10 years ago

Better patch against 2.9 branch also checks for EMPTY_TRASH_DAYS.

@nacin
10 years ago

Patch against trunk to add EMPTY_TRASH_DAYS check.

#9 @nacin
10 years ago

#11676 for posts, pages, comments.

#10 follow-up: @rob1n
10 years ago

On a side note, the CC feature didn't do jack for me.

#11 @azaozz
10 years ago

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

(In [12593]) Add AYS popup when deleting media, fixes #11673 for 2.9

#12 in reply to: ↑ 8 @azaozz
10 years ago

Replying to nacin:

I'm noticing that we need to check for EMPTY_TRASH_DAYS as well here...

The current functionality is to delete without AYS when trash is disabled, we keep that for attachments as well.

In any case the disabling of trash for attachments was a temporary thing and is expected to be reworked, hopefully for 3.0. Then I think the constant MEDIA_TRASH will be removed.

#13 in reply to: ↑ 10 ; follow-up: @nacin
10 years ago

Replying to azaozz:

The current functionality is to delete without AYS when trash is disabled, we keep that for attachments as well.

I suppose when comparing it to the other screens that use "Delete Permanently," then yes. But as you pointed out, links, tags and cats all have AYS because there is no trash built in (like media, kinda), so it could go either way. The inconsistent experience is outside the scope of this ticket, though.

In any case the disabling of trash for attachments was a temporary thing and is expected to be reworked, hopefully for 3.0. Then I think the constant MEDIA_TRASH will be removed.

Very true. Thanks for patching the 2.9 branch here.

Replying to rob1n:

On a side note, the CC feature didn't do jack for me.

I read somewhere it needs to be an email address, but maybe it's broken? I've never used it, since I get wp-trac emails, I just star the ticket in Gmail.

#14 in reply to: ↑ 13 @westi
10 years ago

Replying to nacin:

Replying to rob1n:

On a side note, the CC feature didn't do jack for me.

I read somewhere it needs to be an email address, but maybe it's broken? I've never used it, since I get wp-trac emails, I just star the ticket in Gmail.

You need to put the email address in your trac profile - http://core.trac.wordpress.org/wiki#Notification

#15 @hakre
10 years ago

Do we have that question in case javascript is disabled as well?

#16 @hakre
10 years ago

  • Milestone changed from 2.9.1 to 2.9.2
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopened because the bug is still in if javascript is disabled. I assume if this was patched (improperly) for 2.9.1 it should be done properly in 2.9.2.

#17 @nacin
10 years ago

Can't say we've ever had AYS when JavaScript was disabled??

#18 @azaozz
10 years ago

Replying to hakre:

Reopened because the bug is still in if javascript is disabled. I assume if this was patched (improperly) for 2.9.1 it should be done properly in 2.9.2.

As @nacin mentions, don't think we ever had AYS when JS is disabled. Do you propose another (mostly empty) page saying "Are you sure" for every click on "delete"? I'm -1 on that. Also see the comment above that trash for attachments needs to be reintroduced back.

This is not a regression or blocker and has no patch, don't think it belongs in a maintenance release.

#19 @azaozz
10 years ago

  • Milestone changed from 2.9.2 to Future Release
  • Priority changed from high to low
  • Severity changed from normal to minor
  • Type changed from defect (bug) to enhancement
  • Version 2.9 deleted

#20 @nacin
10 years ago

azaozz: We should restore this ticket to 2.9.1 / fixed / defect (bug). What hakre proposed belongs in another ticket. (I'd do it, but I can't set a 2.9.1 milestone.)

#21 @azaozz
10 years ago

  • Milestone changed from Future Release to 2.9.1
  • Priority changed from low to normal
  • Resolution set to fixed
  • Severity changed from minor to normal
  • Status changed from reopened to closed
  • Type changed from enhancement to defect (bug)
  • Version set to 2.9

Agreed with @nacin. Please open another (enhancement) ticket if proposing non-Javascript AYS.

Note: See TracTickets for help on using tickets.