Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DDLS-360 Add new clients found in the LayDeputy csv #1771

Merged
merged 9 commits into from
Dec 19, 2024
2 changes: 2 additions & 0 deletions api/app/src/Command/ProcessLayCSVCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,8 @@ private function process(mixed $data): bool
$result = $this->csvProcessing->layProcessing($chunk, $index);
$this->storeOutput($result);
}
$this->logger->notice('Directly creating any new Lay clients for active deputies');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't appear in logs unless you use our verboseLogger (private LoggerInterface $verboseLogger) as normal logger is set to warning

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the same thing, I think, apart from this class happens to use a different variable name? I can change the variable names from $logger to $verboseLogger to make it clearer

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe Symfony uses the variable name to determine which logger to actually inject into the service. So if this message needs logging, I would suggest changing it to warning.
Changing the logger to the verbose logger will require reviewing the rest of the logging in this file to make sure it is appropriate for logging (i.e. no personally identifiable information)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it uses the variable name and there's a setup for it where we call it verbose logger. If you search for verboseLogger in the repo you should see other setups for it. I think it should be fine to replace everything in the file with it as it's just logging errors elsewhere and I don't think the verbose logger actually logs more stuff, it's just the log level that changes.. It's a bit confusing, we did it this way so we didn't have to review all our notices across the app but in the long run it would probably have been easier to set base logger to notice and review everything... oh well.

$this->csvProcessing->layProcessingHandleNewMultiClients();

return true;
}
Expand Down
2 changes: 1 addition & 1 deletion api/app/src/Entity/Client.php
Original file line number Diff line number Diff line change
Expand Up @@ -725,7 +725,7 @@ public function setAddress5(?string $address5): Client
return $this;
}

public function getAddress5(): string
public function getAddress5(): ?string
{
return $this->address5;
}
Expand Down
44 changes: 44 additions & 0 deletions api/app/src/Repository/PreRegistrationRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -60,4 +60,48 @@ public function findByCaseNumber(string $caseNumber)
->getQuery()
->getResult();
}

public function getNewClientsForExistingDeputiesArray(): array
{
$conn = $this->getEntityManager()->getConnection();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we would need to disable the softdeletable filter here before running the query.
The reason being is that when we probably should include the discharged clients in the check so that if they appear in the CSV for this particular deputy, they aren't recreated again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that we're going for strings as that's whats on sirius for deputy uid I'd do the cast the other way round and cast the bigint (that we're ditching eventually) as a string (or varchar or whatever it's called in database language) instead

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it just a case of removing the softdeletable filter Gugs on line 103? I think the SQL should bring back all the discharged ones fine but then the filter gets rid of them right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you have to disable it before the query, run the query (it will now ignore the filter), and then enable it again afterwards so it doesn't affect any other queries afterwards.

Thinking about it, it's not the greatest implementation because if you've got a query that is going to run for a long time then the filter is disabled for a long time too. Which means there's a higher chance some other query in the app is going to be running and that might be expecting the filter to be enabled.


$newMultiClentsQuery = <<<SQL
SELECT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this query locally, and it does return clients that are already created.
However this isn't an immediate issue as the logic on the LayDeputyshipUploader will only create the client if they don't already exist. Just wanted to comment for awareness if there are any issues with this in the future

pr.client_case_number AS "Case",
pr.client_lastname AS "ClientSurname",
pr.deputy_uid AS "DeputyUid",
pr.deputy_lastname AS "DeputySurname",
pr.deputy_address_1 AS "DeputyAddress1",
pr.deputy_address_2 AS "DeputyAddress2",
pr.deputy_address_3 AS "DeputyAddress3",
pr.deputy_address_4 AS "DeputyAddress4",
pr.deputy_address_5 AS "DeputyAddress5",
pr.deputy_postcode AS "DeputyPostcode",
pr.type_of_report AS "ReportType",
pr.order_date AS "MadeDate",
pr.order_type AS "OrderType",
CASE WHEN pr.is_co_deputy THEN 'yes' ELSE 'no' END AS "CoDeputy",
pr.created_at AS "MadeDate",
pr.hybrid AS "Hybrid",
pr.deputy_firstname AS "DeputyFirstname",
pr.client_firstname AS "ClientFirstname",
pr.client_address_1 AS "ClientAddress1",
pr.client_address_2 AS "ClientAddress2",
pr.client_address_3 AS "ClientAddress3",
pr.client_address_4 AS "ClientAddress4",
pr.client_address_5 AS "ClientAddress5",
pr.client_postcode AS "ClientPostcode"
FROM pre_registration pr
LEFT JOIN dd_user u ON pr.deputy_uid::bigint = u.deputy_uid
LEFT JOIN deputy_case dc ON u.id = dc.user_id
LEFT JOIN client c ON dc.client_id = c.id
WHERE c.case_number != pr.client_case_number
SQL;

$stmt = $conn->executeQuery($newMultiClentsQuery);
$result = $stmt->fetchAllAssociative();
$this->_em->getFilters()->enable('softdeleteable');

return $result;
}
}
19 changes: 19 additions & 0 deletions api/app/src/Repository/UserRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -419,4 +419,23 @@ public function findDeputyUidsForClient(int $clientId)

return $query->getArrayResult();
}

public function findActiveClientsCountForDeputyUid(string $deputyUid): int
{
$query = $this
->getEntityManager()
->createQuery("SELECT COUNT(u) FROM App\Entity\User u INNER JOIN u.clients c where c.archivedAt IS NULL and c.deletedAt IS NULL AND u.deputyUid = :deputyUid")
->setParameter('deputyUid', $deputyUid);

return $query->getSingleScalarResult();
}

public function findPrimaryUserByDeputyUid(string $deputyUid): ?User
{
$query = $this
->getEntityManager()
->createQuery("SELECT u FROM App\Entity\User u WHERE u.deputyUid = :deputyUid AND u.isPrimary = True")
->setParameter('deputyUid', $deputyUid);
return $query->getSingleResult();
}
}
23 changes: 23 additions & 0 deletions api/app/src/v2/Assembler/ClientAssembler.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
use App\v2\DTO\DtoPropertySetterTrait;
use App\v2\DTO\OrganisationDto;
use App\v2\DTO\UserDto;
use App\v2\Registration\DTO\LayDeputyshipDto;
use App\v2\Registration\DTO\OrgDeputyshipDto;

class ClientAssembler
Expand Down Expand Up @@ -131,6 +132,28 @@ public function assembleFromOrgDeputyshipDto(OrgDeputyshipDto $dto)

return $client;
}

public function assembleFromLayDeputyshipDto(LayDeputyshipDto $dto)
{
$client = (new Client())
->setCaseNumber($dto->getCaseNumber())
->setFirstname($dto->getClientFirstname() ?: null)
->setLastname($dto->getClientSurname())
->setAddress($dto->getClientAddress1() ?: null)
->setAddress2($dto->getClientAddress2() ?: null)
->setAddress3($dto->getClientAddress3() ?: null)
->setAddress4($dto->getClientAddress4() ?: null)
->setAddress5($dto->getClientAddress5() ?: null)
->setPostcode($dto->getClientPostcode() ?: null)
->setCourtDate($dto->getOrderDate() ?: null);

if (!empty($dto->getClientPostCode())) {
$client->setPostcode($dto->getClientPostCode());
$client->setCountry('GB'); // postcode given means a UK address is given
}

return $client;
}

private function assembleClientDeputies(array $deputies)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,12 @@ public function layProcessing(array $data, ?int $chunkId)
return $result;
}

public function layProcessingHandleNewMultiClients(): array
{
$result = $this->layUploader->handleNewMultiClients();
return $result;
}

public function orgProcessing(array $data)
{
$rowCount = count($data);
Expand Down
74 changes: 73 additions & 1 deletion api/app/src/v2/Registration/Uploader/LayDeputyshipUploader.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,21 @@

namespace App\v2\Registration\Uploader;

use App\Entity\Client;
use App\Entity\PreRegistration;
use App\Entity\Report\Report;
use App\Entity\User;
use App\Repository\ReportRepository;
use App\v2\Assembler\ClientAssembler;
use App\v2\Registration\Assembler\SiriusToLayDeputyshipDtoAssembler;
use App\v2\Registration\DTO\LayDeputyshipDto;
use App\v2\Registration\DTO\LayDeputyshipDtoCollection;
use App\v2\Registration\SelfRegistration\Factory\PreRegistrationCreationException;
use App\v2\Registration\SelfRegistration\Factory\PreRegistrationFactory;
use Doctrine\Common\Persistence\Mapping\MappingException;
use Doctrine\ORM\EntityManagerInterface;
use Doctrine\ORM\NonUniqueResultException;
use Doctrine\ORM\NoResultException;
use Doctrine\ORM\OptimisticLockException;
use Doctrine\ORM\ORMException;
use Psr\Log\LoggerInterface;
Expand All @@ -34,7 +39,9 @@ public function __construct(
private EntityManagerInterface $em,
private ReportRepository $reportRepository,
private PreRegistrationFactory $preRegistrationFactory,
private LoggerInterface $logger
private LoggerInterface $logger,
private SiriusToLayDeputyshipDtoAssembler $assembler,
private ClientAssembler $clientAssembler
) {
}

Expand Down Expand Up @@ -80,6 +87,71 @@ public function upload(LayDeputyshipDtoCollection $collection): array
'source' => 'sirius',
];
}

public function handleNewMultiClients(): array
{
$preRegistrationNewClients = $this->em->getRepository(PreRegistration::class)->getNewClientsForExistingDeputiesArray();
$clientsAdded = 0;
$errors = [];

foreach ($preRegistrationNewClients AS $preReg){
$layDeputyshipDto = $this->assembler->assembleFromArray($preReg);
try {
$this->em->beginTransaction();
$user = $this->handleNewUser($layDeputyshipDto);
$client = $this->handleNewClient($layDeputyshipDto, $user);
// create report and associate with client
$this->commitTransactionToDatabase();
++$clientsAdded;
} catch (\Throwable $e) {
$message = sprintf('Error when creating additional client for deputyUID %s for case %s: %s',
$layDeputyshipDto->getDeputyUid(),
$layDeputyshipDto->getCaseNumber(),
str_replace(PHP_EOL, '', $e->getMessage())
);
$this->logger->warning($message);
$errors[] = $message;
continue;
}
}

return [
'new-clients-found' => count($preRegistrationNewClients),
'clients-added' => $clientsAdded,
'errors' => $errors
];
}

private function handleNewUser(LayDeputyshipDto $dto): ?User
{
try {
$primaryDeputyUser = $this->em->getRepository(User::class)->findPrimaryUserByDeputyUid($dto->getDeputyUid());
} catch (NoResultException|NonUniqueResultException $e) {
throw new \RuntimeException(sprintf('The primary user for deputy UID %s was either missing or not unique', $dto->getDeputyUid()));
}

$users = $this->em->getRepository(User::class)->findBy(['deputyUid' => $dto->getDeputyUid()]);
$newSecondaryUser = clone $primaryDeputyUser;
$newSecondaryUser->setEmail('secondary-'.count($users).'-'.$primaryDeputyUser->getEmail());
$newSecondaryUser->setIsPrimary(false);
$this->em->persist($newSecondaryUser);
return $newSecondaryUser;
}

private function handleNewClient(LayDeputyshipDto $dto, User $newUser): ?Client
{
$existingClient = $this->em->getRepository(Client::class)->findByCaseNumber($dto->getCaseNumber());

if ($existingClient instanceof Client){
throw new \RuntimeException('client already exists');
} else {
$newClient = $this->clientAssembler->assembleFromLayDeputyshipDto($dto);
$newClient->addUser($newUser);
$this->em->persist($newClient);
$this->added['clients'][] = $dto->getCaseNumber();
}
return $existingClient;
}

private function throwExceptionIfDataTooLarge(LayDeputyshipDtoCollection $collection): void
{
Expand Down
45 changes: 45 additions & 0 deletions api/app/tests/Behat/bootstrap/v2/Registration/IngestTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use App\Entity\Organisation;
use App\Entity\PreRegistration;
use App\Entity\Report\Report;
use App\Entity\User;
use App\Tests\Behat\BehatException;
use Behat\Gherkin\Node\TableNode;
use Symfony\Component\Console\Input\ArrayInput;
Expand Down Expand Up @@ -1107,6 +1108,50 @@ public function iRunTheLayCsvCommandTheFileContainsNewPreRegistrationEntitesForT
$this->uploadCsvAndCountCreatedEntities($this->csvFileName);
}

/**
* @Given the Lay deputy with deputy UID :deputyUid has :deputyClientCount associated active clients
*/
public function theLayDeputyWithDeputyUidHasAssociatedClient($deputyUid, $deputyClientCount): void
{
$clientCount = $this->em
->getRepository(User::class)
->findActiveClientsCountForDeputyUid($deputyUid);

if ($clientCount != $deputyClientCount) {
throw new BehatException(sprintf("Unexpected number of active clients associated with this Deputy UID. Expected: '%s', got '%s'", $deputyClientCount, $clientCount));
}
}

/**
* @Then the client with case number :caseNumber should have the address :fullAddress
*/
public function clientWithCaseNumberShouldHaveAddress(string $caseNumber, string $fullAddress): void
{
$client = $this->em
->getRepository(Client::class)
->findOneBy(['caseNumber' => $caseNumber]);

if (is_null($client)) {
throw new BehatException(sprintf('Client not found with case number "%s"', $caseNumber));
}

$actualClientAddress = sprintf(
'%s, %s, %s, %s, %s, %s',
$client->getAddress() ?? '',
$client->getAddress2() ?? '',
$client->getAddress3() ?? '',
$client->getAddress4() ?? '',
$client->getAddress5() ?? '',
$client->getPostcode() ?? ''
);

$this->assertStringEqualsString(
$fullAddress,
$actualClientAddress,
'Comparing address defined in step against actual client address'
);
}

protected function runCSVCommand(string $type, string $fileName)
{
$command = ('lay' === $type) ?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,22 @@ Feature: Lay CSV data ingestion - sirius source data
And the count of the new 'lay' entities added should be in the command output

@super-admin
Scenario: Uploading a Lay CSV that contains contains a row with an invalid report type
Scenario: Uploading a Lay CSV that contains a row with an invalid report type
Given a csv has been uploaded to the sirius bucket with the file 'lay-1-row-invalid-report-type-1-valid-row.csv'
When I run the lay CSV command the file has 1 row with an invalid report type and 1 valid row
Then the new 'lay' entities should be added to the database
And the count of the new 'lay' entities added should be in the command output

@super-admin
Scenario: Uploading a Lay CSV that contains details of a new deputyship for an existing Lay deputy with a single active client
Given the Lay deputy with deputy UID 700761111001 has 1 associated active clients
And a csv has been uploaded to the sirius bucket with the file 'lay-2-rows-deputy-has-multiple-client-deputyships.csv'
When I run the lay CSV command the file contains 2 new pre-registration entities
And the Lay deputy with deputy UID 700761111001 has 2 associated active clients
And the client with case number '12345673' should have the address '64 zoo lane, vrombaut, beebies, london, , cl1 3nt'
When I run the lay CSV command the file contains 2 new pre-registration entities
And the Lay deputy with deputy UID 700761111001 has 2 associated active clients

# Needs further rewrite so we're gracefully handling missing columns & not just stopping the process.
# Currently throws critical error
# @super-admin
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Case,ClientFirstname,ClientSurname,ClientAddress1,ClientAddress2,ClientAddress3,ClientAddress4,ClientAddress5,ClientPostcode,DeputyUid,DeputyFirstname,DeputySurname,DeputyAddress1,DeputyAddress2,DeputyAddress3,DeputyAddress4,DeputyAddress5,DeputyPostcode,ReportType,MadeDate,OrderType,CoDeputy,Hybrid,CourtOrderUid
61111001,Client 1,Clientsurname,Client Road,,,,,CL1 3NT,700761111001,Lay-OPG102-User-1,User,ABC Road,,,,,AB1 2CD,OPG102,2011-04-14,pfa,no,SINGLE,45123468745
12345673,Client 2,SecondClient,64 Zoo Lane,Vrombaut,Beebies,London,,CL1 3NT,700761111001,Julie,DUCK,ABC Road,,,,,D3P UTY,OPG102,2011-04-15,pfa,no,SINGLE,34518742346
8 changes: 8 additions & 0 deletions api/app/tests/Unit/Command/ProcessLayCSVCommandTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use Psr\Log\LoggerInterface;
use Symfony\Bundle\FrameworkBundle\Console\Application;
use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;
use Symfony\Component\Console\Output\ConsoleOutput;
use Symfony\Component\Console\Tester\CommandTester;
use App\Command\ProcessLayCSVCommand;
use Symfony\Component\DependencyInjection\ParameterBag\ParameterBagInterface;
Expand Down Expand Up @@ -75,6 +76,13 @@ public function testExecuteWithSuccessfulFilePull(): void
'source' => 'sirius',
]);

$this->csvProcessing->layProcessingHandleNewMultiClients()
->shouldBeCalled()
->willReturn([
'added' => 0,
'errors' => []
]);

$this->commandTester->execute(['csv-filename' => $this->csvFilename]);
$this->commandTester->assertCommandIsSuccessful();
$output = $this->commandTester->getDisplay();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@
use Doctrine\ORM\EntityManager;
use PHPUnit\Framework\TestCase;
use Psr\Log\LoggerInterface;
use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;

class LayDeputyshipUploaderTest extends TestCase
class LayDeputyshipUploaderTest extends KernelTestCase
{
/** @var EntityManager|\PHPUnit_Framework_MockObject_MockObject */
protected $em;
Expand All @@ -35,16 +36,21 @@ class LayDeputyshipUploaderTest extends TestCase

protected function setUp(): void
{
$container = static::getContainer();
$this->em = $this->getMockBuilder(EntityManager::class)->disableOriginalConstructor()->getMock();
$this->reportRepository = $this->getMockBuilder(ReportRepository::class)->disableOriginalConstructor()->getMock();
$this->factory = $this->getMockBuilder(PreRegistrationFactory::class)->disableOriginalConstructor()->enableArgumentCloning()->getMock();
$this->logger = $this->getMockBuilder(LoggerInterface::class)->disableOriginalConstructor()->getMock();
$layDeputyshipDtoAssembler = $container->get('App\v2\Registration\Assembler\SiriusToLayDeputyshipDtoAssembler');
$clientAssembler = $container->get('App\v2\Assembler\ClientAssembler');

$this->sut = new LayDeputyshipUploader(
$this->em,
$this->reportRepository,
$this->factory,
$this->logger
$this->logger,
$layDeputyshipDtoAssembler,
$clientAssembler
);
}

Expand Down
Loading