-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Filter the results of user directory searching via the spam checker #6888
Conversation
I think this probably needs another test or two (at least checking the backwards compatibility code-path). |
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.
looks good to me. A few nits and suggestions here. I think the only thing I really want changed is use of the term banned
.
I'm also wondering if this is the right time to start a quick doc in https://github.com/matrix-org/synapse/blob/master/docs about what API the spam checker should implement is. WDYT? |
Thanks for the comments @richvdh! I can certainly add a document about the spam checker too. |
@richvdh I handle the review comments, my inclination is to do the documentation as a separate PR so that this doesn't block the actual need. Does that make sense? |
See #6906 for the documentation. Note that it doesn't actually include the documentation for this PR. I'll either update that PR or this one based on which is merged first! |
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.
lgtm, though if @turt2live wants a profile dict, better give him one.
Co-Authored-By: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
* Change the filtering to a list comprehension. * Update the type comments to be type annotations. * Modify the method name.
caaa63b
to
2be58bd
Compare
@turt2live / @richvdh I believe I've made all the requested changes, please take another look! |
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.
lgtm from a design perspective. Will leave the Synapse bits to vdh/the synapse team.
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.
lgtm too!
Fixes #5648