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

StCR Import Bug: Incorrect number of Post and Subscriptions imported #166

Closed
Reedyseth opened this issue Dec 1, 2015 · 71 comments
Closed

Comments

@Reedyseth
Copy link
Contributor

I detected this issue a while a go in a previous versions of CM, don't remember what version :), but as for the current version the issue still there.

This is what I did:

  • Checked my current Subscriptions with this query.
SELECT * FROM wp_postmeta WHERE meta_key LIKE '\_stcr@\_%'; -- 36 subscriptions.
SELECT DISTINCT post_id FROM wp_postmeta WHERE meta_key LIKE '\_stcr@\_%'; -- 7 Post IDs
  • Updated Comment Mail to the latest version 27e0663.
  • Activate Comment Mail.
  • Use the import wizard and got this result.

image

If you could try to replicate the issue that will confirm the bug.

@jaswrks
Copy link

jaswrks commented Dec 1, 2015

Updated Comment Mail to the latest version 27e0663.

Could be. Did you run this test with the 000000-dev branch?
https://github.com/websharks/comment-mail/archive/000000-dev.zip

@jaswrks
Copy link

jaswrks commented Dec 1, 2015

SELECT * FROM wp_postmeta WHERE meta_key LIKE 'stcr@%'; -- 36 subscriptions.
SELECT DISTINCT post_id FROM wp_postmeta WHERE meta_key LIKE 'stcr@%'; -- 7 Post IDs

You have 36 subscriptions with StCR, but only 3 were imported by Comment Mail? That seems very wrong. Were all 36 of those confirmed and had a Y or R status?

I just tried to reproduce that but no luck yet.

@Reedyseth
Copy link
Contributor Author

You have 36 subscriptions with StCR, but only 3 were imported by Comment Mail?

Correct,

Were all 36 of those confirmed and had a Y or R status?

I did not check this one, will update with the result.

@raamdev
Copy link
Contributor

raamdev commented Dec 1, 2015

@jaswsinc writes...

Were all 36 of those confirmed and had a Y or R status?

That was my first thought too, which brings up a good thing to point out to users migrating from StCR → Comment Mail: unconfirmed subscriptions are not imported. Is there any specific reason that is the case or are unconfirmed subscriptions simply being ignored?

@jaswrks
Copy link

jaswrks commented Dec 1, 2015

They are ignored, because:

  • I didn't see any reason they needed to be imported.
  • I was not entirely clear about what each of these possible flags meant. All I could understand for sure was that Y meant all comments and active and that R meant replies only and active.
  • We worked together to achieve a specific set of rules that would be followed whenever the import takes place. See: Feature Request: Import data from Subscribe to Comments Reloaded #7 (comment)
// Possible statuses: `Y|R|YC|RC|C|-C`.
// A `Y` indicates they want notifications for all comments.
// An `R` indicates they want notifications for replies only.
// A `C` indicates "suspended" or "unconfirmed".
if($_status !== 'Y' && $_status !== 'R') // Active?
    continue; // Not an active subscriber.

@Reedyseth
Copy link
Contributor Author

The code is my laptop but I will try to replicate the behavior on my desktop.

@jaswrks
Copy link

jaswrks commented Dec 1, 2015

@Reedyseth
Copy link
Contributor Author

Referencing StCR import class members: https://github.com/websharks/comment-mail-pro/blob/000000-dev/comment-mail-pro/includes/classes/import-stcr.php

Thanks that will help to debug.

@Reedyseth
Copy link
Contributor Author

I was not entirely clear about what each of these possible flags meant. All I could understand for sure was that Y meant all comments and active and that R meant replies only and active.

OK, just to keep updating this issue, I just ran this query:

SELECT * FROM wp_postmeta WHERE meta_key LIKE '\_stcr@\_%'
AND meta_value LIKE '%|R' OR meta_value LIKE '%|Y';
-- Got 35 records.
meta_id post_id meta_key meta_value
2 1 _stcr@_Doe@dot.com 2013-07-08 18:05:20~R
6 4 _stcr@_Doe@dot.com 2013-07-08 18:23:07~R
10 7 _stcr@_Doe@dot.com 2013-07-08 18:45:18~R
43 1 _stcr@_arkamus@gmail.com 2014-05-02 21:57:28~Y
48 31 _stcr@_arkamus@gmail.com 2014-05-02 22:20:20~Y
93 40 _stcr@_bigtiger@jungle.com 2014-11-03 10:42:25~Y
59 31 _stcr@_clark@kof.com 2014-05-13 21:04:48~Y
29 7 _stcr@_dutch@dot.com 2014-01-31 20:01:16~Y
60 31 _stcr@_gret@dlf.com 2014-05-13 21:04:49~Y
62 31 _stcr@_inco@net.bom 2014-05-28 20:41:47~Y
104 40 _stcr@_kokoro@ark.com 2015-02-07 02:19:03~Y
45 1 _stcr@_kola@gmail.com 2014-05-02 22:16:25~Y
49 31 _stcr@_kola@gmail.com 2014-05-02 22:20:27~Y
61 31 _stcr@_louis@dfd.com 2014-05-13 21:04:49~Y
23 7 _stcr@_mike@dot.com 2014-01-13 16:54:10~Y
39 7 _stcr@_mkate@dert.com 2014-04-28 15:51:21~Y
18 4 _stcr@_nomore@reply.com 2013-11-15 19:32:49~R
19 7 _stcr@_other@dfo.com 2014-01-09 16:17:12~R
46 1 _stcr@_pch@gmail.com 2014-05-02 22:16:35~Y
44 1 _stcr@_reed@gmail.com 2014-05-02 21:57:52~Y
47 31 _stcr@_reed@gmail.com 2014-05-02 22:19:53~Y
91 40 _stcr@_reed@gmail.com 2014-10-31 18:29:34~Y
25 7 _stcr@_stcr_001@dot.com 2014-01-15 15:40:49~Y
24 7 _stcr@_stcr_002@dot.com 2014-01-15 15:40:13~Y
20 7 _stcr@_test002@dfo.com 2014-01-11 00:00:38~R
21 7 _stcr@_test003@dfo.com 2014-01-11 00:07:34~R
22 7 _stcr@_test004@dfo.com 2014-01-11 00:19:09~R
105 34 _stcr@_test0@con.com 2015-02-07 13:55:50~Y
106 34 _stcr@_test@aaa.com 2015-02-07 13:57:17~Y
11 7 _stcr@_test@test.com 2013-07-10 22:53:31~R
38 7 _stcr@_test@test00x.com 2014-04-09 20:52:48~Y
72 40 _stcr@_test@wp4.com 2014-10-24 21:23:32~Y
68 37 _stcr@_tj@dot.com 2014-06-20 17:03:33~Y
74 40 _stcr@_urden@cjupo.com 2014-10-24 21:34:12~R
92 7 _stcr@urden@cjupo.com 2014-10-31 18:41:17~Y

Just for posting with markdown I replaced the last pipe with the tilde character. So all this subscription should had been imported and the were not.

@jaswrks
Copy link

jaswrks commented Dec 2, 2015

So all this subscription should had been imported and the were not.

Agree! Interesting. Thanks for posting that table. Really helps to clarify a lot.

@jaswrks
Copy link

jaswrks commented Dec 2, 2015

@Reedyseth Can you please run one more query for me?

SELECT * FROM wp_postmeta WHERE meta_key LIKE '%imported\_stcr\_subs%'

How many results do you get?

@Reedyseth
Copy link
Contributor Author

Can you please run one more query for me?

SELECT * FROM wp_postmeta WHERE meta_key LIKE '%imported_stcr_subs%'
How many results do you get?

SELECT * FROM wp_postmeta WHERE meta_key LIKE '%imported\_stcr\_subs%'
ORDER BY post_id;
meta_id post_id meta_key meta_value
94 1 comment_mail_imported_stcr_subs 1
95 4 comment_mail_imported_stcr_subs 1
96 7 comment_mail_imported_stcr_subs 1
97 31 comment_mail_imported_stcr_subs 1
245 34 comment_mail_imported_stcr_subs 1
98 37 comment_mail_imported_stcr_subs 1
99 40 comment_mail_imported_stcr_subs 1

But running this query:

SELECT * FROM wp_postmeta WHERE meta_key LIKE '%imported\_stcr\_subs%'
ORDER BY post_id;

I get:

post_id

1
4
7
31
34
37
40

EDITED: Both queries match in the number of post ids.

@jaswrks
Copy link

jaswrks commented Dec 2, 2015

Cool. Thanks! Do any of your posts have a status that is not publish?

See: https://github.com/websharks/comment-mail-pro/blob/000000-dev/comment-mail-pro/includes/classes/import-stcr.php#L353

@Reedyseth
Copy link
Contributor Author

Cool. Thanks! Do any of your posts have a status that is not publish?

They are. Now I am debugging Comment Mail.

@Reedyseth
Copy link
Contributor Author

OK, I run another test by cleaning up the database and now it imported the correct number of post_ids. On the results of the importation says 7 post IDs and 70 subscriptions, when I went to the CM subscription page there was 59 subscriptions as for the subscriptions in StCR are 36. I will keep debugging and clearing this up.

@jaswrks
Copy link

jaswrks commented Dec 2, 2015

by cleaning up the database and now it imported the correct number of post_ids.

Cool. What did you have to clean up for it to work?

@Reedyseth
Copy link
Contributor Author

Cool. What did you have to clean up for it to work?

I truncated the data on the wp_comment_mail_subs table as well as the entries on the Post Meta table with the filter LIKE '%imported\_stcr\_subs%'.

I poluted the database by running different test. The only thing that now concern me is the 59 Subscriptios on CM vs the 36 on StCR.

@jaswrks
Copy link

jaswrks commented Dec 2, 2015

Copy that. Thank you for that testing! :-)

The only thing that now concern me is the 59 Subscriptios on CM vs the 36 on StCR.

It's normal to have some difference in the counts. This can occur whenever you have R status subscribers. StCR doesn't have a record of which specific comments that a user subscribed to, only that they should receive replies to their comments only. In Comment Mail, a subscription is added to the entire post, or to specific comments by comment ID.

See my note here. So if some of your existing subscribers had an R status, in Comment Mail we create (i.e., import) them in a slightly different way. Instead of having just one R subscription, the subscriber is added multiple times, once for each of the comments that they authored.

That may explain 36 in StCR vs. 59 in Comment Mail. The same email address might be counted more than once, only because there are multiple per-comment subscriptions.

Terminology in Comment Mail:

  • Subscription An email address that is connected to notifications for a specific post (by ID), or to a specific comment (by comment ID). One Subscriber can have multiple Subscriptions in a single post.
  • Subscriber A unique email address.

@Reedyseth
Copy link
Contributor Author

So if some of your existing subscribers had an R status, in Comment Mail we create (i.e., import) them in a slightly different way. Instead of having just one R subscription, the subscriber is added multiple times, once for each of the comments that they authored.

Alright, yes I noticed this behavior on the debugging session, but this clarify things up. I will run another detail debug about this and then come back to close the issue, thank you for the feedback.

@raamdev
Copy link
Contributor

raamdev commented Dec 2, 2015

@Reedyseth Please make sure that you're testing the latest dev version of Comment Mail, i.e., after the work that Jason did to fix #162; see #162 (comment).

I was experiencing the same odd behavior with Comment Mail appearing to import a lot more subscriptions than it should (i.e., my Comment Mail Subscription Event Log showed many "inserts" followed by "overwrites"). Now that I'm testing the latest dev branch (after Jason's work on #162), the StCR → Comment Mail import process is working as expected and the correct number of subscriptions appear.

@Reedyseth
Copy link
Contributor Author

Please make sure that you're testing the latest dev version of Comment Mail, i.e., after the work that Jason did to fix #162; see #162 (comment).

Thanks for the insight.

@Reedyseth
Copy link
Contributor Author

Please make sure that you're testing the latest dev version of Comment Mail, i.e., after the work that Jason did to fix #162; see #162 (comment).

I was experiencing the same odd behavior with Comment Mail appearing to import a lot more subscriptions than it should (i.e., my Comment Mail Subscription Event Log showed many "inserts" followed by "overwrites"). Now that I'm testing the latest dev branch (after Jason's work on #162), the StCR → Comment Mail import process is working as expected and the correct number of subscriptions appear.

So I started with a new fresh installation of comment mail by doing this #163 (comment) and then started the migration process and I got this again:

image

image

So the first time I thought I was doing something wrong, now I now there is something weird on the importation routine so I will spend more time on this matter.

By entering to the StCR subscription page I see 39 subscription with status R or Y.

image

One more thing, So I did execute the Importing routine but I still see the message "IMPORTANT TIP: Comment Mail can import your existing StCR subscribers automatically too! [click here to begin]" this should dissapear after running my routine.

image

@raamdev
Copy link
Contributor

raamdev commented Dec 9, 2015

@Reedyseth writes...

One more thing, So I did execute the Importing routine but I still see the message "IMPORTANT TIP: Comment Mail can import your existing StCR subscribers automatically too! [click here to begin]" this should dissapear after running my routine.

Thank you. We have a GitHub issue open for this already here: #169

By entering to the StCR subscription page I see 39 subscription with status R or Y.

Would you be able to provide us with a MySQL dump of the StCR data so that we can import and test this on our own sites?

@Reedyseth
Copy link
Contributor Author

Thank you. We have a GitHub issue open for this already here: #169

Copy That I totally missed it, 😄

Would you be able to provide us with a MySQL dump of the StCR data so that we can import and test this on our own sites?

Here is the SQL inserts http://paste.behstant.com/?paste=204 use the password wpshark to view the file.

Let me know if you need more information.

@raamdev
Copy link
Contributor

raamdev commented Dec 13, 2015

I just tested the latest dev branch (as of 2015-12-12 22:08:18 EST) on my personal site, where I have 1126 StCR subscriptions. When I ran the Comment Mail import process, only 930 subscriptions were imported.

I confirmed that all 1126 StCR subscriptions have either an R or a Y status.

Still investigating...

@raamdev
Copy link
Contributor

raamdev commented Dec 13, 2015

So after lots of digging and manually adding logging routines to the sctr_subs_for_post() routine in includes/classes/import-stcr.php, I confirmed the following:

  • Comment Mail is not "missing" or "skipping" any valid/active StCR subscriptions during the import Edit: This may not be true; see further research below.

However, there was still the issue of why StCR was reporting 1126 subscriptions (and the database confirmed this number) while Comment Mail was only importing 930 subscriptions.

That led me to investigate the SQL queries that Comment Mail runs in unimported_post_ids(). I then manually ran various SQL queries on my database via phpMyAdmin. Finally, this SQL query led o

Post IDs that StCR has subscriptions for:

SELECT DISTINCT `post_id` FROM `wp_postmeta` WHERE `meta_key` LIKE '%\_stcr@\_%'

Returned: 346 Post IDs


What Comment Mail runs to search for Post IDs that have StCR subscriptions:

SELECT `ID` FROM `wp_posts` WHERE `post_status` = 'publish' AND `post_type` NOT IN('revision', 'nav_menu_item', 'redirect', 'snippet') AND `ID` IN (SELECT DISTINCT `post_id` FROM `wp_postmeta` WHERE `meta_key` LIKE '%\_stcr@\_%')

Returned: 341 Post IDs


So this left me with a discrepancy of 5 Post IDs... Why did StCR have 346 posts with subscriptions but Comment Mail was only picking up 341 posts during the import???

Perhaps some of the posts were deleted or no longer existed? (I rarely delete--almost never--delete published posts on my site, so I thought this was odd.) In any case, I decided to compare all of the Post IDs in wp_postmeta that contained a meta_key like _stcr@_ (i.e., all StCR subscription rows), with the main wp_posts table to see if any of the post_ids from the wp_postmeta table did not exist in the wp_posts table.

Sure enough, that's exactly what was happening:

SELECT DISTINCT `post_id` FROM `wp_postmeta` WHERE `meta_key` LIKE '%\_stcr@\_%' AND `post_id` NOT IN (SELECT DISTINCT `ID` FROM `wp_posts`)

Returned: 5 Post IDs


So, the reason that the number of posts Comment Mail imported (930) was different from the number of subscriptions reported by StCR (1129) was because during the import process Comment Mail validates that the Posts associated with the StCR subscriptions still exists. If the Post no longer exists, Comment Mail doesn't import that StCR subscription (which makes sense; there's no point in importing a subscription for a post that no longer exists).

@Reedyseth
Copy link
Contributor Author

My PR for this issue also includes a logging routine that we can use to report failures. I need to clean a few things up, but otherwise that work is mostly done.

Can you point me where is your dev branch so that I can test this as well?

@raamdev
Copy link
Contributor

raamdev commented Dec 17, 2015

@Reedyseth I referenced the PRs earlier: #166 (reference)

@raamdev raamdev modified the milestone: Next Release Dec 17, 2015
@Reedyseth
Copy link
Contributor Author

I referenced the PRs earlier: #166 (reference)

Thank you 👍

@raamdev
Copy link
Contributor

raamdev commented Dec 17, 2015

Ugh, well, I'm ALMOST there, quite literally 99.74% of the way there!!

With 1126 StCR subscriptions, Comment Mail now (as of my last commit: 5b99972) reports 831 imported and 292 skipped, which totals 1123 StCR subscriptions... so I'm missing 3, somehow (and those are not the 3 that are blacklisted; I tried removing the blacklisted email list and skipped went down to 289).

I will resume this tomorrow morning, in about 8 hours.


@Reedyseth The PR (#180) is now in a good state to start testing, including the logging routines (which create a file in the plugin folder called stcr-import-failures.log if there are any failures (if there are no failures, no log is created).

@raamdev
Copy link
Contributor

raamdev commented Dec 17, 2015

With 1126 StCR subscriptions, Comment Mail now (as of my last commit: 5b99972) reports 831 imported and 292 skipped, which totals 1123 StCR subscriptions... so I'm missing 3, somehow

Found it! These lines are giving precedence to R subscriptions and overwriting 6 StCR subscriptions with a Y status!

The crazy thing is that I was looking for 3 missing subscriptions when it was really 6, and I only thought it was 3 because the 3 blacklisted subscriptions were being double-counted as skipped. What are the chances that I'd have 3 blacklisted, exactly half as many as were being skipped by the line mentioned above?! Argh!!

@jaswrks
Copy link

jaswrks commented Dec 17, 2015

aha! That sounds like a good explanation to me.

@raamdev
Copy link
Contributor

raamdev commented Dec 17, 2015

2015-12-17_10-00-41

I got it! FINALLY! All skipped StCR subscriptions fully accounted for:

  • StCR Subscriptions Skipped via Blacklisted Email Addresses
  • StCR Subscriptions Skipped via missing Post IDs
  • StCR Subscriptions Skipped via missing or deleted/trashed/spammed comment
  • StCR Subscriptions Skipped via giving precedence to R subscription

This was the trickiest area. I added several comments to this section to explain things better. Also, this line was what I was missing to get my skipped count correct (i.e., to account for the skipped blacklisted subscriptions properly).


This is finally done. The only related thing is a new option to allow site owners to prevent the email blacklist from affecting subscription imports, but that's for another GitHub issue. :-)


@jaswsinc @Reedyseth If you could both review this PR and let me know if you have any questions or spot anything out of place, that would be helpful. I'm going to merge that later today and finally do an RC tonight.

@jaswrks
Copy link

jaswrks commented Dec 17, 2015

@raamdev Woohoo! That looks great. I'll review the PR shortly.

@Reedyseth
Copy link
Contributor Author

If you could both review this PR and let me know if you have any questions or spot anything out of place, that would be helpful. I'm going to merge that later today and finally do an RC tonight.

@raamdev I don't want to add more variants to the issue, I just pulled the branch feature/166 and tested it and my result was that I got no subscription imported at all. Take my comment only as a warning but it is what I just experience.

image

@jaswrks
Copy link

jaswrks commented Dec 17, 2015

@Reedyseth Can you try the following please?

  • Click the "Restore Default Options" button in Comment Mail.
  • Try running the import again.

Different results?

@Reedyseth
Copy link
Contributor Author

Can you try the following please?

Click the "Restore Default Options" button in Comment Mail.
Try running the import again.
Different results?

That worked ! Huurrayy !

image

raamdev added a commit to wpsharks/comment-mail-pro that referenced this issue Dec 18, 2015
raamdev added a commit that referenced this issue Dec 18, 2015
raamdev added a commit to wpsharks/comment-mail-pro that referenced this issue Dec 18, 2015
raamdev added a commit that referenced this issue Dec 18, 2015
raamdev added a commit to wpsharks/comment-mail-pro that referenced this issue Dec 18, 2015
raamdev added a commit that referenced this issue Dec 18, 2015
@raamdev
Copy link
Contributor

raamdev commented Dec 18, 2015

Next Lite Release Changelog:

  • Bug Fix: StCR Importer: Fixed a bug in how the import routine for importing Subscribe to Comments Reloaded (StCR) subscription counts imported subscriptions. Comment Mail was previously reporting the number of subscriptions that it created during the import as the total number imported, however due to a difference in how Comment Mail and StCR store subscriptions, the total number of StCR subscriptions imported is almost always going to be different from the number of subscriptions that Comment Mail creates. This difference resulted in confusion about why the total StCR subscriptions did not match the total reported by Comment Mail as "imported".

    This has been fixed by adjusting what is reported as 'total imported' and including two new pieces of information: 'total skipped' and 'total created'. When you add the 'total imported' and 'total skipped' numbers together, that number should equal the number of subscriptions reported in StCR. Props @Reedyseth @jaswsinc. See Issue #166.

  • Enhancement: StCR Importer: During the import process, Comment Mail now keeps track of and reports total number of StCR subscriptions skipped and total number of Comment Mail subscriptions created. Skipped subscriptions are the number of StCR subscriptions that were not imported for one of several possible reasons. See Issue #166.

  • Enhancement: StCR Importer: The importer now generates a log file in comment-mail/stcr-import-failures.log that includes reports of any failures or skipped subscriptions, along with information about why they failed or why they were skipped. Note that this file is only created if there were failures or skipped subscriptions during import. See Issue #166.

@raamdev
Copy link
Contributor

raamdev commented Dec 18, 2015

Next Pro Release Changelog:

  • Bug Fix: StCR Importer: Fixed a bug in how the import routine for importing Subscribe to Comments Reloaded (StCR) subscription counts imported subscriptions. Comment Mail was previously reporting the number of subscriptions that it created during the import as the total number imported, however due to a difference in how Comment Mail and StCR store subscriptions, the total number of StCR subscriptions imported is almost always going to be different from the number of subscriptions that Comment Mail creates. This difference resulted in confusion about why the total StCR subscriptions did not match the total reported by Comment Mail as "imported".

    This has been fixed by adjusting what is reported as 'total imported' and including two new pieces of information: 'total skipped' and 'total created'. When you add the 'total imported' and 'total skipped' numbers together, that number should equal the number of subscriptions reported in StCR. Props @Reedyseth @jaswsinc. See Issue #166.

  • Enhancement: StCR Importer: During the import process, Comment Mail now keeps track of and reports total number of StCR subscriptions skipped and total number of Comment Mail subscriptions created. Skipped subscriptions are the number of StCR subscriptions that were not imported for one of several possible reasons. See Issue #166.

  • Enhancement: StCR Importer: The importer now generates a log file in comment-mail-pro/stcr-import-failures.log that includes reports of any failures or skipped subscriptions, along with information about why they failed or why they were skipped. Note that this file is only created if there were failures or skipped subscriptions during import. See Issue #166.

@raamdev
Copy link
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 (#166).

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

No branches or pull requests

3 participants