Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notifications: Include list of people who were notified in the editorial comment emails #478

Closed
jerclarke opened this issue Nov 5, 2018 · 3 comments

Comments

@jerclarke
Copy link
Contributor

One of our editors wants to be able to add a Gmail filter to add tags to EF email based on which group was ticked in the notifications box. This seems reasonable, but right now there's no way to know why you got a given notification, it just comes to the email of your WP account.

If you've got a complicated EF+WP+email setup, some people are liable to get many EF emails for a single post change, and not knowing why the emails are coming can be confusing.

A related issue is that it would be nice to know who else got the notification from right in the email, e.g. so you know that the correct person saw it and you can trust them to handle the situation for now. This is basically the "other side" of #270 , that one is about knowing who gets the notification while sending it, this one is about knowing who else received it.

I see two main solutions that address these related goals:

Option 1: Indicate the list of subscribers in all emails

___________
This notification was delivered to:

Group: Sub-Editors
Group: Time-Sensitive
Jer Clarke
Adam Adamson
Susan Susanson

Option 2: Indicate in individual emails why it is being sent to that address

__________
You are subscribed to this post as: Jer Clarke
You were subscribed to this post via the group(s): Sub-Editors, Time-Sensitive

Either of these would solve the precise use-case of my user (adding a Gmail filer) and generally enable users to put together why they got an email, since at least for the use cases I'm imagining, seeing the full list of subscribers would usually let someone figure out how it ended up in their inbox.

The second option has a theoretical privacy benefit, as we don't leak the full list of subscribers, but that's of limited value because the user can presumably log in and see the same information on the post itself. Also the "display names" of fellow subscribers will rarely be a security risk, though it's possible. Being able to disable the show_other_subscribers feature via. a filter should be enough for the few sites security-conscious enough to consider this a problem.

"Option 2" has the precise benefit of making it totally explicit why this exact email is coming to you, which could have utility e.g. debugging why you get multiple copies of the same email (multiple WP accounts). That said, I suspect that personalizing each email would be more coding and complexity, in which case IMHO "Option 1" is a very nice compromise that would just involve generating a list of subscribers for the post and adding it to the generic template that goes out to everyone.

Of course there's also an "Option 3" that would clearly be the most work to execute but maybe not that much more work than options 1 or 2:

Option 3: Indicate in individual emails why it is being sent to that address AND include a list of other subscribers

__________
You are subscribed to this post as: **Jer Clarke**
You were subscribed to this post via the group(s): **Sub-Editors**, **Time-Sensitive**

**Other subscribers:**

Group: Photographers
Adam Adamson
Susan Susanson

This gives the maximum possible information to the recipient and, in conjunction with a show_other_subscribers filter to disable the second list, would keep the security risk very controlable.

I'm going to try to code "Option 1" for myself as a "plugin" and see how it goes. Wanted to create this first as a reference.

P.S. Sorry I didn't reply to the older tickets yet, you know how it is. Will try to test the other updates soon. Looks like 0.8.3 still hasn't come out yet so I guess there's still time 😛

@jerclarke
Copy link
Contributor Author

Alright, that's what I got! Please take a look @WPprodigy and maybe @sboisvert

I think it's directly in line with the #452 changes:

  • Uses the same comment meta field where the text is stored
  • Uses just one translated string that matches Display who was notified for an editorial comment #452
  • With this change, that same bit of text now shows in both places, which IMHO is totally logical, and basically it's one feature implemented in both places

Differences:

  • I don't bother doing the module_enabled( 'notifications' ) check from maybe_output_comment_meta because it will always be true, we're sending an email
  • If there's nothing in $notification_list I show nothing, rather than the "No users or groups were notified." message, because that would make no sense. AFAICT there should always be a value if an email is being sent, and if not silence is probably for the best.

Notes:

  • I was going to try to re-use maybe_output_comment_meta but couldn't find any way to repurpose the code
    • It echoes directly, so we'd have to add a switch for that to use it in the email template
    • I was going to build a get_notification_list helper function to call in both places, but I can't find any way to access a variable copy of EF_Editorial_Comments for the sake of making a wrapper outside the EF_Editorial_Comments class that I could use in `notifications.
  • Looking deeper at this code now, I kind of wish that the notification_list was stored as an array, with separate users/groups, in case we want to interact with it as data. It's okay though. This solves our immediate problem, and we could rework it later if we really need the data separated.

Also:

  • Looking at the email template, it's clear that my earlier speculation about personalizing the email for each person was correct. It would be a PITA to display exactly why each email goes out, since currently the message is generated once, then sent to each person, so we'd have to rework that whole system to add a message like "You got this because you are in the group X".
    • i.e: It definitely makes sense to just show the whole list of users who were notified, as it's stored in notification_list. Trying to make it any more intelligent would send us down a rabbit hole.

Related:

  • I want to go down the rabbit hole of making these emails nicer to look at and more useful at some point. All that ASCII formatting isn't even very well executed, and a very lightly HTML version would be so much easier to work with See HTML templates for email notifications #39

@jerclarke
Copy link
Contributor Author

ef-editorial-comment-email-with-notified-list

Screenshot of how it looks after the PR. "Notified" list comes straight from the string saved to comment meta for the wp-admin side of this feature.

@jerclarke jerclarke changed the title Email notifications: Mention group memberships that caused notification for user (or just list all subscribers) Notifications: Include list of people who were notified in the editorial comment emails Nov 9, 2018
@WPprodigy
Copy link
Contributor

Looks resolved in #479. Closing this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants