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

[F] Spam mitigation #3782

Open
wants to merge 99 commits into
base: master
Choose a base branch
from
Open

[F] Spam mitigation #3782

wants to merge 99 commits into from

Conversation

1aurend
Copy link
Contributor

@1aurend 1aurend commented Oct 31, 2024

This PR improves instance admins' ability to deal with spam by:

  • Adding new searchable lists in admin for
    • Annotations
    • Comments
    • Reading Groups
    • Users
  • Allowing admins to delete those records
  • Updating flags functionality to make it clearer to users
  • Surfacing flags and flagged records in admin and allowing admins to resolve flags

Comment on lines +378 to +389
def build_keyword_scope(value)
escaped = value.gsub("%", "\\%")

needle = "%#{escaped}%"

body_matches = where arel_table[:body].matches(needle)

creator_matches = joins(:creator).where(User.arel_table[:first_name].matches(needle).or(User.arel_table[:last_name].matches(needle)))

creator_matches.or(body_matches).distinct
end
Copy link
Contributor Author

@1aurend 1aurend Nov 26, 2024

Choose a reason for hiding this comment

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

This scope is my main outstanding question on this PR— Does this scope need to be called something other than keyword to not conflict with search indexing? I don't think using the existing search for admin is right for two reasons:

  1. There are various conditions on visibility that exclude annotations from indexing that we don't care about in admin.
  2. A full text search of the annotation subject is probably not helpful for identifying spam, since spam users aren't creating texts. Creator name and body seem most likely to be what admins are searching for.

@1aurend 1aurend marked this pull request as ready for review December 17, 2024 18:31
@1aurend 1aurend requested a review from dananjohnson December 18, 2024 18:30
@1aurend
Copy link
Contributor Author

1aurend commented Dec 31, 2024

@scryptmouse I know you've looked at a lot of this already, but I want to make sure to get your final 👍 before this gets merged. No rush, though, since Dana's also going to take a look when he's back.

@1aurend 1aurend requested a review from scryptmouse December 31, 2024 22:36
Copy link
Member

@dananjohnson dananjohnson left a comment

Choose a reason for hiding this comment

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

@1aurend, this is great work and a really significant improvement to Manifold! I didn't review your code but did test as much as I could think of to test, and I caught a number of little issues. I doubt I was exhaustive, so be sure to click around a bit more before this gets merged just to be sure everything is tip-top!


Reader

  • The Flag modal style could use some polish. The bold text below the title seems like it should be unbolded, the font size of the textarea should be smaller to match the modal body text, and button styles should be made consistent with others on the frontend. I'd take a look around and see if there's some precedent in other parts of the frontend to borrow from. The Archive RG confirmation modal comes to mind as one but there may be better examples.
  • Some of these buttons don't have a focus style.
Screenshot 2025-01-08 at 1 09 17 PM
  • Should we have a confirmation modal for the Unflag button?
  • I incidentally found a missing i18n key:
Screenshot 2025-01-08 at 1 47 16 PM

Admin

  • The flags filter on annotations doesn't seem to work right. When I set it to filter "with flags" it still shows all my annotations.
  • The state of the Select All/None bulk toggle is only partially responsive to the checked state of the individual items. Like I see that if I select all then uncheck one item, the toggle switches from "Clear selection" to "Select all". But if I manually check all the items, then the toggle should also switch from "Select all" to "Clear selection". Also, the toggle should have a corresponding aria-pressed=true|false|mixed attribute for a11y. See https://adrianroselli.com/2024/03/check-all-expand-all-controls.html.
  • The Bulk Delete button and Cancel buttons change size when hovered.
Screen.Recording.2025-01-08.at.1.33.17.PM.mov
  • The focus states for those buttons also need a color contrast pass. Are there maybe some BEM modifier classes that can be used to match focus styles for similar buttons elsewhere?
  • I get a 500 error when attempting to filter a single RG's annotations to "With flags"
Unsupported argument type: #\u003cAnnotation::ActiveRecord_AssociationRelation:0x000000011e5fc5c0\u003e (Annotation::ActiveRecord_AssociationRelation)
  • Oops, WIP warning text:
Screenshot 2025-01-08 at 1 49 22 PM
  • This list style looks a bit off. I would tighten up the bottom padding on each item and make the border fainter (see the border color on the Makers list view, for instance).
Screenshot 2025-01-08 at 1 52 12 PM
  • I might also change this a bit. Change copy to <b>2</b> resolved flags and drop the bottom border.
Screenshot 2025-01-08 at 1 54 16 PM
  • Some of the record list views have double thickness bottom borders on the items. See Makers, Annotations, Comments.
  • When deleting an annotation, the deletion was successful but the drawer didn't close, so the deleted record was still sitting there on screen.
  • Unrelated regression here?
Screenshot 2025-01-08 at 1 57 05 PM

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