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

Add lead to the log only after persisted to avoid cascade persist error #5804

Merged
merged 4 commits into from
Mar 22, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions app/bundles/LeadBundle/Model/ImportModel.php
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,8 @@ public function process(Import $import, Progress $progress, $limit = 0)
$import->increaseIgnoredCount();
$this->logImportRowError($eventLog, $errorMessage);
$this->logDebug('Line '.$lineNumber.' error: '.$errorMessage, $import);
} else {
$this->leadEventLogRepo->saveEntity($eventLog);
}

// Release entities in Doctrine's memory to prevent memory leak
Expand Down
5 changes: 4 additions & 1 deletion app/bundles/LeadBundle/Model/LeadModel.php
Original file line number Diff line number Diff line change
Expand Up @@ -1907,7 +1907,6 @@ public function import($fields, $data, $owner = null, $list = null, $tags = null
if ($eventLog) {
$action = $merged ? 'updated' : 'inserted';
$eventLog->setAction($action);
$lead->addEventLog($eventLog);
}

if ($persist) {
Expand All @@ -1920,6 +1919,10 @@ public function import($fields, $data, $owner = null, $list = null, $tags = null
if ($company !== null) {
$this->companyModel->addLeadToCompany($company, $lead);
}

if ($eventLog) {
$lead->addEventLog($eventLog);
Copy link
Contributor

@Maxell92 Maxell92 Mar 21, 2018

Choose a reason for hiding this comment

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

Are you sure this change will be saved to database? No flush is called here.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Yep. Because of https://github.com/mautic/mautic/pull/5804/files#diff-9784b7fe0e4e02b9768b1efa4c50fe25R393 and https://github.com/mautic/mautic/pull/5804/files#diff-9784b7fe0e4e02b9768b1efa4c50fe25R396.

I also tested it and it stores all rows to the log table. Before the last commit it stored only errors.

}
}

return $merged;
Expand Down
89 changes: 82 additions & 7 deletions app/bundles/LeadBundle/Tests/Model/LeadModelTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

namespace Mautic\LeadBundle\Tests\Model;

use Mautic\LeadBundle\Entity\Lead;
use Mautic\LeadBundle\Entity\LeadEventLog;
use Mautic\LeadBundle\Model\CompanyModel;
use Mautic\LeadBundle\Model\FieldModel;
use Mautic\LeadBundle\Model\LeadModel;

Expand Down Expand Up @@ -37,10 +40,7 @@ public function testCheckForDuplicateContact()
->setMethods(null)
->getMock();

$reflectedProp = new \ReflectionProperty(LeadModel::class, 'leadFieldModel');

$reflectedProp->setAccessible(true);
$reflectedProp->setValue($mockLeadModel, $mockFieldModel);
$this->setProperty($mockLeadModel, LeadModel::class, 'leadFieldModel', $mockFieldModel);

$this->assertAttributeEquals(
[],
Expand All @@ -52,11 +52,86 @@ public function testCheckForDuplicateContact()
$mockLeadModel->checkForDuplicateContact([]);
$this->assertAttributeEquals(['a' => 1], 'availableLeadFields', $mockLeadModel);

$reflectedProp = new \ReflectionProperty(LeadModel::class, 'availableLeadFields');
$reflectedProp->setAccessible(true);
$reflectedProp->setValue($mockLeadModel, []);
$this->setProperty($mockLeadModel, LeadModel::class, 'availableLeadFields', []);

$mockLeadModel->checkForDuplicateContact([], null, false, true);
$this->assertAttributeEquals(['a' => 3], 'availableLeadFields', $mockLeadModel);
}

/**
* Test that the Lead won't be set to the LeadEventLog if the Lead save fails.
*/
public function testImportWillNotSetLeadToLeadEventLogWhenLeadSaveFails()
{
$leadEventLog = new LeadEventLog();
$mockLeadModel = $this->getMockBuilder(LeadModel::class)
->disableOriginalConstructor()
->setMethods(['saveEntity', 'checkForDuplicateContact'])
->getMock();

$mockCompanyModel = $this->getMockBuilder(CompanyModel::class)
->disableOriginalConstructor()
->setMethods(['extractCompanyDataFromImport'])
->getMock();

$mockCompanyModel->expects($this->once())->method('extractCompanyDataFromImport')->willReturn([[], []]);

$this->setProperty($mockLeadModel, LeadModel::class, 'companyModel', $mockCompanyModel);
$this->setProperty($mockLeadModel, LeadModel::class, 'leadFields', [['alias' => 'email', 'type' => 'email', 'defaultValue' => '']]);

$mockLeadModel->expects($this->once())->method('saveEntity')->willThrowException(new \Exception());
$mockLeadModel->expects($this->once())->method('checkForDuplicateContact')->willReturn(new Lead());

try {
$mockLeadModel->import([], [], null, null, null, true, $leadEventLog);
} catch (\Exception $e) {
$this->assertNull($leadEventLog->getLead());
}
}

/**
* Test that the Lead will be set to the LeadEventLog if the Lead save succeed.
*/
public function testImportWillSetLeadToLeadEventLogWhenLeadSaveSucceed()
{
$leadEventLog = new LeadEventLog();
$lead = new Lead();
$mockLeadModel = $this->getMockBuilder(LeadModel::class)
->disableOriginalConstructor()
->setMethods(['saveEntity', 'checkForDuplicateContact'])
->getMock();

$mockCompanyModel = $this->getMockBuilder(CompanyModel::class)
->disableOriginalConstructor()
->setMethods(['extractCompanyDataFromImport'])
->getMock();

$mockCompanyModel->expects($this->once())->method('extractCompanyDataFromImport')->willReturn([[], []]);

$this->setProperty($mockLeadModel, LeadModel::class, 'companyModel', $mockCompanyModel);
$this->setProperty($mockLeadModel, LeadModel::class, 'leadFields', [['alias' => 'email', 'type' => 'email', 'defaultValue' => '']]);

$mockLeadModel->expects($this->once())->method('checkForDuplicateContact')->willReturn($lead);

try {
$mockLeadModel->import([], [], null, null, null, true, $leadEventLog);
} catch (\Exception $e) {
$this->assertEquals($lead, $leadEventLog->getLead());
}
}

/**
* Set protected property to an object.
*
* @param object $object
* @param string $class
* @param string $property
* @param mixed $value
*/
private function setProperty($object, $class, $property, $value)
{
$reflectedProp = new \ReflectionProperty($class, $property);
$reflectedProp->setAccessible(true);
$reflectedProp->setValue($object, $value);
}
}