Activating Pro version should auto-deactivate Lite version if active

raamdev commented 8 years ago

If Comment Mail Pro is installed on a site where Comment Mail Lite is currently running, it should gracefully handle that scenario by deactivating Comment Mail Lite and displaying a "Thank you for upgrading to Comment Mail Pro!" message. As of now, the dev version does not handle this scenario gracefully and instead produces an error message.

Also, if activating Comment Mail Lite on a site running Comment Mail Pro, the Pro version should be automatically deactivated (no need to thank the site owner for downgrading though).

renzms commented 8 years ago

@raamdev / @jaswsinc

Hi, is there any example I can follow on how to setup this up in Comment Mail?

raamdev commented 8 years ago

Noting Jason's response on Slack:

This class file in Comment Mail Pro is called upon whenever the plugin is activated in WordPress. https://github.com/websharks/comment-mail-pro/blob/000000-dev/src/includes/classes/Installer.php

So having those routines (in that file) check for the existence of Comment Mail Lite would be the best way to start. You can take a look at this file in Comet Cache Pro to get some ideas about how to do that. See: https://github.com/websharks/comet-cache-pro/blob/000000-dev/src/includes/classes/Conflicts.php

raamdev commented 8 years ago

It looks like Comment Mail already includes a conflicts check to handle auto-deactivation of the Lite/Pro version when the other is being installed (see src/includes/classes/Conflicts.php) and that class is already called in src/includes/plugin.php.

So it looks like we have a bug in the Conflicts class. I seems like Comment Mail Lite is not being properly deactivated when Comment Mail Pro is installed. I suggest a review of the Conflicts class and some further testing to determine exactly what's going on.

renzms commented 8 years ago

@raamdev

I tested changing the code in Comment Mail and testing out having Lite/Pro installed and installing the other version while it was activated. Referring specifically to this line

class_alias(__NAMESPACE__.'\\ApiBase', GLOBAL_NS);

Comment Mail error:

Warning: Cannot declare class comment_mail, because the name is already in use in /wp-content/plugins/comment-mail/src/includes/api.php on line 12

I also tested Comet Cache to see if the conflicts.php would detect and deactivate, but that has the same issue.

Comet Cache error:

Warning: Cannot declare class WebSharks\Comet_Cache\AdvCacheBackCompat, because the name is already in use in /wp-content/plugins/comet-cache/src/includes/stub.php on line 44

Warning: Cannot declare class WebSharks\Comet_Cache\AdvancedCache, because the name is already in use in /wp-content/plugins/comet-cache/src/includes/stub.php on line 45

Warning: Cannot declare class WebSharks\CometCache\AdvCacheBackCompat, because the name is already in use in /wp-content/plugins/comet-cache/src/includes/stub.php on line 49

Warning: Cannot declare class WebSharks\CometCache\AdvancedCache, because the name is already in use in wp-content/plugins/comet-cache/src/includes/stub.php on line 50

Warning: Cannot declare class comet_cache, because the name is already in use in wp-content/plugins/comet-cache/src/includes/api.php on line 14

renzms commented 8 years ago

Steps to reproduce error

For Comment Mail / Comment Mail Pro

Install Comment Mail Lite Version 160215 or Comment Mail Pro Version 160215
Activate Lite/Pro version like normal
Then, install and activate the other version as if you were upgrading/downgrading without deactivating the first plugin.

Ideal Behaviour:

The Lite/Pro version should deactivate upon activating the other version.

Observed Behaviour:

Displays Comment Mail error:

Warning: Cannot declare class comment_mail, because the name is already in use in /wp-content/plugins/comment-mail/src/includes/api.php on line 12

For Comet Cache / Comet Cache Pro

Install Comet Cache Lite Version 160521 or Comet Cache Pro Version 160521
Activate Lite/Pro version like normal
Then, install and activate the other version as if you were upgrading/downgrading without deactivating the first plugin.

Ideal Behaviour:

The Lite/Pro version should deactivate upon activating the other version.

Observed Behaviour:

Displays Comet Cache error:

Warning: Cannot declare class WebSharks\Comet_Cache\AdvCacheBackCompat, because the name is already in use in /wp-content/plugins/comet-cache/src/includes/stub.php on line 44

Warning: Cannot declare class WebSharks\Comet_Cache\AdvancedCache, because the name is already in use in /wp-content/plugins/comet-cache/src/includes/stub.php on line 45

Warning: Cannot declare class WebSharks\CometCache\AdvCacheBackCompat, because the name is already in use in /wp-content/plugins/comet-cache/src/includes/stub.php on line 49

Warning: Cannot declare class WebSharks\CometCache\AdvancedCache, because the name is already in use in wp-content/plugins/comet-cache/src/includes/stub.php on line 50

Warning: Cannot declare class comet_cache, because the name is already in use in wp-content/plugins/comet-cache/src/includes/api.php on line 14

raamdev commented 8 years ago

@renzms I see you're using the latest public version of Comment Mail (v160215). Have you tested Comment Mail against the dev branch? A LOT of the Comment Mail codebase has changed since v160215, so I just want to confirm this hasn't already been fixed there.

renzms commented 8 years ago

@raamdev Confirmed that the latest dev branch versions of Comment Mail Lite/Pro still produces the error:

Warning: Cannot declare class comment_mail, because the name is already in use in /wp-content/plugins/comment-mail/src/includes/api.php on line 12

raamdev commented 7 years ago

@renzms writes...

Confirmed that the latest dev branch versions of Comment Mail Lite/Pro still produces the error:

Have you investigated what might be causing that error?

renzms commented 7 years ago

@raamdev Tried looking into this one and these are my findings:

In https://github.com/websharks/comment-mail/blob/dev/src/includes/api.php

shouldn't it also include:

use WebSharks\CommentMail\Classes;

after the Namespace?

Then have line 12 written instead as:

class_alias(__NAMESPACE__.'\\Classes\\ApiBase', GLOBAL_NS);

In https://github.com/websharks/comment-mail/blob/dev/src/includes/classes/ApiBase.php, should there be a function to get the current plugin instance like in Comet Cache here?

jaswrks commented 7 years ago

Next Release Changelog:

Bug Fix: Conflict checks between lite and pro corrected. This was not working properly in the previous release; i.e., installing Comment Mail Pro when Comment Mail Lite is already running should result in Comment Mail Lite being deactivated automatically. See Issue #270.

renzms commented 7 years ago

@jaswsinc @raamdev

Confirmed partly working with possible bug

Tested Using Comment Mail Lite / Pro Version 161210-RC WordPress v 4.7

Observed Behaviour

Installing/Activating Comment Mail Pro while Comment Mail Lite is active automatically disables Comment Mail Lite quietly.

Prompt is Plugin Activated. Should there be a "Thank you for upgrading to Comment Mail Pro!" message?

screen shot 2016-12-13 at 2 57 45 pm

Possible Bug

I did notice though, that the opposite is not true. When attempting to activate Comment Mail Lite when Comment Mail Pro is active, the plugin only keeps Comment Mail Pro active but header prompt says Plugin Activated

raamdev commented 7 years ago

@renzms Thank you. Yes, that seems confusing and incorrect.

@jaswsinc Can you add a Dashboard message that says "Thank you for upgrading" when going from Lite → Pro, and have it properly deactivate Pro when going from Pro → Lite? I realize the latter is a less common scenario, but it should still be handled gracefully, as that scenario will occur.

jaswrks commented 7 years ago

I can do that but it's not a quick fix. It might take another day or so because there is some logic that needs to get worked out, and this is rather tricky given the loading sequence in core.

Actually, I suggest not going this route. I think stuff like this is on the user to get right, and trying to put too much logic in code that can deal with something that is such a fundamental part of installing a piece of software to begin with, is asking for problems.

For example, I'd hate to add logic that works out something so simple like this, only to have that break in a future release and actually prevent the software from being installed! That would be rather embarrassing, since all you need do is decide whether you're going to run the lite version or the pro version to begin with. I mean, really. I think they can get that part right.

raamdev commented 7 years ago

@jaswsinc writes...

I mean, really. I think they can get that part right.

That sounds exactly like something a customer would say referring to developers who "can't even produce software that can tell the Pro version from the Lite version!". :-)

I'm fine with deferring that work to a later time if it's going to delay releasing today, but I'll have to open another bug report so that it's documented that we're aware of that issue. My feeling is that two variations of our software should play nicely with each other and be smart enough to deactivate the other when one is activated.

I do still think we need a "Thank you for upgrading" Dashboard message when going from Lite → Pro; that should be easy enough to add, no?

jaswrks commented 7 years ago

That sounds exactly like something a customer would say referring to developers who "can't even produce software that can tell the Pro version from the Lite version!". :-)

Oh, give me a break! The crazy thing here would be having pro installed, then trying to install the lite version on top of the pro version and expecting that to work. What we do right now is that the pro version takes precedence over the lite version. What you're asking for is that we also detect a case where a site owner has pro installed, then for some unknown reason tries to install the lite version (because they want less power?, and they also forget to deactivate the pro version before doing so), and in that case, the lite should version should win. That's the way I understand this request.

So fine, we can do that, but it's not a simple thing to do, and my feeling is that it's a waste of time, since that's an extremely odd thing to do, and likely will never happen outside of testing or fiddling. If it does happen, pro wins the battle. If you don't want pro to win the battle, you can simply uninstall it.

I do still think we need a "Thank you for upgrading" Dashboard message when going from Lite → Pro; that should be easy enough to add, no?

I don't think we need one, no. I'm not against adding one, but again, this is little stuff that is already established in WordPress core. There's no rule that when you install a plugin it needs to reach out and hug you by specifically reinforcing something that a user will already know.

raamdev commented 7 years ago

There's no rule that when you install a plugin it needs to reach out and hug you

If they just paid for the Pro version, it would be nice to say thank you, much like we do with s2Member Pro.

jaswrks commented 7 years ago

Yep, nothing against that note. We thanked them for their purchase at comment-mail.com, but we can thank them again in the software, which is great.

raamdev commented 7 years ago

What you're asking for is that we also detect a case where a site owner has pro installed, then for some unknown reason tries to install the lite version

As I said, I'm fine with leaving that extra behavior out. But if you have the Pro version installed and you decide to ask for a refund and then choose to voluntarily revert back to the Lite version (just one example scenario where this might happen), being unable to activate the Lite version because the Pro version silently prevents you from activating it is not good UX. A Dashboard message saying "you must disable the Pro version first"? Auto-deactivating the Pro version in favor the version being activated? Either of those would make sense. But silently preventing the user from taking the action and assuming they're smart enough to figure it out doesn't make sense.

Again, I see your point about not wanting to add unnecessary complexity to the code and I think this is an uncommon enough scenario that I'm not too concerned with fixing it.

raamdev commented 7 years ago

Comment Mail v161213 has been released and includes changes from this GitHub Issue. See the v161213 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 (#270).

wpsharks / comment-mail