Skip to content

Hide comments from banned users

Jay requested to merge jswain_hide_notes_from_banned_users into master

What does this MR do and why?

This MR hides comments from banned users. Admins are not affected.

This is the first pass, agreed upon by trust and safety. The next pass will involve decorating the banned comment with a badge, so it is easily identifiable for admins.

part of: https://gitlab.com/gitlab-org/gitlab/-/issues/327356

Screenshots or screen recordings

banned_hidden_comment_demo

How to set up and validate locally

  1. Create a user that will be banned, lets call them BadActor
  2. Invite BadActor to a project, and make a comment on an issue with their account.
  3. Create a 2nd user, lets call them GoodUser
  4. Ban BadActor User.find_by(username: 'BadActor').ban!
  5. Confirm you can see the comment BadActor made from GoodUser's account.
  6. Enable the feature Feature.enable(:hidden_notes)
  7. Confirm you can no longer see the comment BadActor made from GoodUser's account.
  8. Confirm an Admin account can see BadActor's comment.

Database

I referenced the following queries from spec/controllers/projects/notes_controller_spec.rb:72.

This is the query with the feature flag disabled (explained)

Click to expand
SELECT "notes"."id",
       "notes"."note",
       "notes"."noteable_type",
       "notes"."author_id",
       "notes"."created_at",
       "notes"."updated_at",
       "notes"."project_id",
       "notes"."attachment",
       "notes"."line_code",
       "notes"."commit_id",
       "notes"."noteable_id",
       "notes"."system",
       "notes"."st_diff",
       "notes"."updated_by_id",
       "notes"."type",
       "notes"."position",
       "notes"."original_position",
       "notes"."resolved_at",
       "notes"."resolved_by_id",
       "notes"."discussion_id",
       "notes"."note_html",
       "notes"."cached_markdown_version",
       "notes"."change_position",
       "notes"."resolved_by_push",
       "notes"."review_id",
       "notes"."confidential",
       "notes"."last_edited_at",
       "notes"."internal"
FROM   "notes"
WHERE  "notes"."noteable_id" = 327356
       AND "notes"."noteable_type" = 'Issue'
       AND ( updated_at > '1969-12-31 23:59:55' ) 

This is the query with the feature flag enabled (explained)

Click to expand
explain SELECT "notes"."id",
       "notes"."note",
       "notes"."noteable_type",
       "notes"."author_id",
       "notes"."created_at",
       "notes"."updated_at",
       "notes"."project_id",
       "notes"."attachment",
       "notes"."line_code",
       "notes"."commit_id",
       "notes"."noteable_id",
       "notes"."system",
       "notes"."st_diff",
       "notes"."updated_by_id",
       "notes"."type",
       "notes"."position",
       "notes"."original_position",
       "notes"."resolved_at",
       "notes"."resolved_by_id",
       "notes"."discussion_id",
       "notes"."note_html",
       "notes"."cached_markdown_version",
       "notes"."change_position",
       "notes"."resolved_by_push",
       "notes"."review_id",
       "notes"."confidential",
       "notes"."last_edited_at",
       "notes"."internal"
FROM   "notes"
WHERE  "notes"."noteable_id" = 327356
       AND "notes"."noteable_type" = 'Issue'
       AND ( updated_at > '1969-12-31 23:59:55' )
       AND ( NOT EXISTS (SELECT 1
                         FROM   "banned_users"
                         WHERE  ( notes.author_id = banned_users.user_id )) ) 

This is the query with a system note filter. I referenced the following queries from spec/controllers/projects/notes_controller_spec.rb:63.

This is the query with the feature flag disabled (explained)

Click to expand
explain SELECT "notes"."id",
       "notes"."note",
       "notes"."noteable_type",
       "notes"."author_id",
       "notes"."created_at",
       "notes"."updated_at",
       "notes"."project_id",
       "notes"."attachment",
       "notes"."line_code",
       "notes"."commit_id",
       "notes"."noteable_id",
       "notes"."system",
       "notes"."st_diff",
       "notes"."updated_by_id",
       "notes"."type",
       "notes"."position",
       "notes"."original_position",
       "notes"."resolved_at",
       "notes"."resolved_by_id",
       "notes"."discussion_id",
       "notes"."note_html",
       "notes"."cached_markdown_version",
       "notes"."change_position",
       "notes"."resolved_by_push",
       "notes"."review_id",
       "notes"."confidential",
       "notes"."last_edited_at",
       "notes"."internal"
FROM   "notes"
WHERE  "notes"."noteable_id" = 327356
       AND "notes"."noteable_type" = 'Issue'
       AND ( updated_at > '1969-12-31 23:59:55' )
       AND "notes"."system" = false 

This is the query with the feature flag enabled (explained)

Click to expand

SELECT "notes"."id",
       "notes"."note",
       "notes"."noteable_type",
       "notes"."author_id",
       "notes"."created_at",
       "notes"."updated_at",
       "notes"."project_id",
       "notes"."attachment",
       "notes"."line_code",
       "notes"."commit_id",
       "notes"."noteable_id",
       "notes"."system",
       "notes"."st_diff",
       "notes"."updated_by_id",
       "notes"."type",
       "notes"."position",
       "notes"."original_position",
       "notes"."resolved_at",
       "notes"."resolved_by_id",
       "notes"."discussion_id",
       "notes"."note_html",
       "notes"."cached_markdown_version",
       "notes"."change_position",
       "notes"."resolved_by_push",
       "notes"."review_id",
       "notes"."confidential",
       "notes"."last_edited_at",
       "notes"."internal"
FROM   "notes"
WHERE  "notes"."noteable_id" = 327356
       AND "notes"."noteable_type" = 'Issue'
       AND ( updated_at > '1969-12-31 23:59:55' )
       AND "notes"."system" = false
       AND ( NOT EXISTS (SELECT 1
                         FROM   "banned_users"
                         WHERE  ( notes.author_id = banned_users.user_id )) ) 

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Jay

Merge request reports

Loading