Skip to content

Conversation

@LaithSaq
Copy link
Contributor

@LaithSaq LaithSaq commented Feb 12, 2025

Short Summary of the Issue

Currently, the EmailMessage API does not support CC recipients. Related issue #37

Changes made to Resolve the Issue

  • Expands on the EmailMessage API to support CCs. This is done by levereging the CCs property provided by MimeMessage.
  • Updated message serialization and deserialization to handle the CC field.
  • Updated Elasticsearch document indexing to log the new CC recipients property.
  • Ensured compatibility with older messages in the queue by implementing null checks and default empty lists.

Backward Compatibility Testing

To ensure the changes do not break pending emails in the queue (with the old format lacking CC), the following steps were performed:

  1. Spun up RabbitMQ, Redis, and smtp4dev containers.
  2. Used MemoryStorage instead of Elastic for simplified local testing.
  3. Built MiddleMail with the old message format (from master).
  4. Ran MiddleMail Server to subscribe to RabbitMQ and create the exchange.
  5. Stopped MiddleMail Server.
  6. Generated emails into the RabbitMQ queue using tools/EmailMessageGenerator.
  7. Rebuilt MiddleMail with the new message format (including CC support).
  8. Ran MiddleMail Server.
  9. Observed logged emails in smtp4dev.

This enabled me to verify that emails without CCs will have a null CC value when parsing emails into the new type. Here null checks and using empty lists saves the program from throwing a null reference exception.

It also works the other way around (swapping the old/new types). It just drops the added field entirely when parsing into a type with no CC.

I've tested with many more scenarios and as long as the exchange is created, no emails are being dropped and no parsing is failing. Which aligns with this warning

Additional Validation in Elasticsearch

  • Set up Elasticsearch and verified indices using:
    curl http://127.0.0.1:9200/_cat/indices?v
  • Checked docs.count to confirm email tracking.
  • Queried Elasticsearch using _search and confirmed that CC fields are correctly stored in the new format:
    "_source": {
      "ccNames": ["Shakira McLaughlin", "Jordyn Pfannerstill", "Damian Rice"],
      "ccAddresses": ["Greta.Wiegand@hotmail.com", "Morris_Beatty89@gmail.com", "Catherine75@gmail.com"]
    }
  • Verified that older emails (without CC) are stored correctly without breaking the index schema.

Release Notes

Technical Improvements

  • Added support for CC recipients in the EmailMessage API.
  • Updated Elasticsearch logging to track CC recipients.

Checklist

  • Unit tests added or updated.
  • Tested locally with RabbitMQ, Redis, smtp4dev, and ElasticSearch.
  • Checked for typos and formatting issues.
  • Release Notes are clear and understandable for non-developers.
  • Instructions for testers are self-contained and clear.
  • Self-reviewed the MR description and changes.
  • Has been approved by a developer.
  • Has been approved by a reviewer.

@LaithSaq
Copy link
Contributor Author

LaithSaq commented Feb 12, 2025

Note: Commit 2772fef only contains a question and need to be dropped before merging

@LaithSaq LaithSaq marked this pull request as draft February 12, 2025 11:11
@LaithSaq LaithSaq marked this pull request as ready for review February 12, 2025 11:12
Copy link

@MiaMark MiaMark left a comment

Choose a reason for hiding this comment

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

Looks good, assuming you are able to test and resolve the question in the first file.

@leo-labs
Copy link
Member

@LaithSaq Can you please fix your line ending settings and rebase, so there are no unnecessary line ending changes in the changeset?

@LaithSaq
Copy link
Contributor Author

LaithSaq commented Mar 12, 2025

Can you please fix your line ending settings and rebase, so there are no unnecessary line ending changes in the changeset?

That's me actually "fixing" the line endings. I set my git client to checkout Windows style (CRLF) and commit Unix style (LF). The unnecessary changes you're noticing are caused by the fact that files didn't have consistent line endings to begin with. Do you suggest I re-use CRLF in files that originally used that?

UPDATE:
I've created this commit 89bc8f7 to undo line ending changes. If you prefer the current version, I can just squash it with the commit that introduced such changes, otherwise, I'll just drop the new commit :P

UPDATE:
I just did the squashing I mentioned above, along with some other commit history cleaning.

Line endings were not changed within this formatting to avoid
making the change log huge for reviewing purposes.
@LaithSaq LaithSaq force-pushed the provide-cc-support branch from 89bc8f7 to 57bfae0 Compare March 12, 2025 16:01
@LaithSaq LaithSaq force-pushed the provide-cc-support branch from 7dac281 to d68d833 Compare March 19, 2025 13:10
@LaithSaq LaithSaq force-pushed the provide-cc-support branch from dd05338 to 141e50c Compare March 24, 2025 14:23
Copy link
Member

@leo-labs leo-labs left a comment

Choose a reason for hiding this comment

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

See my comments that can be resolved in follow up issues.

Great job, thanks for adding this feature :) 🚀

@LaithSaq LaithSaq force-pushed the provide-cc-support branch from 141e50c to 2a62588 Compare March 24, 2025 14:28
@leo-labs leo-labs mentioned this pull request Mar 24, 2025
12 tasks
@LaithSaq LaithSaq force-pushed the provide-cc-support branch from 2a62588 to 9d30a41 Compare March 24, 2025 19:32
@leo-labs leo-labs merged commit 9062392 into Miaplaza:master Mar 25, 2025
1 check passed
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.

3 participants