-
Notifications
You must be signed in to change notification settings - Fork 356
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
Conversation
Wow thanks @J-Wicks! We will review this today 💯 |
There was a problem hiding this 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.
There was a problem hiding this 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 apremodNewUsersAfter: { type: Date, default: null }
. - Remove the
newUser
Boolean
and instead query the comments to see if this user has at least oneACCEPTED
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!
Thanks for the feedback! I should be able to get to work on this on Friday and turn it around in about a day. |
Still working on this, but I have some work stuff going on this week. |
Still on it! Having issues getting the date set but should be able to figure it out soon. |
Awesome thanks @J-Wicks! |
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. |
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! |
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! |
Thanks Kim. I will look at this ASAP. |
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, |
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.