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

Remove personal information from log entries - Recruitment #849

Merged
merged 4 commits into from
Nov 22, 2019

Conversation

VasanthaKasirajan3008
Copy link
Contributor

No description provided.

Copy link
Contributor

@shomavg shomavg left a 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!

Copy link
Contributor

@DevenKShah DevenKShah left a 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}");
Copy link
Contributor

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);
Copy link
Contributor

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?

Copy link
Contributor

@DevenKShah DevenKShah left a comment

Choose a reason for hiding this comment

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

Looks good.

@shomavg shomavg merged commit a900069 into master Nov 22, 2019
@shomavg shomavg deleted the CON-946-RemovePersonalInformation branch November 22, 2019 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants