From 7c15214e4eddd2b57b6089da40966de4ba9f18b5 Mon Sep 17 00:00:00 2001 From: Richard Mohammed Date: Wed, 23 Oct 2024 14:10:14 +0100 Subject: [PATCH 1/6] Check if a person is already part of organisation. --- .../Model/Exceptions.cs | 3 ++- .../CreateOrganisationJoinRequestUseCase.cs | 20 +++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/Services/CO.CDP.Organisation.WebApi/Model/Exceptions.cs b/Services/CO.CDP.Organisation.WebApi/Model/Exceptions.cs index d216cb0b0..c6f8b26eb 100644 --- a/Services/CO.CDP.Organisation.WebApi/Model/Exceptions.cs +++ b/Services/CO.CDP.Organisation.WebApi/Model/Exceptions.cs @@ -19,4 +19,5 @@ public class MissingOrganisationIdException(string message, Exception? cause = n public class EmptyAuthenticationKeyNameException(string message, Exception? cause = null) : Exception(message, cause); public class UnknownAuthenticationKeyException(string message, Exception? cause = null) : Exception(message, cause); public class InvalidSupportUpdateOrganisationCommand(string message, Exception? cause = null) : Exception(message, cause); -public class DuplicateEmailWithinOrganisationException(string message, Exception? cause = null) : Exception(message, cause); \ No newline at end of file +public class DuplicateEmailWithinOrganisationException(string message, Exception? cause = null) : Exception(message, cause); +public class PersonAlreadyAddedToOrganisationException(string message, Exception? cause = null) : Exception(message, cause); \ No newline at end of file diff --git a/Services/CO.CDP.Organisation.WebApi/UseCase/CreateOrganisationJoinRequestUseCase.cs b/Services/CO.CDP.Organisation.WebApi/UseCase/CreateOrganisationJoinRequestUseCase.cs index 172711430..c7b8533ed 100644 --- a/Services/CO.CDP.Organisation.WebApi/UseCase/CreateOrganisationJoinRequestUseCase.cs +++ b/Services/CO.CDP.Organisation.WebApi/UseCase/CreateOrganisationJoinRequestUseCase.cs @@ -42,6 +42,14 @@ public async Task Execute((Guid organisationId, CreateO var person = await personRepository.Find(command.createOrganisationJoinRequestCommand.PersonId) ?? throw new UnknownPersonException($"Unknown person {command.createOrganisationJoinRequestCommand.PersonId}."); + var alreadyAdded = await CheckPersonAlreadyAdded(organisation, person); + + if (alreadyAdded) + { + throw new PersonAlreadyAddedToOrganisationException( + $"Person {command.createOrganisationJoinRequestCommand.PersonId} already added to organisation {command.organisationId}."); + } + var organisationJoinRequest = CreateOrganisationJoinRequest(organisation, person); organisationJoinRequestRepository.Save(organisationJoinRequest); @@ -157,4 +165,16 @@ private async Task> GetOrganisationAdminUsers(Persistence.Organisat var organisationPersons = await organisationRepository.FindOrganisationPersons(organisation.Guid); return organisationPersons.Where(op => op.Scopes.Contains(Constants.OrganisationPersonScope.Admin)).Select(op => op.Person).ToList(); } + + private async Task CheckPersonAlreadyAdded(Persistence.Organisation organisation, Person person) + { + var matchingOrganisationPerson = await organisationRepository.FindOrganisationPerson(organisation.Guid, person.Guid); + + if (matchingOrganisationPerson != null) + { + return true; + } + + return false; + } } \ No newline at end of file From 31b9dcf41d898ff1ebd5913a515dc180ba5e29ae Mon Sep 17 00:00:00 2001 From: Richard Mohammed Date: Thu, 24 Oct 2024 15:43:26 +0100 Subject: [PATCH 2/6] Add check to see if person is already a member of the organisation. --- .../Constants/ApiExceptionMapper.cs | 3 ++- .../Constants/ErrorCodes.cs | 1 + .../Constants/ErrorMessagesList.cs | 1 + .../Registration/JoinOrganisation.cshtml.cs | 22 +++++++++++++++---- .../Extensions/ServiceCollectionExtensions.cs | 3 ++- 5 files changed, 24 insertions(+), 6 deletions(-) diff --git a/Frontend/CO.CDP.OrganisationApp/Constants/ApiExceptionMapper.cs b/Frontend/CO.CDP.OrganisationApp/Constants/ApiExceptionMapper.cs index e3bdffabe..d8d822611 100644 --- a/Frontend/CO.CDP.OrganisationApp/Constants/ApiExceptionMapper.cs +++ b/Frontend/CO.CDP.OrganisationApp/Constants/ApiExceptionMapper.cs @@ -19,7 +19,7 @@ public static void MapApiExceptions( var errorMessage = GetErrorMessageByCode(code); modelState.AddModelError(string.Empty, errorMessage); - + } private static string? ExtractErrorCode(ApiException aex) @@ -42,6 +42,7 @@ private static string GetErrorMessageByCode(string errorCode) ErrorCodes.UNKNOWN_ORGANISATION => ErrorMessagesList.UnknownOrganisation, ErrorCodes.BUYER_INFO_NOT_EXISTS => ErrorMessagesList.BuyerInfoNotExists, ErrorCodes.UNKNOWN_BUYER_INFORMATION_UPDATE_TYPE => ErrorMessagesList.UnknownBuyerInformationUpdateType, + ErrorCodes.PERSON_ALREADY_ADDED_TO_ORGANISATION => ErrorMessagesList.AlreadyMemberOfOrganisation, _ => ErrorMessagesList.UnexpectedError }; } diff --git a/Frontend/CO.CDP.OrganisationApp/Constants/ErrorCodes.cs b/Frontend/CO.CDP.OrganisationApp/Constants/ErrorCodes.cs index 019e696c7..663032596 100644 --- a/Frontend/CO.CDP.OrganisationApp/Constants/ErrorCodes.cs +++ b/Frontend/CO.CDP.OrganisationApp/Constants/ErrorCodes.cs @@ -20,4 +20,5 @@ public static class ErrorCodes public const string PERSON_INVITE_ALREADY_CLAIMED = "PERSON_INVITE_ALREADY_CLAIMED"; public const string APIKEY_NAME_ALREADY_EXISTS = "APIKEY_NAME_ALREADY_EXISTS"; public const string EMAIL_ALREADY_EXISTS_WITHIN_ORGANISATION = "EMAIL_ALREADY_EXISTS_WITHIN_ORGANISATION"; + public const string PERSON_ALREADY_ADDED_TO_ORGANISATION = "PERSON_ALREADY_ADDED_TO_ORGANISATION"; } \ No newline at end of file diff --git a/Frontend/CO.CDP.OrganisationApp/Constants/ErrorMessagesList.cs b/Frontend/CO.CDP.OrganisationApp/Constants/ErrorMessagesList.cs index cf9b13eee..eb7645fe1 100644 --- a/Frontend/CO.CDP.OrganisationApp/Constants/ErrorMessagesList.cs +++ b/Frontend/CO.CDP.OrganisationApp/Constants/ErrorMessagesList.cs @@ -22,5 +22,6 @@ public static class ErrorMessagesList public const string UnknownBuyerInformationUpdateType = "The requested buyer information update type is unknown."; public const string DuplicateApiKeyName = "An API key with this name already exists. Please try again."; + public const string AlreadyMemberOfOrganisation = "You are already a member of this organisation."; } \ No newline at end of file diff --git a/Frontend/CO.CDP.OrganisationApp/Pages/Registration/JoinOrganisation.cshtml.cs b/Frontend/CO.CDP.OrganisationApp/Pages/Registration/JoinOrganisation.cshtml.cs index 54092162f..e45c21cd8 100644 --- a/Frontend/CO.CDP.OrganisationApp/Pages/Registration/JoinOrganisation.cshtml.cs +++ b/Frontend/CO.CDP.OrganisationApp/Pages/Registration/JoinOrganisation.cshtml.cs @@ -1,5 +1,7 @@ using System.ComponentModel.DataAnnotations; using CO.CDP.Organisation.WebApiClient; +using CO.CDP.OrganisationApp.Constants; +using CO.CDP.OrganisationApp.Models; using Microsoft.AspNetCore.Mvc; using OrganisationWebApiClient = CO.CDP.Organisation.WebApiClient; @@ -7,7 +9,8 @@ namespace CO.CDP.OrganisationApp.Pages.Registration; public class JoinOrganisationModel( IOrganisationClient organisationClient, - ISession session) : LoggedInUserAwareModel(session) + ISession session, + ITempDataService tempDataService) : LoggedInUserAwareModel(session) { public OrganisationWebApiClient.Organisation? OrganisationDetails { get; set; } @@ -15,6 +18,8 @@ public class JoinOrganisationModel( [Required(ErrorMessage = "Select an option")] public bool Join { get; set; } + public FlashMessage NotificationBannerAlreadyMemberOfOrganisation { get { return new FlashMessage("You are already a member of this organisation. View Organisation"); } } + public async Task OnGet(string identifier) { try @@ -41,9 +46,18 @@ public async Task OnPost(string identifier) { if (Join == true) { - await organisationClient.CreateJoinRequestAsync(OrganisationDetails.Id, new CreateOrganisationJoinRequest( - personId: UserDetails.PersonId.Value - )); + try + { + await organisationClient.CreateJoinRequestAsync(OrganisationDetails.Id, + new CreateOrganisationJoinRequest( + personId: UserDetails.PersonId.Value + )); + } + catch (ApiException aex) + { + tempDataService.Put(FlashMessageTypes.Important, NotificationBannerAlreadyMemberOfOrganisation); + return Page(); + } return Redirect("/registration/" + identifier + "/join-organisation/success"); } diff --git a/Services/CO.CDP.Organisation.WebApi/Extensions/ServiceCollectionExtensions.cs b/Services/CO.CDP.Organisation.WebApi/Extensions/ServiceCollectionExtensions.cs index 3ed48a701..6a48d3ece 100644 --- a/Services/CO.CDP.Organisation.WebApi/Extensions/ServiceCollectionExtensions.cs +++ b/Services/CO.CDP.Organisation.WebApi/Extensions/ServiceCollectionExtensions.cs @@ -21,7 +21,8 @@ public static class ServiceCollectionExtensions { typeof(InvalidUpdateSupplierInformationCommand), (StatusCodes.Status400BadRequest, "INVALID_SUPPLIER_INFORMATION_UPDATE_ENTITY") }, { typeof(InvalidQueryException), (StatusCodes.Status400BadRequest, "ISSUE_WITH_QUERY_PARAMETERS") }, { typeof(DuplicateAuthenticationKeyNameException), (StatusCodes.Status400BadRequest, "APIKEY_NAME_ALREADY_EXISTS") }, - { typeof(DuplicateEmailWithinOrganisationException), (StatusCodes.Status400BadRequest, "EMAIL_ALREADY_EXISTS_WITHIN_ORGANISATION") } + { typeof(DuplicateEmailWithinOrganisationException), (StatusCodes.Status400BadRequest, "EMAIL_ALREADY_EXISTS_WITHIN_ORGANISATION") }, + { typeof(PersonAlreadyAddedToOrganisationException), (StatusCodes.Status400BadRequest, "PERSON_ALREADY_ADDED_TO_ORGANISATION") } }; public static IServiceCollection AddOrganisationProblemDetails(this IServiceCollection services) From 572bc63d0ba0c6848fe08fd9bb8454f81a037b85 Mon Sep 17 00:00:00 2001 From: Richard Mohammed Date: Thu, 24 Oct 2024 16:25:17 +0100 Subject: [PATCH 3/6] Fix build and unit tests. --- .../Pages/Registration/JoinOrganisationTests.cs | 4 +++- .../Pages/Registration/JoinOrganisation.cshtml.cs | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/Frontend/CO.CDP.OrganisationApp.Tests/Pages/Registration/JoinOrganisationTests.cs b/Frontend/CO.CDP.OrganisationApp.Tests/Pages/Registration/JoinOrganisationTests.cs index 45e3481e3..786978aa1 100644 --- a/Frontend/CO.CDP.OrganisationApp.Tests/Pages/Registration/JoinOrganisationTests.cs +++ b/Frontend/CO.CDP.OrganisationApp.Tests/Pages/Registration/JoinOrganisationTests.cs @@ -12,6 +12,7 @@ public class JoinOrganisationModelTests { private readonly Mock _organisationClientMock; private readonly Mock _sessionMock; + private readonly Mock _tempDataMock; private readonly JoinOrganisationModel _joinOrganisationModel; private readonly Guid _organisationId = Guid.NewGuid(); private readonly string _identifier = "GB-COH:123456789"; @@ -22,9 +23,10 @@ public JoinOrganisationModelTests() { _organisationClientMock = new Mock(); _sessionMock = new Mock(); + _tempDataMock = new Mock(); _sessionMock.Setup(s => s.Get(Session.UserDetailsKey)) .Returns(new UserDetails() { UserUrn = "testUserUrn", PersonId = _personId}); - _joinOrganisationModel = new JoinOrganisationModel(_organisationClientMock.Object, _sessionMock.Object); + _joinOrganisationModel = new JoinOrganisationModel(_organisationClientMock.Object, _sessionMock.Object, _tempDataMock.Object); _organisation = new CO.CDP.Organisation.WebApiClient.Organisation(null, null, null, null, _organisationId, null, "Test Org", []); } diff --git a/Frontend/CO.CDP.OrganisationApp/Pages/Registration/JoinOrganisation.cshtml.cs b/Frontend/CO.CDP.OrganisationApp/Pages/Registration/JoinOrganisation.cshtml.cs index e45c21cd8..1d2197419 100644 --- a/Frontend/CO.CDP.OrganisationApp/Pages/Registration/JoinOrganisation.cshtml.cs +++ b/Frontend/CO.CDP.OrganisationApp/Pages/Registration/JoinOrganisation.cshtml.cs @@ -44,7 +44,7 @@ public async Task OnPost(string identifier) if (UserDetails.PersonId != null && OrganisationDetails != null) { - if (Join == true) + if (Join) { try { @@ -53,7 +53,7 @@ await organisationClient.CreateJoinRequestAsync(OrganisationDetails.Id, personId: UserDetails.PersonId.Value )); } - catch (ApiException aex) + catch (ApiException) { tempDataService.Put(FlashMessageTypes.Important, NotificationBannerAlreadyMemberOfOrganisation); return Page(); From aedf2c9b12233bfbca91c3a1eb8e2440760c4a85 Mon Sep 17 00:00:00 2001 From: Richard Mohammed Date: Fri, 25 Oct 2024 10:12:40 +0100 Subject: [PATCH 4/6] Add unit test. --- .../Registration/JoinOrganisationTests.cs | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/Frontend/CO.CDP.OrganisationApp.Tests/Pages/Registration/JoinOrganisationTests.cs b/Frontend/CO.CDP.OrganisationApp.Tests/Pages/Registration/JoinOrganisationTests.cs index 786978aa1..0d6939451 100644 --- a/Frontend/CO.CDP.OrganisationApp.Tests/Pages/Registration/JoinOrganisationTests.cs +++ b/Frontend/CO.CDP.OrganisationApp.Tests/Pages/Registration/JoinOrganisationTests.cs @@ -1,9 +1,11 @@ using CO.CDP.Organisation.WebApiClient; +using CO.CDP.OrganisationApp.Constants; using CO.CDP.OrganisationApp.Models; using CO.CDP.OrganisationApp.Pages.Registration; using FluentAssertions; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.RazorPages; +using OrganisationWebApiClient = CO.CDP.Organisation.WebApiClient; using Moq; namespace CO.CDP.OrganisationApp.Tests.Pages.Registration; @@ -121,4 +123,28 @@ public async Task OnPost_UserHasNoPersonId_RedirectsToHomePage() _organisationClientMock.Verify(client => client.CreateJoinRequestAsync(It.IsAny(), It.IsAny()), Times.Never); } + + [Fact] + public async Task OnPost_CreateJoinRequestThrowsApiException_SetsFlashMessageAndReturnsPage() + { + _organisationClientMock.Setup(client => client.LookupOrganisationAsync(string.Empty, _identifier)) + .ReturnsAsync(_organisation); + _joinOrganisationModel.Join = true; + + _organisationClientMock.Setup(client => client.CreateJoinRequestAsync( + _organisationId, + It.Is(r => r.PersonId == _joinOrganisationModel.UserDetails.PersonId))) + .ThrowsAsync(new ApiException("Error", 400, "Error", null!, null!, null!)); + + var result = await _joinOrganisationModel.OnPost(_identifier); + + _tempDataMock.Verify(tempData => tempData.Put( + FlashMessageTypes.Important, + It.IsAny()), + Times.Once); + + result.Should().BeOfType(); + } + + } From 3cfcb1872573b97e0e877c3cc27853b7a9716e1a Mon Sep 17 00:00:00 2001 From: Richard Mohammed Date: Fri, 25 Oct 2024 12:56:29 +0100 Subject: [PATCH 5/6] Use message ErrorMessagesList.AlreadyMemberOfOrganisation --- .../Pages/Registration/JoinOrganisation.cshtml.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Frontend/CO.CDP.OrganisationApp/Pages/Registration/JoinOrganisation.cshtml.cs b/Frontend/CO.CDP.OrganisationApp/Pages/Registration/JoinOrganisation.cshtml.cs index 1d2197419..67afaecd8 100644 --- a/Frontend/CO.CDP.OrganisationApp/Pages/Registration/JoinOrganisation.cshtml.cs +++ b/Frontend/CO.CDP.OrganisationApp/Pages/Registration/JoinOrganisation.cshtml.cs @@ -18,7 +18,7 @@ public class JoinOrganisationModel( [Required(ErrorMessage = "Select an option")] public bool Join { get; set; } - public FlashMessage NotificationBannerAlreadyMemberOfOrganisation { get { return new FlashMessage("You are already a member of this organisation. View Organisation"); } } + public FlashMessage NotificationBannerAlreadyMemberOfOrganisation { get { return new FlashMessage(ErrorMessagesList.AlreadyMemberOfOrganisation + " View Organisation"); } } public async Task OnGet(string identifier) { From 68030fb8638d1ea5688cb6eaad8aabe207a3dc85 Mon Sep 17 00:00:00 2001 From: Richard Mohammed Date: Fri, 25 Oct 2024 14:38:54 +0100 Subject: [PATCH 6/6] Create GuardPersonIsNotAlreadyAdded that throws the error. --- .../CreateOrganisationJoinRequestUseCase.cs | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/Services/CO.CDP.Organisation.WebApi/UseCase/CreateOrganisationJoinRequestUseCase.cs b/Services/CO.CDP.Organisation.WebApi/UseCase/CreateOrganisationJoinRequestUseCase.cs index c7b8533ed..897f46869 100644 --- a/Services/CO.CDP.Organisation.WebApi/UseCase/CreateOrganisationJoinRequestUseCase.cs +++ b/Services/CO.CDP.Organisation.WebApi/UseCase/CreateOrganisationJoinRequestUseCase.cs @@ -42,13 +42,7 @@ public async Task Execute((Guid organisationId, CreateO var person = await personRepository.Find(command.createOrganisationJoinRequestCommand.PersonId) ?? throw new UnknownPersonException($"Unknown person {command.createOrganisationJoinRequestCommand.PersonId}."); - var alreadyAdded = await CheckPersonAlreadyAdded(organisation, person); - - if (alreadyAdded) - { - throw new PersonAlreadyAddedToOrganisationException( - $"Person {command.createOrganisationJoinRequestCommand.PersonId} already added to organisation {command.organisationId}."); - } + await GuardPersonIsNotAlreadyAdded(organisation, person); var organisationJoinRequest = CreateOrganisationJoinRequest(organisation, person); @@ -166,15 +160,14 @@ private async Task> GetOrganisationAdminUsers(Persistence.Organisat return organisationPersons.Where(op => op.Scopes.Contains(Constants.OrganisationPersonScope.Admin)).Select(op => op.Person).ToList(); } - private async Task CheckPersonAlreadyAdded(Persistence.Organisation organisation, Person person) + private async Task GuardPersonIsNotAlreadyAdded(Persistence.Organisation organisation, Person person) { var matchingOrganisationPerson = await organisationRepository.FindOrganisationPerson(organisation.Guid, person.Guid); if (matchingOrganisationPerson != null) { - return true; + throw new PersonAlreadyAddedToOrganisationException( + $"Person {person.Guid} already added to organisation {organisation.Guid}."); } - - return false; } } \ No newline at end of file