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: Comments that are "Awaiting Approval" are not imported #182

Closed
raamdev opened this Issue Dec 15, 2015 · 7 comments

Comments

Projects
None yet
2 participants
@raamdev
Contributor

raamdev commented Dec 15, 2015

The StCR Importer only imports Replies Only (R) subscriptions for comments that are approved (see this line). However, that introduces a small problem:

What if the site owner is switching to Comment Mail and doing the StCR → Comment Mail migration while they have several (valid) comments awaiting approval? Those comments could have (valid) StCR subscriptions. However, if a site owner does the StCR import, disables StCR, enables Comment Mail, and then approves those comments, the comments won't have any subscriptions associated with them (because Comment Mail ignored the StCR subscriptions for those comments that were awaiting approval).

That means we have two options here:

  • Ignore the approved status of the comment when importing StCR subscriptions
  • Show a big warning message to site owners on the StCR Import page when there are comments awaiting approval that tells them they should approve those comments first.

I would normally vote for the second option, however what if a site owner has thousands of comments with a lot of unapproved comments and they simply don't want to spend the time going through all the unapproved comments right now? In that scenario, they couldn't do the import without losing all those subscriptions.

So my vote is to allow unapproved comments during the import. What harm is there in importing StCR subscriptions for for comments that have not yet been approved? Since no email will be sent to those unapproved comments anyway (see this line, where the comment's status is validated before any associated subscription email is sent by Comment Mail), importing subscriptions for unapproved comments should cause no harm.

@raamdev raamdev added bug stcr labels Dec 15, 2015

@jaswrks

This comment has been minimized.

Member

jaswrks commented Dec 15, 2015

Great observation. Thinking.... 💭

@jaswrks

This comment has been minimized.

Member

jaswrks commented Dec 15, 2015

This is undocumented, but the last time I checked (i.e., whenever I built CM originally), comment statuses in WP can be any of following. Lots of crazy variations in the WP core going back one version to the next.

One of the following:
Not approved yet: `0`, ``, `hold`, `unapprove`, `unapproved`, `moderated`
Approved: `1`, `approve`, `approved`
Trashed or spammed: `trash`, `post-trashed`, `spam`, `delete`

So my vote is to allow unapproved comments during the import.

I don't have any objection to the behavior being changed. You know the existing StCR user base better than I do, so I think whatever you'd like is fine. I would just caution that importing subscribers associated with comments that have a status of trashy-ish, spamy-ish, or delete could be considered a bug. Weeding those during the move to CM could be a good thing, depending on how you look at it.

@raamdev

This comment has been minimized.

Contributor

raamdev commented Dec 16, 2015

@jaswsinc writes...

This is undocumented, but the last time I checked (i.e., whenever I built CM originally), comment statuses in WP can be any of following. Lots of crazy variations in the WP core going back one version to the next.

Ah, thanks! I did not realize that there were so many possibilities.

I agree that keeping trash, post-trashed, spam, and delete out makes sense, so that means we should just be able to update the SQL query from this:

AND `comment_approved` IN('approve', 'approved', '1')

to this, right?

AND `comment_approved`  NOT IN ('trash', 'post-trashed', 'spam', 'delete')
@jaswrks

This comment has been minimized.

Member

jaswrks commented Dec 16, 2015

Yes. That looks right to me also.

@raamdev

This comment has been minimized.

Contributor

raamdev commented Dec 17, 2015

Next Lite Release Changelog:

  • Bug Fix: Fixed a bug with the StCR Import routine that was preventing subscriptions for comments awaiting approval from being imported. See Issue #182.
@raamdev

This comment has been minimized.

Contributor

raamdev commented Dec 17, 2015

Next Pro Release Changelog:

  • Bug Fix: Fixed a bug with the StCR Import routine that was preventing subscriptions for comments awaiting approval from being imported. See Issue #182.

@raamdev raamdev closed this Dec 17, 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 (#182).

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

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