Skip to content
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/Application/CustomField/Ports/CustomFieldDataService.php
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,17 @@ public function updateOrCreate(CustomFieldData $customFieldData): void;
*/
public function delete(array $itemsId, int $moduleId): void;

/**
* Delete one field's value, leaving the item's other fields alone
*
* @param int $itemId
* @param int $moduleId
* @param int $definitionId
*
* @throws ServiceException
*/
public function deleteForDefinition(int $itemId, int $moduleId, int $definitionId): void;

/**
* Creates an item
*
Expand Down
12 changes: 12 additions & 0 deletions src/Application/CustomField/Services/CustomFieldData.php
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,18 @@ public function delete(array $itemsId, int $moduleId): void
}
}

/**
* @inheritDoc
*/
public function deleteForDefinition(int $itemId, int $moduleId, int $definitionId): void
{
try {
$this->customFieldDataRepository->deleteForDefinition($itemId, $moduleId, $definitionId);
} catch (SPException $e) {
throw ServiceException::from($e);
}
}

/**
* Updates an item
*
Expand Down
9 changes: 6 additions & 3 deletions src/Domain/Common/Adapters/ItemTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ protected function deleteCustomFieldsForItem(
* Update the item's custom fields
*
* @param int $moduleId
* @param int|int[] $itemId
* @param int $itemId
* @param RequestService $request
* @param CustomFieldDataService<CustomFieldDataModel> $customFieldDataService
*
Expand All @@ -200,7 +200,7 @@ protected function deleteCustomFieldsForItem(
*/
protected function updateCustomFieldsForItem(
int $moduleId,
int|array $itemId,
int $itemId,
RequestService $request,
CustomFieldDataService $customFieldDataService
): void {
Expand Down Expand Up @@ -233,7 +233,10 @@ protected function updateCustomFieldsForItem(
);

if (empty($customFieldData->getData())) {
$customFieldDataService->delete([$itemId], $moduleId);
// Only this field. delete() is keyed on the item and the module alone — which
// is what deleting the item itself wants — so clearing one field used to take
// every other custom field on the item with it, silently, in the same save.
$customFieldDataService->deleteForDefinition($itemId, $moduleId, $id);
} else {
$customFieldDataService->updateOrCreate($customFieldData);
}
Expand Down
13 changes: 13 additions & 0 deletions src/Domain/CustomField/Ports/CustomFieldDataRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,19 @@ public function create(CustomFieldDataModel $customFieldData): QueryResult;
*/
public function deleteBatch(array $itemIds, int $moduleId): QueryResult;

/**
* Delete one field's value, leaving the item's other fields alone
*
* @param int $itemId
* @param int $moduleId
* @param int $definitionId
*
* @return QueryResult<Simple>
* @throws QueryException
* @throws ConstraintException
*/
public function deleteForDefinition(int $itemId, int $moduleId, int $definitionId): QueryResult;

/**
* Returns all the items that were encrypted
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,40 @@ public function deleteBatch(array $itemIds, int $moduleId): QueryResult
return $this->db->runQuery(QueryData::build($query));
}

/**
* Delete one field's value, leaving the item's other fields alone.
*
* deleteBatch() is keyed on the item and the module only, which is what deleting the item
* itself wants. Clearing a single field needs the definition in the WHERE too, or it takes
* every other field on the item with it.
*
* @param int $itemId
* @param int $moduleId
* @param int $definitionId
*
* @return QueryResult<Simple>
* @throws QueryException
* @throws ConstraintException
*/
public function deleteForDefinition(int $itemId, int $moduleId, int $definitionId): QueryResult
{
$query = $this->queryFactory
->newDelete()
->from(CustomFieldDataModel::TABLE)
->where('moduleId = :moduleId')
->where('itemId = :itemId')
->where('definitionId = :definitionId')
->bindValues(
[
'moduleId' => $moduleId,
'itemId' => $itemId,
'definitionId' => $definitionId,
]
);

return $this->db->runQuery(QueryData::build($query));
}

/**
* Returns all the items
*
Expand Down
43 changes: 42 additions & 1 deletion tests/Unit/Domain/Common/Adapters/ItemTraitTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -229,17 +229,58 @@ public function updatingSavesTheFieldsThatHaveAValue()
/**
* Clearing a field deletes the row rather than saving an empty one. Only the delete removes the
* stored value, so saving the blank would leave the old ciphertext behind.
*
* It deletes that field, keyed on its definition. It used to call delete(), which is keyed on
* the item and the module alone.
*/
#[Test]
public function clearingAFieldDeletesItRatherThanSavingItEmpty()
{
$service = $this->createMock(CustomFieldDataService::class);
$service->expects(self::never())->method('updateOrCreate');
$service->expects(self::once())->method('delete')->with([7], self::MODULE_ID);
$service->expects(self::never())->method('delete');
$service->expects(self::once())->method('deleteForDefinition')->with(7, self::MODULE_ID, 4);

$this->host->updateCustomFields(self::MODULE_ID, 7, $this->givenThePostedFields([4 => '']), $service);
}

/**
* Clearing one field leaves the item's other fields alone.
*
* The delete this used to make carried only the item and the module, so emptying a single
* field removed every custom field value on the item — including encrypted ones the person
* saving could not see and had not touched. Nothing reported it: the save succeeded and the
* other fields simply came back blank.
*/
#[Test]
public function clearingOneFieldLeavesTheOthersAlone()
{
$deleted = [];
$saved = [];

$service = $this->createStub(CustomFieldDataService::class);
$service->method('deleteForDefinition')->willReturnCallback(
static function (int $itemId, int $moduleId, int $definitionId) use (&$deleted): void {
$deleted[] = $definitionId;
}
);
$service->method('updateOrCreate')->willReturnCallback(
static function ($customFieldData) use (&$saved): void {
$saved[] = $customFieldData->getDefinitionId();
}
);

$this->host->updateCustomFields(
self::MODULE_ID,
7,
$this->givenThePostedFields([4 => 'kept', 5 => '', 6 => 'also kept']),
$service
);

self::assertSame([5], $deleted, 'only the cleared field is deleted');
self::assertSame([4, 6], $saved, 'the others are still saved');
}

/**
* A masked value is not saved over the secret it stands in for.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,45 @@ public function testDeleteBatchWithNoItems()
$this->assertEquals(0, $result->getNumRows());
}

/**
* The delete that clears one field carries the definition as well as the item and the module.
*
* deleteBatch() above is keyed on the item and the module only — right for deleting the item,
* wrong for clearing one of its fields, which used to take every other field with it. The
* assertion is on the emitted statement's bound values rather than on a row count, because a
* WHERE clause that is missing a condition still deletes successfully.
*
* @throws ConstraintException
* @throws QueryException
*/
public function testDeleteForDefinitionIsKeyedOnTheDefinitionToo()
{
$itemId = self::$faker->numberBetween(1, 1000);
$moduleId = self::$faker->numberBetween(1, 1000);
$definitionId = self::$faker->numberBetween(1, 1000);

$callback = new Callback(
static function (QueryData $arg) use ($itemId, $moduleId, $definitionId) {
$query = $arg->getQuery();
$bindValues = $query->getBindValues();

return count($bindValues) === 3
&& $bindValues['moduleId'] === $moduleId
&& $bindValues['itemId'] === $itemId
&& $bindValues['definitionId'] === $definitionId
&& is_a($query, DeleteInterface::class)
&& !empty($query->getStatement());
}
);

$this->database
->expects(self::once())
->method('runQuery')
->with($callback);

$this->customFieldData->deleteForDefinition($itemId, $moduleId, $definitionId);
}

/**
* @throws ConstraintException
* @throws QueryException
Expand Down