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

Avoid display of "Back to subscriptions" button when no subscription available to the user. #229

Closed
Reedyseth opened this Issue Feb 12, 2016 · 18 comments

Comments

Projects
None yet
5 participants
@Reedyseth
Contributor

Reedyseth commented Feb 12, 2016

I noticed that on the Add Subscription page there is a button to return to my subscriptions, so I clicked on it an got an error retrieving my subscription. An enhacement could be to not display that button if there are not subscriptions associated

image

This is display when I click on the button

image

@kristineds

This comment has been minimized.

Contributor

kristineds commented Feb 12, 2016

I was able to replicate this error after subscribing without commenting on a post as a non logged-in user. Also, after creating my subscription, I encountered that error again when I clicked on Back to My Subscriptions. If a confirmation from the subscriber needs to be made before one can view their subscriptions, shouldn't there be a note to say that instead of the error message?

Tested with Comment Mail Lite and Pro Version 160211-RC on a clean install

screen shot 2016-02-12 at 2 12 01 pm

@renzms

This comment has been minimized.

Contributor

renzms commented Feb 12, 2016

@raamdev @Reedyseth @kristineds

screen shot 2016-02-12 at 5 11 47 pm

Working fine on my end.

  • Subscribed to a new post without commenting
  • When asked for the delivery option I chose instantly
  • On the next page I clicked the Back to My Subscriptions link
  • Subscriptions displaying successfully

I did however experience this before in an older version of Comment-Mail, #134 (comment)

The issue disappeared after disabling all plugins except Comment-Mail Pro. When I re-enabled all plugins again, everything was still working fine.

@raamdev

This comment has been minimized.

Contributor

raamdev commented Feb 12, 2016

@renzms writes...

Working fine on my end.

Are you logged in? That screenshot looks like you might be logged in, or you might already have a bunch of subscription associated with the renz@websharks-inc.com email address, so the page that lists the subscription will work as expected.

I believe @kristineds and @Reedyseth are referring specifically to non-logged-in users, as Kristine mentioned in her last comment.


I was able to confirm the behavior that Israel and Kristine have observed:

Scenario 1 (not logged in)

  1. Visit a post as a user who IS NOT logged in

  2. Click the "Subscribe without Commenting" link on a post

  3. When you get to the "Add New Subscription" page, try clicking the Back icon or the "My Comment Subscriptions" link at the bottom

    OR

    Click "Create Subscription" at the bottom and when you see the success message, try clicking the "Back to My Subscriptions" link

Scenario 2 (logged in, no subscriptions)

  1. Visit a post as a user who IS logged in but has NO subscriptions

  2. Click the "Subscribe without Commenting" link on a post

  3. When you get to the "Add New Subscription" page, try clicking the Back icon or the "My Comment Subscriptions" link at the bottom

    OR

    Click "Create Subscription" at the bottom and when you see the success message, try clicking the "Back to My Subscriptions" link

2016-02-12_09-40-21
2016-02-12_09-40-35
2016-02-12_09-41-21

In all cases, that link leads to a page with an error:

2016-02-12_09-42-53

This error makes sense, because there are no subscriptions to list for this user because they are not yet known to Comment Mail (i.e., they don't have a Subscription Key yet). Once they confirm their first subscription, a Subscription Key is created for them and whenever they visit those pages after confirming their subscription, the presence of the key (for non-logged-in users) makes all of the pages work (for logged-in users, they just need to have a confirmed subscription; the key does not need to be present in the URL).


How this needs to be fixed

So, what we need to do is detect various scenarios where those links would lead to that error page and hide them so that users cannot easily reach that error page and get confused.

From what I've tested, these are the two common scenarios where those links should be hidden:

  • User is not logged in and no Subscription Key is present
  • User is logged in but does not have any subscriptions

@raamdev raamdev added the bug label Feb 12, 2016

@raamdev raamdev added this to the Next Release milestone Feb 12, 2016

@raamdev raamdev modified the milestones: Future Release, Next Release Feb 12, 2016

@renzms

This comment has been minimized.

Contributor

renzms commented Feb 12, 2016

Are you logged in?

Yes, I tried it without being logged in and now I see the error now too.

From what I've tested, these are the two common scenarios where those links should be hidden:

User is not logged in and no Subscription Key is present
User is logged in but does not have any subscriptions

Agree, I tried both scenarios also to duplicate and used a 3rd scenario.

  • User is not logged in, subscribes for the first time and then subscribes again on the success page selecting a different post from the dropdown without first confirming their first subscription.
@raamdev

This comment has been minimized.

Contributor

raamdev commented Feb 12, 2016

@renzms writes...

User is not logged in, subscribes for the first time and then subscribes again on the success page selecting a different post from the dropdown without first confirming their first subscription.

Can you provide screenshots please? I'm not following...

@Reedyseth

This comment has been minimized.

Contributor

Reedyseth commented Feb 12, 2016

So, what we need to do is detect various scenarios where those links would lead to that error page and hide them so that users cannot easily reach that error page and get confused.

@raamdev I can work on this issue if it is ok with you ?

@raamdev

This comment has been minimized.

Contributor

raamdev commented Feb 12, 2016

@Reedyseth Sure. :-)

@jaswrks

This comment has been minimized.

Member

jaswrks commented Mar 29, 2016

@Reedyseth Here are some lines that should help you out.

$current_email     = $this->plugin->utils_sub->currentEmail();
$has_subscriptions = $current_email ? (bool)$this->plugin->utils_sub->queryTotal(null, ['sub_email' => $current_email, 'status' => 'subscribed', 'sub_email_or_user_ids' => true]) : false;

So a conditional might then look something like this...

if ($is_edit || ($current_email && $has_subscriptions)) {
    // Display the link.
}

See also: https://github.com/websharks/comment-mail-pro/blob/000000-dev/src/includes/classes/UtilsSub.php#L566

@raamdev raamdev modified the milestones: Future Release, Next Release Apr 4, 2016

@raamdev

This comment has been minimized.

Contributor

raamdev commented Apr 4, 2016

Punting this to the Future Release milestone.

@Reedyseth

This comment has been minimized.

Contributor

Reedyseth commented Jun 16, 2016

@raamdev
This pull request closes this issue websharks/comment-mail-pro#71

Thanks for the help !

@kristineds kristineds self-assigned this Jul 12, 2016

kristineds added a commit to websharks/comment-mail-pro that referenced this issue Jul 21, 2016

@kristineds

This comment has been minimized.

Contributor

kristineds commented Aug 3, 2016

@raamdev I tried changing this line of code to this:

$sub_summary_return_url = $has_subscriptions && $plugin->utils_url->subManageSummaryUrl($sub_key, null, true);

But I'm getting this error message (found here):

Parse error: syntax error, unexpected 'elseif' (T_ELSEIF), expecting end of file in /home4/wsksdev/public_html/kristine/wp-content/plugins/comment-mail-pro/src/includes/classes/UtilsPhp.php(87) : eval()'d code on line 91

Any idea how to fix it? :) cc @jaswsinc

@raamdev

This comment has been minimized.

Contributor

raamdev commented Aug 3, 2016

@kristineds It's not clear to me which line you're trying to change. The link you provided doesn't lead to a specific line, but rather to the entire diff, so I have no idea which specific line you're referring to. :-)

@kristineds

This comment has been minimized.

Contributor

kristineds commented Aug 3, 2016

@raamdev Sorry about that. 😞 I'm trying to change this line: https://github.com/websharks/comment-mail-pro/blob/000000-dev/src/includes/templates/type-s/site/footer-tag.php#L34

Or should I be looking at another file to fix this issue?

@raamdev

This comment has been minimized.

Contributor

raamdev commented Aug 3, 2016

This line doesn't look right to me because it's basically giving $sub_summary_return_url a boolean true/false value, which is not what you want.

$sub_summary_return_url = $has_subscriptions && $plugin->utils_url->subManageSummaryUrl($sub_key, null, true);

Can you explain to me what you're trying to do on that line?

@kristineds

This comment has been minimized.

Contributor

kristineds commented Aug 3, 2016

Comment moved inside PR issue.


@raamdev Since the error message that I'm getting (when clicking on the Back button) is because of a missing $sub_key, https://github.com/websharks/comment-mail-pro/blob/6d2629d4c2815bc6a6c3448992896c2a82517121/src/includes/templates/type-a/site/sub-actions/manage-summary.php#L104

Missing subscription key; unable to display summary

I'm trying to fix the issue you pointed out here. websharks/comment-mail-pro#71 (comment)

If I'm looking at it the wrong way, please point me to the right direction. :)

@raamdev

This comment has been minimized.

Contributor

raamdev commented Aug 3, 2016

I'm trying to fix the issue you pointed out here. websharks/comment-mail-pro#71 (comment)

Have you confirmed there's even an issue there? :-)

kristineds added a commit to websharks/comment-mail-pro that referenced this issue Aug 9, 2016

kristineds added a commit to websharks/comment-mail-pro that referenced this issue Aug 10, 2016

raamdev added a commit to websharks/comment-mail-pro that referenced this issue Aug 10, 2016

@raamdev

This comment has been minimized.

Contributor

raamdev commented Aug 10, 2016

Next Release Changelog:

  • Bug Fix: Fixed a bug where the "My Comment Subscriptions" link would appear on the Add New Subscription page (when Subscribing without Commenting) and would lead to a page that displayed an error message stating that there were no subscriptions to list. That link is now hidden when there are no subscriptions to list. Props @Reedyseth @kristineds. See Issue #229.

@raamdev raamdev closed this Aug 10, 2016

raamdev added a commit that referenced this issue Aug 18, 2016

Phing release of v160818 with the following changes:
- **Bug Fix**: Fixed a bug where the "My Comment Subscriptions" link would appear on the Add New Subscription page (when Subscribing without Commenting) and would lead to a page that displayed an error message stating that there were no subscriptions to list. That link is now hidden when there are no subscriptions to list. Props @Reedyseth @kristineds. See [Issue #229](#229).
- **Bug Fix** (Pro): Removed an erroneous anchor tag in the Advanced Template for Comment Notification Message Body. Props @kristineds. See [Issue #287](#287).
- **UI Enhancement:** Improved the nav bar at the top of the options pages to reduce unnecessary whitespace. Also moved the Restore button to the nav bar so that it's not so prominent. Props @renzms. See [Issue #284](#284).
- **UI Enhancement:** Added links to the Comment Mail [Twitter](http://twitter.com/CommentMail) and [Facebook](https://www.facebook.com/Comment-Mail-565683256946855/) pages to the nav bar on the options page. Props @renzms. See [Issue #286](#286).
- **UX Enhancement:** Removed IP address information from email notification templates to better comply with data protection laws in certain countries. Props @kristineds. See [Issue #288](#288).
- **SEO Improvement:** Added `rel="nofollow"` to the "Subscribe without Commenting" link and "Manage Subscriptions" link on the comment subscription form to avoid indexing or transferring PageRank. Props @IvanRF. See [Issue #80](websharks/comment-mail-pro#80).
- Removed several development-only files from the distributable that were inadvertently included during the build process. See [Issue #285](#285).
- Added Renz Sevilla (`renzms`) to the contributors list.
@raamdev

This comment has been minimized.

Contributor

raamdev commented Aug 18, 2016

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

@websharks websharks locked and limited conversation to collaborators Aug 18, 2016

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