From 1824537d976408cff40ca4715a894d82cddccaa3 Mon Sep 17 00:00:00 2001 From: Adam Cooper Date: Tue, 19 Oct 2021 16:15:18 +0100 Subject: [PATCH] Uml 1643 force activation key (pt4) (#1146) * Refactor retry of db write and UUID creation to be a part of the DB mapper service * Remove code that reappeared in merge. * Add lpa-actor token to the validateRequest method. * Add some error catching/correction * A letter request now updates an existing record if specified. * Pass the existing record token through if found. * Tidy up logging * Remove token from return if not discovered in resolution. * Improve test coverage * Update 'already-have-key' form to have the appropriate buttons. * Improve testing coverage of the record update * Update service-api/app/src/App/src/Service/Lpa/AddOlderLpa.php Co-authored-by: lukejolliffe <84718230+lukejolliffe@users.noreply.github.com> --- .../features/actor-add-an-older-lpa.feature | 7 + .../context/Acceptance/LpaContext.php | 136 ++++++++++++++++++ .../context/Integration/LpaContext.php | 68 ++++++++- .../Handler/OlderLpaConfirmationHandler.php | 3 +- .../src/App/src/Service/Lpa/AddOlderLpa.php | 9 +- .../App/src/Service/Lpa/OlderLpaService.php | 35 ++++- .../AppTest/Service/Lpa/AddOlderLpaTest.php | 1 + .../Service/Lpa/OlderLpaServiceTest.php | 27 +++- .../already-have-activation-key.html.twig | 4 +- .../Common/src/Service/Lpa/AddOlderLpa.php | 5 +- 10 files changed, 272 insertions(+), 23 deletions(-) diff --git a/service-api/app/features/actor-add-an-older-lpa.feature b/service-api/app/features/actor-add-an-older-lpa.feature index 2d99072888..5e28b6f04c 100644 --- a/service-api/app/features/actor-add-an-older-lpa.feature +++ b/service-api/app/features/actor-add-an-older-lpa.feature @@ -35,6 +35,13 @@ Feature: Add an older LPA And a letter is requested containing a one time use code And A record of my activation key request is saved + @integration @acceptance @pact @ff:save_older_lpa_requests:true + Scenario: The user can re-request an older LPA be added to their account and we update the record in our DB + Given I have previously requested the addition of a paper LPA to my account + When I repeat my request for an activation key + Then a repeat request for a letter containing a one time use code is made + And a record of my activation key request is updated + @integration @acceptance @pact Scenario: The user cannot add an older LPA to their account that does not exist Given I am on the add an older LPA page diff --git a/service-api/app/features/context/Acceptance/LpaContext.php b/service-api/app/features/context/Acceptance/LpaContext.php index de0473ddaf..88f021fec2 100644 --- a/service-api/app/features/context/Acceptance/LpaContext.php +++ b/service-api/app/features/context/Acceptance/LpaContext.php @@ -33,6 +33,14 @@ class LpaContext implements Context use BaseAcceptanceContextTrait; use SetupEnv; + /** + * @Given I have previously requested the addition of a paper LPA to my account + */ + public function iHavePreviouslyRequestedTheAdditionOfAPaperLPAToMyAccount() + { + // Not necessary for this context + } + /** * @Given /^A record of my activation key request is not saved$/ */ @@ -54,6 +62,25 @@ public function aRecordOfMyActivationKeyRequestIsSaved() assertArrayHasKey('ActivateBy', $lastCommand->toArray()['Item']); } + /** + * @Then /^a record of my activation key request is updated/ + */ + public function aRecordOfMyActivationKeyRequestIsUpdated() + { + $dt = (new DateTime('now'))->add(new \DateInterval('P1Y')); + + $lastCommand = $this->awsFixtures->getLastCommand(); + assertEquals($lastCommand->getName(), 'UpdateItem'); + assertEquals($lastCommand->toArray()['TableName'], 'user-actor-lpa-map'); + assertEquals($lastCommand->toArray()['Key']['Id'], ['S' => '111222333444']); + assertEquals( + intval($lastCommand->toArray()['ExpressionAttributeValues'][':a']['N']), + $dt->getTimestamp(), + '', + 5 + ); + } + /** * @Then /^A record of the LPA requested is saved to the database$/ */ @@ -2378,6 +2405,114 @@ public function iAmOnTheAddAnOlderLPAPage() // Not needed for this context } + /** + * @Then /^a repeat request for a letter containing a one time use code is made$/ + */ + public function aRepeatRequestForALetterContainingAOneTimeUseCodeIsMade() + { + //UserLpaActorMap: getByUserId + $this->awsFixtures->append( + new Result( + [ + 'Items' => [ + $this->marshalAwsResultData( + [ + 'Id' => $this->userLpaActorToken, + 'UserId' => $this->base->userAccountId, + 'SiriusUid' => $this->lpaUid, + 'ActorId' => $this->actorId, + 'Added' => (new DateTime())->format('Y-m-d\TH:i:s.u\Z'), + 'ActivateBy' => (new DateTime())->add(new \DateInterval('P1Y'))->getTimestamp() + ] + ), + ] + ] + ) + ); + + //UserLpaActorMap: get + $this->awsFixtures->append( + new Result( + [ + 'Item' => $this->marshalAwsResultData( + [ + 'Id' => $this->userLpaActorToken, + 'UserId' => $this->base->userAccountId, + 'SiriusUid' => $this->lpaUid, + 'ActorId' => $this->actorId, + 'Added' => (new DateTime())->format('Y-m-d\TH:i:s.u\Z'), + 'ActivateBy' => (new DateTime())->add(new \DateInterval('P1Y'))->getTimestamp() + ] + ), + ] + ) + ); + + // LpaRepository::get + $this->apiFixtures->get('/v1/use-an-lpa/lpas/' . $this->lpaUid) + ->respondWith( + new Response( + StatusCodeInterface::STATUS_OK, + [], + json_encode($this->lpa) + ) + ); + + // Done twice due to our codes interdependencies + // LpaRepository::get + $this->apiFixtures->get('/v1/use-an-lpa/lpas/' . $this->lpaUid) + ->respondWith( + new Response( + StatusCodeInterface::STATUS_OK, + [], + json_encode($this->lpa) + ) + ); + + // request a code to be generated and letter to be sent + $this->apiFixtures->post('/v1/use-an-lpa/lpas/requestCode') + ->respondWith( + new Response( + StatusCodeInterface::STATUS_NO_CONTENT, + [] + ) + ); + + $this->awsFixtures->append( + new Result( + [ + 'Item' => $this->marshalAwsResultData( + [ + 'Id' => $this->userLpaActorToken, + 'UserId' => $this->base->userAccountId, + 'SiriusUid' => $this->lpaUid, + 'ActorId' => $this->actorId, + 'Added' => (new DateTime())->format('Y-m-d\TH:i:s.u\Z') + ] + ), + ] + ) + ); + + // API call to request an activation key + $this->apiPatch( + '/v1/older-lpa/confirm', + [ + 'reference_number' => $this->lpaUid, + 'first_names' => $this->userFirstnames, + 'last_name' => $this->userSurname, + 'dob' => $this->userDob, + 'postcode' => $this->userPostCode, + 'force_activation_key' => true + ], + [ + 'user-token' => $this->base->userAccountId, + ] + ); + + $this->ui->assertSession()->statusCodeEquals(StatusCodeInterface::STATUS_NO_CONTENT); + } + /** * @Then /^a letter is requested containing a one time use code$/ */ @@ -2844,6 +2979,7 @@ public function iLostTheLetterContainingMyActivationKey() /** * @When /^I request for a new activation key again$/ + * @When /^I repeat my request for an activation key$/ */ public function iRequestForANewActivationKeyAgain() { diff --git a/service-api/app/features/context/Integration/LpaContext.php b/service-api/app/features/context/Integration/LpaContext.php index 2df1bb25e1..dd57f0e86b 100644 --- a/service-api/app/features/context/Integration/LpaContext.php +++ b/service-api/app/features/context/Integration/LpaContext.php @@ -64,6 +64,14 @@ class LpaContext extends BaseIntegrationContext private RemoveLpa $deleteLpa; private LpaService $lpaService; + /** + * @Given I have previously requested the addition of a paper LPA to my account + */ + public function iHavePreviouslyRequestedTheAdditionOfAPaperLPAToMyAccount() + { + // Not necessary for this context + } + /** * @Then /^a letter is requested containing a one time use code$/ */ @@ -94,6 +102,36 @@ public function aLetterIsRequestedContainingAOneTimeUseCode() } } + /** + * @Then /^a repeat request for a letter containing a one time use code is made$/ + */ + public function aRepeatRequestForALetterContainingAOneTimeUseCodeIsMade() + { + // Lpas::requestLetter + $this->pactPostInteraction( + $this->apiGatewayPactProvider, + '/v1/use-an-lpa/lpas/requestCode', + [ + 'case_uid' => (int)$this->lpaUid, + 'actor_uid' => (int)$this->actorLpaId, + ], + StatusCodeInterface::STATUS_NO_CONTENT + ); + + if ($this->container->get(FeatureEnabled::class)('save_older_lpa_requests')) { + // Update activation key request in the DB + $this->awsFixtures->append(new Result([])); + } + + $olderLpaService = $this->container->get(OlderLpaService::class); + + try { + $olderLpaService->requestAccessByLetter($this->lpaUid, $this->actorLpaId, $this->userId, '00-0-0-0-00'); + } catch (ApiException $exception) { + throw new Exception('Failed to request access code letter'); + } + } + /** * @Then /^A record of my activation key request is saved$/ */ @@ -106,6 +144,25 @@ public function aRecordOfMyActivationKeyRequestIsSaved() assertArrayHasKey('ActivateBy', $lastCommand->toArray()['Item']); } + /** + * @Then /^a record of my activation key request is updated/ + */ + public function aRecordOfMyActivationKeyRequestIsUpdated() + { + $dt = (new DateTime('now'))->add(new \DateInterval('P1Y')); + + $lastCommand = $this->awsFixtures->getLastCommand(); + assertEquals($lastCommand->getName(), 'UpdateItem'); + assertEquals($lastCommand->toArray()['TableName'], 'user-actor-lpa-map'); + assertEquals($lastCommand->toArray()['Key']['Id'], ['S' => '00-0-0-0-00']); + assertEquals( + intval($lastCommand->toArray()['ExpressionAttributeValues'][':a']['N']), + $dt->getTimestamp(), + '', + 5 + ); + } + /** * @Then /^A record of my activation key request is not saved$/ */ @@ -2288,6 +2345,7 @@ public function iShouldHaveAnOptionToRegenerateAnActivationKeyForTheOldLPAIWantT /** * @When /^I request for a new activation key again$/ + * @When /^I repeat my request for an activation key$/ */ public function iRequestForANewActivationKeyAgain() { @@ -2529,11 +2587,11 @@ public function iProvideDetailsDetailsOfAnLpaThatIsNotRegistered() $this->lpa->status = 'Pending'; $data = [ - 'reference_number' => $this->lpaUid, - 'dob' => $this->userDob, - 'postcode' => $this->userPostCode, - 'first_names' => $this->userFirstname, - 'last_name' => $this->userSurname, + 'reference_number' => $this->lpaUid, + 'dob' => $this->userDob, + 'postcode' => $this->userPostCode, + 'first_names' => $this->userFirstname, + 'last_name' => $this->userSurname, ]; //UserLpaActorMap: getAllForUser diff --git a/service-api/app/src/App/src/Handler/OlderLpaConfirmationHandler.php b/service-api/app/src/App/src/Handler/OlderLpaConfirmationHandler.php index d1011a6431..d16e9c4e4a 100644 --- a/service-api/app/src/App/src/Handler/OlderLpaConfirmationHandler.php +++ b/service-api/app/src/App/src/Handler/OlderLpaConfirmationHandler.php @@ -68,7 +68,8 @@ public function handle(ServerRequestInterface $request): ResponseInterface $this->olderLpaService->requestAccessByLetter( (string) $requestData['reference_number'], $lpaMatchResponse['actor']['uId'], - $userId + $userId, + $lpaMatchResponse['lpaActorToken'] ?? null ); return new EmptyResponse(); diff --git a/service-api/app/src/App/src/Service/Lpa/AddOlderLpa.php b/service-api/app/src/App/src/Service/Lpa/AddOlderLpa.php index fe044248fa..8e0409cc33 100644 --- a/service-api/app/src/App/src/Service/Lpa/AddOlderLpa.php +++ b/service-api/app/src/App/src/Service/Lpa/AddOlderLpa.php @@ -6,8 +6,8 @@ use App\Exception\BadRequestException; use App\Exception\NotFoundException; -use Psr\Log\LoggerInterface; use DateTime; +use Psr\Log\LoggerInterface; class AddOlderLpa { @@ -59,7 +59,8 @@ public function __construct( public function validateRequest(string $userId, array $matchData): array { // Check if it's been added to the users account already - if (null !== $lpaAddedData = ($this->lpaAlreadyAdded)($userId, (string) $matchData['reference_number'])) { + $lpaAddedData = ($this->lpaAlreadyAdded)($userId, (string) $matchData['reference_number']); + if ($lpaAddedData !== null) { if (!array_key_exists('notActivated', $lpaAddedData)) { $this->logger->notice( 'User {id} attempted to request a key for the LPA {uId} which already exists in their account', @@ -111,6 +112,10 @@ public function validateRequest(string $userId, array $matchData): array ]; } + if ($token = $lpaAddedData['lpaActorToken'] ?? null) { + $resolvedActor['lpaActorToken'] = $token; + } + $resolvedActor['caseSubtype'] = $lpaData['caseSubtype']; $resolvedActor['donor'] = [ 'uId' => $lpaData['donor']['uId'], diff --git a/service-api/app/src/App/src/Service/Lpa/OlderLpaService.php b/service-api/app/src/App/src/Service/Lpa/OlderLpaService.php index 51c4d5e2ab..3044158988 100644 --- a/service-api/app/src/App/src/Service/Lpa/OlderLpaService.php +++ b/service-api/app/src/App/src/Service/Lpa/OlderLpaService.php @@ -79,15 +79,26 @@ private function removeLpa(string $requestId) * address of the specified actor with a new one-time-use registration code. * This will allow them to add the LPA to their UaLPA account. * - * @param string $uid Sirius uId for an LPA - * @param string $actorUid uId of an actor on that LPA - * @param string $userId + * In order to preserve some semblance of atomicity to the required steps they're + * carried out in a reverse order, with the one item we cannot revert carried out + * last. + * + * @param string $uid Sirius uId for an LPA + * @param string $actorUid uId of an actor on that LPA + * @param string $userId The user ID of an actor in the ualpa database + * @param string|null $existingRecordId If an existing LPA record has been stored this is the ID */ - public function requestAccessByLetter(string $uid, string $actorUid, string $userId): void - { + public function requestAccessByLetter( + string $uid, + string $actorUid, + string $userId, + ?string $existingRecordId = null + ): void { $recordId = null; if (($this->featureEnabled)('save_older_lpa_requests')) { - $recordId = $this->userLpaActorMap->create($userId, $uid, $actorUid, 'P1Y'); + if ($existingRecordId === null) { + $recordId = $this->userLpaActorMap->create($userId, $uid, $actorUid, 'P1Y'); + } } $uidInt = (int)$uid; @@ -116,5 +127,17 @@ public function requestAccessByLetter(string $uid, string $actorUid, string $use } throw $apiException; } + + /** + * This is the exception to the method documentation. We cannot easily roll this alteration + * back so we'll do it last. The potential is that this operation could fail even though + * the API request worked. That being the case the users record will not have + * an up to date ActivateBy column. This isn't the end of the world. + */ + if (($this->featureEnabled)('save_older_lpa_requests')) { + if ($existingRecordId !== null) { + $this->userLpaActorMap->renewActivationPeriod($existingRecordId, 'P1Y'); + } + } } } diff --git a/service-api/app/test/AppTest/Service/Lpa/AddOlderLpaTest.php b/service-api/app/test/AppTest/Service/Lpa/AddOlderLpaTest.php index 5b1e4c71a0..11b2723b3a 100644 --- a/service-api/app/test/AppTest/Service/Lpa/AddOlderLpaTest.php +++ b/service-api/app/test/AppTest/Service/Lpa/AddOlderLpaTest.php @@ -218,6 +218,7 @@ public function older_lpa_lookup_successful_if_lpa_already_requested_but_force_f 'caseSubtype' => 'pfa', 'actor' => $this->lpaData['donor'], 'role' => 'donor', + 'lpaActorToken' => 'qwerty-54321', 'donor' => [ 'uId' => $this->lpaData['donor']['uId'], 'firstname' => 'Donor', diff --git a/service-api/app/test/AppTest/Service/Lpa/OlderLpaServiceTest.php b/service-api/app/test/AppTest/Service/Lpa/OlderLpaServiceTest.php index 8a5bd512dc..8ebc262e45 100644 --- a/service-api/app/test/AppTest/Service/Lpa/OlderLpaServiceTest.php +++ b/service-api/app/test/AppTest/Service/Lpa/OlderLpaServiceTest.php @@ -91,6 +91,25 @@ public function request_access_code_letter(): void $service->requestAccessByLetter($this->lpaUid, $this->actorUid, $this->userId); } + /** @test */ + public function request_access_code_letter_record_exists(): void + { + $this->lpasInterfaceProphecy + ->requestLetter((int) $this->lpaUid, (int) $this->actorUid) + ->shouldBeCalled(); + + $this->featureEnabledProphecy->__invoke('save_older_lpa_requests')->willReturn(true); + + $this->userLpaActorMapProphecy->create(Argument::cetera())->shouldNotBeCalled(); + + $this->userLpaActorMapProphecy + ->renewActivationPeriod('token-12345', 'P1Y') + ->shouldBeCalled(); + + $service = $this->getOlderLpaService(); + $service->requestAccessByLetter($this->lpaUid, $this->actorUid, $this->userId, 'token-12345'); + } + /** @test */ public function request_access_code_letter_without_flag(): void { @@ -100,12 +119,8 @@ public function request_access_code_letter_without_flag(): void $this->featureEnabledProphecy->__invoke('save_older_lpa_requests')->willReturn(false); - $this->userLpaActorMapProphecy->create( - $this->userId, - $this->lpaUid, - $this->actorUid, - 'P1Y' - )->shouldNotBeCalled(); + $this->userLpaActorMapProphecy->create(Argument::cetera())->shouldNotBeCalled(); + $this->userLpaActorMapProphecy->renewActivationPeriod(Argument::cetera())->shouldNotBeCalled(); $service = $this->getOlderLpaService(); $service->requestAccessByLetter($this->lpaUid, $this->actorUid, $this->userId); diff --git a/service-front/app/src/Actor/templates/actor/already-have-activation-key.html.twig b/service-front/app/src/Actor/templates/actor/already-have-activation-key.html.twig index 9b228cea5b..ebcd59accd 100644 --- a/service-front/app/src/Actor/templates/actor/already-have-activation-key.html.twig +++ b/service-front/app/src/Actor/templates/actor/already-have-activation-key.html.twig @@ -50,7 +50,9 @@ {{ govuk_form_element(form.get('__csrf')) }} {{ govuk_form_element(form.get('force_activation')) }} - diff --git a/service-front/app/src/Common/src/Service/Lpa/AddOlderLpa.php b/service-front/app/src/Common/src/Service/Lpa/AddOlderLpa.php index 0880bf0a05..851a37a08f 100644 --- a/service-front/app/src/Common/src/Service/Lpa/AddOlderLpa.php +++ b/service-front/app/src/Common/src/Service/Lpa/AddOlderLpa.php @@ -153,12 +153,13 @@ public function confirm( } throw $apiEx; } - $eventCode = ($forceActivationKey) ? EventCodes::OLDER_LPA_FORCE_ACTIVATION_KEY : EventCodes::OLDER_LPA_SUCCESS; $this->logger->notice( 'Successfully matched LPA {uId} and requested letter for account with Id {id} ', [ - 'event_code' => $eventCode, + 'event_code' => $forceActivationKey + ? EventCodes::OLDER_LPA_FORCE_ACTIVATION_KEY + : EventCodes::OLDER_LPA_SUCCESS, 'id' => $data['identity'], 'uId' => $data['reference_number'] ]