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

Activating Pro version should auto-deactivate Lite version if active #270

Closed
raamdev opened this Issue May 17, 2016 · 19 comments

Comments

Projects
None yet
3 participants
@raamdev
Contributor

raamdev commented May 17, 2016

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

This comment has been minimized.

Contributor

renzms commented May 18, 2016

@raamdev / @jaswsinc

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

@raamdev

This comment has been minimized.

Contributor

raamdev commented May 20, 2016

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

This comment has been minimized.

Contributor

raamdev commented Jun 8, 2016

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

This comment has been minimized.

Contributor

renzms commented Jun 8, 2016

@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

This comment has been minimized.

Contributor

renzms commented Jun 8, 2016

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

This comment has been minimized.

Contributor

raamdev commented Jun 8, 2016

@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

This comment has been minimized.

Contributor

renzms commented Jun 8, 2016

@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 raamdev modified the milestones: Next Release, Future Release Jul 12, 2016

@raamdev

This comment has been minimized.

Contributor

raamdev commented Sep 8, 2016

@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

This comment has been minimized.

Contributor

renzms commented Sep 15, 2016

@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?

@raamdev raamdev modified the milestones: Next Release, Future Release Oct 11, 2016

@raamdev raamdev assigned jaswrks and unassigned renzms Nov 29, 2016

@raamdev raamdev removed the needs estimate label Nov 29, 2016

jaswrks pushed a commit to websharks/comment-mail-pro that referenced this issue Dec 2, 2016

jaswsinc

@jaswrks jaswrks referenced this issue Dec 2, 2016

Merged

PR: feature/270 #93

jaswrks pushed a commit to websharks/comment-mail-pro that referenced this issue Dec 2, 2016

jaswsinc
@jaswrks

This comment has been minimized.

Member

jaswrks commented Dec 2, 2016

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

This comment has been minimized.

Contributor

renzms commented Dec 13, 2016

@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

This comment has been minimized.

Contributor

raamdev commented Dec 13, 2016

@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

This comment has been minimized.

Member

jaswrks commented Dec 13, 2016

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

This comment has been minimized.

Contributor

raamdev commented Dec 13, 2016

@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

This comment has been minimized.

Member

jaswrks commented Dec 13, 2016

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

This comment has been minimized.

Contributor

raamdev commented Dec 13, 2016

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

This comment has been minimized.

Member

jaswrks commented Dec 13, 2016

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

This comment has been minimized.

Contributor

raamdev commented Dec 13, 2016

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 added a commit that referenced this issue Dec 13, 2016

Phing release of v161213 with the following changes:
- **Bug Fix:** Prevent browser autocomplete in Comment Mail options. See [Issue #319](#319).
- **Bug Fix:** Searching by email address alone should always narrow to the search to that specific email address and not result in any fuzzy or fulltext matching. See [Issue #226](#226).
- **Bug Fix:** The conflict check for 'Subscribe to Comments Reloaded' was not working in the previous release; i.e., if you attempt to activate both Comment Mail and the 'Subscribe to Comments Reloaded' plugin at the same, this should result in a Dashboard warning. Fixed in this release. See [Issue #315](#315).
- **Bug Fix:** Notify 'Subscribe to Comments Reloaded' users about the comment form template being disabled under certain scenarios. See [Issue #314](#314).
- **Bug Fix:** Do not attempt to import 'Subscribe to Comments Reloaded' (StCR) settings if StCR is no longer installed, even if old StCR options exist in the database. See [Issue #294](#294).
- **Bug Fix** (Pro): Do not show SparkPost partner image when Mandrill is selected as the RVE handler. See [Issue #318](#318).
- **Bug Fix** (Pro): 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](#270).
- **New Feature:** It is now possible to manually process the outgoing mail queue. See: **WP Dashboard → Comment Mail → Mail Queue**. See also [Issue #282](#282).
- **New Feature** (Pro): In Comment Mail Pro it is now possible to enable/disable comment content clipping entirely; e.g., if you prefer that email notifications include the full original comment content in raw HTML instead of being clipped and displayed in the email as plain text. See: **WP Dashboard → Comment Mail → Config. Options → Email Notification Clips**. See also: [Issue #281](#281).
- **Accessibility:** This release improves screen reader accessibility by adding `aria-hidden="true"` to all FontAwesome icons. See [Issue #304](#304).
- **Accessibility:** This release improves screen reader accessibility by offering a new setting that allows a site owner to enable or disable select menu option enhancement via jQuery. Disabling select menu option enhancement has the benefit of improving accessibility for screen readers whenever accessibility is of more concern than presentation. See: **Dashboard → Comment Mail → Config Options → Misc. UI-Related Settings**. See also [Issue #304](#304).
@raamdev

This comment has been minimized.

Contributor

raamdev commented Dec 13, 2016

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).

@raamdev raamdev closed this Dec 13, 2016

@websharks websharks locked and limited conversation to collaborators Dec 13, 2016

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