From 33839e5dd5db186f8cbe9e5bf702cac2cd5d7b99 Mon Sep 17 00:00:00 2001 From: its-amann Date: Fri, 15 May 2026 18:17:02 +0530 Subject: [PATCH 1/3] fix civil id upload and removal flow --- .../v1/controllers/AccountController.php | 75 +++++++-- candidate/tests/functional/AccountCest.php | 28 ++++ common/components/S3ResourceManager.php | 53 ++++-- common/models/Candidate.php | 139 ++++++++++++---- .../unit/models/CandidateS3BehaviorTest.php | 151 ++++++++++++++++++ 5 files changed, 390 insertions(+), 56 deletions(-) create mode 100644 common/tests/unit/models/CandidateS3BehaviorTest.php diff --git a/candidate/modules/v1/controllers/AccountController.php b/candidate/modules/v1/controllers/AccountController.php index 171d01ff..a920f220 100644 --- a/candidate/modules/v1/controllers/AccountController.php +++ b/candidate/modules/v1/controllers/AccountController.php @@ -384,10 +384,11 @@ public function actionRemoveCivilPhotoBack() { $model = Candidate::findOne(Yii::$app->user->getId()); if ($model->candidate_civil_photo_back) { - $model->deleteFile('civil-id', 'back'); + $model->deleteFile('civil-id', 'back', true); } $model->candidate_civil_photo_back = null; + $model->candidate_civil_need_verification = true; $model->scenario = 'updateCivilPhotoBack'; @@ -410,10 +411,11 @@ public function actionRemoveCivilPhotoFront() { $model = Candidate::findOne(Yii::$app->user->getId()); if ($model->candidate_civil_photo_front) { - $model->deleteFile('civil-id', 'front'); + $model->deleteFile('civil-id', 'front', true); } $model->candidate_civil_photo_front = null; + $model->candidate_civil_need_verification = true; $model->scenario = 'updateCivilPhotoFront'; @@ -1301,7 +1303,12 @@ public function actionUpdateCivilPhotoBack() { ]; } - $model->updateCivilId('back'); + if (!$model->updateCivilId('back')) { + return [ + 'operation' => 'error', + 'message' => $model->getErrors() + ]; + } //reset to remove old id's data $model->candidate_civil_expiry_date = null; @@ -1353,7 +1360,12 @@ public function actionUpdateCivilPhotoFront() { ]; } - $model->updateCivilId('front'); + if (!$model->updateCivilId('front')) { + return [ + 'operation' => 'error', + 'message' => $model->getErrors() + ]; + } //reset to remove old id's data $model->candidate_civil_expiry_date = null; @@ -1430,24 +1442,65 @@ public function actionUpdateCivilIdExpiryDate() { throw new \yii\web\HttpException(404, Yii::t('candidate', 'The requested Item could not be found.')); } - $candidate_civil_id = Yii::$app->request->getBodyParam('civil_id'); - + $candidate_civil_id = trim((string) Yii::$app->request->getBodyParam('civil_id', '')); $candidate_civil_expiry_date = Yii::$app->request->getBodyParam('civil_expiry_date'); - $candidate->candidate_civil_id = $candidate_civil_id; + if ($candidate_civil_id === '') { + return [ + 'operation' => 'error', + 'message' => Yii::t('candidate', 'Civil ID is required') + ]; + } - if($candidate_civil_expiry_date) - $candidate->candidate_civil_expiry_date = date('Y-m-d', strtotime($candidate_civil_expiry_date)); + if (!preg_match('/^\d{12}$/', $candidate_civil_id)) { + return [ + 'operation' => 'error', + 'message' => Yii::t('candidate', 'Civil ID must be 12 digit number') + ]; + } + + if (!is_scalar($candidate_civil_expiry_date) || trim((string) $candidate_civil_expiry_date) === '') { + return [ + 'operation' => 'error', + 'message' => Yii::t('candidate', 'Civil expiry date is required') + ]; + } + + $expiryTimestamp = strtotime((string) $candidate_civil_expiry_date); + if ($expiryTimestamp === false) { + return [ + 'operation' => 'error', + 'message' => Yii::t('candidate', 'Invalid civil expiry date') + ]; + } + + $candidate->candidate_civil_id = $candidate_civil_id; + $candidate->candidate_civil_expiry_date = date('Y-m-d', $expiryTimestamp); $candidate->candidate_civil_need_verification = true; $candidate->scenario = "updateCivilExpiryDateAndCivilID"; - if (!$candidate->save()) { + try { + if (!$candidate->save()) { + return [ + "operation" => "error", + "message" => $candidate->getErrors() + ]; + } + } catch (\Throwable $e) { + Yii::error([ + 'message' => 'Failed to update civil ID and expiry date', + 'route' => Yii::$app->requestedRoute, + 'candidate_id' => $candidate->candidate_id, + 'civil_id' => $candidate_civil_id, + 'civil_expiry_date' => $candidate->candidate_civil_expiry_date, + 'error' => $e->getMessage(), + ], 'candidate'); return [ "operation" => "error", - "message" => $candidate->errors + "message" => Yii::t('candidate', 'Unable to update Civil ID and expiry date right now') ]; } diff --git a/candidate/tests/functional/AccountCest.php b/candidate/tests/functional/AccountCest.php index b25e116a..3f5fdd84 100644 --- a/candidate/tests/functional/AccountCest.php +++ b/candidate/tests/functional/AccountCest.php @@ -183,6 +183,34 @@ public function tryUpdateCivilId(FunctionalTester $I) $I->seeResponseCodeIs(HttpCode::OK); // 200 $I->seeResponseContainsJson([ 'operation' => 'success','message' => 'Candidate Civil ID Info Updated Successfully']); } + + public function tryUpdateCivilIdExpiryDateRequiresCivilId(FunctionalTester $I) + { + $I->amGoingTo('reject empty civil id while updating civil id and expiry date'); + $I->sendPOST('v1/account/update-civil-id-expiry-date', [ + 'civil_id' => '', + 'civil_expiry_date' => date('Y-m-d', strtotime('+1 month')), + ]); + $I->seeResponseCodeIs(HttpCode::OK); + $I->seeResponseContainsJson([ + 'operation' => 'error', + 'message' => 'Civil ID is required', + ]); + } + + public function tryUpdateCivilIdExpiryDateRejectsInvalidDate(FunctionalTester $I) + { + $I->amGoingTo('reject invalid civil expiry date while updating civil id and expiry date'); + $I->sendPOST('v1/account/update-civil-id-expiry-date', [ + 'civil_id' => '123456789012', + 'civil_expiry_date' => 'not-a-date', + ]); + $I->seeResponseCodeIs(HttpCode::OK); + $I->seeResponseContainsJson([ + 'operation' => 'error', + 'message' => 'Invalid civil expiry date', + ]); + } public function tryUpdateNationality(FunctionalTester $I) { diff --git a/common/components/S3ResourceManager.php b/common/components/S3ResourceManager.php index 608a407e..3601e5f0 100644 --- a/common/components/S3ResourceManager.php +++ b/common/components/S3ResourceManager.php @@ -166,25 +166,58 @@ public function delete($name) } /** - * Checks whether a file exists or not. This method only works for public resources, private resources will throw - * a 403 error exception. + * Returns the bucket/key pair for a stored object reference. + * Supports raw keys and S3 object URLs. + * @param string $filenameOrUrl + * @return array + */ + protected function resolveObjectLocation($filenameOrUrl) + { + $bucket = $this->bucket; + $key = ltrim((string) $filenameOrUrl, '/'); + + if (strpos((string) $filenameOrUrl, 'http') !== false) { + $parts = parse_url($filenameOrUrl); + $key = isset($parts['path']) ? ltrim($parts['path'], '/') : ''; + + if (!empty($parts['host']) && preg_match('/^([^.]+)\.s3[.-]/', $parts['host'], $matches)) { + $bucket = $matches[1]; + } + } + + return [$bucket, $key]; + } + + /** + * Returns S3 object metadata. + * @param string $filenameOrUrl + * @return \Aws\Result + */ + public function headObject($filenameOrUrl) + { + [$bucket, $key] = $this->resolveObjectLocation($filenameOrUrl); + + return $this->getClient()->headObject([ + 'Bucket' => $bucket, + 'Key' => $key, + ]); + } + + /** + * Checks whether a file exists or not using S3 metadata. * @param string $filenameOrUrl the name or url of the file * @return boolean */ public function fileExists($filenameOrUrl) { - $isUrl = false; - if (strpos($filenameOrUrl, 'http') !== false) { - $isUrl = true; - } - - $http = new \GuzzleHttp\Client(['base_uri' => $isUrl ? $filenameOrUrl : $this->getUrl($filenameOrUrl)]); try { - $response = $http->request('HEAD'); + $this->headObject($filenameOrUrl); + return true; + } catch (AwsException $e) { + return false; } catch (\Exception $e) { return false; } - return $response->getStatusCode() == 200; } /** diff --git a/common/models/Candidate.php b/common/models/Candidate.php index b1c5694b..acef1211 100644 --- a/common/models/Candidate.php +++ b/common/models/Candidate.php @@ -338,10 +338,10 @@ public function scenarios() { $scenarios['tmpProfilePhoto'] = ['profile_photo', 'is_incomplete_profile']; $scenarios['updateCivilPhotoBack'] = ['candidate_civil_photo_back', "candidate_civil_expiry_date", - "candidate_civil_id", 'is_incomplete_profile']; + "candidate_civil_id", "candidate_civil_need_verification", 'is_incomplete_profile']; $scenarios['updateCivilPhotoFront'] = ['candidate_civil_photo_front', "candidate_civil_expiry_date", - "candidate_civil_id", 'is_incomplete_profile']; + "candidate_civil_id", "candidate_civil_need_verification", 'is_incomplete_profile']; $scenarios['updateNationality'] = ['country_id', 'is_incomplete_profile']; @@ -2693,41 +2693,92 @@ public function setProfileByUrl($url) { * delete file from aws * @param string $type * @param string $side - * @return false + * @param bool $bestEffort + * @return bool + */ + private function resolveStoredFileAttribute($type = 'resume', $side = 'front') + { + if ($type === 'civil-id') { + return $side === 'back' ? 'candidate_civil_photo_back' : 'candidate_civil_photo_front'; + } + + return 'candidate_resume'; + } + + /** + * @param string $type + * @param string $side + * @param bool $useOldAttributes + * @return string|null */ - public function deleteFile($type = 'resume', $side = 'front') { + private function resolveStoredFileKey($type = 'resume', $side = 'front', $useOldAttributes = true) + { + $attribute = $this->resolveStoredFileAttribute($type, $side); + $fileName = $useOldAttributes ? ($this->oldAttributes[$attribute] ?? null) : $this->$attribute; + + if (!$fileName) { + return null; + } + + $directory = $type === 'civil-id' ? 'photos' : 'candidate-resume'; + + return $directory . '/' . $fileName; + } + + /** + * @param string $operation + * @param string $attribute + * @param string|null $fileKey + * @param \Throwable $exception + * @param array $extra + * @return void + */ + private function logS3FileFailure($operation, $attribute, $fileKey, \Throwable $exception, array $extra = []) + { + Yii::error(array_merge([ + 'message' => sprintf('Failed to %s candidate file', $operation), + 'operation' => $operation, + 'route' => Yii::$app->requestedRoute, + 'candidate_id' => $this->candidate_id, + 'attribute' => $attribute, + 'filename' => $fileKey ? basename($fileKey) : null, + 'file_key' => $fileKey, + 'error' => $exception->getMessage(), + ], $extra), 'candidate'); + } + + public function deleteFile($type = 'resume', $side = 'front', $bestEffort = false) { + $attribute = $this->resolveStoredFileAttribute($type, $side); + $fileKey = $this->resolveStoredFileKey($type, $side, true); + + if (!$fileKey || !isset($this->oldPrimaryKey)) { + return true; + } try { - if (isset($this->oldPrimaryKey)) { - - $file = null; - - if ($type == 'resume' && isset($this->oldAttributes['candidate_resume'])) { - $file = "candidate-resume/" . $this->oldAttributes['candidate_resume']; - } else if ($type == 'civil-id' && $side == 'front' && isset($this->oldAttributes['candidate_civil_photo_front'])) { - $file = "candidate-civil-id/" . $this->oldAttributes['candidate_civil_photo_front']; - } else if ($type == 'civil-id' && $side == 'back' && isset($this->oldAttributes['candidate_civil_photo_back'])) { - $file = "candidate-civil-id/" . $this->oldAttributes['candidate_civil_photo_back']; - } - - if ($file) { - Yii::$app->resourceManager->delete($file); - } - } + Yii::$app->resourceManager->delete($fileKey); + + return true; } catch (\Aws\S3\Exception\S3Exception $e) { + $this->logS3FileFailure('delete', $attribute, $fileKey, $e); - Yii::error($e->getMessage(), 'candidate'); + if ($bestEffort) { + return true; + } - $this->addError('candidate_resume', Yii::t('app', 'file not available to delete.')); + $this->addError($attribute, Yii::t('app', 'file not available to delete.')); return false; } catch (\Exception $e) { + $this->logS3FileFailure('delete', $attribute, $fileKey, $e); - Yii::error($e->getMessage(), 'candidate'); + if ($bestEffort) { + return true; + } - $this->addError('candidate_resume', Yii::t('app', 'file not available to delete.')); + $this->addError($attribute, Yii::t('app', 'file not available to delete.')); return false; } @@ -2739,34 +2790,52 @@ public function deleteFile($type = 'resume', $side = 'front') { public function updateCivilId($side = 'front') { $idSide = ($side == 'front') ? 'candidate_civil_photo_front' : 'candidate_civil_photo_back'; + $fileName = $this->$idSide; - if (!empty($this->oldAttributes[$idSide])) { - $this->deleteFile('civil-id', $side); + if (!$fileName) { + $this->addError($idSide, Yii::t('app', 'file not available to save.')); + return false; } - $fileName = $this->$idSide; - $sourceBucket = Yii::$app->temporaryBucketResourceManager->bucket; - $targetPath = "photos/" . $fileName; + $oldFilePath = $this->resolveStoredFileKey('civil-id', $side, true); // Copy using S3ResourceManager Component - try { + Yii::$app->resourceManager->copy($fileName, $targetPath, $sourceBucket); - return Yii::$app->resourceManager->copy($fileName, $targetPath, $sourceBucket); + if (!Yii::$app->resourceManager->fileExists($targetPath)) { + $exception = new \RuntimeException('Copied file could not be verified in the permanent bucket.'); + $this->logS3FileFailure('verify-copy', $idSide, $targetPath, $exception, [ + 'source_bucket' => $sourceBucket, + 'source_filename' => $fileName, + ]); + $this->addError($idSide, Yii::t('app', 'file not available to save.')); + return false; + } - } catch (\Aws\S3\Exception\S3Exception $e) { + if ($oldFilePath && $oldFilePath !== $targetPath) { + $this->deleteFile('civil-id', $side, true); + } - Yii::error($e->getMessage(), 'candidate'); + return true; + + } catch (\Aws\S3\Exception\S3Exception $e) { + $this->logS3FileFailure('copy', $idSide, $targetPath, $e, [ + 'source_bucket' => $sourceBucket, + 'source_filename' => $fileName, + ]); $this->addError($idSide, Yii::t('app', 'file not available to save.')); return false; } catch (\Exception $e) { - - Yii::error($e->getMessage(), 'candidate'); + $this->logS3FileFailure('copy', $idSide, $targetPath, $e, [ + 'source_bucket' => $sourceBucket, + 'source_filename' => $fileName, + ]); $this->addError($idSide, Yii::t('app', 'file not available to save.')); diff --git a/common/tests/unit/models/CandidateS3BehaviorTest.php b/common/tests/unit/models/CandidateS3BehaviorTest.php new file mode 100644 index 00000000..9dfaf9ab --- /dev/null +++ b/common/tests/unit/models/CandidateS3BehaviorTest.php @@ -0,0 +1,151 @@ +candidate_id = 1; + $candidate->setOldAttributes([ + 'candidate_id' => 1, + 'candidate_civil_photo_front' => 'front.jpg', + 'candidate_civil_photo_back' => 'back.jpg', + ]); + + $originalManager = Yii::$app->get('resourceManager'); + + $trackingManager = new class extends \yii\base\Component { + public $deleted = []; + + public function delete($name) + { + $this->deleted[] = $name; + return true; + } + }; + + try { + Yii::$app->set('resourceManager', $trackingManager); + $candidate->clearErrors(); + + $this->assertTrue($candidate->deleteFile('civil-id', 'front')); + $this->assertSame(['photos/front.jpg'], $trackingManager->deleted); + + $failingManager = new class extends \yii\base\Component { + public function delete($name) + { + throw new \Exception('delete failed'); + } + }; + + Yii::$app->set('resourceManager', $failingManager); + $candidate->clearErrors(); + + $this->assertFalse($candidate->deleteFile('civil-id', 'back')); + $this->assertArrayHasKey('candidate_civil_photo_back', $candidate->getErrors()); + $this->assertArrayNotHasKey('candidate_resume', $candidate->getErrors()); + } finally { + Yii::$app->set('resourceManager', $originalManager); + } + } + + public function testUpdateCivilIdCopiesBeforeDeletingAndVerifiesTheNewFile() + { + $candidate = new CandidateS3BehaviorTestModel(); + $candidate->candidate_id = 1; + $candidate->candidate_civil_photo_front = 'new-front.jpg'; + $candidate->setOldAttributes([ + 'candidate_id' => 1, + 'candidate_civil_photo_front' => 'old-front.jpg', + ]); + + $originalManager = Yii::$app->get('resourceManager'); + $originalTempManager = Yii::$app->get('temporaryBucketResourceManager'); + + $resourceManager = new class extends \yii\base\Component { + public $operations = []; + public $exists = true; + + public function copy($oldFile, $newFile, $sourceBucket = "", $options = []) + { + $this->operations[] = ['copy', $oldFile, $newFile, $sourceBucket]; + return true; + } + + public function fileExists($name) + { + $this->operations[] = ['fileExists', $name]; + return $this->exists; + } + + public function delete($name) + { + $this->operations[] = ['delete', $name]; + return true; + } + }; + + $tempManager = new class extends \yii\base\Component { + public $bucket = 'temp-upload-bucket'; + }; + + try { + Yii::$app->set('resourceManager', $resourceManager); + Yii::$app->set('temporaryBucketResourceManager', $tempManager); + + $candidate->clearErrors(); + $this->assertTrue($candidate->updateCivilId('front')); + $this->assertSame([ + ['copy', 'new-front.jpg', 'photos/new-front.jpg', 'temp-upload-bucket'], + ['fileExists', 'photos/new-front.jpg'], + ['delete', 'photos/old-front.jpg'], + ], $resourceManager->operations); + + $candidate->candidate_civil_photo_front = 'verify-miss.jpg'; + $candidate->setOldAttributes([ + 'candidate_id' => 1, + 'candidate_civil_photo_front' => 'old-front.jpg', + ]); + $candidate->clearErrors(); + + $resourceManager->operations = []; + $resourceManager->exists = false; + + $this->assertFalse($candidate->updateCivilId('front')); + $this->assertSame([ + ['copy', 'verify-miss.jpg', 'photos/verify-miss.jpg', 'temp-upload-bucket'], + ['fileExists', 'photos/verify-miss.jpg'], + ], $resourceManager->operations); + $this->assertArrayHasKey('candidate_civil_photo_front', $candidate->getErrors()); + } finally { + Yii::$app->set('resourceManager', $originalManager); + Yii::$app->set('temporaryBucketResourceManager', $originalTempManager); + } + } +} From 8d35124b4e7243bc28df04ebbcaaea2caafea684 Mon Sep 17 00:00:00 2001 From: its-amann Date: Fri, 15 May 2026 19:37:44 +0530 Subject: [PATCH 2/3] Address civil ID review findings --- .../v1/controllers/AccountController.php | 6 ++- candidate/tests/functional/AccountCest.php | 17 +++++++ common/components/S3ResourceManager.php | 6 +++ common/models/Candidate.php | 42 +++++++++++++++- .../unit/models/CandidateS3BehaviorTest.php | 48 ++++++++++++++++++- 5 files changed, 115 insertions(+), 4 deletions(-) diff --git a/candidate/modules/v1/controllers/AccountController.php b/candidate/modules/v1/controllers/AccountController.php index a920f220..d2fa0e5d 100644 --- a/candidate/modules/v1/controllers/AccountController.php +++ b/candidate/modules/v1/controllers/AccountController.php @@ -1321,6 +1321,8 @@ public function actionUpdateCivilPhotoBack() { ]; } + $model->deletePendingCivilIdFiles('back'); + return [ 'operation' => 'success', "candidate_civil_photo_back" => $model->candidate_civil_photo_back, @@ -1378,6 +1380,8 @@ public function actionUpdateCivilPhotoFront() { ]; } + $model->deletePendingCivilIdFiles('front'); + return [ 'operation' => 'success', @@ -1493,8 +1497,6 @@ public function actionUpdateCivilIdExpiryDate() { 'message' => 'Failed to update civil ID and expiry date', 'route' => Yii::$app->requestedRoute, 'candidate_id' => $candidate->candidate_id, - 'civil_id' => $candidate_civil_id, - 'civil_expiry_date' => $candidate->candidate_civil_expiry_date, 'error' => $e->getMessage(), ], 'candidate'); diff --git a/candidate/tests/functional/AccountCest.php b/candidate/tests/functional/AccountCest.php index 3f5fdd84..985ad57b 100644 --- a/candidate/tests/functional/AccountCest.php +++ b/candidate/tests/functional/AccountCest.php @@ -198,6 +198,23 @@ public function tryUpdateCivilIdExpiryDateRequiresCivilId(FunctionalTester $I) ]); } + /** + * Verifies non-empty but invalid Civil IDs are rejected before saving. + */ + public function tryUpdateCivilIdExpiryDateRejectsShortCivilId(FunctionalTester $I) + { + $I->amGoingTo('reject short civil id while updating civil id and expiry date'); + $I->sendPOST('v1/account/update-civil-id-expiry-date', [ + 'civil_id' => '70', + 'civil_expiry_date' => date('Y-m-d', strtotime('+1 month')), + ]); + $I->seeResponseCodeIs(HttpCode::OK); + $I->seeResponseContainsJson([ + 'operation' => 'error', + 'message' => 'Civil ID must be 12 digits', + ]); + } + public function tryUpdateCivilIdExpiryDateRejectsInvalidDate(FunctionalTester $I) { $I->amGoingTo('reject invalid civil expiry date while updating civil id and expiry date'); diff --git a/common/components/S3ResourceManager.php b/common/components/S3ResourceManager.php index 3601e5f0..505939b0 100644 --- a/common/components/S3ResourceManager.php +++ b/common/components/S3ResourceManager.php @@ -182,7 +182,13 @@ protected function resolveObjectLocation($filenameOrUrl) if (!empty($parts['host']) && preg_match('/^([^.]+)\.s3[.-]/', $parts['host'], $matches)) { $bucket = $matches[1]; + } elseif (!empty($parts['host']) && preg_match('/^s3[.-]/', $parts['host']) && $key !== '') { + $segments = explode('/', $key, 2); + $bucket = $segments[0] ?: $bucket; + $key = $segments[1] ?? ''; } + + $key = rawurldecode($key); } return [$bucket, $key]; diff --git a/common/models/Candidate.php b/common/models/Candidate.php index acef1211..495f52e0 100644 --- a/common/models/Candidate.php +++ b/common/models/Candidate.php @@ -114,6 +114,11 @@ class Candidate extends \yii\db\ActiveRecord implements \yii\web\IdentityInterfa const NOT_HAVE_DRIVING_LICENCE = 2; public $pendingProfile = []; + + /** + * @var array Civil ID S3 keys staged for cleanup after the DB row is saved. + */ + private $pendingCivilIdDeletionKeys = []; // Array of attribute names and folder names to store them in the permanent bucket public $FILE_ATTRIBUTES = [ @@ -2784,6 +2789,38 @@ public function deleteFile($type = 'resume', $side = 'front', $bestEffort = fals } } + /** + * Deletes old Civil ID files that were staged after a verified replacement copy. + * @param string|null $side + * @return bool + */ + public function deletePendingCivilIdFiles($side = null) + { + $pending = $side === null + ? $this->pendingCivilIdDeletionKeys + : [$side => $this->pendingCivilIdDeletionKeys[$side] ?? null]; + + $deleted = true; + foreach ($pending as $pendingSide => $entry) { + if (!$entry || empty($entry['file_key'])) { + continue; + } + + try { + Yii::$app->resourceManager->delete($entry['file_key']); + unset($this->pendingCivilIdDeletionKeys[$pendingSide]); + } catch (\Aws\S3\Exception\S3Exception $e) { + $this->logS3FileFailure('delete', $entry['attribute'], $entry['file_key'], $e); + unset($this->pendingCivilIdDeletionKeys[$pendingSide]); + } catch (\Exception $e) { + $this->logS3FileFailure('delete', $entry['attribute'], $entry['file_key'], $e); + unset($this->pendingCivilIdDeletionKeys[$pendingSide]); + } + } + + return $deleted; + } + /** * @return bool */ @@ -2816,7 +2853,10 @@ public function updateCivilId($side = 'front') { } if ($oldFilePath && $oldFilePath !== $targetPath) { - $this->deleteFile('civil-id', $side, true); + $this->pendingCivilIdDeletionKeys[$side] = [ + 'attribute' => $idSide, + 'file_key' => $oldFilePath, + ]; } return true; diff --git a/common/tests/unit/models/CandidateS3BehaviorTest.php b/common/tests/unit/models/CandidateS3BehaviorTest.php index 9dfaf9ab..00043478 100644 --- a/common/tests/unit/models/CandidateS3BehaviorTest.php +++ b/common/tests/unit/models/CandidateS3BehaviorTest.php @@ -4,6 +4,7 @@ use Yii; use common\models\Candidate; +use common\components\S3ResourceManager; class CandidateS3BehaviorTestModel extends Candidate { @@ -24,6 +25,19 @@ public function attributes() } } +class CandidateS3BehaviorResourceManager extends S3ResourceManager +{ + /** + * Exposes S3 URL parsing for focused regression tests. + * @param string $filenameOrUrl + * @return array + */ + public function resolveLocation($filenameOrUrl) + { + return $this->resolveObjectLocation($filenameOrUrl); + } +} + class CandidateS3BehaviorTest extends \Codeception\Test\Unit { protected $tester; @@ -75,7 +89,10 @@ public function delete($name) } } - public function testUpdateCivilIdCopiesBeforeDeletingAndVerifiesTheNewFile() + /** + * Verifies old Civil ID files are deleted only after callers finish saving. + */ + public function testUpdateCivilIdCopiesAndDefersOldFileDeletionUntilAfterSave() { $candidate = new CandidateS3BehaviorTestModel(); $candidate->candidate_id = 1; @@ -121,6 +138,12 @@ public function delete($name) $candidate->clearErrors(); $this->assertTrue($candidate->updateCivilId('front')); + $this->assertSame([ + ['copy', 'new-front.jpg', 'photos/new-front.jpg', 'temp-upload-bucket'], + ['fileExists', 'photos/new-front.jpg'], + ], $resourceManager->operations); + + $this->assertTrue($candidate->deletePendingCivilIdFiles('front')); $this->assertSame([ ['copy', 'new-front.jpg', 'photos/new-front.jpg', 'temp-upload-bucket'], ['fileExists', 'photos/new-front.jpg'], @@ -148,4 +171,27 @@ public function delete($name) Yii::$app->set('temporaryBucketResourceManager', $originalTempManager); } } + + /** + * Verifies S3 object resolution handles both URL styles and encoded keys. + */ + public function testS3ResourceManagerResolvesPathStyleAndEncodedUrls() + { + $manager = new CandidateS3BehaviorResourceManager([ + 'key' => 'test-key', + 'secret' => 'test-secret', + 'region' => 'eu-west-1', + 'bucket' => 'default-bucket', + ]); + + $this->assertSame( + ['candidate-bucket', 'photos/front id.jpg'], + $manager->resolveLocation('https://candidate-bucket.s3.eu-west-1.amazonaws.com/photos/front%20id.jpg') + ); + + $this->assertSame( + ['candidate-bucket', 'photos/back id.jpg'], + $manager->resolveLocation('https://s3.eu-west-1.amazonaws.com/candidate-bucket/photos/back%20id.jpg') + ); + } } From 892dcf89a5f5f0c671510369f228f460149b203e Mon Sep 17 00:00:00 2001 From: its-amann Date: Fri, 15 May 2026 19:54:49 +0530 Subject: [PATCH 3/3] Harden deferred civil ID cleanup --- .../v1/controllers/AccountController.php | 6 +- common/models/Candidate.php | 18 +++- .../unit/models/CandidateS3BehaviorTest.php | 100 ++++++++++++++++++ 3 files changed, 120 insertions(+), 4 deletions(-) diff --git a/candidate/modules/v1/controllers/AccountController.php b/candidate/modules/v1/controllers/AccountController.php index d2fa0e5d..99a38506 100644 --- a/candidate/modules/v1/controllers/AccountController.php +++ b/candidate/modules/v1/controllers/AccountController.php @@ -1321,7 +1321,7 @@ public function actionUpdateCivilPhotoBack() { ]; } - $model->deletePendingCivilIdFiles('back'); + $oldCivilIdCleanupComplete = $model->deletePendingCivilIdFiles('back'); return [ 'operation' => 'success', @@ -1331,6 +1331,7 @@ public function actionUpdateCivilPhotoBack() { "candidate_civil_id" => $model->candidate_civil_id, 'civilExpired' => $model->candidate_civil_expiry_date && (strtotime($model->candidate_civil_expiry_date) < strtotime(date('Y-m-d'))), + 'civil_photo_cleanup_pending' => !$oldCivilIdCleanupComplete, 'message' => Yii::t('candidate', 'Civil Photo Back Uploaded Successfully') ]; @@ -1380,7 +1381,7 @@ public function actionUpdateCivilPhotoFront() { ]; } - $model->deletePendingCivilIdFiles('front'); + $oldCivilIdCleanupComplete = $model->deletePendingCivilIdFiles('front'); return [ 'operation' => 'success', @@ -1391,6 +1392,7 @@ public function actionUpdateCivilPhotoFront() { "candidate_civil_id" => $model->candidate_civil_id, 'civilExpired' => $model->candidate_civil_expiry_date && (strtotime($model->candidate_civil_expiry_date) < strtotime(date('Y-m-d'))), + 'civil_photo_cleanup_pending' => !$oldCivilIdCleanupComplete, 'message' => Yii::t('candidate', 'Civil Photo Front Uploaded Successfully') ]; diff --git a/common/models/Candidate.php b/common/models/Candidate.php index 495f52e0..fe93623d 100644 --- a/common/models/Candidate.php +++ b/common/models/Candidate.php @@ -2726,6 +2726,20 @@ private function resolveStoredFileKey($type = 'resume', $side = 'front', $useOld } $directory = $type === 'civil-id' ? 'photos' : 'candidate-resume'; + $fileName = ltrim((string) $fileName, '/'); + + if (preg_match('/^https?:\/\//i', $fileName)) { + $parts = parse_url($fileName); + $fileName = isset($parts['path']) ? ltrim(rawurldecode($parts['path']), '/') : ''; + } + + if ($fileName === '') { + return null; + } + + if (strpos($fileName, '/') !== false) { + return $fileName; + } return $directory . '/' . $fileName; } @@ -2811,10 +2825,10 @@ public function deletePendingCivilIdFiles($side = null) unset($this->pendingCivilIdDeletionKeys[$pendingSide]); } catch (\Aws\S3\Exception\S3Exception $e) { $this->logS3FileFailure('delete', $entry['attribute'], $entry['file_key'], $e); - unset($this->pendingCivilIdDeletionKeys[$pendingSide]); + $deleted = false; } catch (\Exception $e) { $this->logS3FileFailure('delete', $entry['attribute'], $entry['file_key'], $e); - unset($this->pendingCivilIdDeletionKeys[$pendingSide]); + $deleted = false; } } diff --git a/common/tests/unit/models/CandidateS3BehaviorTest.php b/common/tests/unit/models/CandidateS3BehaviorTest.php index 00043478..cac0e36a 100644 --- a/common/tests/unit/models/CandidateS3BehaviorTest.php +++ b/common/tests/unit/models/CandidateS3BehaviorTest.php @@ -71,6 +71,16 @@ public function delete($name) $this->assertTrue($candidate->deleteFile('civil-id', 'front')); $this->assertSame(['photos/front.jpg'], $trackingManager->deleted); + $candidate->setOldAttributes([ + 'candidate_id' => 1, + 'candidate_civil_photo_front' => 'photos/existing-front.jpg', + ]); + $this->assertTrue($candidate->deleteFile('civil-id', 'front')); + $this->assertSame([ + 'photos/front.jpg', + 'photos/existing-front.jpg', + ], $trackingManager->deleted); + $failingManager = new class extends \yii\base\Component { public function delete($name) { @@ -79,6 +89,10 @@ public function delete($name) }; Yii::$app->set('resourceManager', $failingManager); + $candidate->setOldAttributes([ + 'candidate_id' => 1, + 'candidate_civil_photo_back' => 'back.jpg', + ]); $candidate->clearErrors(); $this->assertFalse($candidate->deleteFile('civil-id', 'back')); @@ -166,6 +180,92 @@ public function delete($name) ['fileExists', 'photos/verify-miss.jpg'], ], $resourceManager->operations); $this->assertArrayHasKey('candidate_civil_photo_front', $candidate->getErrors()); + + $candidate->candidate_civil_photo_front = 'new-front.jpg'; + $candidate->setOldAttributes([ + 'candidate_id' => 1, + 'candidate_civil_photo_front' => 'photos/persisted-old-front.jpg', + ]); + $candidate->clearErrors(); + + $resourceManager->operations = []; + $resourceManager->exists = true; + + $this->assertTrue($candidate->updateCivilId('front')); + $this->assertTrue($candidate->deletePendingCivilIdFiles('front')); + $this->assertSame([ + ['copy', 'new-front.jpg', 'photos/new-front.jpg', 'temp-upload-bucket'], + ['fileExists', 'photos/new-front.jpg'], + ['delete', 'photos/persisted-old-front.jpg'], + ], $resourceManager->operations); + } finally { + Yii::$app->set('resourceManager', $originalManager); + Yii::$app->set('temporaryBucketResourceManager', $originalTempManager); + } + } + + /** + * Verifies failed deferred Civil ID cleanup remains visible to callers. + */ + public function testPendingCivilIdDeletionReturnsFalseAndCanBeRetried() + { + $candidate = new CandidateS3BehaviorTestModel(); + $candidate->candidate_id = 1; + $candidate->candidate_civil_photo_front = 'new-front.jpg'; + $candidate->setOldAttributes([ + 'candidate_id' => 1, + 'candidate_civil_photo_front' => 'old-front.jpg', + ]); + + $originalManager = Yii::$app->get('resourceManager'); + $originalTempManager = Yii::$app->get('temporaryBucketResourceManager'); + + $resourceManager = new class extends \yii\base\Component { + public $operations = []; + public $failDelete = true; + + public function copy($oldFile, $newFile, $sourceBucket = "", $options = []) + { + $this->operations[] = ['copy', $oldFile, $newFile, $sourceBucket]; + return true; + } + + public function fileExists($name) + { + $this->operations[] = ['fileExists', $name]; + return true; + } + + public function delete($name) + { + $this->operations[] = ['delete', $name]; + if ($this->failDelete) { + throw new \Exception('delete failed'); + } + + return true; + } + }; + + $tempManager = new class extends \yii\base\Component { + public $bucket = 'temp-upload-bucket'; + }; + + try { + Yii::$app->set('resourceManager', $resourceManager); + Yii::$app->set('temporaryBucketResourceManager', $tempManager); + + $this->assertTrue($candidate->updateCivilId('front')); + $this->assertFalse($candidate->deletePendingCivilIdFiles('front')); + + $resourceManager->failDelete = false; + $this->assertTrue($candidate->deletePendingCivilIdFiles('front')); + $this->assertSame([ + ['copy', 'new-front.jpg', 'photos/new-front.jpg', 'temp-upload-bucket'], + ['fileExists', 'photos/new-front.jpg'], + ['delete', 'photos/old-front.jpg'], + ['delete', 'photos/old-front.jpg'], + ], $resourceManager->operations); } finally { Yii::$app->set('resourceManager', $originalManager); Yii::$app->set('temporaryBucketResourceManager', $originalTempManager);