-
Notifications
You must be signed in to change notification settings - Fork 246
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
Attachments support #134
base: master
Are you sure you want to change the base?
Attachments support #134
Conversation
When not using the Django sites framework, you get a RuntimeError because it is not installed, even if you set DJANGO_MESSAGES_NOTIFY to False. This can be avoided by importing the site model only when necessary.
Now django-messages support attachments when composing or replying. Attachments are stored by default in an "attachments" folder relative to the ``MEDIA_ROOT``, but this can be changed with the new ``DJANGO_MESSAGES_UPLOAD_TO`` setting.
Thanks for the contribution! I like the idea. I will need to test this a bit before merging though ... The one thing that bothers me currently is how the attachments are stored. I'm not sure if a simple folder in MEDIA_ROOT is the best choice. This approach has at least some privacy-issues... It might be a good idea to have a configurable storage-engine for the attachments. With that it would be possible to store attachments outside of MEDIA_ROOT, so they are not publicly available to everyone who knows the url. The default could still be MEDIA_ROOT, but we should provide a mechanism, so that it can be changed on a per-project basis. If stored in MEDIA_ROOT we should at least do something with the filename/path. Maybe store in a folder named with the md5 hash of file contents or so... Example: |
Hi, You are totally right ! (note that the attachment code will still use the configured What we could do at best IMO:
def get_storage_backend():
backend = getattr(settings, 'DJANGO_MESSAGES_STORAGE_BACKEND', None)
if backend is None:
return None
# build backend from string here, not sure if we should handle backend args/kwargs ?
return backend
@python_2_unicode_compatible
class Attachment(models.Model):
"""
A message attachment. You can configure where attachments are stored with
the ``DJANGO_MESSAGES_UPLOAD_TO`` setting, note that this setting is
relative to your ``MEDIA_ROOT`` setting.
"""
file = models.FileField(
_('File'),
upload_to=getattr(settings, 'DJANGO_MESSAGES_UPLOAD_TO', 'attachments'),
storage=get_storage_backend(),
)
message = models.ForeignKey(
Message,
on_delete=models.CASCADE,
verbose_name=_('Message'),
related_name='attachments'
)
class Meta:
verbose_name = _("Attachment") What do you think ? |
I opt for the third solution: allow a custom storage backend. |
The default django filesystem storage being not very secure, and thus not appropriate for private messages attachments, now one can pass a custom storage backend. Also added a note on security in the relevant documentation.
Done ! |
Looks good, thank you! I still would like to test the changes in a little example project on my dev machine. So please give me some time before it's getting merged. |
Of course, no hurry :) |
Now django-messages support attachments when composing or replying.
Attachments are stored by default in an "attachments" folder relative to the
MEDIA_ROOT
, but this can be changed with the newDJANGO_MESSAGES_UPLOAD_TO
setting.Fixes #109.