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

InboundEmail mime parser fix #7922

Merged

Conversation

Dillon-Brown
Copy link
Contributor

Description

Fixes an issue with the email body encoding and broken multipart/alternative emails. This PR also starts to refactor InboundEmail.php to use zbateson/mail-mime-parser.

This should, in theory, reduce a large amount of the email issues and regressions we receive in relation to improper email parsing while also reducing the overall complexity of inbound email handling in SuiteCRM.

Motivation and Context

Issue reference: #7880

How To Test This

This will require full testing of the email module. A few key areas I would recommend testing is the inline images functionality, attachments and HTML styling. This should be consistent across unimported and imported emails.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Final checklist

  • My code follows the code style of this project found here.
  • My change requires a change to the documentation.
  • I have read the How to Contribute guidelines.

@Dillon-Brown Dillon-Brown changed the title Fix/email encoding InboundEmail mime parser fix Sep 24, 2019
@Dillon-Brown Dillon-Brown marked this pull request as ready for review September 24, 2019 16:15
@codecov-io
Copy link

codecov-io commented Sep 24, 2019

Codecov Report

Merging #7922 into hotfix-7.10.x will increase coverage by 0.01%.
The diff coverage is 17.47%.

@@                Coverage Diff                @@
##           hotfix-7.10.x    #7922      +/-   ##
=================================================
+ Coverage           7.27%    7.29%   +0.01%     
=================================================
  Files               3700     3701       +1     
  Lines             386188   386105      -83     
=================================================
+ Hits               28106    28154      +48     
+ Misses            358082   357951     -131

@cameronblaikie
Copy link
Contributor

Looks good, Assessed 👍

Copy link
Member

@mattlorimer mattlorimer left a comment

Choose a reason for hiding this comment

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

Needs to be tested, but a few points to looks at

modules/InboundEmail/InboundEmail.php Outdated Show resolved Hide resolved
modules/InboundEmail/InboundEmail.php Show resolved Hide resolved
modules/InboundEmail/InboundEmail.php Show resolved Hide resolved
This change allows us to detect the header type of inbound emails and
prevents html being cleaned when the type is "text/plain".
This change populated the email->description field when the type of
inbound email is plaintext.
@mattlorimer
Copy link
Member

retested and it still not populating the description field when I import an email (e.g create a case import) looks like you update it only on returnNonImportedEmail and not returnImportedEmail

@mattlorimer mattlorimer added the Status:Requires Updates Issues & PRs which requires input or update from the author label Oct 8, 2019
@Dillon-Brown Dillon-Brown removed the Status:Requires Updates Issues & PRs which requires input or update from the author label Oct 10, 2019
…encoding

# Conflicts:
#	composer.json
#	composer.lock
Copy link
Contributor

@craigpanton craigpanton left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@craigpanton craigpanton added the Status:Needs Assessed Needs the core team to assess label Oct 22, 2019
@Mac-Rae Mac-Rae added the Status:In Review Pull Requests that are activity being reviewed by the core team label Oct 22, 2019
@cameronblaikie
Copy link
Contributor

Reassessed and LGTM 👍

@cameronblaikie cameronblaikie added Status:Assessed PRs that have been tested and confirmed to resolve an issue by a core team member and removed Status:In Review Pull Requests that are activity being reviewed by the core team labels Oct 22, 2019
@Dillon-Brown Dillon-Brown dismissed mattlorimer’s stale review October 22, 2019 14:53

Raised issues have been addressed and PR re code-reviewed + assessed.

@Dillon-Brown Dillon-Brown merged commit e9768bb into salesagility:hotfix-7.10.x Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status:Assessed PRs that have been tested and confirmed to resolve an issue by a core team member Status:Needs Assessed Needs the core team to assess
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants