Skip to content

Commit cb1c806

Browse files
alzotaalzota
committed
ACP2E-3493: addressed 2 additional edge cases, improved runtime and avoided out of memory.
1 parent 93e640f commit cb1c806

File tree

4 files changed

+141
-63
lines changed

4 files changed

+141
-63
lines changed

‎app/code/Magento/Persistent/Model/CleanExpiredPersistentQuotes.php

Lines changed: 38 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,9 @@
88
namespace Magento\Persistent\Model;
99

1010
use Magento\Framework\Exception\LocalizedException;
11+
use Magento\Framework\Model\ResourceModel\Db\VersionControl\Snapshot;
12+
use Magento\Store\Api\Data\StoreInterface;
1113
use Magento\Store\Model\StoreManagerInterface;
12-
use Magento\Quote\Model\ResourceModel\Quote\Collection as QuoteCollection;
1314
use Magento\Persistent\Model\ResourceModel\ExpiredPersistentQuotesCollection;
1415
use Magento\Quote\Model\QuoteRepository;
1516
use Psr\Log\LoggerInterface;
@@ -24,18 +25,20 @@ class CleanExpiredPersistentQuotes
2425
* @param StoreManagerInterface $storeManager
2526
* @param ExpiredPersistentQuotesCollection $expiredPersistentQuotesCollection
2627
* @param QuoteRepository $quoteRepository
28+
* @param Snapshot $snapshot
2729
* @param LoggerInterface $logger
2830
*/
2931
public function __construct(
3032
private readonly StoreManagerInterface $storeManager,
3133
private readonly ExpiredPersistentQuotesCollection $expiredPersistentQuotesCollection,
3234
private readonly QuoteRepository $quoteRepository,
35+
private Snapshot $snapshot,
3336
private readonly LoggerInterface $logger
3437
) {
3538
}
3639

3740
/**
38-
* Removes expired persistent quotes for a specific website, identified by its ID
41+
* Execute the cron job
3942
*
4043
* @param int $websiteId
4144
* @return void
@@ -44,43 +47,48 @@ public function __construct(
4447
public function execute(int $websiteId): void
4548
{
4649
$stores = $this->storeManager->getWebsite($websiteId)->getStores();
47-
4850
foreach ($stores as $store) {
49-
/** @var $quoteCollection QuoteCollection */
50-
$quoteCollection = $this->expiredPersistentQuotesCollection->getExpiredPersistentQuotes($store);
51-
$quoteCollection->setPageSize(50);
52-
53-
// Last page returns 1 even when we don't have any results
54-
$lastPage = $quoteCollection->getSize() ? $quoteCollection->getLastPageNumber() : 0;
55-
56-
for ($currentPage = $lastPage; $currentPage >= 1; $currentPage--) {
57-
$quoteCollection->setCurPage($currentPage);
58-
59-
$this->deletePersistentQuotes($quoteCollection);
60-
}
51+
$this->processStoreQuotes($store);
6152
}
6253
}
6354

6455
/**
65-
* Deletes all quotes in the collection.
56+
* Process store quotes in batches
6657
*
67-
* @param QuoteCollection $quoteCollection
58+
* @param StoreInterface $store
59+
* @return void
6860
*/
69-
private function deletePersistentQuotes(QuoteCollection $quoteCollection): void
61+
private function processStoreQuotes(StoreInterface $store): void
7062
{
71-
foreach ($quoteCollection as $quote) {
72-
try {
73-
$this->quoteRepository->delete($quote);
74-
} catch (Exception $e) {
75-
$message = sprintf(
76-
'Unable to delete expired quote (ID: %s): %s',
77-
$quote->getId(),
78-
(string)$e
79-
);
80-
$this->logger->error($message);
63+
$batchSize = 500;
64+
$lastProcessedId = 0;
65+
66+
while (true) {
67+
$quotesToProcess = $this->expiredPersistentQuotesCollection
68+
->getExpiredPersistentQuotes($store, $lastProcessedId, $batchSize);
69+
70+
if (!$quotesToProcess->count()) {
71+
break;
8172
}
82-
}
8373

84-
$quoteCollection->clear();
74+
foreach ($quotesToProcess as $quote) {
75+
try {
76+
$this->quoteRepository->delete($quote);
77+
$lastProcessedId = (int)$quote->getId();
78+
} catch (Exception $e) {
79+
$this->logger->error(sprintf(
80+
'Unable to delete expired quote (ID: %s): %s',
81+
$quote->getId(),
82+
(string)$e
83+
));
84+
}
85+
$quote->clearInstance();
86+
unset($quote);
87+
}
88+
89+
$quotesToProcess->clear();
90+
$this->snapshot->clear();
91+
unset($quotesToProcess);
92+
}
8593
}
8694
}

‎app/code/Magento/Persistent/Model/ResourceModel/ExpiredPersistentQuotesCollection.php

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -36,26 +36,44 @@ public function __construct(
3636
* Filters and returns all quotes that have expired based on the persistent lifetime threshold.
3737
*
3838
* @param StoreInterface $store
39+
* @param int $lastId
40+
* @param int $batchSize
3941
* @return AbstractCollection
4042
*/
41-
public function getExpiredPersistentQuotes(StoreInterface $store): AbstractCollection
43+
public function getExpiredPersistentQuotes(StoreInterface $store, int $lastId, int $batchSize): AbstractCollection
4244
{
4345
$lifetime = $this->scopeConfig->getValue(
4446
Data::XML_PATH_LIFE_TIME,
4547
ScopeInterface::SCOPE_WEBSITE,
4648
$store->getWebsiteId()
4749
);
4850

51+
$lastLoginCondition = gmdate("Y-m-d H:i:s", time() - $lifetime);
52+
4953
/** @var $quotes Collection */
5054
$quotes = $this->quoteCollectionFactory->create();
51-
$quotes->getSelect()->join(
52-
['cl' => $quotes->getTable('customer_log')],
53-
'cl.customer_id = main_table.customer_id',
55+
56+
$select = $quotes->getSelect();
57+
$select->joinLeft(
58+
['cl1' => $quotes->getTable('customer_log')],
59+
'cl1.customer_id = main_table.customer_id
60+
AND cl1.last_login_at < cl1.last_logout_at
61+
AND cl1.last_logout_at IS NOT NULL',
62+
[]
63+
)->joinLeft(
64+
['cl2' => $quotes->getTable('customer_log')],
65+
'cl2.customer_id = main_table.customer_id
66+
AND cl2.last_login_at < "' . $lastLoginCondition . '"
67+
AND (cl2.last_logout_at IS NULL OR cl2.last_login_at > cl2.last_logout_at)',
5468
[]
55-
)->where('cl.last_logout_at > cl.last_login_at');
69+
);
70+
5671
$quotes->addFieldToFilter('main_table.store_id', $store->getId());
57-
$quotes->addFieldToFilter('main_table.updated_at', ['lt' => gmdate("Y-m-d H:i:s", time() - $lifetime)]);
72+
$quotes->addFieldToFilter('main_table.updated_at', ['lt' => $lastLoginCondition]);
5873
$quotes->addFieldToFilter('main_table.is_persistent', 1);
74+
$quotes->addFieldToFilter('main_table.entity_id', ['gt' => $lastId]);
75+
$quotes->setOrder('entity_id', Collection::SORT_ORDER_ASC);
76+
$quotes->setPageSize($batchSize);
5977

6078
return $quotes;
6179
}

‎app/code/Magento/Persistent/Test/Unit/Model/CleanExpiredPersistentQuotesTest.php

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,17 +7,19 @@
77

88
namespace Magento\Persistent\Test\Unit\Model;
99

10+
use Magento\Framework\Exception\LocalizedException;
11+
use Magento\Framework\Model\ResourceModel\Db\VersionControl\Snapshot;
1012
use Magento\Persistent\Model\CleanExpiredPersistentQuotes;
1113
use Magento\Persistent\Model\ResourceModel\ExpiredPersistentQuotesCollection;
1214
use Magento\Quote\Model\Quote;
1315
use Magento\Quote\Model\QuoteRepository;
1416
use Magento\Store\Model\StoreManagerInterface;
1517
use Magento\Quote\Model\ResourceModel\Quote\Collection;
18+
use PHPUnit\Framework\MockObject\Exception;
1619
use PHPUnit\Framework\TestCase;
1720
use Psr\Log\LoggerInterface;
1821
use Magento\Store\Api\Data\StoreInterface;
1922
use Magento\Store\Model\Website;
20-
use Magento\Customer\Model\Log;
2123

2224
class CleanExpiredPersistentQuotesTest extends TestCase
2325
{
@@ -51,16 +53,25 @@ protected function setUp(): void
5153
$this->storeManagerMock = $this->createMock(StoreManagerInterface::class);
5254
$this->expiredPersistentQuotesCollectionMock = $this->createMock(ExpiredPersistentQuotesCollection::class);
5355
$this->quoteRepositoryMock = $this->createMock(QuoteRepository::class);
56+
$this->snapshotMock = $this->createMock(Snapshot::class);
5457
$this->loggerMock = $this->createMock(LoggerInterface::class);
5558

5659
$this->cleanExpiredPersistentQuotes = new CleanExpiredPersistentQuotes(
5760
$this->storeManagerMock,
5861
$this->expiredPersistentQuotesCollectionMock,
5962
$this->quoteRepositoryMock,
63+
$this->snapshotMock,
6064
$this->loggerMock
6165
);
6266
}
6367

68+
/**
69+
* Test execute method
70+
*
71+
* @return void
72+
* @throws LocalizedException
73+
* @throws Exception
74+
*/
6475
public function testExecuteDeletesExpiredQuotes(): void
6576
{
6677
$websiteId = 1;
@@ -81,6 +92,19 @@ public function testExecuteDeletesExpiredQuotes(): void
8192
$quoteCollectionMock->method('getLastPageNumber')->willReturn(1);
8293
$quoteCollectionMock->method('setPageSize')->willReturnSelf();
8394
$quoteCollectionMock->method('setCurPage')->willReturnSelf();
95+
$quoteCollectionMock->expects($this->exactly(2))
96+
->method('count')
97+
->willReturnCallback(function () use ($quoteCollectionMock) {
98+
static $filterCallCount = 0;
99+
$filterCallCount++;
100+
101+
match ($filterCallCount) {
102+
1 => $count = 1,
103+
2 => $count = 0
104+
};
105+
106+
return $count;
107+
});
84108

85109
$this->expiredPersistentQuotesCollectionMock
86110
->method('getExpiredPersistentQuotes')

‎app/code/Magento/Persistent/Test/Unit/Model/ResourceModel/ExpiredPersistentQuotesCollectionTest.php

Lines changed: 54 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
use Magento\Persistent\Helper\Data;
1616
use Magento\Store\Model\ScopeInterface;
1717
use Magento\Framework\DB\Select;
18+
use PHPUnit\Framework\MockObject\Exception;
1819
use PHPUnit\Framework\TestCase;
1920

2021
class ExpiredPersistentQuotesCollectionTest extends TestCase
@@ -45,6 +46,12 @@ protected function setUp(): void
4546
);
4647
}
4748

49+
/**
50+
* Test getExpiredPersistentQuotes method
51+
*
52+
* @return void
53+
* @throws Exception
54+
*/
4855
public function testGetExpiredPersistentQuotes(): void
4956
{
5057
$storeMock = $this->createMock(StoreInterface::class);
@@ -56,40 +63,61 @@ public function testGetExpiredPersistentQuotes(): void
5663
->willReturn(60);
5764

5865
$quoteCollectionMock = $this->createMock(Collection::class);
59-
6066
$this->quoteCollectionFactoryMock->method('create')->willReturn($quoteCollectionMock);
6167

6268
$dbSelectMock = $this->createMock(Select::class);
6369
$quoteCollectionMock->method('getSelect')->willReturn($dbSelectMock);
6470
$quoteCollectionMock->method('getTable')->willReturn('customer_log');
65-
$dbSelectMock->expects($this->once())
66-
->method('join')
67-
->with(
68-
$this->equalTo(['cl' => 'customer_log']),
69-
$this->equalTo('cl.customer_id = main_table.customer_id'),
70-
$this->equalTo([])
71-
)
72-
->willReturnSelf();
73-
$dbSelectMock->expects($this->once())
74-
->method('where')
75-
->with($this->equalTo('cl.last_logout_at > cl.last_login_at'))
71+
72+
$dbSelectMock->method('joinLeft')
73+
->willReturnCallback(function ($table, $condition) use ($dbSelectMock) {
74+
static $callCount = 0;
75+
$callCount++;
76+
77+
if ($callCount === 1) {
78+
$this->assertEquals(['cl1' => 'customer_log'], $table);
79+
$this->assertStringContainsString('cl1.customer_id = main_table.customer_id', $condition);
80+
$this->assertStringContainsString('cl1.last_login_at < cl1.last_logout_at', $condition);
81+
$this->assertStringContainsString('cl1.last_logout_at IS NOT NULL', $condition);
82+
}
83+
84+
if ($callCount === 2) {
85+
$this->assertEquals(['cl2' => 'customer_log'], $table);
86+
$this->assertStringContainsString('cl2.customer_id = main_table.customer_id', $condition);
87+
$this->assertStringContainsString('cl2.last_login_at <', $condition);
88+
$this->assertStringContainsString(
89+
'cl2.last_logout_at IS NULL OR cl2.last_login_at > cl2.last_logout_at',
90+
$condition
91+
);
92+
}
93+
94+
return $dbSelectMock;
95+
});
96+
97+
$quoteCollectionMock->method('addFieldToFilter')
98+
->willReturnCallback(function ($field) use ($quoteCollectionMock) {
99+
static $filterCallCount = 0;
100+
$filterCallCount++;
101+
102+
match ($filterCallCount) {
103+
1 => $this->assertEquals('main_table.store_id', $field),
104+
2 => $this->assertEquals('main_table.updated_at', $field),
105+
3 => $this->assertEquals('main_table.is_persistent', $field),
106+
4 => $this->assertEquals('main_table.entity_id', $field)
107+
};
108+
109+
return $quoteCollectionMock;
110+
});
111+
112+
$quoteCollectionMock->method('setOrder')
113+
->with('entity_id', Collection::SORT_ORDER_ASC)
76114
->willReturnSelf();
77115

78-
$quoteCollectionMock->expects($this->exactly(3))
79-
->method('addFieldToFilter')
80-
->with(
81-
$this->callback(
82-
function ($field) {
83-
return in_array(
84-
$field,
85-
['main_table.store_id', 'main_table.updated_at', 'main_table.is_persistent']
86-
);
87-
}
88-
)
89-
);
90-
91-
$result = $this->model->getExpiredPersistentQuotes($storeMock);
116+
$quoteCollectionMock->method('setPageSize')
117+
->with($this->isType('integer'))
118+
->willReturnSelf();
92119

120+
$result = $this->model->getExpiredPersistentQuotes($storeMock, 0, 100);
93121
$this->assertSame($quoteCollectionMock, $result);
94122
}
95123
}

0 commit comments

Comments
 (0)