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

Improve Dashboard Notices #163

Closed
raamdev opened this Issue Nov 23, 2015 · 23 comments

Comments

Projects
None yet
4 participants
@raamdev
Contributor

raamdev commented Nov 23, 2015

Improve "Upgrading from StCR" message:

Before:
2015-11-23_16-06-26

After:
2015-11-23_16-16-31

Add "Comment Mail is disabled" message

It's easy to forget that Comment Mail has not yet been enabled, especially when upgrading from StCR to Comment Mail and going through the import process. It would be a good idea to have a message (NOT dismissable) that indicates Comment Mail is currently disabled:

2015-11-23_16-46-55

@raamdev raamdev added the enhancement label Nov 23, 2015

@raamdev raamdev added this to the Next Release milestone Nov 23, 2015

@jaswrks

This comment has been minimized.

Member

jaswrks commented Nov 24, 2015

Regarding this issue. There is a special file in Comment Mail that is dedicated to StCR migration routines. See: https://github.com/websharks/comment-mail-pro/blob/28bbe8b88b59a77e3632938d5c72314ff86488a2/comment-mail-pro/includes/stcr.php#L96

The notice that Raam referenced in this issue can be found and tweaked in that file.

@kristineds

This comment has been minimized.

Contributor

kristineds commented Nov 26, 2015

@jaswsinc @raamdev Where should I be adding the "Comment Mail is disabled" message? I'm seeing this after enabling both CM and StCR on my plugin directory.

CM + StCR notice

@jaswrks

This comment has been minimized.

Member

jaswrks commented Dec 1, 2015

@kristineds Great question. I will work on an outline for this and come back to that soon.

@kristineds

This comment has been minimized.

Contributor

kristineds commented Dec 1, 2015

I will work on an outline for this and come back to that soon.

Great. Thanks! 👍

@jaswrks

This comment has been minimized.

Member

jaswrks commented Dec 1, 2015

@kristineds Next Actions

  • After this line add the following and edit as needed.

    if (!$this->options['enable']) {
      $this->enqueue_notice(__('NOTICE GOES HERE', $this->text_domain));
    }

Please repeat in the lite version.

@jaswrks

This comment has been minimized.

Member

jaswrks commented Dec 1, 2015

@raamdev I suggest that we also alter the code snippet that I gave Kristine above. Once she pushes her next commit and you have a chance to test, let's see if it is better if do it this way.

  if (!$this->options['enable'] && !get_option(__NAMESPACE__.'_notices')) {
    $this->enqueue_notice(__('NOTICE GOES HERE', $this->text_domain));
  }

This way the new (additional) notice is not shown unless there are no others; i.e., it will not be shown on activation, because we already have a notice for that, and we have several other StCR and conflict/warning notices too. So I think showing this notice is going to be best when there are no others.

@jaswrks

This comment has been minimized.

Member

jaswrks commented Dec 2, 2015

@kristineds Is this ready for another review also?

@jaswrks

This comment has been minimized.

Member

jaswrks commented Dec 2, 2015

Nevermind! Too much vodka. I reviewed the PRs.

@raamdev

This comment has been minimized.

Contributor

raamdev commented Dec 2, 2015

@jaswsinc writes...

I suggest that we also alter the code snippet that I gave Kristine above. Once she pushes her next commit and you have a chance to test, let's see if it is better if do it this way.
[...]
I think showing this notice is going to be best when there are no others.

I actually disagree. I think it's fine to have that notice always show up, even when there are other notices. The reason? Because if you have the Comment Mail plugin active, you probably intend to use it. If it's not enabled, then you're not using it and it makes sense to inform the site owner of as much. If the "Comment Mail is disabled" notice is annoying, then you should just deactivate the plugin.

The only reason I'm slightly bullish on having a notice there at all is that Comment Mail provides important front-end functionality. If Comment Mail is disabled, that functionality is not being provided and therefore we should alert the site owner of that fact. (I wouldn't add such a notice for a plugin like ZenCache only because if ZenCache is disabled, the site doesn't lose any functionality: it just gets a little slower).

What made me think of adding such a notice at all was having StCR + Comment Mail installed: I was losing track of which plugin was enabled and found myself wondering, "Is Comment Mail enabled?" when I was looking at other areas of the site. Basically, it felt like Comment Mail being disabled was simply "too quiet". A persistent Dashboard notice is perfect.


@kristineds I tested the latest PRs and noticed two things:

The "Comment Mail is disabled" message should have an error (red) class not an updated (green) class:

2015-12-02_16-17-27
2015-12-02_16-18-42

Also, please change the "Comment Mail settings" link to just "settings". Now that I see the notice in action, mentioning "Comment Mail" a second time in that notice seems redundant and unnecessary. 😄

@kristineds

This comment has been minimized.

Contributor

kristineds commented Dec 3, 2015

@jaswsinc @raamdev Here are the updated PRs for both Pro and Lite

screen shot 2015-12-03 at 5 36 41 pm

@raamdev

This comment has been minimized.

Contributor

raamdev commented Dec 3, 2015

@kristineds Thank you! That looks great. 😄

@Reedyseth

This comment has been minimized.

Contributor

Reedyseth commented Dec 5, 2015

OK, I just pull the latest commit c632e1c and after installing Comment mail I got the following notices.

image

So basically the second notices is duplicated, at first I thought that I had my database corrupted but then after deleting the CM tables the same behavior pop up. Can you give it a try ?

@raamdev

This comment has been minimized.

Contributor

raamdev commented Dec 5, 2015

@Reedyseth When testing dev branches or feature branches, you need to make sure that the previous version is completely uninstalled. The best way to do this:

  1. Visit Comment Mail → Config. Options → Data Safeguards and turn them off (so that no data is saved in the database)
  2. Deactivate and then delete the plugin in Dashboard → Plugins

When Data Safeguards are disabled and you delete the plugin, the deletion routines will clean up your database to ensure that there are no settings left behind. Then, you can install the new version that you want to test.


I just followed those steps and installed the latest Lite version from this branch and it appears to be working fine. Here's what I saw after installing (with StCR active):

2015-12-04_19-50-31

@Reedyseth

This comment has been minimized.

Contributor

Reedyseth commented Dec 5, 2015

When testing dev branches or feature branches, you need to make sure that the previous version is completely uninstalled.

I just get rid of the folder and the CM tables, I will follow those steps and let you now.

@raamdev

This comment has been minimized.

Contributor

raamdev commented Dec 5, 2015

@Reedyseth writes...

OK, I just pull the latest commit c632e1c

Also, the best way to test is to checkout the branch you want to test. So to test this branch you would git fetch origin && git checkout -b feature/163 origin/feature/163. Then you can simply test the comment-mail/ directory. If the feature/163 branch is updated (i.e., someone pushes more commits to it), you can simply run git pull from inside the feature/163 branch to get the latest commits and then test again.

@Reedyseth

This comment has been minimized.

Contributor

Reedyseth commented Dec 9, 2015

Also, the best way to test is to checkout the branch you want to test. So to test this branch you would git fetch origin && git checkout -b feature/163 origin/feature/163.

Thank you for that.

When testing dev branches or feature branches, you need to make sure that the previous version is completely uninstalled. The best way to do this:

Visit Comment Mail → Config. Options → Data Safeguards and turn them off (so that no data is saved in the database)
Deactivate and then delete the plugin in Dashboard → Plugins
When Data Safeguards are disabled and you delete the plugin, the deletion routines will clean up your database to ensure that there are no settings left behind. Then, you can install the new version that you want to test.

I had information contaminated on my database, so after allowing comment mail to get rid of all the plugin by clicking on the delete button some features remained on the database.

  • I deleted the plugin directory from wp_content
  • Delete all the Comment Mail tables:
DROP TABLE wp_comment_mail_queue;
DROP TABLE wp_comment_mail_subs;
DROP TABLE wp_comment_mail_queue_event_log;
DROP TABLE wp_comment_mail_sub_event_log;
  • Delete all the CM options, on this case this is where all the corrupted data was located, more specific the comment_mail_notices option.
DELETE FROM wp_options WHERE option_name LIKE 'comment_mail%';
  • Update the latest changes of comment mail with git fetch --all && git merge 000000-dev
  • Added the CM plugin directory again into my WordPress.
  • Activated.
  • Now everything work as it should be.

image

@raamdev

This comment has been minimized.

Contributor

raamdev commented Dec 10, 2015

Next Pro Release Changelog:

  • Enhancement: Improved Dashboard notices. The "Upgrading from Subscribe to Comments Reloaded" message has been improved and a new Dashboard notice lets you know when Comment Mail is disabled. Props @kristineds. See Issue #163.

@raamdev raamdev closed this Dec 10, 2015

@raamdev

This comment has been minimized.

Contributor

raamdev commented Dec 10, 2015

Next Lite Release Changelog:

  • Enhancement: Improved Dashboard notices. The "Upgrading from Subscribe to Comments Reloaded" message has been improved and a new Dashboard notice lets you know when Comment Mail is disabled. Props @kristineds. See Issue #163.
@kristineds

This comment has been minimized.

Contributor

kristineds commented Dec 11, 2015

👍

@raamdev

This comment has been minimized.

Contributor

raamdev commented Dec 18, 2015

Updated Comment Mail to use a yellow warning notice instead of a red error notice when Comment Mail is disabled. Also updated the notice routines to support a new enqueue_warning() method.

2015-12-18_09-35-10

@raamdev raamdev closed this Dec 18, 2015

@jaswrks

This comment has been minimized.

Member

jaswrks commented Dec 18, 2015

Nice! I did not know that class existed in core. Great!

@raamdev

This comment has been minimized.

Contributor

raamdev commented Dec 18, 2015

@jaswsinc Yep, I didn't either! I learned about it when I finally noticed the yellow "Update to WordPress v4.4" Dashboard notice. 😁

@websharks websharks locked and limited conversation to collaborators Dec 24, 2015

@raamdev

This comment has been minimized.

Contributor

raamdev commented Dec 24, 2015

Comment Mail v151224 has been released and includes changes from this GitHub Issue: See the v151224 announcement for further details.


This issue will now be locked to further updates. If you have something to add related to this GitHub Issue, please open a new GitHub Issue and reference this one (#163).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.