Code review comment for lp:~widelands-dev/widelands-website/anti_spam

Revision history for this message
kaputtnik (franku) wrote :

Just short answers because i am in hurry:

> 1) Is it necessary that we ship the askimet client in our repo, i.e. can it
> not be installed wia pip_requirements.txt?

I don't why i am downloaded the complete zip file, whereas one could also download the py-file (containing all classes and functions) I have removed the complete zip file now in this branch. Regarding to pip_requirements: I actually don't know, sorry. But a single file shouldn't be a problem for our repo.

> 3) You should also check in the title of the post, not only the body. We had
> some spam that only had data in the title. You can do that cheaply by just
> concatenating: text = title + body and then working over text.

This is already in there. See the description of this merge proposal "Topic name filter: "

> 4) It might make sense to add a regular expression checking for international
> phone numbers - I do not think such a post can ever be legit.

Yes, but i am not a regular expression freak :-D Will take some time and tests for me to do something like that.

> 5) You are marking any post of a user with 'ji' in its name as SPAM. Is that
> not a bit too broad?

Yes, maybe change it into ' ji ' or ' ji'

> 6) There is no admin notification right now - so we will not know if posts are
> in the queue for moderation. I think that is fine for now, but we will need to
> remember to check daily once this is deployed until we have that. I am all for
> deploying this soon though, this SPAM is annoying.

Yes, if we add admin notification, we should also prevent notification of normal users (who has 'Inform me on new topics" activated).

Thanks for the changes :-)

« Back to merge proposal