diff --git a/src/Application/CustomField/Ports/CustomFieldDataService.php b/src/Application/CustomField/Ports/CustomFieldDataService.php index 9788a9233..3778aa05b 100644 --- a/src/Application/CustomField/Ports/CustomFieldDataService.php +++ b/src/Application/CustomField/Ports/CustomFieldDataService.php @@ -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 * diff --git a/src/Application/CustomField/Services/CustomFieldData.php b/src/Application/CustomField/Services/CustomFieldData.php index 2aa402b93..81fc12518 100644 --- a/src/Application/CustomField/Services/CustomFieldData.php +++ b/src/Application/CustomField/Services/CustomFieldData.php @@ -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 * diff --git a/src/Domain/Common/Adapters/ItemTrait.php b/src/Domain/Common/Adapters/ItemTrait.php index e7a95df22..dc54ff479 100644 --- a/src/Domain/Common/Adapters/ItemTrait.php +++ b/src/Domain/Common/Adapters/ItemTrait.php @@ -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 $customFieldDataService * @@ -200,7 +200,7 @@ protected function deleteCustomFieldsForItem( */ protected function updateCustomFieldsForItem( int $moduleId, - int|array $itemId, + int $itemId, RequestService $request, CustomFieldDataService $customFieldDataService ): void { @@ -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); } diff --git a/src/Domain/CustomField/Ports/CustomFieldDataRepository.php b/src/Domain/CustomField/Ports/CustomFieldDataRepository.php index 47ce41b6a..753911bce 100644 --- a/src/Domain/CustomField/Ports/CustomFieldDataRepository.php +++ b/src/Domain/CustomField/Ports/CustomFieldDataRepository.php @@ -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 + * @throws QueryException + * @throws ConstraintException + */ + public function deleteForDefinition(int $itemId, int $moduleId, int $definitionId): QueryResult; + /** * Returns all the items that were encrypted * diff --git a/src/Infrastructure/Adapter/Out/CustomField/Repositories/CustomFieldData.php b/src/Infrastructure/Adapter/Out/CustomField/Repositories/CustomFieldData.php index 30c627d0c..f22204da9 100644 --- a/src/Infrastructure/Adapter/Out/CustomField/Repositories/CustomFieldData.php +++ b/src/Infrastructure/Adapter/Out/CustomField/Repositories/CustomFieldData.php @@ -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 + * @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 * diff --git a/tests/Unit/Domain/Common/Adapters/ItemTraitTest.php b/tests/Unit/Domain/Common/Adapters/ItemTraitTest.php index 53f31afaf..c1966f0d1 100644 --- a/tests/Unit/Domain/Common/Adapters/ItemTraitTest.php +++ b/tests/Unit/Domain/Common/Adapters/ItemTraitTest.php @@ -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. * diff --git a/tests/Unit/Infrastructure/Adapter/Out/CustomField/Repositories/CustomFieldDataTest.php b/tests/Unit/Infrastructure/Adapter/Out/CustomField/Repositories/CustomFieldDataTest.php index 781245e54..39de7dbad 100644 --- a/tests/Unit/Infrastructure/Adapter/Out/CustomField/Repositories/CustomFieldDataTest.php +++ b/tests/Unit/Infrastructure/Adapter/Out/CustomField/Repositories/CustomFieldDataTest.php @@ -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