Skip to content

Commit e760472

Browse files
tatevikg1tatevikg1
committed
After review 1
1 parent d7502dc commit e760472

File tree

6 files changed

+85
-22
lines changed

6 files changed

+85
-22
lines changed

‎src/Domain/Subscription/Repository/DynamicListAttrRepository.php‎

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
use PhpList\Core\Domain\Subscription\Model\Dto\DynamicListAttrDto;
1313
use Symfony\Component\Serializer\Exception\ExceptionInterface;
1414
use Symfony\Component\Serializer\Normalizer\DenormalizerInterface;
15+
use Throwable;
1516

1617
class DynamicListAttrRepository
1718
{
@@ -33,7 +34,7 @@ public function transactional(callable $callback): mixed
3334
$result = $callback();
3435
$this->connection->commit();
3536
return $result;
36-
} catch (\Throwable $e) {
37+
} catch (Throwable $e) {
3738
$this->connection->rollBack();
3839
throw $e;
3940
}
@@ -151,24 +152,38 @@ public function fetchSingleOptionName(string $listTable, int $id): ?string
151152
return $val === false ? null : (string) $val;
152153
}
153154

154-
public function existsByName(string $listTable, string $name): bool
155+
public function existsByName(string $listTable, DynamicListAttrDto $dto): bool
155156
{
156157
if (!preg_match('/^[A-Za-z0-9_]+$/', $listTable)) {
157158
throw new InvalidArgumentException('Invalid list table');
158159
}
159160

160161
$table = $this->fullTablePrefix . $listTable;
161162

162-
try {
163-
$sql = 'SELECT 1 FROM ' . $table . ' WHERE LOWER(name) = LOWER(?) LIMIT 1';
164-
$result = $this->connection->fetchOne($sql, [$name], [PDO::PARAM_STR]);
163+
$sql = 'SELECT 1 FROM ' . $table . ' WHERE LOWER(name) = LOWER(:name)';
164+
$params = ['name' => $dto->name];
165+
$types = ['name' => PDO::PARAM_STR];
166+
167+
if ($dto->id !== null) {
168+
$sql .= ' AND id <> :excludeId';
169+
$params['excludeId'] = $dto->id;
170+
$types['excludeId'] = PDO::PARAM_INT;
171+
}
165172

173+
$sql .= ' LIMIT 1';
174+
175+
try {
176+
$result = $this->connection->fetchOne($sql, $params, $types);
166177
return $result !== false;
167-
} catch (Exception $e) {
178+
} catch (Throwable $e) {
168179
throw $e;
169180
}
170181
}
171182

183+
/**
184+
* @throws InvalidArgumentException
185+
* @return DynamicListAttrDto[]
186+
*/
172187
public function getAll(string $listTable): array
173188
{
174189
if (!preg_match('/^[A-Za-z0-9_]+$/', $listTable)) {

‎src/Domain/Subscription/Repository/SubscriberAttributeValueRepository.php‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,4 +76,18 @@ public function getForSubscriber(Subscriber $subscriber): array
7676
->getQuery()
7777
->getResult();
7878
}
79+
80+
public function existsByAttributeAndValue(string $tableName, int $optionId): bool
81+
{
82+
$row = $this->createQueryBuilder('sa')
83+
->join('sa.attributeDefinition', 'ad')
84+
->andWhere('ad.tableName = :tableName')
85+
->setParameter('tableName', $tableName)
86+
->andWhere('sa.value = :value')
87+
->setParameter('value', $optionId)
88+
->getQuery()
89+
->getOneOrNullResult();
90+
91+
return $row !== null;
92+
}
7993
}

‎src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,10 @@ public function update(
8888
if ($attributeDefinition->getTableName()) {
8989
$this->dynamicListAttrManager
9090
->syncOptions($attributeDefinition->getTableName(), $attributeDefinitionDto->options);
91+
} elseif ($attributeDefinitionDto->type->isMultiValued()) {
92+
$tableName = $this->dynamicTablesManager
93+
->resolveTableName(name: $attributeDefinitionDto->name, type: $attributeDefinitionDto->type);
94+
$this->dynamicListAttrManager->insertOptions($tableName, $attributeDefinitionDto->options);
9195
}
9296

9397
return $attributeDefinition;

‎src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php‎

Lines changed: 28 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,18 +6,18 @@
66

77
use PhpList\Core\Domain\Subscription\Model\Dto\DynamicListAttrDto;
88
use PhpList\Core\Domain\Subscription\Repository\DynamicListAttrRepository;
9+
use PhpList\Core\Domain\Subscription\Repository\SubscriberAttributeValueRepository;
910
use RuntimeException;
1011

1112
class DynamicListAttrManager
1213
{
13-
public function __construct(private readonly DynamicListAttrRepository $dynamicListAttrRepository,)
14-
{
14+
public function __construct(
15+
private readonly DynamicListAttrRepository $dynamicListAttrRepository,
16+
private readonly SubscriberAttributeValueRepository $attributeValueRepo,
17+
) {
1518
}
1619

1720
/**
18-
* Seed options into the just-created options table.
19-
*
20-
* @param string $listTable logical table (without global prefix)
2121
* @param DynamicListAttrDto[] $rawOptions
2222
*/
2323
public function insertOptions(string $listTable, array $rawOptions, array &$usedOrders = []): array
@@ -130,7 +130,7 @@ public function syncOptions(string $getTableName, array $options): array
130130
foreach ($currentByName as $lowerKey => $row) {
131131
if (!isset($keptByLowerName[$lowerKey])) {
132132
// This row is not in desired input → consider removal
133-
if (!$this->optionHasReferences($getTableName, (int)$row->id)) {
133+
if (!$this->optionHasReferences($getTableName, $row->id)) {
134134
$this->dynamicListAttrRepository->deleteById($getTableName, (int)$row->id);
135135
} else {
136136
$result[] = $row;
@@ -142,9 +142,10 @@ public function syncOptions(string $getTableName, array $options): array
142142
});
143143
}
144144

145-
private function optionHasReferences(string $listTable, int $id): bool
145+
private function optionHasReferences(string $listTable, int $optionId): bool
146146
{
147-
return $this->dynamicListAttrRepository->existsById($listTable, $id);
147+
return $this->attributeValueRepo
148+
->existsByAttributeAndValue(tableName: $listTable, optionId: $optionId);
148149
}
149150

150151
private function getSetListOrders(array $options): array
@@ -158,6 +159,14 @@ private function getSetListOrders(array $options): array
158159
return array_flip($nonNullListOrders);
159160
}
160161

162+
/**
163+
* Splits the dynamic list attribute set into lookup maps.
164+
*
165+
* @return array{
166+
* 0: array<int, DynamicListAttrDto>,
167+
* 1: array<string, DynamicListAttrDto>
168+
* }
169+
*/
161170
private function splitOptionsSet(array $options, array &$usedOrders): array
162171
{
163172
$incomingById = [];
@@ -192,6 +201,15 @@ private function splitOptionsSet(array $options, array &$usedOrders): array
192201
return [$incomingById, $incomingNew];
193202
}
194203

204+
/**
205+
* Splits the current dynamic list attribute set into lookup maps.
206+
*
207+
* @param string $listTable
208+
* @return array{
209+
* 0: array<int, DynamicListAttrDto>,
210+
* 1: array<string, DynamicListAttrDto>
211+
* }
212+
*/
195213
private function splitCurrentSet(string $listTable): array
196214
{
197215
$currentById = [];
@@ -210,6 +228,7 @@ private function updateRowsById(array $incomingById, array $currentById, string
210228
{
211229
$result = [];
212230

231+
/** @var DynamicListAttrDto $dto */
213232
foreach ($incomingById as $dto) {
214233
if (!isset($currentById[$dto->id])) {
215234
// Unexpected: incoming has id but the current table does not — insert a new row
@@ -222,7 +241,7 @@ private function updateRowsById(array $incomingById, array $currentById, string
222241
$cur = $currentById[$dto->id];
223242
$updates = [];
224243
if ($cur->name !== $dto->name) {
225-
$nameExists = $this->dynamicListAttrRepository->existsByName($listTable, $dto->name);
244+
$nameExists = $this->dynamicListAttrRepository->existsByName($listTable, $dto);
226245
if ($nameExists) {
227246
throw new RuntimeException('Option name ' . $dto->name . ' already exists.');
228247
}

‎tests/Integration/Domain/Subscription/Service/DynamicListAttrManagerFunctionalTest.php‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
use PhpList\Core\Domain\Subscription\Model\Dto\DynamicListAttrDto;
88
use PhpList\Core\Domain\Subscription\Repository\DynamicListAttrRepository;
9+
use PhpList\Core\Domain\Subscription\Repository\SubscriberAttributeValueRepository;
910
use PhpList\Core\Domain\Subscription\Service\Manager\DynamicListAttrManager;
1011
use PhpList\Core\Domain\Subscription\Service\Manager\DynamicListAttrTablesManager;
1112
use PhpList\Core\TestingSupport\Traits\DatabaseTestTrait;
@@ -21,7 +22,7 @@ class DynamicListAttrManagerFunctionalTest extends KernelTestCase
2122
{
2223
use DatabaseTestTrait;
2324

24-
private ?DynamicListAttrRepository $repo = null;
25+
private ?DynamicListAttrRepository $dynamicListAttrRepo = null;
2526
private ?DynamicListAttrManager $manager = null;
2627
private ?DynamicListAttrTablesManager $tablesManager = null;
2728

@@ -36,19 +37,23 @@ protected function setUp(): void
3637

3738
// Use real repository and manager services (but we construct them explicitly
3839
// to be independent from service wiring changes).
39-
$this->repo = new DynamicListAttrRepository(
40+
$this->dynamicListAttrRepo = new DynamicListAttrRepository(
4041
connection: $connection,
4142
serializer: $serializer,
4243
dbPrefix: 'phplist_',
4344
dynamicListTablePrefix: 'listattr_'
4445
);
4546

47+
$subscriberAttributeValueRepo = null;
48+
$subscriberAttributeValueRepo = self::getContainer()->get(SubscriberAttributeValueRepository::class);
49+
4650
// Get tables manager from container for creating/ensuring dynamic tables
4751
$this->tablesManager = self::getContainer()->get(DynamicListAttrTablesManager::class);
4852

4953
// Create manager with actual constructor signature
5054
$this->manager = new DynamicListAttrManager(
51-
dynamicListAttrRepository: $this->repo,
55+
dynamicListAttrRepository: $this->dynamicListAttrRepo,
56+
subscriberAttributeValueRepository: $subscriberAttributeValueRepo
5257
);
5358
}
5459

@@ -72,19 +77,19 @@ public function testCreateInsertAndFetchOptionsEndToEnd(): void
7277
Assert::assertSame(['Blue', 'Red'], $names);
7378

7479
// Now fetch through the repository
75-
$all = $this->repo->getAll('colours');
80+
$all = $this->dynamicListAttrRepo->getAll('colours');
7681
Assert::assertCount(2, $all);
7782

7883
// Collect ids to test fetchOptionNames/fetchSingleOptionName
7984
$ids = array_map(fn(DynamicListAttrDto $d) => (int)$d->id, $inserted);
8085
sort($ids);
8186

82-
$fetchedNames = $this->repo->fetchOptionNames('colours', $ids);
87+
$fetchedNames = $this->dynamicListAttrRepo->fetchOptionNames('colours', $ids);
8388
sort($fetchedNames);
8489
Assert::assertSame(['Blue', 'Red'], $fetchedNames);
8590

8691
// Single fetch
87-
$oneName = $this->repo->fetchSingleOptionName('colours', $ids[0]);
92+
$oneName = $this->dynamicListAttrRepo->fetchSingleOptionName('colours', $ids[0]);
8893
Assert::assertNotNull($oneName);
8994
Assert::assertTrue(in_array($oneName, ['Blue', 'Red'], true));
9095
}
@@ -102,7 +107,7 @@ protected function tearDown(): void
102107
} catch (Throwable $e) {
103108
// Ignore cleanup failures to not mask test results
104109
} finally {
105-
$this->repo = null;
110+
$this->dynamicListAttrRepo = null;
106111
$this->manager = null;
107112
$this->tablesManager = null;
108113
parent::tearDown();

‎tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrManagerTest.php‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,23 +6,28 @@
66

77
use PhpList\Core\Domain\Subscription\Model\Dto\DynamicListAttrDto;
88
use PhpList\Core\Domain\Subscription\Repository\DynamicListAttrRepository;
9+
use PhpList\Core\Domain\Subscription\Repository\SubscriberAttributeValueRepository;
910
use PhpList\Core\Domain\Subscription\Service\Manager\DynamicListAttrManager;
1011
use PHPUnit\Framework\MockObject\MockObject;
1112
use PHPUnit\Framework\TestCase;
1213

1314
class DynamicListAttrManagerTest extends TestCase
1415
{
1516
private DynamicListAttrRepository&MockObject $listAttrRepo;
17+
private ?SubscriberAttributeValueRepository $subscriberAttributeValueRepo = null;
18+
1619

1720
protected function setUp(): void
1821
{
22+
$this->subscriberAttributeValueRepo = $this->createMock(SubscriberAttributeValueRepository::class);
1923
$this->listAttrRepo = $this->createMock(DynamicListAttrRepository::class);
2024
}
2125

2226
private function makeManager(): DynamicListAttrManager
2327
{
2428
return new DynamicListAttrManager(
2529
dynamicListAttrRepository: $this->listAttrRepo,
30+
subscriberAttributeValueRepository: $this->subscriberAttributeValueRepo
2631
);
2732
}
2833

@@ -31,6 +36,7 @@ private function makePartialManager(array $methods): DynamicListAttrManager|Mock
3136
return $this->getMockBuilder(DynamicListAttrManager::class)
3237
->setConstructorArgs([
3338
$this->listAttrRepo,
39+
$this->subscriberAttributeValueRepo,
3440
])
3541
->onlyMethods($methods)
3642
->getMock();

0 commit comments

Comments
 (0)