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

DPMMA-3031 Configurable email address length limit to prevent delivery issues #14577

Merged
merged 4 commits into from
Feb 11, 2025

Conversation

patrykgruszka
Copy link
Member

@patrykgruszka patrykgruszka commented Feb 10, 2025

Q A
Bug fix? (use the a.b branch) 🟢
New feature/enhancement? (use the a.x branch) 🔴
Deprecations? 🔴
BC breaks? (use the c.x branch) 🔴
Automated tests included? 🟢
Related user documentation PR URL mautic/user-documentation#350
Related developer documentation PR URL mautic/developer-documentation-new#...
Issue(s) addressed Fixes #7732, https://forum.mautic.org/t/logs-are-full-of-mail-error/12245

Description

This PR introduces a new configuration option in the Email settings to set a maximum length limit for email addresses (including display names). This feature helps prevent delivery issues with email servers that have strict length restrictions.

Changes:

  • Added a new configuration field mailer_address_length_limit in the Email settings
  • Implemented logic in the MailHelper class to truncate long email addresses to only the email part (without display name) when they exceed the configured limit

📋 Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Go to the Mautic configuration page and navigate to the Email Settings section
  3. Locate the new "Email Address Length Limit" field
  4. Set a value (e.g., 100) and save the configuration
  5. Create a new email with a recipient that has a very long display name
  6. Send a test email and verify that the recipient address is truncated correctly if it exceeds the set limit

Copy link

codecov bot commented Feb 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.49%. Comparing base (47d2e86) to head (393a905).
Report is 5 commits behind head on 5.2.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##                5.2   #14577   +/-   ##
=========================================
  Coverage     63.49%   63.49%           
- Complexity    34637    34638    +1     
=========================================
  Files          2273     2273           
  Lines        103613   103621    +8     
=========================================
+ Hits          65784    65792    +8     
  Misses        37829    37829           
Files with missing lines Coverage Δ
app/bundles/EmailBundle/Form/Type/ConfigType.php 100.00% <100.00%> (ø)
app/bundles/EmailBundle/Helper/MailHelper.php 78.40% <100.00%> (+0.16%) ⬆️

@mautibot
Copy link

This pull request has been mentioned on Mautic Forums. There might be relevant details there:

https://forum.mautic.org/t/logs-are-full-of-mail-error/12245/3

Copy link
Member

@kuzmany kuzmany left a comment

Choose a reason for hiding this comment

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

Code is good to me.
Just thinking If shouldn't be also part of EmailValidator https://github.com/mautic/mautic/blob/189dadcfb697e3985ef894ec9d66b6c0a3147fcb/app/bundles/EmailBundle/Helper/EmailValidator.php

But could be part of another PR.

👍

Copy link
Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

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

Since this doesn't prevent email sending but only decides whether to add the contact name to the email address or not based on the length of the email address + name. I don't see an issue with this.

The provided tests shows this works as described. But I leave this open for additional manual test.

@escopecz escopecz added pending-test-confirmation PR's that require one test before they can be merged enhancement Any improvement to an existing feature or functionality email Anything related to email code-review-passed PRs which have passed code review labels Feb 10, 2025
@escopecz
Copy link
Member

One thing is unclear. This looks more like an enhancement than a bug fix. Shouldn't we target 6.x instead of 5.2?

@patrykgruszka
Copy link
Member Author

@escopecz while this is technically an enhancement, it addresses an issue that prevents emails from being sent to a specific group of contacts. That's why I choose 5.2, but if you believe that version 6.x would be more appropriate, I can rebase it.

@escopecz
Copy link
Member

I think technically this doesn't solve a bug in Mautic but in an integration. But that's politics. It might help fix an issue that Mautic users are having. Let's leave it in 5.2.

@escopecz escopecz added this to the 5.2.3 milestone Feb 11, 2025
@escopecz escopecz removed the pending-test-confirmation PR's that require one test before they can be merged label Feb 11, 2025
@escopecz escopecz merged commit 915d3b3 into mautic:5.2 Feb 11, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-review-passed PRs which have passed code review email Anything related to email enhancement Any improvement to an existing feature or functionality
Projects
Status: 🥳 Done
Development

Successfully merging this pull request may close these issues.

4 participants