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.

Sign up for GitHub

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

CON-5253-lastname-blank added logging and Unknown to the Names #1518

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+107 −8
Open

CON-5253-lastname-blank added logging and Unknown to the Names #1518

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/EmployerFinance/SFA.DAS.EmployerFinance.sln
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@

Microsoft Visual Studio Solution File, Format Version 12.00
# Visual Studio Version 16
VisualStudioVersion = 16.0.31424.327
# Visual Studio Version 17
VisualStudioVersion = 17.9.34902.65
MinimumVisualStudioVersion = 10.0.40219.1
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "SFA.DAS.SharedOuterApi", "..\Shared\SFA.DAS.SharedOuterApi\SFA.DAS.SharedOuterApi.csproj", "{988FB7FC-2B13-4611-AB77-A1B16C964505}"
EndProject
Expand All @@ -13,6 +13,8 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "SFA.DAS.EmployerFinance.Api
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "SFA.DAS.EmployerFinance.UnitTests", "SFA.DAS.EmployerFinance.UnitTests\SFA.DAS.EmployerFinance.UnitTests.csproj", "{EF8844A9-7309-4E8B-9306-8B4A805B5C7B}"
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "SFA.DAS.SharedOuterApi.UnitTests", "..\Shared\SFA.DAS.SharedOuterApi.UnitTests\SFA.DAS.SharedOuterApi.UnitTests.csproj", "{9F5BF86B-5F9F-42FB-A2E2-7E5AF73618F7}"
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Debug|Any CPU = Debug|Any CPU
Expand All @@ -39,6 +41,10 @@ Global
{EF8844A9-7309-4E8B-9306-8B4A805B5C7B}.Debug|Any CPU.Build.0 = Debug|Any CPU
{EF8844A9-7309-4E8B-9306-8B4A805B5C7B}.Release|Any CPU.ActiveCfg = Release|Any CPU
{EF8844A9-7309-4E8B-9306-8B4A805B5C7B}.Release|Any CPU.Build.0 = Release|Any CPU
{9F5BF86B-5F9F-42FB-A2E2-7E5AF73618F7}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{9F5BF86B-5F9F-42FB-A2E2-7E5AF73618F7}.Debug|Any CPU.Build.0 = Debug|Any CPU
{9F5BF86B-5F9F-42FB-A2E2-7E5AF73618F7}.Release|Any CPU.ActiveCfg = Release|Any CPU
{9F5BF86B-5F9F-42FB-A2E2-7E5AF73618F7}.Release|Any CPU.Build.0 = Release|Any CPU
EndGlobalSection
GlobalSection(SolutionProperties) = preSolution
HideSolutionNode = FALSE
Expand Down
83 changes: 81 additions & 2 deletions ...erApi.UnitTests/Services/EmployerAccountsServiceTests/WhenGettingEmployerAccountsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ public async Task Then_If_The_Id_Is_Not_A_Guid_Then_User_Account_IsNot_Found_The
actual.TrueForAll(c => c.DisplayName.Equals(profileUserResponse.DisplayName)).Should().BeTrue();
actual.TrueForAll(c => c.IsSuspended.Equals(profileUserResponse.IsSuspended)).Should().BeTrue();
}

[Test, MoqAutoData]
public async Task Then_If_The_Id_Is_Not_A_Guid_And_A_Existing_User_With_Different_Email_Then_User_Upserted_And_User_Information_Returned(
Guid userId,
Expand Down Expand Up @@ -189,9 +190,87 @@ public async Task Then_If_The_Id_Is_Not_A_Guid_And_A_New_User_With_No_Accounts_T
actualRecord.DasAccountName.Should().BeNullOrEmpty();
actualRecord.EncodedAccountId.Should().BeNullOrEmpty();
actualRecord.Role.Should().BeNullOrEmpty();

}


[Test, MoqAutoData]
public async Task Then_If_The_Id_Is_A_Guid_And_A_No_User_Name_With_No_Accounts_Then_User_Upserted_And_User_Information_Returned(
EmployerProfile employerProfile,
EmployerProfileUsersApiResponse profileUserResponse,
[Frozen] Mock<IEmployerProfilesApiClient<EmployerProfilesApiConfiguration>> employerProfilesApiClient,
[Frozen] Mock<IAccountsApiClient<AccountsConfiguration>> accountsApiClient,
EmployerAccountsService handler)
{
employerProfile.UserId = Guid.NewGuid().ToString();

accountsApiClient
.Setup(x => x.GetAll<GetUserAccountsResponse>(
It.Is<GetUserAccountsRequest>(c => c.GetAllUrl.Contains($"user/{profileUserResponse.Id}/accounts"))))
.ReturnsAsync(new List<GetUserAccountsResponse>());
accountsApiClient
.Setup(x => x.GetAll<GetAccountTeamMembersResponse>(
It.IsAny<GetAccountTeamMembersRequest>()))
.ReturnsAsync(new List<GetAccountTeamMembersResponse>());
employerProfilesApiClient.Setup(x => x.GetWithResponseCode<EmployerProfileUsersApiResponse>(
It.Is<GetEmployerUserAccountRequest>(c =>
c.GetUrl.Contains($"api/users/{HttpUtility.UrlEncode(employerProfile.UserId)}"))))
.ReturnsAsync(new ApiResponse<EmployerProfileUsersApiResponse>(null, HttpStatusCode.NotFound, "Not Found"));

var actual = (await handler.GetEmployerAccounts(employerProfile)).ToList();

actual.Count.Should().Be(1);
var actualRecord = actual.First();
actualRecord.UserId.Should().Be(employerProfile.UserId);
actualRecord.FirstName.Should().Be("Unknown");
actualRecord.LastName.Should().Be("Unknown");
actualRecord.IsSuspended.Should().BeFalse();
actualRecord.DasAccountName.Should().BeNullOrEmpty();
actualRecord.EncodedAccountId.Should().BeNullOrEmpty();
actualRecord.Role.Should().BeNullOrEmpty();
}

[Test, MoqAutoData]
public async Task Then_If_The_Id_Is_Not_A_Guid_And_A_No_User_Name_With_No_Accounts_Then_User_Upserted_And_User_Information_Returned(
EmployerProfile employerProfile,
EmployerProfileUsersApiResponse profileUserResponse,
[Frozen] Mock<IEmployerProfilesApiClient<EmployerProfilesApiConfiguration>> employerProfilesApiClient,
[Frozen] Mock<IAccountsApiClient<AccountsConfiguration>> accountsApiClient,
EmployerAccountsService handler)
{
employerProfile.LastName = null;
employerProfile.FirstName = null;
profileUserResponse.LastName = null;
profileUserResponse.FirstName = null;

accountsApiClient
.Setup(x => x.GetAll<GetUserAccountsResponse>(
It.Is<GetUserAccountsRequest>(c => c.GetAllUrl.Contains($"user/{profileUserResponse.Id}/accounts"))))
.ReturnsAsync(new List<GetUserAccountsResponse>());
accountsApiClient
.Setup(x => x.GetAll<GetAccountTeamMembersResponse>(
It.IsAny<GetAccountTeamMembersRequest>()))
.ReturnsAsync(new List<GetAccountTeamMembersResponse>());
employerProfilesApiClient.Setup(x => x.GetWithResponseCode<EmployerProfileUsersApiResponse>(
It.Is<GetEmployerUserAccountRequest>(c =>
c.GetUrl.Contains($"api/users/{HttpUtility.UrlEncode(employerProfile.UserId)}"))))
.ReturnsAsync(new ApiResponse<EmployerProfileUsersApiResponse>(null, HttpStatusCode.NotFound, "Not Found"));
employerProfilesApiClient.Setup(x => x.PutWithResponseCode<EmployerProfileUsersApiResponse>(
It.Is<PutUpsertEmployerUserAccountRequest>(c =>
c.PutUrl.Contains($"api/users/"))))
.ReturnsAsync(new ApiResponse<EmployerProfileUsersApiResponse>(profileUserResponse, HttpStatusCode.Created, ""));

var actual = (await handler.GetEmployerAccounts(employerProfile)).ToList();

actual.Count.Should().Be(1);
var actualRecord = actual.First();
actualRecord.UserId.Should().Be(profileUserResponse.Id);
actualRecord.FirstName.Should().Be("Unknown");
actualRecord.LastName.Should().Be("Unknown");
actualRecord.IsSuspended.Should().Be(profileUserResponse.IsSuspended);
actualRecord.DasAccountName.Should().BeNullOrEmpty();
actualRecord.EncodedAccountId.Should().BeNullOrEmpty();
actualRecord.Role.Should().BeNullOrEmpty();
}

[Test, MoqAutoData]
public async Task Then_If_The_Id_Is_A_Guid_Then_User_Account_Found_And_Not_Upserted(
EmployerProfile employerProfile,
Expand Down
22 changes: 18 additions & 4 deletions src/Shared/SFA.DAS.SharedOuterApi/Services/EmployerAccountsService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
using System.Linq;
using System.Net;
using System.Threading.Tasks;
using Microsoft.Extensions.Logging;
using SFA.DAS.SharedOuterApi.Configuration;
using SFA.DAS.SharedOuterApi.Infrastructure;
using SFA.DAS.SharedOuterApi.InnerApi.Requests;
Expand All @@ -22,11 +23,13 @@ public class EmployerAccountsService : IEmployerAccountsService
{
private readonly IEmployerProfilesApiClient<EmployerProfilesApiConfiguration> _employerProfilesApiClient;
private readonly IAccountsApiClient<AccountsConfiguration> _accountsApiClient;
private readonly ILogger<EmployerAccountsService> _logger;

public EmployerAccountsService(IEmployerProfilesApiClient<EmployerProfilesApiConfiguration> employerProfilesApiClient, IAccountsApiClient<AccountsConfiguration> accountsApiClient)
public EmployerAccountsService(IEmployerProfilesApiClient<EmployerProfilesApiConfiguration> employerProfilesApiClient, IAccountsApiClient<AccountsConfiguration> accountsApiClient, ILogger<EmployerAccountsService> logger)
{
_employerProfilesApiClient = employerProfilesApiClient;
_accountsApiClient = accountsApiClient;
_logger = logger;
}

public async Task<IEnumerable<EmployerAccountUser>> GetEmployerAccounts(EmployerProfile employerProfile)
Expand All @@ -41,7 +44,6 @@ public async Task<IEnumerable<EmployerAccountUser>> GetEmployerAccounts(Employer
await _employerProfilesApiClient.GetWithResponseCode<EmployerProfileUsersApiResponse>(
new GetEmployerUserAccountRequest(employerProfile.UserId));


if (userResponse.StatusCode == HttpStatusCode.NotFound)
{
if (!Guid.TryParse(employerProfile.UserId, out _))
Expand All @@ -56,6 +58,18 @@ await _employerProfilesApiClient.PutWithResponseCode<EmployerProfileUsersApiResp
lastName = employerUserResponse.Body.LastName;
displayName = employerUserResponse.Body.DisplayName;
isSuspended = employerUserResponse.Body.IsSuspended;

// TODO Review this
// It is possible that the name is missing when this function is called,
// this logging should help identify this
_logger.LogInformation("UserId {0} added to Employer Profile: Is Lastname blank {1}",
employerProfile.UserId, string.IsNullOrWhiteSpace(lastName));
}
else
{
// TODO Review this
// It may be called when the if user id is a valid guid name will be blank
_logger.LogInformation("UserId {0} is a guid: Lastname will be blank", employerProfile.UserId);
}
}
else
Expand Down Expand Up @@ -109,8 +123,8 @@ await _employerProfilesApiClient.PutWithResponseCode<EmployerProfileUsersApiResp
{
returnList.Add(new EmployerAccountUser
{
FirstName = firstName,
LastName = lastName,
FirstName = string.IsNullOrWhiteSpace(firstName) ? "Unknown" : firstName,
Copy link
Contributor

Choose a reason for hiding this comment

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

if we store the name as Unknown, do we run the risk of emailing "Unknown Unkown"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

possibly, it depends on how those 2 claims are used down the line. I think they will only exist within EmployerAccounts and I assume for the duration of the auth session

Copy link
Contributor

Choose a reason for hiding this comment

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

the user in EmployerAccounts is upserted with the values same as in finance.

LastName = string.IsNullOrWhiteSpace(lastName) ? "Unknown" : lastName,
UserId = userId,
DisplayName = displayName,
IsSuspended = isSuspended
Expand Down
Loading