Suitecrm: 7.9.1 Bounce email handling broken; importOneEmail only half-deprecated

[pgorod]

Issue

From the forums. A user complains bounce handling is not working in Campaigns.

Expected Behavior

Bounced emails should be recognized as such in the Campaign.

Actual Behavior

The Campaign data ignores bounced emails.

Possible Fix

I have a somewhat speculative possibility for why this is happening. Feel free to ignore it if you discover that it's not the real cause, or if you can't even reproduce this issue.

It seems in 7.9 a function called importOneEmail was deprecated. It says so in a comment in the code. See here.

However, quite a few calls to that function are still present in the code.

The function replacing this seems to be called returnImportedEmail. It includes a call to HandleMailboxType which handles bounced emails.

But the current flow from the Scheduler job doesn't seem to use the new function, this seems to be the sequence (I hope I'm correct, I just followed this "manually", not while debugging):

1. The scheduler job runs
2. It Calls ImportMessages, which calls ImportMailboxMessages
3. That calls importOneEmail (instead of calling the new function)
4. Maybe when it then reaches the call to HandleMailboxType there is something not quite right, and it doesn't work.

Should we just change the call to importOneEmail to cal returnImportedEmail instead?

Steps to Reproduce

1. Define bounce account
2. Run a campaign and make sure some address bounces
3. check the campaign status and see the bounce isn't there

Your Environment

• SuiteCRM Version used: 7.9.1

[sergio91pt]

Some public "bounce emails", may be useful for testing:

• [email protected] (immediately rejected with SMTP error 550)
• [email protected] (multipart/mixed with message/delivery-status; Status 5.1.1; Date header missing)
• [email protected] (multipart/report; report-type=feedback-report with message/feedback-report (abuse) and message/rfc822; Date header missing)

[pgorod]

@Mausino and @sergio91pt does your "thumbs up" mean you also have the problem, or are you just congratulating me on what looks like a nice bug report? :-)

I'm still not 100% sure this is not just a problem in one installation, and I can't test this thoroughly in my install at the moment.

[sergio91pt]

@pgorod Both :wink: Had tested email campaigns on 7.9.0 and bounces weren't registering.
I did change the scheduler to send campaign emails and check for bounces every 15 minutes, instead of during the night.

[Mausino]

@pgorod it means that i have same issue :) i don't see the reason celebrate your report 👍 it will cool if will here some "thumbs up" which will show that other users have this issue too... It's for me tragedy to spam every ticket with words... Hello, the issue appears also on my side 🗡 :D

What is your opinion ???

[pgorod]

@Mausino yeah, I know what you mean, sometimes "me too" is a bit like spam. I guess it depends on each situation.

Maybe the first "me too" is the most important, when there are doubts whether it is a generic problem or not. Or a "me too" that adds some new information. Well thanks anyway, I'm more certain now about this Issue.

[gymad]

I created a pull request #3777 which contains some related bugfix such as fix for IE duplicate check and replaced the deprecated function importOneEmail. Can you confirm that it's solve this issue?

[pgorod]

Thanks @gymad, let's hope @Mausino and @sergio91pt can help test this. I also asked the original user that raised this in the forums to test.

Meanwhile, I worry about all these other references to the deprecated importOneEmail in v.7.9.1:

tests/tests/modules/InboundEmail/InboundEmailTest.php:1787: public function testimportOneEmail()
tests/tests/modules/InboundEmail/InboundEmailTest.php:1796: $result = $inboundEmail->importOneEmail('1', '1');

modules/Schedulers/_AddJobsHere.php:165: if ($ieX->importOneEmail($msgNo, $uid)) {
modules/Schedulers/_AddJobsHere.php:209: $ieX->importOneEmail($msgNo, $uid);
modules/Schedulers/_AddJobsHere.php:615: $aopInboundEmailX->importOneEmail($msgNo, $uid);

modules/InboundEmail/InboundEmail.php:4542: * If the importOneEmail returns false, then findout if the duplicate email
modules/InboundEmail/InboundEmail.php:4583: * shiny new importOneEmail() method
modules/InboundEmail/InboundEmail.php:4590: public function importOneEmail($msgNo, $uid, $forDisplay = false, $clean_email = true)
modules/InboundEmail/InboundEmail.php:6381: $importStatus = $this->importOneEmail($msgNo, $uid);
modules/InboundEmail/InboundEmail.php:6656: $this->importOneEmail($msgNo, $uid, true);
modules/InboundEmail/InboundEmail.php:7573: $this->importOneEmail($msgNumber, $uid, false, false);

modules/Emails/EmailUI.php:1700: if ($ie->importOneEmail($msgNo, $uid)) {

modules/Emails/javascript/EmailUI.js:1121: AjaxObject.startRequest(callbackImportOneEmail, urlStandard + '&emailUIAction=getImportForm&uid=' + ser + "&ieId=" + ieId + "&mbox=" + folder);
modules/Emails/javascript/EmailUI.js:1629: AjaxObject.startRequest(callbackImportOneEmail, urlStandard + '&emailUIAction=getImportForm' + vars);
modules/Emails/javascript/EmailUI.js:3038: AjaxObject.startRequest(callbackImportOneEmail, urlStandard + '&emailUIAction=moveEmails&emailUids=' + uids + baseUrl);

modules/Emails/javascript/ajax.js:1362:var callbackImportOneEmail = {

modules/Emails/EmailUIAjax.php:410: if (!$ie->importOneEmail($_REQUEST['uid'], $uid)) {
modules/Emails/EmailUIAjax.php:699: $status = $ie->importOneEmail($msgNo, $uid);
modules/Emails/EmailUIAjax.php:701: $status = $ie->importOneEmail($ie->getCorrectMessageNoForPop3($msgNo), $uid);
modules/Emails/EmailUIAjax.php:714: $status = $ie->importOneEmail($msgNo, $_REQUEST['uid']);
modules/Emails/EmailUIAjax.php:716: $status = $ie->importOneEmail($ie->getCorrectMessageNoForPop3($msgNo), $_REQUEST['uid']);

Is it safe to just change all these to the new returnImportedEmail function? Do you think it can be done in this same Issue and PR?

[sergio91pt]

Is it safe to just change all these to the new returnImportedEmail function? Do you think it can be done in this same Issue and PR?

^^ @daniel-samson

@pgorod If you do a side-by-side diff of those _250 line_ methods, you can see they're not equivalent (ignoring the return value).

Ideally importOneEmail function would be deprecated like this:

public function importOneEmail($msgNo, $uid, $forDisplay = false, $clean_email = true)
{
    // @returns bool|string
    $result = $this->returnImportedEmail($msgNo, $uid, $forDisplay, $clean_email);
    return empty($string) ? false : true;
}

[samus-aran]

This should be resolved in latest release (7.9.2)

However noted that the test emails provided by @sergio91pt two out of the three ([email protected], [email protected]) could not be tracked BUT also confirmed that the were not also picked up on a 7.8.x so may require to raise a separate issue specifically for both LTS and 7.9.x to tackle these use cases.

[sergio91pt]

That's interesting. The only one that _succeded_ was the immediate SMTP error (the send should fail, so no bounce account needed, I guess).

I was expecting it would work with the message/rfc822, since there's some reference in the code.

$ grep -Frni rfc822 modules/*Email*
modules/InboundEmail/InboundEmail.php:4142:                     if(isset($part->parts) && !empty($part->parts) && !( isset($part->subtype) && strtolower($part->subtype) == 'rfc822')  ) {
modules/InboundEmail/InboundEmail.php:4173:            elseif ($part->type == 2 && isset($part->subtype) && strtolower($part->subtype) == 'rfc822') {
modules/InboundEmail/InboundEmail.php:4176:                $attach->file_mime_type = 'messsage/rfc822';

I still haven't tested the fix, I'm sorry @gymad @pgorod