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

Ensure parsed e-mail message is not discarded #67

Merged
merged 3 commits into from Aug 10, 2023

Conversation

sergei-maertens
Copy link
Contributor

@sergei-maertens sergei-maertens commented Aug 8, 2023

Related to #66

  • Added extensive regression test. I do not believe this covers all possible situations yet.
  • Generalized attachment handling in Message.get_email_message
  • Fixed other missing e-mail message information (Reply-To, BCC, custom headers)

The custom header treatment feels a bit fragile, though I do believe that:

  • re-using the date from the original queued message makes sense - this is effectlively when the attempt to send the email was given
  • re-using the message ID -> this helps correlating what you see in application logging + database log/message entries + mail server logging and final received messages
  • message boundaries are re-created when sending them, this requires us to parse the e-mail content type header to determine the appropriate subtype

@sergei-maertens sergei-maertens force-pushed the issue/66-fix-lost-information branch 4 times, most recently from 8ff1be4 to 36d2f7f Compare August 9, 2023 13:27
@sergei-maertens sergei-maertens changed the title Add regression test(s) for discarded e-mail message information Ensure parsed e-mail message is not discarded Aug 9, 2023
@coveralls
Copy link

coveralls commented Aug 9, 2023

Coverage Status

coverage: 77.209% (+0.8%) from 76.447% when pulling c535118 on maykinmedia:issue/66-fix-lost-information into 2a0d59a on APSL:master.

@sergei-maertens sergei-maertens marked this pull request as ready for review August 9, 2023 14:22
@sastred sastred self-requested a review August 9, 2023 15:07
django_yubin/models.py Show resolved Hide resolved
django_yubin/models.py Outdated Show resolved Hide resolved
tests/tests/test_models.py Outdated Show resolved Hide resolved
b"\x00\x00\x00\rIDAT\x18Wc\xf8\xff\xef\xdf\x7f\x00\t\xf6\x03\xfbW\xfe{\x1b"
b"\x00\x00\x00\x00IEND\xaeB`\x82"
)
image.add_header("Content-ID", f"<123456789>")
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit nit, but flake8 is complaining about the f-string: f-string is missing placeholdersflake8(F541).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no worries, I aim to comply to flake8! It used to be an interpolated thing but I updated it afterwards.

As for flake8 and the likes - have you considered adding the check to CI? I checked but couldn't find anything for isort/black either.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we have considered adding at least flake8 and we will probably do it in the future. Not 100% sure about adding black and reformat all the code.

BCC addresses are not included in e-mail headers, as this would disclose
the other recipients, breaking the 'blind' aspect. As such,
there is no point in trying to read this from the parsed email,
it will never yield valid addresses. Removing the line removes
confusion and prevents wrong impressions.
Message reconstruction from string dropped custom headers and particular
attachment content disposition/relations.
@sastred sastred merged commit 8cf564f into APSL:master Aug 10, 2023
6 checks passed
@sastred
Copy link
Member

sastred commented Aug 10, 2023

Closes #66

@Viicos Viicos deleted the issue/66-fix-lost-information branch November 8, 2023 09:38
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.

None yet

3 participants