Merge updates to attachments scanning into master#11
Conversation
Also changed email alerts from virus uploads to use default from email
into lbrant/master Conflicts: attachments/exceptions.py attachments/views.py
| from django.utils.encoding import force_text | ||
| from django.views.decorators.csrf import csrf_exempt | ||
| from django.core.mail import send_mail | ||
| from django.conf.global_settings import DEFAULT_FROM_EMAIL |
There was a problem hiding this comment.
I think you want settings.DEFAULT_FROM_EMAIL
| content_type = 'text/plain' if request.POST.get('X-Requested-With', '') == 'IFrame' else 'application/json' | ||
| try: | ||
| f = request.FILES['attachment'] | ||
| if getattr(settings, 'ATTACHMENTS_MAXIMUM_FILE_SIZE', False): |
There was a problem hiding this comment.
Nitpicky, but you can just grab the setting once with getattr, instead of every time down below. It's less maintenance if we ever need to change a default value or something, and just shorter lines.
| #send email to email list | ||
| email_list = getattr(settings, 'ATTACHMENTS_VIRUS_EMAIL') | ||
| subject = 'VIRUS UPLOAD ALERT: " + user_prefix + "attempted to upload a file containing a virus to the system' | ||
| message = log_message |
There was a problem hiding this comment.
Instead of building this message in code, why not add a new templates/attachments/virus_email.txt template, and use loader.render_to_string to build the message. Has the advantage that sites can provide their own templates if they want.
| now = datetime.datetime.now() | ||
| now = now.strftime('%m-%d-%Y %H:%M:%S') | ||
| #request.user may not exist so set up user_prefix to use right prefix for messages on virus upload | ||
| user = getattr(request, 'user', None) |
There was a problem hiding this comment.
I would just pass user into the template and let it decide, via something like {{ user|default_if_none:"Some Guy" }}. The less formatting in code the better.
| {'user_prefix': user_prefix, | ||
| 'filename' : f.name, | ||
| 'virus_signature' : virus[path][1], | ||
| 'time' : now, |
There was a problem hiding this comment.
Don't need to pass in now - Django has a {% now %} template tag.
| #send email to email list | ||
| email_list = getattr(settings, 'ATTACHMENTS_VIRUS_EMAIL') | ||
| subject = 'VIRUS UPLOAD ALERT: " + user_prefix + " attempted to upload a file containing a virus to the system' | ||
| message = log_message |
There was a problem hiding this comment.
Maybe just pass log_message to the call below, why set a separate variable?
| from django.core.mail import send_mail | ||
|
|
||
| from attachments.exceptions import VirusFoundException | ||
| from settings import DEFAULT_FROM_EMAIL |
There was a problem hiding this comment.
settings are already imported above (from django.conf import settings) - you can get rid of this and just use settings.DEFAULT_FROM_EMAIL below.
| now = datetime.datetime.now() | ||
| now = now.strftime('%m-%d-%Y %H:%M:%S') | ||
| user = getattr(request, 'user', None) | ||
| log_message = loader.render_to_string('virus_email.txt', |
There was a problem hiding this comment.
Shouldn't this be attachments/virus_email.txt? Same for virus_subject.txt below? Also, it doesn't seem that the now variable is used anywhere, can it be removed?
2 participants
No description provided.