From 9ca88d1069ef820cfb383cd4d6434951c3cc980e Mon Sep 17 00:00:00 2001 From: Jim Washbrook Date: Wed, 28 Aug 2024 11:09:22 +0100 Subject: [PATCH 1/3] feat(content): Add error handling + logging to content route --- .../Controllers/ContentController.cs | 40 ++++++++++++++----- .../Controllers/ContentControllerTests.cs | 3 +- 2 files changed, 31 insertions(+), 12 deletions(-) diff --git a/src/Dfe.ContentSupport.Web/Controllers/ContentController.cs b/src/Dfe.ContentSupport.Web/Controllers/ContentController.cs index 210f61f..039f5e4 100644 --- a/src/Dfe.ContentSupport.Web/Controllers/ContentController.cs +++ b/src/Dfe.ContentSupport.Web/Controllers/ContentController.cs @@ -9,7 +9,7 @@ namespace Dfe.ContentSupport.Web.Controllers; [Route("/content")] [AllowAnonymous] -public class ContentController(IContentService contentService, ILayoutService layoutService) +public class ContentController(IContentService contentService, ILayoutService layoutService, ILogger logger) : Controller { public async Task Home() @@ -28,23 +28,41 @@ public async Task Home() return View(defaultModel); } - - [HttpGet("{slug}/{page?}")] public async Task Index(string slug, string page = "", bool isPreview = false, [FromQuery] List? tags = null) { - if (!ModelState.IsValid) return RedirectToAction("error"); - if (string.IsNullOrEmpty(slug)) return RedirectToAction("error"); + if (!ModelState.IsValid) + { + logger.LogError("Invalid model state received for {Slug}", slug); + return RedirectToAction("error"); + } - var resp = await contentService.GetContent(slug, isPreview); - if (resp is null) return RedirectToAction("error"); + if (string.IsNullOrEmpty(slug)) + { + logger.LogError("No slug received for C&S {Controller} {Action} ", nameof(ContentController), nameof(Index)); + return RedirectToAction("error"); + } - resp = layoutService.GenerateLayout(resp, Request, page); - ViewBag.tags = tags; + try + { + var resp = await contentService.GetContent(slug, isPreview); + if (resp is null) + { + logger.LogError("Failed to load content for C&S page {Slug}; no content received.", slug); + return RedirectToAction("error"); + } - return View("CsIndex", resp); - } + resp = layoutService.GenerateLayout(resp, Request, page); + ViewBag.tags = tags; + return View("CsIndex", resp); + } + catch (Exception ex) + { + logger.LogError(ex, "Error loading C&S content page {Slug}", slug); + return RedirectToAction("error"); + } + } public IActionResult Privacy() { diff --git a/tests/Dfe.ContentSupport.Web.Tests/Controllers/ContentControllerTests.cs b/tests/Dfe.ContentSupport.Web.Tests/Controllers/ContentControllerTests.cs index eb5f3de..70263bb 100644 --- a/tests/Dfe.ContentSupport.Web.Tests/Controllers/ContentControllerTests.cs +++ b/tests/Dfe.ContentSupport.Web.Tests/Controllers/ContentControllerTests.cs @@ -2,6 +2,7 @@ using Dfe.ContentSupport.Web.Models.Mapped; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; +using Microsoft.Extensions.Logging.Abstractions; namespace Dfe.ContentSupport.Web.Tests.Controllers; @@ -11,7 +12,7 @@ public class ContentControllerTests private ContentController GetController() { - return new ContentController(_contentServiceMock.Object, new LayoutService()); + return new ContentController(_contentServiceMock.Object, new LayoutService(), new NullLogger()); } From af09d71ba4895be9bfcb6e27cba25c8c290f9fe0 Mon Sep 17 00:00:00 2001 From: Jim Washbrook Date: Wed, 28 Aug 2024 11:25:55 +0100 Subject: [PATCH 2/3] tests(content): Test missing slug message tests(content): Add unit tests for logging messages tests(content): Additional logging tests chore: Remove whitespace from end --- .../Controllers/ContentController.cs | 4 +- .../Controllers/ContentControllerTests.cs | 48 +++++++++++++++++-- .../Dfe.ContentSupport.Web.Tests.csproj | 17 +++---- 3 files changed, 56 insertions(+), 13 deletions(-) diff --git a/src/Dfe.ContentSupport.Web/Controllers/ContentController.cs b/src/Dfe.ContentSupport.Web/Controllers/ContentController.cs index 039f5e4..57dc70c 100644 --- a/src/Dfe.ContentSupport.Web/Controllers/ContentController.cs +++ b/src/Dfe.ContentSupport.Web/Controllers/ContentController.cs @@ -33,13 +33,13 @@ public async Task Index(string slug, string page = "", bool isPre { if (!ModelState.IsValid) { - logger.LogError("Invalid model state received for {Slug}", slug); + logger.LogError("Invalid model state received for {Controller} {Action} with slug {Slug}", nameof(ContentController), nameof(Index), slug); return RedirectToAction("error"); } if (string.IsNullOrEmpty(slug)) { - logger.LogError("No slug received for C&S {Controller} {Action} ", nameof(ContentController), nameof(Index)); + logger.LogError("No slug received for C&S {Controller} {Action}", nameof(ContentController), nameof(Index)); return RedirectToAction("error"); } diff --git a/tests/Dfe.ContentSupport.Web.Tests/Controllers/ContentControllerTests.cs b/tests/Dfe.ContentSupport.Web.Tests/Controllers/ContentControllerTests.cs index 70263bb..04b5d40 100644 --- a/tests/Dfe.ContentSupport.Web.Tests/Controllers/ContentControllerTests.cs +++ b/tests/Dfe.ContentSupport.Web.Tests/Controllers/ContentControllerTests.cs @@ -2,17 +2,18 @@ using Dfe.ContentSupport.Web.Models.Mapped; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; -using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Logging; namespace Dfe.ContentSupport.Web.Tests.Controllers; public class ContentControllerTests { + private readonly Mock> _loggerMock = new(); private readonly Mock _contentServiceMock = new(); private ContentController GetController() { - return new ContentController(_contentServiceMock.Object, new LayoutService(), new NullLogger()); + return new ContentController(_contentServiceMock.Object, new LayoutService(), _loggerMock.Object); } @@ -37,6 +38,15 @@ public async Task Index_NoSlug_Returns_ErrorAction() result.Should().BeOfType(); (result as RedirectToActionResult)!.ActionName.Should().BeEquivalentTo("error"); + + _loggerMock.Verify( + x => x.Log( + LogLevel.Error, + It.IsAny(), + It.Is((o, t) => o.ToString() != null && o.ToString()!.StartsWith($"No slug received for C&S {nameof(ContentController)} {nameof(sut.Index)}")), + It.IsAny(), + It.IsAny>()), + Times.Once); } [Fact] @@ -54,15 +64,47 @@ public async Task Index_Calls_Service_GetContent() [Fact] public async Task Index_NullResponse_ReturnsErrorAction() { + var slug = "slug"; _contentServiceMock.Setup(o => o.GetContent(It.IsAny(), It.IsAny())) .ReturnsAsync((CsPage?)null); var sut = GetController(); - var result = await sut.Index("slug"); + var result = await sut.Index(slug); + + result.Should().BeOfType(); + (result as RedirectToActionResult)!.ActionName.Should().BeEquivalentTo("error"); + + _loggerMock.Verify(x => x.Log( + LogLevel.Error, + It.IsAny(), + It.Is((o, t) => o.ToString() != null && o.ToString()!.Equals($"Failed to load content for C&S page {slug}; no content received.")), + It.IsAny(), + It.IsAny>()), + Times.Once); + } + + [Fact] + public async Task Index_ExceptionThrown_ReturnsErrorAction() + { + var slug = "slug"; + _contentServiceMock.Setup(o => o.GetContent(It.IsAny(), It.IsAny())) + .ThrowsAsync(new Exception("An exception occurred loading content")); + + var sut = GetController(); + + var result = await sut.Index(slug); result.Should().BeOfType(); (result as RedirectToActionResult)!.ActionName.Should().BeEquivalentTo("error"); + + _loggerMock.Verify(x => x.Log( + LogLevel.Error, + It.IsAny(), + It.Is((o, t) => o.ToString() != null && o.ToString()!.Equals($"Error loading C&S content page {slug}")), + It.IsAny(), + It.IsAny>()), + Times.Once); } [Fact] diff --git a/tests/Dfe.ContentSupport.Web.Tests/Dfe.ContentSupport.Web.Tests.csproj b/tests/Dfe.ContentSupport.Web.Tests/Dfe.ContentSupport.Web.Tests.csproj index 3121e2b..e9ee0bd 100644 --- a/tests/Dfe.ContentSupport.Web.Tests/Dfe.ContentSupport.Web.Tests.csproj +++ b/tests/Dfe.ContentSupport.Web.Tests/Dfe.ContentSupport.Web.Tests.csproj @@ -11,20 +11,21 @@ - - - - - - + + + + + + + - + - + From 43fca655bf834595cf298e85b0822df5184f666a Mon Sep 17 00:00:00 2001 From: Jim Washbrook Date: Wed, 28 Aug 2024 11:31:54 +0100 Subject: [PATCH 3/3] chore: Use constant for error action name --- .../Controllers/ContentController.cs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/Dfe.ContentSupport.Web/Controllers/ContentController.cs b/src/Dfe.ContentSupport.Web/Controllers/ContentController.cs index 57dc70c..95b682b 100644 --- a/src/Dfe.ContentSupport.Web/Controllers/ContentController.cs +++ b/src/Dfe.ContentSupport.Web/Controllers/ContentController.cs @@ -12,6 +12,8 @@ namespace Dfe.ContentSupport.Web.Controllers; public class ContentController(IContentService contentService, ILayoutService layoutService, ILogger logger) : Controller { + public const string ErrorActionName = "error"; + public async Task Home() { var defaultModel = new CsPage @@ -34,13 +36,13 @@ public async Task Index(string slug, string page = "", bool isPre if (!ModelState.IsValid) { logger.LogError("Invalid model state received for {Controller} {Action} with slug {Slug}", nameof(ContentController), nameof(Index), slug); - return RedirectToAction("error"); + return RedirectToAction(ErrorActionName); } if (string.IsNullOrEmpty(slug)) { logger.LogError("No slug received for C&S {Controller} {Action}", nameof(ContentController), nameof(Index)); - return RedirectToAction("error"); + return RedirectToAction(ErrorActionName); } try @@ -49,7 +51,7 @@ public async Task Index(string slug, string page = "", bool isPre if (resp is null) { logger.LogError("Failed to load content for C&S page {Slug}; no content received.", slug); - return RedirectToAction("error"); + return RedirectToAction(ErrorActionName); } resp = layoutService.GenerateLayout(resp, Request, page); @@ -60,7 +62,7 @@ public async Task Index(string slug, string page = "", bool isPre catch (Exception ex) { logger.LogError(ex, "Error loading C&S content page {Slug}", slug); - return RedirectToAction("error"); + return RedirectToAction(ErrorActionName); } }