Feedback on the new Review Queue

Will do next time it happens :+1:

1 Like

The plugin monkey patches the UserDestroyer, which is called when you perform the delete action from the review queue. This looks suspicious:

https://github.com/paviliondev/discourse-follow/blob/master/plugin.rb#L111

If somehow following_ids includes an invalid id, User#find will raise an exception, and the user will see a “404 Not Found” message. I recommend using User.find_by(id: ...) instead.

I can’t tell if this is what happened on @bartv site without looking inside the DB, so next time it happens, I recommend looking at the flagged user’s following_ids.

4 Likes

Thanks. That’s true and a very useful highlight.

I still would still like logs and that 404 URL confirmed.

There are probably even more ways to make this more robust.

4 Likes

Pushed: https://github.com/paviliondev/discourse-follow/commit/b523b3aee2d2f0e01934ef63131bc85e3c7caf34 thanks!

3 Likes

We merged a PR to add this action. Confirm, confirm + delete, and confirm + suspend are now bundled together:

Screen Shot 2020-09-02 at 14.30.23

4 Likes

I removed the Follow Plugin and my issue remained. In the rails log I see:

Started PUT "/review/6793/perform/reject_user_block?version=0" for xx.xx.xx.xx at 2020-09-03 09:45:48 +0000
Processing by ReviewablesController#perform as */*
  Parameters: {"version"=>"0", "reviewable_id"=>"6793", "action_id"=>"reject_user_block"}
Job exception: undefined method `strip' for nil:NilClass

  Rendering text template
  Rendered text template (Duration: 0.0ms | Allocations: 1)
Started GET "/t/global-variables/331828" for xx.xx.xx.xx at 2020-09-03 09:45:48 +0000
Processing by TopicsController#show as HTML
  Parameters: {"slug"=>"global-variables", "topic_id"=>"331828"}
Completed 404 Not Found in 292ms (ActiveRecord: 0.0ms | Allocations: 124598)
ActiveRecord::RecordNotFound (Couldn't find all Topics with 'id': (1185852, 1185853, 1186324, 1186929, 1191089) [WHERE ("topics"."deleted_at" IS NULL)] (found 4 results, but was looking for 5).)
lib/plugin/instance.rb:393:in `block in on'
lib/discourse_event.rb:14:in `block in trigger'
lib/discourse_event.rb:13:in `trigger'
app/models/user.rb:1567:in `trigger_user_destroyed_event'
app/models/reviewable.rb:353:in `perform'
app/controllers/reviewables_controller.rb:192:in `perform'
app/controllers/application_controller.rb:340:in `block in with_resolved_locale'
app/controllers/application_controller.rb:340:in `with_resolved_locale'
lib/middleware/omniauth_bypass_middleware.rb:68:in `call'
lib/content_security_policy/middleware.rb:12:in `call'
lib/middleware/anonymous_cache.rb:336:in `call'
config/initializers/008-rack-cors.rb:25:in `call'
config/initializers/100-quiet_logger.rb:23:in `call'
config/initializers/100-silence_logger.rb:31:in `call'
lib/middleware/enforce_hostname.rb:22:in `call'
lib/middleware/request_tracker.rb:176:in `call'

The ActiveRecord::RecordNotFound line is interesting as it appears every time I try to delete a user from the review queue, and with the exact same topic ID’s.

I checked the topic ID’s and these are all posts that use the Events plugin. And sure enough, one of them has been deleted. I’m puzzled why this action from the review queue would check for these though?

@merefield seems like I’m getting back to you guys again, sorry :slight_smile:

EDIT: changed plugin link

3 Likes

That’s a core plugin.

1 Like

Wait, I’m using

git clone https://github.com/paviliondev/discourse-events.git

Is that not the right one (anymore?) (I think I may have linked to the wrong page there, fixing)

1 Like

Thats Events. @fzngagan any thoughts on this?

3 Likes

CLicking it points me to the correct repo.

1 Like

Haha, no the issue wrt deleting a user from the Review Queue.

4 Likes

This is not related to a plugin, this is a core library file.

Why do you think Events is involved?

3 Likes

Yes, when a user is deleted, We remove them from any events they’ve rsvp’d for. Maybe that code is causing this.

3 Likes

Because all the topic IDs in the error are topics that use Events.

Hmm, I’m fairly certain that these specific users that I’m deleting have not RSVP’ed though., but it could be that these are the only topics that have RSVP enabled?

3 Likes

Oh oh got it.

https://github.com/paviliondev/discourse-events/blob/85cc5a5c01e67135d8b06d5a386aa56192fb294c/plugin.rb#L301

@fzngagan this is using unsafe .find(?

topics = Topic.find(topic_ids) if topic_ids

See above fix I just pushed to Follow (though solution here may need to be different as multiple ids - use where?)

3 Likes

I just updated but still get the 404.

1 Like

Bart, hold off for @fzngagan to apply and confirm a fix.

5 Likes

I’ve pushed a fix. It should hopefully resolve the issue. Please check and confirm.

5 Likes

That fixed it, thanks!!

6 Likes

I have seen now multiple times posts in the review queue with the reason “New user typed their first post suspiciously fast”.
Upon further checking I noticed that those posts contained a watched word, but this was not mentioned in the review queue.

Since the “New user typed their first post suspiciously fast” flagging is 96% of the time wrong, and the “watched words” flaggings are 100% of the time correct, i.e. if a post gets into the review queue due to a watched word, it is 100% of the time a post that really needs attention, I feel that the “watched words” should have precedence over the “new user typed too fast”.

Imagine the following situations:

  1. Post gets into review queue due to “New user typed their first post suspiciously fast”. This post contains an invisible spam link which is in the “watched words” list. -> Post gets approved, since nobody notices the invisible link (e.g. link behind a “.”). -> Fail!
  2. Post gets into review queue due to a watched word (“watched words” having precedence over “typed too fast”). This post contains an invisible spam link which is in the “watched words” list. -> Post gets rejected and spammer gets deleted due to “watched words” -> Win!
5 Likes