OutOfSchool/OutOfSchool.WebApi.Tests/Services/StatisticServiceTest.cs (1)

545-551: ⚠️ Potential issue

Fix incomplete test data in Application test case.

The SubDirection object in this test case is missing the DirectionId and Direction properties that are present in all other test cases. This inconsistency could affect test reliability.

Apply this diff to complete the test data:

                            new SubDirection
                            {
                                Id = 2,
+                               DirectionId = 2,
+                               Direction = new Direction
+                               {
+                                   Id = 2
+                               }
                            },

OutOfSchool/OutOfSchool.BusinessLogic/Services/SubordinationStructure/InstitutionHierarchyService.cs (1)

54-54: ⚠️ Potential issue

Update Create and Update methods to use SubDirections.

The Create and Update methods need to be updated to use the new SubDirection relationship instead of directly using Directions.

Apply this diff to fix the inconsistency:

-        var newInstitutionHierarchy = await repository.Create(institutionHierarchy, dto.Directions.Select(d => d.Id).ToList()).ConfigureAwait(false);
+        var newInstitutionHierarchy = await repository.Create(institutionHierarchy, dto.SubDirections.Select(s => s.Direction.Id).ToList()).ConfigureAwait(false);

-            .Update(
-                institutionHierarchy,
-                dto.Directions.Select(d => d.Id).ToList())
+            .Update(
+                institutionHierarchy,
+                dto.SubDirections.Select(s => s.Direction.Id).ToList())

Also applies to: 236-236

OutOfSchool/OutOfSchool.WebApi.Tests/Controllers/AdminControllerTests.cs (1)

219-239: Consider adding test coverage for failed results.

While the tests cover successful updates and invalid model states, they don't verify how the controller handles failed results from the service layer (e.g., when the service returns Result<DirectionDto>.Failed()).

Consider adding a test case like this:

+[Test]
+public async Task UpdateDirections_WhenServiceReturnsFailed_ReturnsBadRequestObjectResult()
+{
+    // Arrange
+    controller.ControllerContext.HttpContext = fakeHttpContext;
+    controller.ControllerContext.HttpContext.SetContextUser(Role.TechAdmin);
+    var changedDirection = new DirectionDto()
+    {
+        Id = 1,
+        Title = "ChangedTitle",
+    };
+    var returnedResult = Result<DirectionDto>.Failed(new OperationError
+    {
+        Code = "400",
+        Description = "Update failed",
+    });
+    sensitiveDirectionService.Setup(x => x.Update(changedDirection)).ReturnsAsync(returnedResult);
+
+    // Act
+    var result = await controller.UpdateDirections(changedDirection).ConfigureAwait(false);
+
+    // Assert
+    Assert.That(result, Is.TypeOf<BadRequestObjectResult>());
+    Assert.That((result as BadRequestObjectResult).StatusCode, Is.EqualTo(400));
+}

Also applies to: 241-255

OutOfSchool/OutOfSchool.WebApi.Tests/Services/StatisticServiceTest.cs (3)

59-80: Enhance test coverage for SubDirection functionality.

The existing tests verify the workshop retrieval but don't explicitly validate the new SubDirection layer. Consider adding test cases that verify:

1. Correct mapping of SubDirection to Direction
2. Proper filtering based on SubDirection properties

Also applies to: 82-104

---

248-476: Reduce test data duplication using helper methods.

The SubDirection test data structure is repeated across WithWorkshops, WithWorkshopsIncludingCATOTTG, and WithApplications methods. Consider extracting the common SubDirection setup into a helper method.

Example helper method:

private SubDirection CreateSubDirection(int id)
{
    return new SubDirection
    {
        Id = id,
        DirectionId = id,
        Direction = new Direction
        {
            Id = id
        }
    };
}

Also applies to: 488-626

---

106-156: Enhance assertions to verify SubDirection properties.

The current assertions verify the overall results but don't explicitly check the SubDirection properties. Consider adding specific assertions to verify the SubDirection-Direction relationship.

Example assertion:

result
    .Should()
    .Match<Workshop>(w => 
        w.InstitutionHierarchy.SubDirections.All(sd => 
            sd.DirectionId == sd.Direction.Id));

OutOfSchool/OutOfSchool.WebApi.Tests/Controllers/DirectionControllerTests.cs (1)

153-165: Add test coverage for Result failure scenarios.

The Create test only covers the success case. Consider adding tests for failure scenarios using Result, such as:

• Business rule violations
• Validation failures
• Conflict scenarios

Example test:

[Test]
public async Task Create_WhenBusinessRuleViolated_ReturnsBadRequest()
{
    // Arrange
    var error = "Business rule violated";
    var returnedResult = Result<DirectionDto>.Failed(error);
    service.Setup(x => x.Create(direction)).ReturnsAsync(returnedResult);

    // Act
    var result = await controller.Create(direction).ConfigureAwait(false) as BadRequestObjectResult;

    // Assert
    Assert.That(result, Is.Not.Null);
    Assert.That(result.StatusCode, Is.EqualTo(400));
    Assert.That(result.Value, Is.EqualTo(error));
}

OutOfSchool/OutOfSchool.BusinessLogic/Util/Mapping/ElasticProfile.cs (1)

156-156: Consider adding null checks for robustness.

The mapping assumes that both SubDirection and Direction entities are not null.

Add null checks to prevent potential NullReferenceException:

-opt => opt.MapFrom(src => src.InstitutionHierarchy.SubDirections.Where(x => !x.Direction.IsDeleted).Select(d => d.DirectionId)))
+opt => opt.MapFrom(src => src.InstitutionHierarchy?.SubDirections?.Where(x => x.Direction != null && !x.Direction.IsDeleted).Select(d => d.DirectionId) ?? Enumerable.Empty<long>()))

OutOfSchool/OutOfSchool.WebApi.Tests/Services/DirectionServiceTests.cs (3)

103-103: Consider a more specific test name.

The test name could be more descriptive about the specific failure condition being tested.

Consider renaming to Create_DuplicateTitle_ReturnsFailedResult to better describe the exact validation being tested.

---

223-225: Enhance test coverage for SubDirections relationship.

While the filter logic has been updated to use SubDirections, the test assertions only verify the Direction properties.

Consider adding assertions to verify the SubDirections relationship:

 // Assert
 Assert.True(result.Entities.All(d => d.Title == expectedDto.Title));
+Assert.That(expected.FirstOrDefault().SubDirections, Is.Not.Empty, "Direction should have associated SubDirections");
+Assert.That(expected.FirstOrDefault().SubDirections.SelectMany(s => s.InstitutionHierarchies), Is.Not.Empty, "SubDirections should have associated InstitutionHierarchies");

Also applies to: 260-262

---

313-330: Enhance test data coverage.

The test data could be more comprehensive. Currently, only one Direction has SubDirections, which might miss testing important scenarios.

Consider adding SubDirections to other test Directions to ensure proper testing of various scenarios:

 new Direction()
 {
     Title = "Test1",
     Description = "Test1",
+    SubDirections = new List<SubDirection>()
+    {
+        new SubDirection()
+        {
+            Title = "Test1Sub",
+            Description = "Test1Sub",
+            InstitutionHierarchies = new List<InstitutionHierarchy>()
+            {
+                new InstitutionHierarchy()
+                {
+                    Id = Guid.NewGuid(),
+                    Title = "Title1",
+                    HierarchyLevel = 1,
+                    InstitutionId = new Guid("af475193-6a1e-4a75-9ba3-439c4300f771"),
+                },
+            },
+        }
+    }
 },

OutOfSchool/OutOfSchool.BusinessLogic/Util/MappingProfile.cs (2)

505-507: Consider adding XML documentation for Direction mapping.

The mapping configuration for Direction has been updated to ignore SubDirections. Consider adding XML documentation to explain the relationship between Direction and SubDirection entities, similar to the documentation provided for other mappings (e.g., lines 243-252).

+        /// <summary>
+        /// Maps Direction entity to DirectionDto.
+        /// Note: SubDirections are ignored in the reverse mapping as they are managed through the SubDirection entity.
+        /// </summary>
         CreateSoftDeletedMap<DirectionDto, Direction>()
             .ForMember(dest => dest.SubDirections, opt => opt.Ignore())
             .ForMember(c => c.UpdatedAt, m => m.Ignore());

---

493-494: Remove or address the warning comment.

The warning comment indicates that the mapping for ShortUserDto to AdminDto is temporary and needs to be removed or refactored. Consider creating a new issue to track this technical debt.

Would you like me to create a new issue to track the removal or refactoring of this temporary mapping?

OutOfSchool/OutOfSchool.BusinessLogic/Services/DirectionService.cs (5)

110-110: Typo: “succesfully” → “successfully.”
Please correct the misspelling to maintain professional logging output.

- logger.LogDebug("Direction with Id = {id} succesfully deleted.", id);
+ logger.LogDebug("Direction with Id = {id} successfully deleted.", id);

---

116-117: Reconsider concurrency exception usage.
Throwing a DbUpdateConcurrencyException for a missing record can be misleading. Consider a more descriptive custom exception or a Result failure to ensure clarity.

---

160-160: Redundant cast.
Casting g.Count() to int? is superfluous since Count() is already an int. Consider removing it unless needed for a future requirement.

- WorkshopsCount = g.Count() as int?,
+ WorkshopsCount = g.Count(),

---

230-232: Typo: “succesfully” → “successfully.”
Please fix the spelling to maintain consistency in logs.

- logger.LogDebug("Direction with Id = {id} updated succesfully.", direction?.Id);
+ logger.LogDebug("Direction with Id = {id} updated successfully.", direction?.Id);

---

287-287: Keep block style consistent.
The else { statement uses an inline brace, whereas other blocks use new lines. For readability, consider a consistent code style.

OutOfSchool/OutOfSchool.DataAccess/Models/Direction.cs (1)

24-24: Replacing InstitutionHierarchies with SubDirections.
This transition suggests a pivot in data modeling towards a more granular sub-direction structure. Ensure SubDirections is initialized (= new();) if you want to avoid null references.

- public virtual List<SubDirection> SubDirections { get; set; }
+ public virtual List<SubDirection> SubDirections { get; set; } = new();

OutOfSchool/OutOfSchool.DataAccess/Models/SubDirection.cs (1)

18-20: Description property usage.
The optional 500-length description is a practical limit, but consider validating user input to avoid large manual expansions.

OutOfSchool/OutOfSchool.DataAccess/Models/Configurations/SubDirectionConfiguration.cs (1)

1-23: LGTM! Consider adding XML documentation.

The entity configuration is well-structured with proper relationship definitions and performance considerations (index on IsDeleted). The use of DeleteBehavior.Restrict is appropriate to prevent unintended cascading deletes.

Consider adding XML documentation to describe the purpose of this configuration class and its relationships:

+/// <summary>
+/// Entity Framework configuration for the SubDirection entity.
+/// Configures relationships with Direction and InstitutionHierarchy entities.
+/// </summary>
 public class SubDirectionConfiguration : IEntityTypeConfiguration<SubDirection>

OutOfSchool/OutOfSchool.DataAccess/Models/SubordinationStructure/InstitutionHierarchy.cs (1)

30-30: Initialize the collection property.

The virtual collection property should be initialized to prevent potential null reference exceptions.

-    public virtual List<SubDirection> SubDirections { get; set; }
+    public virtual List<SubDirection> SubDirections { get; set; } = new();

OutOfSchool/Tests/OutOfSchool.Tests.Common/TestDataGenerators/SubDirectionsGenerator.cs (2)

9-11: Enhance test data generation rules.

The current implementation might benefit from additional rules and a larger Id range:

 private static readonly Faker<SubDirection> faker = new Faker<SubDirection>()
-    .RuleFor(x => x.Id, f => f.Random.Long(min: 1, max: 1_000_000))
+    .RuleFor(x => x.Id, f => f.Random.Long(min: 1, max: long.MaxValue))
+    .RuleFor(x => x.IsDeleted, f => f.Random.Bool(0.1f))  // 10% chance of being deleted
+    .RuleFor(x => x.Description, f => f.Lorem.Paragraph())
     .RuleFor(x => x.Title, f => f.Company.CompanyName());

---

15-16: Consider adding parallel generation for large datasets.

For large lists of direction IDs, consider using parallel processing:

 public static List<SubDirection> Generate(List<long> directionIds) =>
-    directionIds.Select(id => Generate(id)).ToList();
+    directionIds.AsParallel()
+        .Select(id => Generate(id))
+        .ToList();

OutOfSchool/Tests/OutOfSchool.Tests.Common/TestDataGenerators/InstitutionHierarchyGenerator.cs (1)

41-41: Consider adding null check and filtering deleted SubDirections.

The flattening operation might throw if any Direction in the list is null or if SubDirections is null. Additionally, consider filtering out deleted SubDirections if applicable.

-        hierarchy.SubDirections = directions.SelectMany(d => d.SubDirections).ToList();
+        hierarchy.SubDirections = directions
+            ?.Where(d => d != null)
+            .SelectMany(d => d.SubDirections?.Where(sd => !sd.IsDeleted) ?? Enumerable.Empty<SubDirection>())
+            .ToList() ?? [];

OutOfSchool/OutOfSchool.BusinessLogic/Util/Mapping/ExternalExportMappingProfile.cs (1)

82-82: Consider adding filtering for non-deleted Directions.

For consistency with the Workshop mapping, consider filtering out deleted Directions here as well.

-            .ForMember(dest => dest.DirectionIds, opt => opt.MapFrom(src => src.SubDirections.Select(x => x.DirectionId)));
+            .ForMember(dest => dest.DirectionIds, opt => opt.MapFrom(src => src.SubDirections.Where(x => !x.Direction.IsDeleted).Select(x => x.DirectionId)));

OutOfSchool/OutOfSchool.WebApi/Controllers/V1/DirectionController.cs (2)

130-152: Consider consolidating exception handling.

The exception handling could be simplified by combining the catch blocks since they perform the same action.

         try
         {
             directionDto.Id = default;
             var response = await service.Create(directionDto).ConfigureAwait(false);
             if (response != null && response.Succeeded)
             {
                 return CreatedAtAction(
                     nameof(GetById),
                     new { id = response.Value.Id, },
                     response.Value);
             }
             else
             {
                 return HandleServiceRespone(response);
             }
         }
-        catch (ArgumentException ex)
-        {
-            return BadRequest(ex.Message);
-        }
-        catch (Exception ex)
-        {
-            return BadRequest(ex.Message);
-        }
+        catch (Exception ex) when (ex is ArgumentException || ex is Exception)
+        {
+            return BadRequest(ex.Message);
+        }

---

154-174: Fix typo in method name.

The method name contains a typo: HandleServiceRespone should be HandleServiceResponse.

-    private IActionResult HandleServiceRespone<T>(Result<T> response)
+    private IActionResult HandleServiceResponse<T>(Result<T> response)

OutOfSchool/OutOfSchool.DataAccess/Repository/InstitutionHierarchyRepository.cs (1)

29-59: Consider moving business logic to a service layer.

The repository is currently handling complex business logic related to Direction-SubDirection relationships. This violates the Single Responsibility Principle and makes the code harder to maintain and test.

Consider:

1. Moving the Direction-SubDirection relationship logic to a dedicated service
2. Keeping the repository focused on basic CRUD operations
3. Using a Unit of Work pattern for transaction management

Example service structure:

public interface IInstitutionHierarchyService
{
    Task<InstitutionHierarchy> CreateWithDirections(InstitutionHierarchy entity, List<long> directionsIds);
    Task<InstitutionHierarchy> UpdateWithDirections(InstitutionHierarchy entity, List<long> directionsIds);
}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

OutOfSchool/OutOfSchool.WebApi.Tests/Controllers/DirectionControllerTests.cs (1)

181-198: Consider verifying the error message content.

While the test properly verifies the response type and status code, it would be more robust to also assert that the error description matches the expected message.

 // Assert
 Assert.That(result, Is.TypeOf<BadRequestObjectResult>());
 Assert.That(result.StatusCode, Is.EqualTo(400));
+var error = (result.Value as Result<DirectionDto>)?.Error;
+Assert.That(error?.Description, Is.EqualTo("There is already a Direction with such a data."));

OutOfSchool/OutOfSchool.WebApi.Tests/Services/DirectionServiceTests.cs (1)

223-225: Enhance test coverage for the SubDirection relationship.

While the query has been updated to use the new SubDirection relationship, consider adding assertions to verify:

1. The complete hierarchy (Direction -> SubDirection -> InstitutionHierarchy)
2. The correct mapping of SubDirection properties

 // Assert
 Assert.True(result.Entities.All(d => d.Title == expectedDto.Title));
+Assert.That(result.Entities.All(d => d.SubDirections != null && d.SubDirections.Any()));
+Assert.That(result.Entities.All(d => d.SubDirections.All(s => s.InstitutionHierarchies != null && s.InstitutionHierarchies.Any())));

Also applies to: 260-262

OutOfSchool/OutOfSchool.BusinessLogic/Services/DirectionService.cs (4)

58-74: LGTM! Consider updating XML documentation.

The changes improve error handling by using the Result pattern and collecting validation errors. The logging level change to Debug is appropriate for this context.

Update the XML documentation to reflect the new return type:

/// <summary>
/// Creates new direction.
/// </summary>
/// <param name="dto">Direction entity to add.</param>
-/// <returns>A <see cref="Task{TResult}"/> representing the result of the asynchronous operation.</returns>
+/// <returns>A <see cref="Task{Result{DirectionDto}}"/> representing the result of the asynchronous operation, containing either the created direction or validation errors.</returns>

---

148-154: Extract magic strings and improve query readability.

Consider extracting "SubDirections" as a constant and using method chaining for better readability.

+ private const string SubDirectionsInclude = "SubDirections";

- .Get(skip: filter.From, take: filter.Size, whereExpression: predicate, orderBy: sortExpression, includeProperties: "SubDirections")
+ .Get(
+     skip: filter.From,
+     take: filter.Size,
+     whereExpression: predicate,
+     orderBy: sortExpression,
+     includeProperties: SubDirectionsInclude)

---

235-251: Add input validation.

Consider validating the input dto before checking for duplicates.

private List<OperationError> DirectionValidation(DirectionDto dto)
{
    var errors = new List<OperationError>();

+   if (dto == null)
+   {
+       errors.Add(new OperationError
+       {
+           Code = "400",
+           Description = localizer["Direction data cannot be null."]
+       });
+       return errors;
+   }

+   if (string.IsNullOrWhiteSpace(dto.Title))
+   {
+       errors.Add(new OperationError
+       {
+           Code = "400",
+           Description = localizer["Direction title cannot be empty."]
+       });
+   }

    if (repository.Get(whereExpression: x => x.Title == dto.Title).Any())
    {
        // ... existing duplicate check ...
    }

    return errors;
}

---

272-283: Extract complex predicates into separate methods.

Consider extracting the institution hierarchy predicates into separate methods for better readability and maintainability.

+ private Expression<Func<Direction, bool>> BuildInstitutionHierarchyPredicate(long institutionId)
+ {
+     return PredicateBuilder.True<Direction>()
+         .And<Direction>(d => d.SubDirections
+         .Any(s => s.InstitutionHierarchies.Any(h => h.InstitutionId == institutionId)));
+ }

  if (currentUserService.IsMinistryAdmin())
  {
      var ministryAdmin = await ministryAdminService.GetByUserId(currentUserService.UserId);
-     predicate = predicate
-         .And<Direction>(d => d.SubDirections.
-         Any(s => s.InstitutionHierarchies.Any(h => h.InstitutionId == ministryAdmin.InstitutionId)));
+     predicate = predicate.And(BuildInstitutionHierarchyPredicate(ministryAdmin.InstitutionId));
      // ... rest of the code
  }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

OutOfSchool/OutOfSchool.BusinessLogic/Services/DirectionService.cs (3)

138-183: Add pagination parameter validation.

The method accepts pagination parameters (From, Size) but doesn't validate them. This could lead to performance issues with large page sizes.

Add validation at the beginning of the method:

 public async Task<SearchResult<DirectionDto>> GetByFilter(DirectionFilter filter, bool isAdmins)
 {
     logger.LogDebug("Getting Directions by filter started.");
 
+    if (filter.Size <= 0 || filter.Size > 100)  // Adjust max size as needed
+    {
+        filter.Size = 10;  // Default size
+    }
+
+    if (filter.From < 0)
+    {
+        filter.From = 0;
+    }
+
     var (predicate, workshopCountFilter) = await BuildPredicate(filter, isAdmins).ConfigureAwait(false);

---

239-255: Enhance validation rules.

The validation only checks for duplicate titles. Consider adding validations for:

• Required properties (Title)
• String length limits
• Invalid characters

Add additional validations:

 private List<OperationError> DirectionValidation(DirectionDto dto)
 {
     var errors = new List<OperationError>();
 
+    if (string.IsNullOrWhiteSpace(dto.Title))
+    {
+        errors.Add(new OperationError
+        {
+            Code = "400",
+            Description = localizer["Title is required."]
+        });
+    }
+    else if (dto.Title.Length > 100)  // Adjust max length as needed
+    {
+        errors.Add(new OperationError
+        {
+            Code = "400",
+            Description = localizer["Title exceeds maximum length."]
+        });
+    }
+
     if (repository.Get(whereExpression: x => x.Title == dto.Title).Any())
     {

---

261-266: Optimize string search performance.

The current implementation performs case-insensitive contains check on both Direction and SubDirection titles. Consider normalizing the search term and using an indexed column for better performance.

Optimize the search:

     if (!string.IsNullOrWhiteSpace(filter.Name))
     {
+        var searchTerm = filter.Name.Trim().ToUpperInvariant();
         predicate = predicate
-            .And(direction => direction.Title.Contains(filter.Name, StringComparison.InvariantCultureIgnoreCase) ||
-            direction.SubDirections.Any(s => s.Title.Contains(filter.Name, StringComparison.InvariantCultureIgnoreCase)));
+            .And(direction => direction.NormalizedTitle.Contains(searchTerm) ||
+            direction.SubDirections.Any(s => s.NormalizedTitle.Contains(searchTerm)));
     }

Note: This assumes you'll add a normalized (uppercase) title column to both Direction and SubDirection entities, which should be maintained at insert/update time.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro