-
Notifications
You must be signed in to change notification settings - Fork 130
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
[Improvements] Suggesting some improvements #25
Comments
Thank you for your support! 💯
Thanks for your contribution mate. |
Instead of using a lot of If cases you could use something like. def create_alert(id, site, new_ip, new_ip_second, score):
...
ALERT = {
1:message_ip,
2:message_ip,
3:message_ips,
4:message_web,
5:message_web_ip,
6:message_web_ip,
7:message_web_ip,
8:message_mail
9:message_mail_ip,
10:message_mail_ip,
11:message_mail_ip,
12:message_mail_web,
13:message_mail_ip_web,
14:message_mail_ip_web,
15:message_mail_ip_web,
}
if site.monitored:
type_ = ALERT[id]
try:
new_alert = Alert.objects.create(site=site,
type=type_,
new_ip=new_ip or None,
new_ip_second=new_ip_second or None,
old_ip=site.ip or None,
old_ip_second=site.ip_second or None)
...
except Exception:
... what do you think? |
Great idea! May you pull request your work? |
I could but the Exception handling is not very clear maybe it's better if you push this improvement. |
Ticket ID is no longer required. If empty, an incremental number will be generated automatically.
First of all thanks for creating and sharing such a great project. 💯
I have some suggestions regarding the code and the documentation.
For the documentation it would be useful if you can add docker and docker-compose version which are compatible and used by your project.
Regarding the code:
Django apps contains
core.py
maybe it's better to use a different filename as 'core' in Django also refers to builtin core modules of Django. I'd suggestservices.py
orutils.py
it would make the code more clear and readable.Regarding the
threats_watcher
app why are you saving only tokenized words inTrendyWord
and not the complete post content? IMHO it's more useful to have the complete content of a post.While I was checking your code I found that
RSS-Bridge
is used for Twitter. If the docker is used only for Twitter maybe you can use some tool like https://github.com/InQuest/ThreatIngestor which can extract and aggregate threat intelligence from RSS feeds, Twitter, Pastebin and other sources. This would smoother more your project as a docker image likeRSS-Bridge
is overkill if only used for twitter.In the
site_monitoring
app's model rtir – Identification number of RTIR should be a non required field as some people might not have https://github.com/bestpractical/rt RTIR platform configured o in place. Also theExpiry Date
field in some cases is not the best option as you might want to monitor a certain domain until it's takendown and you don't know the exact date when this will happen at first.Screenshots:
In
site_monitoring
and indns_finder
it's an awesome feature if you can take screenshots of domain and also check for changes of the image. You could use https://hub.docker.com/r/scrapinghub/splash to take screenshots and https://pypi.org/project/ImageHash/ to check how the image was changed.Why are you using MySQL as the DB? There are some other solutions like PostgreSQL which has builtin ArrayFields and JSONFields which could ease some model implementations.
Hope this helps.
The text was updated successfully, but these errors were encountered: