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

[Official] Implement list of all officials (employees) for for the provider #1634

Merged
merged 11 commits into from
Jan 23, 2025
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
using OutOfSchool.Services.Enums;

namespace OutOfSchool.BusinessLogic.Models.Official;
public class OfficialDto
{
public Guid Id { get; set; }
public OfficialPositionDto Position { get; set; }
public OfficialIndividualDto Individual { get; set; }
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

PII fields require data protection implementation

The review comment is valid. While ASP.NET Core Data Protection is configured, it's only used for framework-level protection (cookies, tokens). The PII fields in OfficialIndividualDto (FirstName, MiddleName, LastName, Rnokpp) and other DTOs are currently stored and transmitted without additional protection mechanisms.

🔗 Analysis chain

Consider implementing data protection for PII.

The Individual property contains sensitive personal information (name, RNOKPP). Consider implementing data protection or encryption for these fields.

Run this script to check for other instances of unprotected PII:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for potential PII fields in DTOs
rg -g "**/*Dto.cs" "FirstName|LastName|MiddleName|Rnokpp|Email|Phone|Address" -C 2

Length of output: 36422


Script:

#!/bin/bash
# Search for data protection implementations
rg -g "**/*.cs" "DataProtection|IDataProtector|ProtectData|UnprotectData|Encrypt|Decrypt" -C 2

Length of output: 67263

public string DismissalOrder { get; set; } = string.Empty;
public string RecruitmentOrder { get; set; } = string.Empty;
public string DismissalReason { get; set; } = string.Empty;
public EmploymentType EmploymentType { get; set; }
public DateOnly ActiveFrom { get; set; }
public DateOnly ActiveTo { get; set; }
Comment on lines +13 to +14
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add validation for ActiveFrom and ActiveTo dates.

The temporal data should be validated to ensure ActiveFrom is not later than ActiveTo.

Consider adding a validation attribute or implementing IValidatableObject:

+using System.ComponentModel.DataAnnotations;
+
 public class OfficialDto : IValidatableObject
 {
     // ... existing properties ...

+    public IEnumerable<ValidationResult> Validate(ValidationContext validationContext)
+    {
+        if (ActiveFrom > ActiveTo)
+        {
+            yield return new ValidationResult(
+                "ActiveFrom date must not be later than ActiveTo date",
+                new[] { nameof(ActiveFrom), nameof(ActiveTo) });
+        }
+    }
 }

Committable suggestion skipped: line range outside the PR's diff.

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
namespace OutOfSchool.BusinessLogic.Models.Official;
public class OfficialIndividualDto
{
public string FirstName { get; set; }
public string MiddleName { get; set; }
public string LastName { get; set; }
public string Rnokpp { get; set; }
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
namespace OutOfSchool.BusinessLogic.Models.Official;
public class OfficialPositionDto
{
public Guid Id { get; set; }
public string FullName { get; set; }
}
14 changes: 14 additions & 0 deletions OutOfSchool/OutOfSchool.BusinessLogic/Services/IOfficialService.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
using OutOfSchool.BusinessLogic.Models;
using OutOfSchool.BusinessLogic.Models.Official;

namespace OutOfSchool.BusinessLogic.Services;
public interface IOfficialService
{
/// <summary>
/// Gets officials by filter.
/// </summary>
/// <param name="providerId">Provider's Id.</param>
/// <param name="filter">Filter for list of Officials.</param>
/// <returns>SearchResult that contains a filtered list of Officials and the total amount of officials in the list.</returns>
Task<SearchResult<OfficialDto>> GetByFilter(Guid providerId, SearchStringFilter filter);
}
85 changes: 85 additions & 0 deletions OutOfSchool/OutOfSchool.BusinessLogic/Services/OfficialService.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
using AutoMapper;
using OutOfSchool.BusinessLogic.Models;
using OutOfSchool.BusinessLogic.Models.Official;
using OutOfSchool.BusinessLogic.Services.ProviderServices;
using OutOfSchool.Services.Repository.Base.Api;
using System.Linq.Expressions;

namespace OutOfSchool.BusinessLogic.Services;
public class OfficialService : IOfficialService
{
private readonly IEntityRepositorySoftDeleted<Guid, Official> officialRepository;
private readonly IProviderService providerService;
private readonly ILogger<OfficialService> logger;
private readonly IMapper mapper;

/// <summary>
/// Initializes a new instance of the <see cref="OfficialService"/> class.
/// </summary>
/// <param name="officialRepository">Repository for Officials.</param>
/// <param name="providerService">Service for Provider.</param>
/// <param name="logger">Logger.</param>
/// <param name="mapper">Mapper.</param>
public OfficialService(
IEntityRepositorySoftDeleted<Guid, Official> officialRepository,
IProviderService providerService,
ILogger<OfficialService> logger,
IMapper mapper
)
{
this.officialRepository = officialRepository ?? throw new ArgumentNullException(nameof(officialRepository));
this.providerService = providerService ?? throw new ArgumentNullException(nameof(providerService));
this.logger = logger ?? throw new ArgumentNullException(nameof(logger));
this.mapper = mapper ?? throw new ArgumentNullException(nameof(mapper));
}

/// <inheritdoc/>
public async Task<SearchResult<OfficialDto>> GetByFilter(Guid providerId, SearchStringFilter filter)
{
await providerService.HasProviderRights(providerId);

logger.LogDebug("Getting Officials by filter started.");

filter ??= new SearchStringFilter();
var predicate = BuildPredicate(filter);
int count = await officialRepository.Count(predicate).ConfigureAwait(false);
Comment on lines +38 to +45
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Critical: Filter officials by providerId to ensure correct data retrieval.

The GetByFilter method accepts a providerId, but the query predicate does not filter officials by this providerId. This could result in retrieving officials not associated with the specified provider, leading to potential data leakage or unauthorized access.

Apply this diff to include filtering by providerId:

 public async Task<SearchResult<OfficialDto>> GetByFilter(Guid providerId, SearchStringFilter filter)
 {
     await providerService.HasProviderRights(providerId);

     logger.LogDebug("Getting Officials by filter started.");

     filter ??= new SearchStringFilter();
     var predicate = BuildPredicate(filter);
+    predicate = predicate.And(o => o.ProviderId == providerId);
     int count = await officialRepository.Count(predicate).ConfigureAwait(false);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{
await providerService.HasProviderRights(providerId);
logger.LogDebug("Getting Officials by filter started.");
filter ??= new SearchStringFilter();
var predicate = BuildPredicate(filter);
int count = await officialRepository.Count(predicate).ConfigureAwait(false);
{
await providerService.HasProviderRights(providerId);
logger.LogDebug("Getting Officials by filter started.");
filter ??= new SearchStringFilter();
var predicate = BuildPredicate(filter);
predicate = predicate.And(o => o.ProviderId == providerId);
int count = await officialRepository.Count(predicate).ConfigureAwait(false);


var officials = await officialRepository
.Get(
skip: filter.From,
take: filter.Size,
includeProperties: "Position,Individual",
whereExpression: predicate
).AsNoTracking()
.ToListAsync()
.ConfigureAwait(false);

logger.LogDebug("{Count} records were successfully received from the Officials table", officials.Count);

var result = new SearchResult<OfficialDto>
{
Entities = mapper.Map<List<OfficialDto>>(officials),
TotalAmount = count
};

return result;
}

private static Expression<Func<Official, bool>> BuildPredicate(SearchStringFilter filter)
{
var predicate = PredicateBuilder.True<Official>();

if (!string.IsNullOrEmpty(filter.SearchString))
{
predicate = predicate.And(o => o.Individual.FirstName.Contains(filter.SearchString, StringComparison.OrdinalIgnoreCase)
|| o.Individual.MiddleName.Contains(filter.SearchString, StringComparison.OrdinalIgnoreCase)
|| o.Individual.LastName.Contains(filter.SearchString, StringComparison.OrdinalIgnoreCase)
|| o.Individual.Rnokpp.Contains(filter.SearchString, StringComparison.OrdinalIgnoreCase)
|| o.Position.FullName.Contains(filter.SearchString, StringComparison.OrdinalIgnoreCase));
Comment on lines +74 to +78
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Sanitize and validate search input for security and performance.

The current implementation has several concerns:

  1. No input sanitization for the search string
  2. Multiple Contains operations could impact performance
  3. RNOKPP (tax number) should be validated against a specific format

Consider these improvements:

-predicate = predicate.And(o => o.Individual.FirstName.Contains(filter.SearchString, StringComparison.OrdinalIgnoreCase)
-    || o.Individual.MiddleName.Contains(filter.SearchString, StringComparison.OrdinalIgnoreCase)
-    || o.Individual.LastName.Contains(filter.SearchString, StringComparison.OrdinalIgnoreCase)
-    || o.Individual.Rnokpp.Contains(filter.SearchString, StringComparison.OrdinalIgnoreCase)
-    || o.Position.FullName.Contains(filter.SearchString, StringComparison.OrdinalIgnoreCase));
+// Sanitize input
+var searchTerm = SanitizeSearchInput(filter.SearchString);
+
+// Optimize search by trimming and converting to uppercase once
+var normalizedTerm = searchTerm.Trim().ToUpperInvariant();
+
+predicate = predicate.And(o => 
+    EF.Functions.ILike(o.Individual.FirstName, $"%{normalizedTerm}%")
+    || EF.Functions.ILike(o.Individual.MiddleName, $"%{normalizedTerm}%")
+    || EF.Functions.ILike(o.Individual.LastName, $"%{normalizedTerm}%")
+    || (IsValidRnokpp(normalizedTerm) && o.Individual.Rnokpp == normalizedTerm)
+    || EF.Functions.ILike(o.Position.FullName, $"%{normalizedTerm}%"));

Add these helper methods:

private static string SanitizeSearchInput(string input)
{
    if (string.IsNullOrEmpty(input))
        return string.Empty;
        
    // Remove any potentially dangerous characters
    return Regex.Replace(input, @"[^a-zA-Z0-9\s]", "");
}

private static bool IsValidRnokpp(string rnokpp)
{
    return !string.IsNullOrEmpty(rnokpp) 
        && rnokpp.Length == 10 
        && rnokpp.All(char.IsDigit);
}

}

predicate = predicate.And(o => !o.IsDeleted);

return predicate;
}
}
10 changes: 10 additions & 0 deletions OutOfSchool/OutOfSchool.BusinessLogic/Util/MappingProfile.cs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
using OutOfSchool.Common.Models;
using OutOfSchool.Services.Models.CompetitiveEvents;
using OutOfSchool.Services.Models.Images;
using OutOfSchool.BusinessLogic.Models.Official;

namespace OutOfSchool.BusinessLogic.Util;

Expand Down Expand Up @@ -942,6 +943,15 @@ public MappingProfile()
.ForMember(dest => dest.WorkshopId, opt => opt.Ignore());

CreateMap<Language, LanguageDto>().ReverseMap();

CreateMap<Official, OfficialDto>()
.ForMember(dest => dest.DismissalOrder, opt => opt.MapFrom(src => src.DismissalOrder ?? string.Empty))
.ForMember(dest => dest.RecruitmentOrder, opt => opt.MapFrom(src => src.RecruitmentOrder ?? string.Empty))
.ForMember(dest => dest.DismissalReason, opt => opt.MapFrom(src => src.DismissalReason ?? string.Empty));

CreateMap<Position, OfficialPositionDto>();

CreateMap<Individual, OfficialIndividualDto>();
Comment on lines +947 to +954
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add reverse mappings and validation for Official entities.

The current mappings are one-directional. Consider adding reverse mappings and validation for required fields.

 CreateMap<Official, OfficialDto>()
     .ForMember(dest => dest.DismissalOrder, opt => opt.MapFrom(src => src.DismissalOrder ?? string.Empty))
     .ForMember(dest => dest.RecruitmentOrder, opt => opt.MapFrom(src => src.RecruitmentOrder ?? string.Empty))
     .ForMember(dest => dest.DismissalReason, opt => opt.MapFrom(src => src.DismissalReason ?? string.Empty));

+CreateMap<OfficialDto, Official>()
+    .ForMember(dest => dest.Individual, opt => opt.Condition(src => src.Individual != null))
+    .ForMember(dest => dest.Position, opt => opt.Condition(src => src.Position != null));

 CreateMap<Position, OfficialPositionDto>();
+CreateMap<OfficialPositionDto, Position>();

 CreateMap<Individual, OfficialIndividualDto>();
+CreateMap<OfficialIndividualDto, Individual>();
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
CreateMap<Official, OfficialDto>()
.ForMember(dest => dest.DismissalOrder, opt => opt.MapFrom(src => src.DismissalOrder ?? string.Empty))
.ForMember(dest => dest.RecruitmentOrder, opt => opt.MapFrom(src => src.RecruitmentOrder ?? string.Empty))
.ForMember(dest => dest.DismissalReason, opt => opt.MapFrom(src => src.DismissalReason ?? string.Empty));
CreateMap<Position, OfficialPositionDto>();
CreateMap<Individual, OfficialIndividualDto>();
CreateMap<Official, OfficialDto>()
.ForMember(dest => dest.DismissalOrder, opt => opt.MapFrom(src => src.DismissalOrder ?? string.Empty))
.ForMember(dest => dest.RecruitmentOrder, opt => opt.MapFrom(src => src.RecruitmentOrder ?? string.Empty))
.ForMember(dest => dest.DismissalReason, opt => opt.MapFrom(src => src.DismissalReason ?? string.Empty));
CreateMap<OfficialDto, Official>()
.ForMember(dest => dest.Individual, opt => opt.Condition(src => src.Individual != null))
.ForMember(dest => dest.Position, opt => opt.Condition(src => src.Position != null));
CreateMap<Position, OfficialPositionDto>();
CreateMap<OfficialPositionDto, Position>();
CreateMap<Individual, OfficialIndividualDto>();
CreateMap<OfficialIndividualDto, Individual>();

}

public IMappingExpression<TSource, TDestination> CreateSoftDeletedMap<TSource, TDestination>()
Expand Down
4 changes: 2 additions & 2 deletions OutOfSchool/OutOfSchool.DataAccess/OutOfSchoolDbContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,7 @@ protected override void OnModelCreating(ModelBuilder builder)
builder.ApplyConfiguration(new InstitutionFieldDescriptionConfiguration());
builder.ApplyConfiguration(new InstitutionHierarchyConfiguration());
builder.ApplyConfiguration(new InstitutionStatusConfiguration());
builder.ApplyConfiguration(new LanguageConfiguration());
builder.ApplyConfiguration(new NotificationConfiguration());
builder.ApplyConfiguration(new OfficialConfiguration());
builder.ApplyConfiguration(new OperationWithObjectConfiguration());
Expand All @@ -196,13 +197,12 @@ protected override void OnModelCreating(ModelBuilder builder)
builder.ApplyConfiguration(new RatingConfiguration());
builder.ApplyConfiguration(new RegionAdminConfiguration());
builder.ApplyConfiguration(new SocialGroupConfiguration());
builder.ApplyConfiguration(new StudySubjectConfiguration());
builder.ApplyConfiguration(new TagConfiguration());
builder.ApplyConfiguration(new TeacherConfiguration());
builder.ApplyConfiguration(new UserConfiguration());
builder.ApplyConfiguration(new WorkshopConfiguration());
builder.ApplyConfiguration(new WorkshopDescriptionItemConfiguration());
builder.ApplyConfiguration(new StudySubjectConfiguration());
builder.ApplyConfiguration(new LanguageConfiguration());
builder.ApplyConfiguration(new EntityImagesConfiguration<WorkshopDraft>());
builder.ApplyConfiguration(new WorkshopDraftConfiguration());
builder.ApplyConfiguration(new TeacherDraftConfiguration());
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
using Microsoft.AspNetCore.Mvc;
using Moq;
using NUnit.Framework;
using OutOfSchool.BusinessLogic.Models;
using OutOfSchool.BusinessLogic.Models.Official;
using OutOfSchool.BusinessLogic.Services;
using OutOfSchool.Services.Enums;
using OutOfSchool.WebApi.Controllers.V1;
using System;
using System.Collections.Generic;
using System.Threading.Tasks;

namespace OutOfSchool.WebApi.Tests.Controllers;

[TestFixture]
public class OfficialControllerTests
{
private OfficialController controller;
private Guid providerId;
private Mock<IOfficialService> service;

[SetUp]
public void SetUp()
{
providerId = Guid.NewGuid();
service = new Mock<IOfficialService>();

controller = new OfficialController(service.Object);
}

[Test]
public async Task Get_ReturnsNoContent_WhenListIsEmpty()
{
// Arrange
var emptyResult = new SearchResult<OfficialDto>();
service.Setup(s => s.GetByFilter(providerId, null)).ReturnsAsync(emptyResult);

// Act
var result = await controller.Get(providerId, null).ConfigureAwait(false) as NoContentResult;

// Assert
Assert.That(result, Is.Not.Null);
Assert.That(result.StatusCode, Is.EqualTo(204));
}

[Test]
public async Task Get_ReturnsOk_WhenListIsNotEmpty()
{
// Arrange
var searchResult = new SearchResult<OfficialDto>()
{
Entities = new List<OfficialDto>
{
FakeOfficialDto(),
FakeOfficialDto(),
FakeOfficialDto()
},
TotalAmount = 3
};
service.Setup(s => s.GetByFilter(providerId, null)).ReturnsAsync(searchResult);

// Act
var result = await controller.Get(providerId, null).ConfigureAwait(false) as OkObjectResult;

// Assert
Assert.That(result, Is.Not.Null);
Assert.That(result.StatusCode, Is.EqualTo(200));

var returnedSearchResult = result.Value as SearchResult<OfficialDto>;
Assert.That(returnedSearchResult, Is.Not.Null);
Assert.That(returnedSearchResult.TotalAmount, Is.EqualTo(searchResult.TotalAmount));
Assert.That(returnedSearchResult.Entities, Is.EqualTo(searchResult.Entities));
}
Comment on lines +31 to +73
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance test coverage with additional test cases.

The current tests only cover basic scenarios. Consider adding tests for:

  1. Service throwing exceptions
  2. Filter parameter variations
  3. Invalid providerId (empty GUID)

Example additional test:

[Test]
public async Task Get_ReturnsInternalServerError_WhenServiceThrowsException()
{
    // Arrange
    service.Setup(s => s.GetByFilter(providerId, null))
           .ThrowsAsync(new Exception("Test exception"));

    // Act
    var result = await controller.Get(providerId, null).ConfigureAwait(false) as ObjectResult;

    // Assert
    Assert.That(result, Is.Not.Null);
    Assert.That(result.StatusCode, Is.EqualTo(500));
}


private OfficialDto FakeOfficialDto()
{
return new OfficialDto()
{
ActiveFrom = DateOnly.FromDateTime(DateTime.Now),
ActiveTo = DateOnly.FromDateTime(DateTime.Now),
Id = Guid.NewGuid(),
EmploymentType = EmploymentType.PartTime,
Individual = new OfficialIndividualDto()
{
FirstName = "Test",
LastName = "Testov",
MiddleName = "Testovich",
Rnokpp = "1234567890"
},
Position = new OfficialPositionDto()
{
Id = Guid.NewGuid(),
FullName = "Test"
}
};
}
}
Loading
Loading