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

editorial-comments.js - fix “no one will be notified” message logic #507

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jerclarke
Copy link
Contributor

@jerclarke jerclarke commented Apr 12, 2019

Note: Noticed this while working on #506 and they work on the exact same area of the code, so it may be tricky to merge them both. You could easily update this patch to include the changes from #506 if it makes it easier for you.

PROBLEM:

  • When no one will be notified of an Editorial Comment the "No one will be notified" message should of course show in the space where there would otherwise be a list of usernames
  • Right now, most of the time, the message is missing, and NOTHING shows
  • The reason is that the message currently only shows when exactly zero users are subscribed, but the post author is ALWAYS subscribed. So if the post author is writing a comment when they are the only subscriber the message doesn't show, leaving an empty space.
  • This is an obvious bug. If there’s no usernames to show, then the “no one will be notified” message should show instead.

SPECIFIC CAUSE IN THE CODE:

The JS checks for “no subscribers” BEFORE removing the one’s who won’t be notified (due to being the comment author, not having permission to edit the post, or not having an email), so the current author's presence on the list stops the message from being shown, then it doesn’t check again after running the filter and removing current author.

SOLUTION:

  • In the JS, we run our logic that removes people from the list if they won’t be notified BEFORE we check if there are subscribers (and exit if there are none).

NOTES:

  • I’m very embarrassed I didn’t notice this before, but testing these things gets very abstract, and apparently no one else spotted it either 🤷🏻‍♀️

PROBLEM:

- When no one will be notified of an Editorial Comment the message should show
- Right now, most of the time, the message is missing, and NOTHING shows
- The reason is that the message only shows when exactly zero users are subscribed, but the post author is ALWAYS subscribed. So despite the fact that the author won’t be notified, they still block the “no one will be notified” message from showing.
- This is an obvious bug. If there’s no usernames to show, then the “no one will be notified” message should show instead.
- Specific cause: The JS checks for “no subscribers” BEFORE removing the one’s who won’t be notified (due to being the comment author, not having permission to edit the post, or not having an email), so it doesn’t remove the current author from the list, then it doesn’t check again after running the filter.

SOLUTION:

- In the JS, we run our logic that removes people from the list if they won’t be notified BEFORE  we check if there are subscribers and exit if there are none.

NOTES:

- I’m very embarassed I didn’t notice this before, but testing these things gets very abstract.
@WPprodigy
Copy link
Contributor

Ah, makes sense - nice catch. I tested and this does resolve the issue.

@jerclarke
Copy link
Contributor Author

FWIW I just tested this with 0.9.6 and it still works and is still a really obvious way to make that field work better.

@jerclarke
Copy link
Contributor Author

FWIW if anyone is still working on EF, this is still a great thing to fix!

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

Successfully merging this pull request may close these issues.

2 participants