-
Notifications
You must be signed in to change notification settings - Fork 4
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
Remove personal information from log entries - Recruitment #849
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Congrats on your first commit!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These is nothing wrong with the changes you made, I have questions on the intention of the changes that are blanketly being applied. We need to have a discussion with Chris perhaps, to understand this better.
@@ -74,7 +74,7 @@ public async Task Run(CommunicationMessageIdentifier commMsgId) | |||
|
|||
public async Task SendEmail(CommunicationMessage request) | |||
{ | |||
_logger.LogInformation($"Trying to send message of type {request.RequestType} to {request.Recipient.Email}"); | |||
_logger.LogInformation($"Trying to send message of type {request.RequestType}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually kills the purpose of the log. Instead can we trim the domain from the email so that this log entry makes any sense. You can create an extension method to trim everything after @
symbol in the email.
@@ -50,7 +50,7 @@ public async Task Handle(GeocodeVacancyCommand message, CancellationToken cancel | |||
|
|||
private Task<Geocode> GeocodePostcodeAsync(Vacancy vacancy) | |||
{ | |||
_logger.LogInformation("Attempting to geocode postcode:'{postcode}' for vacancyId:'{vacancyId}'", vacancy.EmployerLocation.Postcode, vacancy.Id); | |||
_logger.LogInformation("Attempting to geocode for vacancyId:'{vacancyId}'", vacancy.Id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we are printing postcode here... but it has no association with any individual... so postcode in itself shouldn't be an issue?
… back Postcode changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
No description provided.