-
Notifications
You must be signed in to change notification settings - Fork 31
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
base: master
Are you sure you want to change the base?
[F] Spam mitigation #3782
Conversation
1ed892f
to
689af48
Compare
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 |
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 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:
- There are various conditions on visibility that exclude annotations from indexing that we don't care about in admin.
- 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.
95b037d
to
2fe11c7
Compare
a1d76da
to
a6ff497
Compare
@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. |
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.
@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.
- Should we have a confirmation modal for the Unflag button?
- I incidentally found a missing i18n key:
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:
- 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).
- I might also change this a bit. Change copy to
<b>2</b> resolved flags
and drop the bottom border.
- 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?
This PR improves instance admins' ability to deal with spam by: