Feedback on the new Review Queue

If one wanted to write a plugin specific to the Review Queue (new users)
Is there a few hints you guys can provide on the getting started side of things?

End goal I want to provide a 3rd option to new users from Delete, Delete and Block I want a 3rd Custom option that will do other things.
I have looked through the getting started with plugins topic and that’s’ all well and good just looking on pointers on how to hook into the Review Queue

4 Likes

This is an interesting use case and I’d like to help you get it working.

Actions for reviewable types are returned from the build_actions method.

What I’d recommend is having your plugin open the ReviewableUser class and alias the build_actions method. In your version, get the actions from the method you aliased, then add your new action to the delete bundle.

That should work, although you might end up with some hacky looking code. I’d suggest once you get it working to post it here and we can see if we can tidy it up, and perhaps add internal APIs to help out improve it further.

8 Likes

Robin,

I’m currently preparing a PR for the Discord OAuth Plugin mainly to store more Discord user information in Discourse. I’m trying to use your ReviewableUser model in order to retain functionality that implements automated approval.

Because the implementation currently enters a review for new users asynchronously I need to check if such a review has been created and clear it.

Unfortunately I’m getting a very strange Ruby error and wondered if you have come across it.

Code is:

  def after_create_account(user, auth)
    super
    
    data = auth[:extra_data]
    if !user.approved && data[:auto_approve]
      user.approved = true
      user.approved_by_id = Discourse.system_user.id
      if reviewable_user = ReviewableUser.find_by(target: user)
          reviewable_user.set_approved_fields!(user, Discourse.system_user)
      end
    end
  end

As soon as it fires ReviewUser.find_by I get an exception:

*** NameError Exception: wrong constant name #<Class:0x000056134e417870>::DiscordAuthenticator

Whilst I thought I was making good strides in my Ruby, i’m not clear why it’s doing this?

Is it a path issue? I’ve tried a bunch of requires but that snowballs.

It’s very like similar patterns elsewhere in the core code. Any thoughts are much appreciated!

Repo here if you need: https://github.com/merefield/discourse-plugin-discord-auth/blob/master/plugin.rb

That constant error isn’t very informative is it! I think it has to do with Rails engines and namespaces. One quick thing you could try is ::ReviewableUser with the two colons before it. That tells it to use the root namespace, not in the engine.

This code also looks somewhat out of date for the reviewable API. It should be something like this:

if !user.approved && data[:auto_approve] && reviewable = ::ReviewableUser.pending.find_by(target: user)
  reviewable.perform(:approve, Discourse.system_user)
end
5 Likes

That cleared the error. I assume that because it couldn’t find the Class before, it was treating it as a Constant? In any case, that’s solved it, brilliant, thanks so much! I’m unstuck!

So the reason I had this:

      user.approved = true
      user.approved_by_id = Discourse.system_user.id

before:

reviewable.perform(:approve, Discourse.system_user)

is because the addition to the review queue is asynchronous. In the job, the review is only created if approve is false from (discourse/app/jobs/regular/create_user_reviewable.rb at 888e68a1637ca784a7bf51a6bbb524dcf7413b13 · discourse/discourse · GitHub):

    if user = User.find_by(id: args[:user_id])
      return if user.approved?

      @reviewable = ReviewableUser.needs_review!(
        target: user,
        created_by: Discourse.system_user,
        reviewable_by_moderator: true,
        payload: {
          username: user.username,
          name: user.name,
          email: user.email
        }
      )

There’s a risk that the job may fire after you’ve tested for the review entry?

The result of that is the review entry doesn’t appear to exist but the job is just waiting to run. The job then runs and creates the review entry and you’ve missed the opportunity to clear it as your code to test this has already run.

It’s a potential race condition?

Make approved true before you check for a review entry and you have solved the problem (because a review will never be created after this as its a dependency).

I ran into the problem testing my code - on dev it worked, then on production it failed as things ran in a different order.

NB I appreciate you have not written this to support this use case, but I think it’s important to allow plugins to be able to auto-approve new users for special circumstances (e.g. like the Discord plugin which does so if the user is a member of a trusted guild).

1 Like

Indeed the reviewable record is created asynchronously and that is problematic in this situation as logging in will create the user and the approval record may not be there.

A better solution would be to not create the reviewable in this situation, however it would require changes to core to work properly. It could work like this:

  • The auth result could return a field like skip_approval: true
  • In core, if the auth result contains that field, it does not not create the reviewable, and if approval is required it updates the fields correctly.
5 Likes

Thanks Robin, yes.

For now I’ve gone with my workaround conscious that it will need to be addressed before the direct API access is removed.

Is there any appetite from the Team to prioritise that before the direct user.approve write access is removed?

I believe that change will break the current the trusted Guild auto-approve feature in the Auth Discord Login plugin (even without the PR I’ve just raised to that plugin). @featheredtoast

Would be happy to update my PR with support for that change if implemented.

I can’t see us removing this any time soon. I don’t want to rely on it forever but it should be good for a while.

3 Likes

The queue is nice and ‘group by topic’ is useful too.

However, as far as I can tell, there isn’t a way to bulk select and process posts. Each post needs to be processed individually.

Bulk selection and processing would be a real time saver!

45

4 Likes

I think it would be a great enhancement. To be fair, the Flag queue never had bulk operations so it’s not like this is a regression.

Also, some operations like “Suspend” are a little weird when you’re doing multiple rows. It would have to be restricted to the basic operations to make any sense.

8 Likes

Found a small problem, someone posted in a category that requires approval so it shows up in the review queue. They posted in the wrong category so I try to change it in the review queue, no problem except the category I’m trying to move it to requires one tag, but the tag is restricted to the new category and the post in review queue still thinks it’s in the original category which does not allow that tag, so I’m stuck. Easy enough to approve the post and change it afterwards but seems like something that should be fixed.

5 Likes

I know this is meant to be fixed, but URLs still aren’t being added to the screened list. It probably doesn’t even really matter, the action for screened URLs says “do nothing” anyway. Just weird.

Just to confirm you’ve upgraded to the correct version? They should be screened if not onebox capable or internal.

3 Likes

Yep, I’m on v2.4.0.beta2 +66. I’ll make a copy of the post next time it happens.

Yep, after clicking on “Delete User” in the review queue the email and IP address were added to the screened lists, but no URLs were. The post was:

Want [Academic Writing Service](https://myassignmenthelp.com/uk/academic-writing-service.html) online at World No.1 Academic writing service provider Myassignmenthelp.com. Professional academic Writing Service offering wide range for students with best price.
1 Like

URLs have never been added to the screened list automatically to my recollection? Well, I guess they are, I checked my instance and there are a bunch of URLs there :wink:

Ah yes, I remembered. They don’t actually do anything, they’re only informational.

You can add those domain names to the watched words so that they can’t be entered but you’ll have to do it yourself.

That’s why it would be nice if they were added to the screened list - even if no automatic action is taken on the URLs it would let us see what spam is being posted. Keep in mind that mods can be using the review queue but can’t (I think) add watched words.

1 Like

Some minor UX feedback: it would be great if the review queue would remember my settings. Since we work with a team of moderators I’m personally mostly interested in seeing new flags and I sort by ‘Created At’, but this setting doesn’t ‘stick’.

image

7 Likes

That’s not a bad suggestion. Fortunately for now there is a workaround - the search filters are persisted to the query string (URL). If you bookmark a filter you can always come back to it.

4 Likes