DDLS-360 Add new clients found in the LayDeputy csv #1771
Conversation
Clients that are associated with existing deputies are now created automatically as part of the daily ingest
| @@ -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'); | |||
There was a problem hiding this comment.
This won't appear in logs unless you use our verboseLogger (private LoggerInterface $verboseLogger) as normal logger is set to warning
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
|
|
||
| public function getNewClientsForExistingDeputiesArray(): array | ||
| { | ||
| $conn = $this->getEntityManager()->getConnection(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| $conn = $this->getEntityManager()->getConnection(); | ||
|
|
||
| $newMultiClentsQuery = <<<SQL | ||
| SELECT |
There was a problem hiding this comment.
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
…ents are included in the lookup, updated fixture csv to use a unique deputy uid
… returning wrong client var, added additional step to behat test to check report type
…ded additional logic to check for discharged clients and to not create clients for the same deputy uid
Clients that are associated with existing deputies are now created automatically as part of the daily ingest
Purpose
Briefly describe the purpose of the change, and/or link to the JIRA ticket for context
Fixes DDLS-360
Approach
Explain how your code addresses the purpose of the change
Learning
Any tips and tricks, blog posts or tools which helped you. Plus anything notable you've discovered about DigiDeps
Checklist
Frontend