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

Implement premod new user #1109

Closed
wants to merge 28 commits into from

Conversation

J-Wicks
Copy link

@J-Wicks J-Wicks commented Oct 17, 2017

What does this PR do?

Reference #1102 This pull requests adds the "Enable Premod New User" feature at the Settings and Asset level. Enabling this setting will cause all non-staff new users comments to go into premoderation.

A "New User" is a user that has not yet had a comment approved by staff. This is indicated by a new "newUser" property that is defaulted to true when a user signs up. Approving a comment will trigger this indicator to flip to false. At this time, staff members will also be set to newUser: true but this will not affect their ability to comment, as the logic checks their roles before moving to premod.

How do I test this PR?

Tests are located at test/server/graph/mutations/createComment.js folder.

@kgardnr
Copy link
Contributor

kgardnr commented Oct 17, 2017

Wow thanks @J-Wicks! We will review this today 💯

kgardnr
kgardnr previously approved these changes Oct 17, 2017
Copy link
Contributor

@kgardnr kgardnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works great! Just need a sign-off on code review and we can merge.

Copy link
Collaborator

@wyattjoh wyattjoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been off and on in the last few days, apologies for the lack of support while developing this feature. It seems like there was an oversight in the original acceptance criteria. In addition to the features described in #1102, this feature should:

  • Not interfere with existing users on the platform, (ie, if a user creates an account before the feature is enabled, they should not be impacted by this setting change).

As such, I'm suggest the following changes:

  • Replace the premodNewUserEnable: { type: Boolean, default: false } with a premodNewUsersAfter: { type: Date, default: null }.
  • Remove the newUser Boolean and instead query the comments to see if this user has at least one ACCEPTED comment.

Why you ask? Simple, we have a created_at timestamp on the user when they are first inserted into the database, insead of checking a newUser boolean. This would enable additional criteria. If I were to design the moderation phase for this:

async ({user}, comment, {assetSettings: {premodNewUsersAfter}}) => {

  // If the premodNewUsersAfter feature is enabled, then ensure that the user
  // is new enough to warrant the check.
  if (
    premodNewUsersAfter !== null &&
    user.created_at >= premodNewUsersAfter
  ) {

    // If the karma for the comment is above 0, then we know that there
    // was at least one approved comment.
    if (
      user.metadata &&
      user.metadata.trust &&
      user.metadata.trust.comment &&
      user.metadata.trust.comment.karma > 0
    ) {
      return;
    }

    // We weren't able to determine with the Karma if the user has had an approved comment
    // or not. We need to check with the comment's collection.
    const acceptedComments = await CommentModel.find({
      author_id: user.id,
      status: 'ACCEPTED',
    }).count();

    if (acceptedComments < 0) {
      return {
        status: 'PREMOD',
      };
    }
  }
}

This would be a forward compatible change, as your current solution would not work for existing installations. The added benifit to this structure is that if we decided down the line to give moderators the ability to enable this feature for a specific article only, it would work without an issue :)

I'm happy for feedback, and greatly appriciate the time and work that's gone into this feature so far.

Let me know if you have any questions @J-Wicks!

@J-Wicks
Copy link
Author

J-Wicks commented Oct 18, 2017

Thanks for the feedback! I should be able to get to work on this on Friday and turn it around in about a day.

@J-Wicks
Copy link
Author

J-Wicks commented Oct 22, 2017

Still working on this, but I have some work stuff going on this week.

@J-Wicks
Copy link
Author

J-Wicks commented Oct 31, 2017

Still on it! Having issues getting the date set but should be able to figure it out soon.

@kgardnr
Copy link
Contributor

kgardnr commented Oct 31, 2017

Awesome thanks @J-Wicks!

@J-Wicks
Copy link
Author

J-Wicks commented Nov 2, 2017

Good Morning!

I've gotten most of the work done, but I'm having a bizarre error. When I add the premodNewUserEnable date it works fine, but then when I toggle the option off and try to save my change, the hasEqualLeaves function throws an error. It seems like the configure container is receiving the premodNewUserEnable setting as an array, but for the life of me I can't figure out why. Perhaps someone else could take a look? I will continue to look into it but need to take a quick break as I'm not making any progress.

@kgardnr
Copy link
Contributor

kgardnr commented Nov 3, 2017

Yes @J-Wicks! Apologies for the delay here, most of our team is out already for the week, but we will def look at this next week and send some direction for ya!

@kgardnr
Copy link
Contributor

kgardnr commented Nov 30, 2017

Hey @J-Wicks, sorry for the delay - we've done a big refactor of Configure that's now in master, and I think this may resolve the issue you were seeing. Would you mind trying again once you resolve the conflicts there? Let us know if you need help!

@J-Wicks
Copy link
Author

J-Wicks commented Dec 1, 2017

Thanks Kim. I will look at this ASAP.

@kgardnr
Copy link
Contributor

kgardnr commented Dec 21, 2017

Hey @J-Wicks, I'm going to close this PR for now - but when you come back to it, feel free to re-open it! Thank you for your contributions, we look forward to working with you more in 2018!

Happy holidays!

<3,
Kim & The Coral Team

@kgardnr kgardnr closed this Dec 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants