From 2a9d4ce769417a1b67f395cb1a431787598c3515 Mon Sep 17 00:00:00 2001 From: Jonathan Idland Olsnes <73334350+Jonathanio123@users.noreply.github.com> Date: Thu, 5 Sep 2024 10:09:58 +0200 Subject: [PATCH] chore: Summary cleanup (#689) Transitioned to ProblemDetails, updated status codes for endpoints, removed todos, added some extra tests. Changed time trigger for weekly-department-recipients-sync to only run on monday instead of every day. --- src/Fusion.Summary.Api/BaseController.cs | 8 ++- .../Controllers/DepartmentsController.cs | 60 ++++++------------- .../Controllers/SummaryReportsController.cs | 31 ++++++---- .../Domain/Commands/PutWeeklySummaryReport.cs | 9 ++- .../Deployment/disabled-functions.json | 6 +- .../Functions/DepartmentResourceOwnerSync.cs | 2 +- .../WeeklyDepartmentSummarySender.cs | 2 +- .../IntegrationTests/SummaryReportTests.cs | 35 ++++++++++- 8 files changed, 85 insertions(+), 68 deletions(-) diff --git a/src/Fusion.Summary.Api/BaseController.cs b/src/Fusion.Summary.Api/BaseController.cs index d24c23bce..e99ab82b1 100644 --- a/src/Fusion.Summary.Api/BaseController.cs +++ b/src/Fusion.Summary.Api/BaseController.cs @@ -7,9 +7,11 @@ namespace Fusion.Summary.Api; public class BaseController : ControllerBase { - // TODO: Transition to ProblemDetails - protected NotFoundObjectResult DepartmentNotFound(string sapDepartmentId) => - NotFound(new { message = $"Department with id '{sapDepartmentId}' was not found" }); + protected ActionResult DepartmentNotFound(string sapDepartmentId) => + FusionApiError.NotFound(sapDepartmentId, $"Department with sap id '{sapDepartmentId}' was not found"); + + protected ActionResult SapDepartmentIdRequired() => + FusionApiError.InvalidOperation("SapDepartmentIdRequired", "SapDepartmentId route parameter is required"); protected Task DispatchAsync(IRequest command) diff --git a/src/Fusion.Summary.Api/Controllers/DepartmentsController.cs b/src/Fusion.Summary.Api/Controllers/DepartmentsController.cs index fc96419cc..7a55f1c2f 100644 --- a/src/Fusion.Summary.Api/Controllers/DepartmentsController.cs +++ b/src/Fusion.Summary.Api/Controllers/DepartmentsController.cs @@ -6,29 +6,22 @@ using Fusion.Summary.Api.Controllers.Requests; using Fusion.Summary.Api.Domain.Commands; using Fusion.Summary.Api.Domain.Queries; +using Microsoft.ApplicationInsights.AspNetCore.Extensions; using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Mvc; +using HttpRequestExtensions = Microsoft.ApplicationInsights.AspNetCore.Extensions.HttpRequestExtensions; namespace Fusion.Summary.Api.Controllers; -/// -/// TODO: Add summary -/// [ApiVersion("1.0")] [Authorize] [ApiController] public class DepartmentsController : BaseController { - /// - /// TODO: Add summary - /// [HttpGet("departments")] [MapToApiVersion("1.0")] - [ProducesResponseType(typeof(ApiDepartment[]), StatusCodes.Status200OK)] - [ProducesResponseType(StatusCodes.Status401Unauthorized)] - [ProducesResponseType(StatusCodes.Status403Forbidden)] - [ProducesResponseType(StatusCodes.Status404NotFound)] - public async Task GetDepartmentsV1() + [ProducesResponseType(StatusCodes.Status200OK)] + public async Task> GetDepartmentsV1() { #region Authorization @@ -43,31 +36,19 @@ public async Task GetDepartmentsV1() #endregion Authorization - var ret = new List(); + var departments = await DispatchAsync(new GetAllDepartments()); - // Query - var departments = (await DispatchAsync(new GetAllDepartments())).ToArray(); - - if (departments.Length == 0) - return NotFound(); - else - { - foreach (var d in departments) ret.Add(ApiDepartment.FromQueryDepartment(d)); - } + var apiDepartments = departments.Select(ApiDepartment.FromQueryDepartment); - return Ok(ret); + return Ok(apiDepartments); } - /// - /// TODO: Add summary - /// [HttpGet("departments/{sapDepartmentId}")] [MapToApiVersion("1.0")] - [ProducesResponseType(typeof(ApiDepartment), StatusCodes.Status200OK)] - [ProducesResponseType(StatusCodes.Status401Unauthorized)] - [ProducesResponseType(StatusCodes.Status403Forbidden)] + [ProducesResponseType(StatusCodes.Status200OK)] + [ProducesResponseType(StatusCodes.Status400BadRequest)] [ProducesResponseType(StatusCodes.Status404NotFound)] - public async Task GetDepartmentV1(string sapDepartmentId) + public async Task> GetDepartmentV1(string sapDepartmentId) { #region Authorization @@ -83,7 +64,7 @@ public async Task GetDepartmentV1(string sapDepartmentId) #endregion Authorization if (string.IsNullOrWhiteSpace(sapDepartmentId)) - return BadRequest("SapDepartmentId route parameter is required"); + return SapDepartmentIdRequired(); var department = await DispatchAsync(new GetDepartment(sapDepartmentId)); @@ -94,15 +75,12 @@ public async Task GetDepartmentV1(string sapDepartmentId) return Ok(ApiDepartment.FromQueryDepartment(department)); } - /// - /// TODO: Add summary - /// + [HttpPut("departments/{sapDepartmentId}")] [MapToApiVersion("1.0")] - [ProducesResponseType(StatusCodes.Status200OK)] [ProducesResponseType(StatusCodes.Status201Created)] - [ProducesResponseType(StatusCodes.Status401Unauthorized)] - [ProducesResponseType(StatusCodes.Status403Forbidden)] + [ProducesResponseType(StatusCodes.Status204NoContent)] + [ProducesResponseType(StatusCodes.Status400BadRequest)] [ProducesResponseType(StatusCodes.Status404NotFound)] public async Task PutV1(string sapDepartmentId, [FromBody] PutDepartmentRequest request) { @@ -120,7 +98,7 @@ public async Task PutV1(string sapDepartmentId, [FromBody] PutDep #endregion Authorization if (string.IsNullOrWhiteSpace(sapDepartmentId)) - return BadRequest("SapDepartmentId route parameter is required"); + return SapDepartmentIdRequired(); var personIdentifiers = request.ResourceOwnersAzureUniqueId .Concat(request.DelegateResourceOwnersAzureUniqueId) @@ -131,7 +109,7 @@ public async Task PutV1(string sapDepartmentId, [FromBody] PutDep .ToList(); if (unresolvedProfiles.Count != 0) - return BadRequest($"Profiles: {string.Join(',', unresolvedProfiles)} could not be resolved"); + return FusionApiError.NotFound(string.Join(',', unresolvedProfiles), "Profiles could not be resolved"); var department = await DispatchAsync(new GetDepartment(sapDepartmentId)); @@ -146,7 +124,7 @@ await DispatchAsync( request.ResourceOwnersAzureUniqueId, request.DelegateResourceOwnersAzureUniqueId)); - return Created(); + return Created(Request.GetUri(), null); } // Check if department owners has changed @@ -160,10 +138,10 @@ await DispatchAsync( request.ResourceOwnersAzureUniqueId, request.DelegateResourceOwnersAzureUniqueId)); - return Ok(); + return NoContent(); } // No change - return Ok(); + return NoContent(); } } \ No newline at end of file diff --git a/src/Fusion.Summary.Api/Controllers/SummaryReportsController.cs b/src/Fusion.Summary.Api/Controllers/SummaryReportsController.cs index ea3d866d2..625cac547 100644 --- a/src/Fusion.Summary.Api/Controllers/SummaryReportsController.cs +++ b/src/Fusion.Summary.Api/Controllers/SummaryReportsController.cs @@ -1,4 +1,5 @@ using System.Net.Mime; +using Fusion.AspNetCore.Api; using Fusion.AspNetCore.FluentAuthorization; using Fusion.AspNetCore.OData; using Fusion.Authorization; @@ -7,6 +8,7 @@ using Fusion.Summary.Api.Controllers.Requests; using Fusion.Summary.Api.Domain.Commands; using Fusion.Summary.Api.Domain.Queries; +using Microsoft.ApplicationInsights.AspNetCore.Extensions; using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Mvc; @@ -18,14 +20,13 @@ namespace Fusion.Summary.Api.Controllers; public class SummaryReportsController : BaseController { [HttpGet("resource-owners-summary-reports/{sapDepartmentId}/weekly")] - [Produces(MediaTypeNames.Application.Json)] + [MapToApiVersion("1.0")] [ProducesResponseType(StatusCodes.Status200OK)] - [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status400BadRequest)] - [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)] + [ProducesResponseType(StatusCodes.Status400BadRequest)] + [ProducesResponseType(StatusCodes.Status404NotFound)] [ODataFilter(nameof(ApiWeeklySummaryReport.Period))] [ODataOrderBy(nameof(ApiWeeklySummaryReport.Period), nameof(ApiWeeklySummaryReport.Id))] [ODataTop(100), ODataSkip] - [ApiVersion("1.0")] public async Task>> GetWeeklySummaryReportsV1( [FromRoute] string sapDepartmentId, ODataQueryParams query) { @@ -44,7 +45,7 @@ await Request.RequireAuthorizationAsync(r => #endregion if (string.IsNullOrWhiteSpace(sapDepartmentId)) - return BadRequest("SapDepartmentId route parameter is required"); + return SapDepartmentIdRequired(); if (await DispatchAsync(new GetDepartment(sapDepartmentId)) is null) return DepartmentNotFound(sapDepartmentId); @@ -55,12 +56,16 @@ await Request.RequireAuthorizationAsync(r => .FromQueryCollection(queryReports, ApiWeeklySummaryReport.FromQuerySummaryReport)); } + /// + /// Summary report key is composed of the department sap id and the period date. + /// If a report already exists for the given period, it will be replaced. + /// [HttpPut("resource-owners-summary-reports/{sapDepartmentId}/weekly")] - [Produces(MediaTypeNames.Application.Json)] + [MapToApiVersion("1.0")] + [ProducesResponseType(StatusCodes.Status201Created)] [ProducesResponseType(StatusCodes.Status204NoContent)] - [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status400BadRequest)] - [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)] - [ApiVersion("1.0")] + [ProducesResponseType(StatusCodes.Status400BadRequest)] + [ProducesResponseType(StatusCodes.Status404NotFound)] public async Task PutWeeklySummaryReportV1([FromRoute] string sapDepartmentId, [FromBody] PutWeeklySummaryReportRequest request) { @@ -79,18 +84,18 @@ await Request.RequireAuthorizationAsync(r => #endregion if (string.IsNullOrWhiteSpace(sapDepartmentId)) - return BadRequest("SapDepartmentId route parameter is required"); + return SapDepartmentIdRequired(); if (await DispatchAsync(new GetDepartment(sapDepartmentId)) is null) return DepartmentNotFound(sapDepartmentId); if (request.Period.DayOfWeek != DayOfWeek.Monday) - return BadRequest("Period date must be the first day of the week"); + return FusionApiError.InvalidOperation("InvalidPeriod", "Weekly summary report period date must be a monday"); var command = new PutWeeklySummaryReport(sapDepartmentId, request); - await DispatchAsync(command); + var newReportCreated = await DispatchAsync(command); - return NoContent(); + return newReportCreated ? Created(Request.GetUri(), null) : NoContent(); } } \ No newline at end of file diff --git a/src/Fusion.Summary.Api/Domain/Commands/PutWeeklySummaryReport.cs b/src/Fusion.Summary.Api/Domain/Commands/PutWeeklySummaryReport.cs index 4ab6df297..116d6c11d 100644 --- a/src/Fusion.Summary.Api/Domain/Commands/PutWeeklySummaryReport.cs +++ b/src/Fusion.Summary.Api/Domain/Commands/PutWeeklySummaryReport.cs @@ -6,7 +6,7 @@ namespace Fusion.Summary.Api.Domain.Commands; -public class PutWeeklySummaryReport : IRequest +public class PutWeeklySummaryReport : IRequest { public string SapDepartmentId { get; private set; } @@ -20,7 +20,7 @@ public PutWeeklySummaryReport(string sapDepartmentId, PutWeeklySummaryReportRequ } - public class Handler : IRequestHandler + public class Handler : IRequestHandler { private readonly SummaryDbContext _dbContext; @@ -29,7 +29,7 @@ public Handler(SummaryDbContext dbContext) _dbContext = dbContext; } - public async Task Handle(PutWeeklySummaryReport request, CancellationToken cancellationToken) + public async Task Handle(PutWeeklySummaryReport request, CancellationToken cancellationToken) { if (!await _dbContext.Departments.AnyAsync(d => d.DepartmentSapId == request.SapDepartmentId, cancellationToken: cancellationToken)) @@ -83,6 +83,9 @@ public async Task Handle(PutWeeklySummaryReport request, CancellationToken cance _dbContext.WeeklySummaryReports.Add(dbSummaryReport); await _dbContext.SaveChangesAsync(cancellationToken); + + // return true if a new report was created + return existingReport is null; } } } \ No newline at end of file diff --git a/src/Fusion.Summary.Functions/Deployment/disabled-functions.json b/src/Fusion.Summary.Functions/Deployment/disabled-functions.json index 7aeeda537..3c70da8da 100644 --- a/src/Fusion.Summary.Functions/Deployment/disabled-functions.json +++ b/src/Fusion.Summary.Functions/Deployment/disabled-functions.json @@ -1,10 +1,6 @@ [ { "environment": "pr", - "disabledFunctions": [ - "weekly-department-recipients-sync", - "weekly-department-summary-sender", - "weekly-department-summary-worker" - ] + "disabledFunctions": [] } ] diff --git a/src/Fusion.Summary.Functions/Functions/DepartmentResourceOwnerSync.cs b/src/Fusion.Summary.Functions/Functions/DepartmentResourceOwnerSync.cs index 7055020f8..8653fe2ad 100644 --- a/src/Fusion.Summary.Functions/Functions/DepartmentResourceOwnerSync.cs +++ b/src/Fusion.Summary.Functions/Functions/DepartmentResourceOwnerSync.cs @@ -47,7 +47,7 @@ public DepartmentResourceOwnerSync( /// [FunctionName("weekly-department-recipients-sync")] public async Task RunAsync( - [TimerTrigger("0 05 00 * * *", RunOnStartup = false)] + [TimerTrigger("0 05 00 * * MON", RunOnStartup = false)] TimerInfo timerInfo, CancellationToken cancellationToken ) { diff --git a/src/Fusion.Summary.Functions/Functions/WeeklyDepartmentSummarySender.cs b/src/Fusion.Summary.Functions/Functions/WeeklyDepartmentSummarySender.cs index 6e23e5ec0..39e46ed43 100644 --- a/src/Fusion.Summary.Functions/Functions/WeeklyDepartmentSummarySender.cs +++ b/src/Fusion.Summary.Functions/Functions/WeeklyDepartmentSummarySender.cs @@ -33,7 +33,7 @@ public WeeklyDepartmentSummarySender(ISummaryApiClient summaryApiClient, INotifi } [FunctionName("weekly-department-summary-sender")] - public async Task RunAsync([TimerTrigger("0 0 8 * * 1", RunOnStartup = false)] TimerInfo timerInfo) + public async Task RunAsync([TimerTrigger("0 0 8 * * MON", RunOnStartup = false)] TimerInfo timerInfo) { var departments = await summaryApiClient.GetDepartmentsAsync(); diff --git a/src/tests/Fusion.Summary.Api.Tests/IntegrationTests/SummaryReportTests.cs b/src/tests/Fusion.Summary.Api.Tests/IntegrationTests/SummaryReportTests.cs index 69f5b3c66..6d9c60c53 100644 --- a/src/tests/Fusion.Summary.Api.Tests/IntegrationTests/SummaryReportTests.cs +++ b/src/tests/Fusion.Summary.Api.Tests/IntegrationTests/SummaryReportTests.cs @@ -42,11 +42,44 @@ public async Task PutAndGetWeeklySummaryReport_ShouldReturnReport() var response = await _client.PutWeeklySummaryReportAsync(department.DepartmentSapId); - response.Should().BeNoContent(); + response.Should().BeCreated(); var getResponse = await _client.GetWeeklySummaryReportsAsync(department.DepartmentSapId); getResponse.Should().BeSuccessfull(); getResponse.Value!.Items.Should().HaveCount(1); } + [Fact] + public async Task PutAndUpdateWeeklySummaryReport_ShouldReturnNoContent() + { + using var adminScope = _fixture.AdminScope(); + var testUser = _fixture.Fusion.CreateUser().AsEmployee().AzureUniqueId!.Value; + + var department = await _client.PutDepartmentAsync(testUser); + + var response = await _client.PutWeeklySummaryReportAsync(department.DepartmentSapId); + response.Should().BeCreated(); + + response = await _client.PutWeeklySummaryReportAsync(department.DepartmentSapId); + response.Should().BeNoContent(); + } + + [Fact] + public async Task PutWeeklySummaryReport_WithInvalidPeriodDate_ShouldReturnBadRequest() + { + using var adminScope = _fixture.AdminScope(); + var testUser = _fixture.Fusion.CreateUser().AsEmployee().AzureUniqueId!.Value; + + var department = await _client.PutDepartmentAsync(testUser); + + var response = await _client.PutWeeklySummaryReportAsync(department.DepartmentSapId, (report) => + { + var nowDate = DateTime.UtcNow; + if (nowDate.DayOfWeek == DayOfWeek.Monday) + report.Period = nowDate.AddDays(1); + report.Period = nowDate; + }); + + response.Should().BeBadRequest(); + } } \ No newline at end of file