WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#11426 closed defect (bug) (fixed)

Allow trash option in comment notification emails

Reported by: nacin Owned by:
Milestone: 2.9 Priority: normal
Severity: normal Version: 2.9
Component: Trash Keywords: has-patch
Focuses: Cc:

Description

wp-admin/comment.php currently doesn't support the ability to trash a comment via the AYS form.

Additionally, wp_notify_postauthor() and wp_notify_moderator() should replace the "Delete it:" links with "Trash it:" links, if trash is enabled.

I do find this important for 2.9 - at the very least, wp-admin/comment.php should gain the ability to trash a post, even if the links are not added to the pluggable functions. That said, it does require up to three new strings...

Working on a patch, any thoughts?

Attachments (9)

11426.diff (3.2 KB) - added by nacin 5 years ago.
First pass…
11426.2.diff (9.6 KB) - added by nacin 5 years ago.
Second pass.
11426.3.diff (10.0 KB) - added by nacin 5 years ago.
Third pass.
11426.alternative.diff (10.5 KB) - added by nacin 5 years ago.
Alternative has no string changes.
11426.alternative.2.diff (10.9 KB) - added by nacin 5 years ago.
Removes duplicated check_admin_referer call.
11426.4.diff (8.3 KB) - added by nacin 5 years ago.
Removes duplicated check_admin_referer call.
11426.4.2.diff (10.5 KB) - added by nacin 5 years ago.
Removes duplicated check_admin_referer call.
11426.alternative.3.diff (11.1 KB) - added by nacin 5 years ago.
Alternative route (no new strings) -- fix for redirect in (un)approvecomment
11426.5.diff (10.7 KB) - added by nacin 5 years ago.
Standard route (4 new strings) -- Fix for redirect

Download all attachments as: .zip

Change History (22)

nacin5 years ago

First pass...

comment:1 scribu5 years ago

  • Keywords has-patch added; 2nd-opinion removed

+1

comment:2 caesarsgrunt5 years ago

Oops, I forgot about the email when implementing Trash.

I notice that in your patch you have used the string "Delete" if Trash is disabled, however in the rest of the interface we use "Delete Permanently".

I don't know if this can be implemented after the string freeze, but the only alternative is really to remove the delete option from the email altogether.

comment:3 caesarsgrunt5 years ago

Actually, we probably don't need an AYS page for Trash; that's part of the point of Trash. That said, we don't use AYS for Approve anywhere else in the interface, but we do use it for the email link...

comment:4 caesarsgrunt5 years ago

action=cdc&dt=trash

We don't do this for trash anywhere else; it is an action of its own.

Actually, Spam is also now its own action in the comments page. I've just noticed that it still uses the old action=delete&dt=spam in the dashboard, which I'd better go and open a new ticket for...

comment:5 nacin5 years ago

In general, I went as simple as possible with the patch to see to a 2.9 commit.

The AYS intention check is necessary because you're coming from e-mail.

I'm not sure about "Delete Permanently." I suppose it can be changed to "Permanently Delete Comment." This patch is already requiring some string changes, so it can't hurt.

mac, cdc, and dt are all artifacts. A few years ago, they stood for moderateapprovecomment, confirmdeletecomment, and delete_type. Realistically we should convert these to action=(approve|delete|trash|spam).

But since mac/cdc/dt still need to be maintained for back compat (particularly since the notify functions are pluggable), I decided against it to keep this simple for 2.9.

I will work up a second patch to clean this up some more. Xref - #11432.

nacin5 years ago

Second pass.

comment:6 nacin5 years ago

Second pass is more ambitious. Does what the ticket is fundamentally for, plus:

  • Changes mac, cdc, dt to approve, delete, spam, trash. Adds back compat check.
  • DRYs up a lot of the comment.php code. (Though it could be taken further.)
  • Adds query args where missing to allow for standardizes messages after an action.

New strings:

 __('Trash it: %s')
 __('You are about to move the following comment to the Trash:');
 __('Trash Comment');
 __('Permanently Delete Comment');

nacin5 years ago

Third pass.

comment:7 nacin5 years ago

Should have read:

  • Adds query args where missing to allow for standardized messages after an action.

Third patch also converts the "No" cancel button from an <input type="button"> with an onclick JavaScript location redirect, to <a class="button"> with an href. Cause that was just driving me crazy.

comment:8 nacin5 years ago

Okay, I thought about an alternative. The following patch has no string changes, but it is less than ideal.

Nutshell:

  • action=trash uses "You are about to delete the following comment:", but then gives you two action buttons: "Move to Trash" or "Delete Permanently".
  • If trash is enabled, then use the action=trash link in the email, which would reveal the two buttons. The string would still be "Delete it: (link)".

nacin5 years ago

Alternative has no string changes.

nacin5 years ago

Removes duplicated check_admin_referer call.

comment:9 caesarsgrunt5 years ago

If we're going to rename the actions used when coming from the email, why not use the same actions that we already use elsewhere? And we could add another parameter to show that we'd come from an email and so needed an AYS - eg &email=true or &confirm=true. Still don't think AYS is needed, but that's a different ticket really and too late for 2.9.

nacin5 years ago

Removes duplicated check_admin_referer call.

nacin5 years ago

Removes duplicated check_admin_referer call.

comment:10 nacin5 years ago

The AYS screen is needed. If it's removed, you'd get tripped up by check_admin_referer and you'd have to answer wp_nonce_ays. Not exactly user-friendly. And removing check_admin_referer, obviously, is backwards in terms of security/intent.

Adding &email=true or &confirm=true is just another way to skin a cat -- the code would largely be the same -- except that it also unnecessarily lengthens the URL by something like 18+ characters.


Speaking of check_admin_referer, I left one in there that needed to be removed. Most recent patches are 11426.alternative.2.diff and 11426.4.2.diff (sorry about the name -- left out pluggable.php in the diff and forgot to click the "Replace file" checkbox).

comment:11 follow-up: caesarsgrunt5 years ago

Oh, of course. I hadn't thought of that...

Regarding the names of the actions, I don't see any point in renaming them at all if the new names are still going to be different from the ones used in other places. There's nothing really wrong with cdc and mac - and if it's shortness you're going for, you can't beat them! (Unless you were to have like t for trash, s for spam, and a for approve... actually, there might be something to be said for that...)

comment:12 in reply to: ↑ 11 nacin5 years ago

Replying to caesarsgrunt:

Regarding the names of the actions, I don't see any point in renaming them at all if the new names are still going to be different from the ones used in other places. There's nothing really wrong with cdc and mac - and if it's shortness you're going for, you can't beat them! (Unless you were to have like t for trash, s for spam, and a for approve... actually, there might be something to be said for that...)

Here's my thought. It really isn't much to ask that the links should be short, readable, and distinguishable. All three options are short enough. However, I didn't have a clue what 'mac' or 'cdc' was until I looked back in the changesets when they were shortened 3 years ago. They're not understandable to an end user, but why shouldn't they be?

The one-character strings could work, but they're not readable or easily distinguishable.

The point of renaming them is simple. We're already creating two new actions, spam and trash, and getting rid of dt. So we might as well standardize 'approve' and 'delete' as well. They're only different because they perform a unique AYS function. And FWIW, adding "comment" to the action generates the post-AYS action, so this standardizes it and makes them alike, not different. And because of that, the code is easier, too.

nacin5 years ago

Alternative route (no new strings) -- fix for redirect in (un)approvecomment

nacin5 years ago

Standard route (4 new strings) -- Fix for redirect

comment:13 azaozz5 years ago

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

(In [12402]) Add 'trash' in comment moderation emails, props nacin, fixes #11426

Note: See TracTickets for help on using tickets.