Skip to content
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

Added new fields to Message and MessageAttachment #127

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rulzart
Copy link

@rulzart rulzart commented Dec 22, 2016

Message:
date (DateTimeField)
property: get_cc (gets the Cc addresses from header)
property: html_with_img (replaces cid image values with actual attachment url, requires BeautifulSoup)

MessageAttachment:
field: cid (charfield, Content-ID from attachment header)

Furthermore altered the _get_dehydrated_message to get and save the Content-ID from the message.

@@ -361,6 +368,24 @@ def _process_message(self, message):
msg.to_header = utils.convert_header_to_unicode(
message['Delivered-To']
)
#rulzart - Save the email date
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know if it's a good place for signature by. It can confuse. Commits are marked with authorship.

#rulzart - added a quick way to get Cc addresses
@property
def get_cc(self):
return self.get_email_object()["Cc"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be handled quietly with the lack of such information.

Returns the message body matching content type 'text/html' with correct image src.
"""
soup = BeautifulSoup(self.html)
for attachment in self.attachments.exclude(cid__isnull=True).exclude(cid__exact=''):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest move this filtering as QuerySet method. I think it's good to add support for prefetch filtered attachments in some way too.

for attachment in self.attachments.exclude(cid__isnull=True).exclude(cid__exact=''):
for img in soup.findAll('img'):
if img['src'] == attachment.cid:
img['src'] = '%s%s' % (django_settings.MEDIA_URL, attachment.document)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not attachment.document.url? See Using files in models.

@@ -578,6 +611,23 @@ def html(self):
self.get_email_object(), 'text', 'html'
).replace('\n', '').strip()

#rulzart - added a quick way to get Cc addresses
@property
def get_cc(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_ prefix is incoherent for other properties in model. I suggest remove get_.

@@ -295,6 +301,7 @@ def _get_dehydrated_message(self, msg, record):
attachment.message = record
for key, value in msg.items():
attachment[key] = value
attachment.cid = cid
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raises exceptions of undefinied variable if bool(msg.get('Content-ID')) == False.

@ad-m
Copy link
Collaborator

ad-m commented Dec 22, 2016

Lack of tests. Do you need any help to prepare tests? I suggest proposes to add at least some examples of message processing to TestProcessEmail.

@@ -1,2 +1,3 @@
django>=1.9,<1.10
six
beautifulsoup4
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rtd_requirements.txt consist requirements for build docs on http://django-mailbox.readthedocs.io/. If you like add new requirements to install use install_requires in /setup.py instead.

@ad-m
Copy link
Collaborator

ad-m commented Jan 9, 2018

@rulzart , do you plan to work on it? You have not implemented my comments for a long time.

@rulzart
Copy link
Author

rulzart commented Jan 9, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants