From 8839a6e2fe5ae917edf7381fb4c669dffa33055f Mon Sep 17 00:00:00 2001 From: RobertGHippo Date: Wed, 3 Jul 2024 16:27:08 +0100 Subject: [PATCH 1/7] These values are known, and don't originate from format info. Use dictionary key, rather than looking up value and getting key. --- .../ContentfulContentFilterService.cs | 35 ++++++++++--------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/src/Dfe.EarlyYearsQualification.Content/Services/ContentfulContentFilterService.cs b/src/Dfe.EarlyYearsQualification.Content/Services/ContentfulContentFilterService.cs index b84a1fb3..ab3d717c 100644 --- a/src/Dfe.EarlyYearsQualification.Content/Services/ContentfulContentFilterService.cs +++ b/src/Dfe.EarlyYearsQualification.Content/Services/ContentfulContentFilterService.cs @@ -1,5 +1,4 @@ using System.Collections.ObjectModel; -using System.Globalization; using Contentful.Core; using Contentful.Core.Models; using Contentful.Core.Search; @@ -15,24 +14,23 @@ public class ContentfulContentFilterService( : IContentFilterService { private const int Day = 28; - private static readonly DateTimeFormatInfo CurrentFormatInfo = CultureInfo.CurrentCulture.DateTimeFormat; - private readonly ReadOnlyDictionary + private readonly ReadOnlyDictionary _months = new( - new Dictionary + new Dictionary { - { 1, CurrentFormatInfo.AbbreviatedMonthNames[0] }, - { 2, CurrentFormatInfo.AbbreviatedMonthNames[1] }, - { 3, CurrentFormatInfo.AbbreviatedMonthNames[2] }, - { 4, CurrentFormatInfo.AbbreviatedMonthNames[3] }, - { 5, CurrentFormatInfo.AbbreviatedMonthNames[4] }, - { 6, CurrentFormatInfo.AbbreviatedMonthNames[5] }, - { 7, CurrentFormatInfo.AbbreviatedMonthNames[6] }, - { 8, CurrentFormatInfo.AbbreviatedMonthNames[7] }, - { 9, CurrentFormatInfo.AbbreviatedMonthNames[8] }, - { 10, CurrentFormatInfo.AbbreviatedMonthNames[9] }, - { 11, CurrentFormatInfo.AbbreviatedMonthNames[10] }, - { 12, CurrentFormatInfo.AbbreviatedMonthNames[11] } + { "Jan", 1 }, + { "Feb", 2 }, + { "Mar", 3 }, + { "Apr", 4 }, + { "May", 5 }, + { "Jun", 6 }, + { "Jul", 7 }, + { "Aug", 8 }, + { "Sep", 9 }, + { "Oct", 10 }, + { "Nov", 11 }, + { "Dec", 12 } }); // Used by the unit tests to inject a mock builder that returns the query params @@ -94,6 +92,7 @@ private List FilterQualificationsByDate(int startDateMonth, int s } else if (qualificationStartDate is null && qualificationEndDate is not null + // ReSharper disable once MergeSequentialChecks && enteredStartDate <= qualificationEndDate) { // if qualification start date is null, check entered start date is <= ToWhichYear & add to results @@ -127,7 +126,9 @@ private List FilterQualificationsByDate(int startDateMonth, int s var splitQualificationDate = qualificationDate.Split('-'); if (splitQualificationDate.Length != 2) return null; - var month = _months.FirstOrDefault(x => x.Value == splitQualificationDate[0]).Key; + var abbreviatedMonth = splitQualificationDate[0]; + + var month = _months[abbreviatedMonth]; var year = Convert.ToInt32(splitQualificationDate[1]) + 2000; return new DateOnly(year, month, Day); From 1fa27c1cb9e51b6c5b603273d9d083159a08a8fb Mon Sep 17 00:00:00 2001 From: RobertGHippo Date: Wed, 3 Jul 2024 16:30:25 +0100 Subject: [PATCH 2/7] Expected date format in data is MMM-yy. If any EYQL date is not in this format, log an error. --- .../ContentfulContentFilterService.cs | 44 ++++++++++- .../ContentfulContentFilterServiceTests.cs | 75 +++++++++++++++++++ 2 files changed, 115 insertions(+), 4 deletions(-) diff --git a/src/Dfe.EarlyYearsQualification.Content/Services/ContentfulContentFilterService.cs b/src/Dfe.EarlyYearsQualification.Content/Services/ContentfulContentFilterService.cs index ab3d717c..0ea56bd8 100644 --- a/src/Dfe.EarlyYearsQualification.Content/Services/ContentfulContentFilterService.cs +++ b/src/Dfe.EarlyYearsQualification.Content/Services/ContentfulContentFilterService.cs @@ -1,4 +1,5 @@ using System.Collections.ObjectModel; +using System.Globalization; using Contentful.Core; using Contentful.Core.Models; using Contentful.Core.Search; @@ -122,15 +123,50 @@ private List FilterQualificationsByDate(int startDateMonth, int s } private DateOnly? ConvertToDateTime(string qualificationDate) + { + var (isValid, month, yearMod2000) = ValidateQualificationDate(qualificationDate); + + if (!isValid) + { + return null; + } + + var year = yearMod2000 + 2000; + + return new DateOnly(year, month, Day); + } + + private (bool isValid, int month, int yearMod2000) ValidateQualificationDate(string qualificationDate) { var splitQualificationDate = qualificationDate.Split('-'); - if (splitQualificationDate.Length != 2) return null; + if (splitQualificationDate.Length != 2) + { + logger.LogError("Found qualification date {QualificationDate} with unexpected format", qualificationDate); + return (false, 0, 0); + } var abbreviatedMonth = splitQualificationDate[0]; + var yearFilter = splitQualificationDate[1]; - var month = _months[abbreviatedMonth]; - var year = Convert.ToInt32(splitQualificationDate[1]) + 2000; + var yearIsValid = int.TryParse(yearFilter, + NumberStyles.Integer, + NumberFormatInfo.InvariantInfo, + out var yearPart); - return new DateOnly(year, month, Day); + if (!yearIsValid) + { + logger.LogError("Qualification date {QualificationDate} contains unexpected year value", qualificationDate); + return (false, 0, 0); + } + + if (!_months.TryGetValue(abbreviatedMonth, out var month)) + { + logger.LogError("Qualification date {QualificationDate} contains unexpected month value", + qualificationDate); + + return (false, 0, 0); + } + + return (true, month, yearPart); } } \ No newline at end of file diff --git a/tests/Dfe.EarlyYearsQualification.UnitTests/Services/ContentfulContentFilterServiceTests.cs b/tests/Dfe.EarlyYearsQualification.UnitTests/Services/ContentfulContentFilterServiceTests.cs index 8ebed119..55c2ae94 100644 --- a/tests/Dfe.EarlyYearsQualification.UnitTests/Services/ContentfulContentFilterServiceTests.cs +++ b/tests/Dfe.EarlyYearsQualification.UnitTests/Services/ContentfulContentFilterServiceTests.cs @@ -3,6 +3,7 @@ using Contentful.Core.Search; using Dfe.EarlyYearsQualification.Content.Entities; using Dfe.EarlyYearsQualification.Content.Services; +using Dfe.EarlyYearsQualification.UnitTests.Extensions; using FluentAssertions; using Microsoft.Extensions.Logging; using Moq; @@ -301,6 +302,80 @@ public async Task GetFilteredQualifications_ContentfulClientThrowsException_Retu filteredQualifications.Should().NotBeNull(); filteredQualifications.Should().BeEmpty(); } + + [TestMethod] + public async Task GetFilteredQualifications_DataContainsInvalidMonth_LogsError() + { + var results = new ContentfulCollection + { + Items = new[] + { + new Qualification( + "EYQ-123", + "test", + "NCFE", + 4, + "Sept-15", // "Sept" in the data should be "Sep" + "Aug-19", + "abc/123/987", + "requirements") + } + }; + + var mockContentfulClient = new Mock(); + mockContentfulClient.Setup(x => x.GetEntries( + It.IsAny>(), + It.IsAny())) + .ReturnsAsync(results); + + var mockQueryBuilder = new MockQueryBuilder(); + var mockLogger = new Mock>(); + var filterService = new ContentfulContentFilterService(mockContentfulClient.Object, mockLogger.Object) + { + QueryBuilder = mockQueryBuilder + }; + + await filterService.GetFilteredQualifications(4, 5, 2016); + + mockLogger.VerifyError("Qualification date Sept-15 contains unexpected month value"); + } + + [TestMethod] + public async Task GetFilteredQualifications_DataContainsInvalidYear_LogsError() + { + var results = new ContentfulCollection + { + Items = new[] + { + new Qualification( + "EYQ-123", + "test", + "NCFE", + 4, + "Sep-15", // "Sept" in the data should be "Sep" + "Aug-1a", + "abc/123/987", + "requirements") + } + }; + + var mockContentfulClient = new Mock(); + mockContentfulClient.Setup(x => x.GetEntries( + It.IsAny>(), + It.IsAny())) + .ReturnsAsync(results); + + var mockQueryBuilder = new MockQueryBuilder(); + var mockLogger = new Mock>(); + var filterService = new ContentfulContentFilterService(mockContentfulClient.Object, mockLogger.Object) + { + QueryBuilder = mockQueryBuilder + }; + + await filterService.GetFilteredQualifications(4, 5, 2016); + + mockLogger.VerifyError("Qualification date Aug-1a contains unexpected year value"); + } } public class MockQueryBuilder : QueryBuilder From 80499ede6645805466ab23999243fdadcaf90e49 Mon Sep 17 00:00:00 2001 From: RobertGHippo Date: Wed, 3 Jul 2024 16:43:45 +0100 Subject: [PATCH 3/7] We could be lenient by making month filter case-insensitive. --- .../ContentfulContentFilterService.cs | 2 +- .../ContentfulContentFilterServiceTests.cs | 67 +++++++++++++++++++ 2 files changed, 68 insertions(+), 1 deletion(-) diff --git a/src/Dfe.EarlyYearsQualification.Content/Services/ContentfulContentFilterService.cs b/src/Dfe.EarlyYearsQualification.Content/Services/ContentfulContentFilterService.cs index 0ea56bd8..b786096b 100644 --- a/src/Dfe.EarlyYearsQualification.Content/Services/ContentfulContentFilterService.cs +++ b/src/Dfe.EarlyYearsQualification.Content/Services/ContentfulContentFilterService.cs @@ -18,7 +18,7 @@ public class ContentfulContentFilterService( private readonly ReadOnlyDictionary _months = new( - new Dictionary + new Dictionary(StringComparer.InvariantCultureIgnoreCase) { { "Jan", 1 }, { "Feb", 2 }, diff --git a/tests/Dfe.EarlyYearsQualification.UnitTests/Services/ContentfulContentFilterServiceTests.cs b/tests/Dfe.EarlyYearsQualification.UnitTests/Services/ContentfulContentFilterServiceTests.cs index 55c2ae94..cf9c58da 100644 --- a/tests/Dfe.EarlyYearsQualification.UnitTests/Services/ContentfulContentFilterServiceTests.cs +++ b/tests/Dfe.EarlyYearsQualification.UnitTests/Services/ContentfulContentFilterServiceTests.cs @@ -285,6 +285,73 @@ public async Task GetFilteredQualifications_FilterOnDates_ReturnsFilteredQualifi filteredQualifications[2].QualificationId.Should().Be("EYQ-746"); } + [TestMethod] + public async Task GetFilteredQualifications_FilterOnDates_MonthIsCaseInsensitive_ReturnsFilteredQualifications() + { + var results = new ContentfulCollection + { + Items = new[] + { + new Qualification( + "EYQ-123", + "test", + "NCFE", + 4, + "Apr-15", + "aug-19", + "abc/123/987", + "requirements"), + new Qualification( + "EYQ-741", + "test", + "Pearson", + 4, + null, + "seP-19", + "def/456/951", + "requirements"), + new Qualification( + "EYQ-746", + "test", + "CACHE", + 4, + "sEp-15", + null, + "ghi/456/951", + "requirements"), + new Qualification( + "EYQ-752", + "test", + "CACHE", + 4, + "SEP-21", + null, + "ghi/456/951", + "requirements") + } + }; + var mockContentfulClient = new Mock(); + mockContentfulClient.Setup(x => x.GetEntries( + It.IsAny>(), + It.IsAny())) + .ReturnsAsync(results); + + var mockQueryBuilder = new MockQueryBuilder(); + var mockLogger = new Mock>(); + var filterService = new ContentfulContentFilterService(mockContentfulClient.Object, mockLogger.Object) + { + QueryBuilder = mockQueryBuilder + }; + + var filteredQualifications = await filterService.GetFilteredQualifications(4, 5, 2016); + + filteredQualifications.Should().NotBeNull(); + filteredQualifications.Count.Should().Be(3); + filteredQualifications[0].QualificationId.Should().Be("EYQ-123"); + filteredQualifications[1].QualificationId.Should().Be("EYQ-741"); + filteredQualifications[2].QualificationId.Should().Be("EYQ-746"); + } + [TestMethod] public async Task GetFilteredQualifications_ContentfulClientThrowsException_ReturnsEmptyList() { From ec8ba85f7c2040e4b898a759de3615a5e20f4648 Mon Sep 17 00:00:00 2001 From: RobertGHippo Date: Wed, 3 Jul 2024 16:48:48 +0100 Subject: [PATCH 4/7] Clarify comments. --- .../Services/ContentfulContentFilterServiceTests.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/Dfe.EarlyYearsQualification.UnitTests/Services/ContentfulContentFilterServiceTests.cs b/tests/Dfe.EarlyYearsQualification.UnitTests/Services/ContentfulContentFilterServiceTests.cs index cf9c58da..632ab03e 100644 --- a/tests/Dfe.EarlyYearsQualification.UnitTests/Services/ContentfulContentFilterServiceTests.cs +++ b/tests/Dfe.EarlyYearsQualification.UnitTests/Services/ContentfulContentFilterServiceTests.cs @@ -382,7 +382,7 @@ public async Task GetFilteredQualifications_DataContainsInvalidMonth_LogsError() "test", "NCFE", 4, - "Sept-15", // "Sept" in the data should be "Sep" + "Sept-15", // "Sept" in the data: we expect "Sep" "Aug-19", "abc/123/987", "requirements") @@ -419,8 +419,8 @@ public async Task GetFilteredQualifications_DataContainsInvalidYear_LogsError() "test", "NCFE", 4, - "Sep-15", // "Sept" in the data should be "Sep" - "Aug-1a", + "Sep-15", + "Aug-1a", // invalid year typo "abc/123/987", "requirements") } From 195c86d4c43315752dcf8afb8754ff3a8bf8d47b Mon Sep 17 00:00:00 2001 From: RobertGHippo Date: Wed, 3 Jul 2024 19:00:15 +0100 Subject: [PATCH 5/7] No we aren't using locale, this can be static. --- .../ContentfulContentFilterService.cs | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/src/Dfe.EarlyYearsQualification.Content/Services/ContentfulContentFilterService.cs b/src/Dfe.EarlyYearsQualification.Content/Services/ContentfulContentFilterService.cs index b786096b..14b55d88 100644 --- a/src/Dfe.EarlyYearsQualification.Content/Services/ContentfulContentFilterService.cs +++ b/src/Dfe.EarlyYearsQualification.Content/Services/ContentfulContentFilterService.cs @@ -16,23 +16,23 @@ public class ContentfulContentFilterService( { private const int Day = 28; - private readonly ReadOnlyDictionary - _months = new( - new Dictionary(StringComparer.InvariantCultureIgnoreCase) - { - { "Jan", 1 }, - { "Feb", 2 }, - { "Mar", 3 }, - { "Apr", 4 }, - { "May", 5 }, - { "Jun", 6 }, - { "Jul", 7 }, - { "Aug", 8 }, - { "Sep", 9 }, - { "Oct", 10 }, - { "Nov", 11 }, - { "Dec", 12 } - }); + private static readonly ReadOnlyDictionary + Months = new( + new Dictionary(StringComparer.InvariantCultureIgnoreCase) + { + { "Jan", 1 }, + { "Feb", 2 }, + { "Mar", 3 }, + { "Apr", 4 }, + { "May", 5 }, + { "Jun", 6 }, + { "Jul", 7 }, + { "Aug", 8 }, + { "Sep", 9 }, + { "Oct", 10 }, + { "Nov", 11 }, + { "Dec", 12 } + }); // Used by the unit tests to inject a mock builder that returns the query params public QueryBuilder QueryBuilder { get; init; } = QueryBuilder.New; @@ -159,7 +159,7 @@ private List FilterQualificationsByDate(int startDateMonth, int s return (false, 0, 0); } - if (!_months.TryGetValue(abbreviatedMonth, out var month)) + if (!Months.TryGetValue(abbreviatedMonth, out var month)) { logger.LogError("Qualification date {QualificationDate} contains unexpected month value", qualificationDate); From 0178e842bab8b5b77cfcd4f39b6b4f0909b807f2 Mon Sep 17 00:00:00 2001 From: RobertGHippo Date: Wed, 3 Jul 2024 19:07:49 +0100 Subject: [PATCH 6/7] Consistency of error-log format --- .../ContentfulContentFilterService.cs | 2 +- .../ContentfulContentFilterServiceTests.cs | 37 +++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/src/Dfe.EarlyYearsQualification.Content/Services/ContentfulContentFilterService.cs b/src/Dfe.EarlyYearsQualification.Content/Services/ContentfulContentFilterService.cs index 14b55d88..2db4ec9e 100644 --- a/src/Dfe.EarlyYearsQualification.Content/Services/ContentfulContentFilterService.cs +++ b/src/Dfe.EarlyYearsQualification.Content/Services/ContentfulContentFilterService.cs @@ -141,7 +141,7 @@ private List FilterQualificationsByDate(int startDateMonth, int s var splitQualificationDate = qualificationDate.Split('-'); if (splitQualificationDate.Length != 2) { - logger.LogError("Found qualification date {QualificationDate} with unexpected format", qualificationDate); + logger.LogError("Qualification date {QualificationDate} has unexpected format", qualificationDate); return (false, 0, 0); } diff --git a/tests/Dfe.EarlyYearsQualification.UnitTests/Services/ContentfulContentFilterServiceTests.cs b/tests/Dfe.EarlyYearsQualification.UnitTests/Services/ContentfulContentFilterServiceTests.cs index 632ab03e..d27c45c9 100644 --- a/tests/Dfe.EarlyYearsQualification.UnitTests/Services/ContentfulContentFilterServiceTests.cs +++ b/tests/Dfe.EarlyYearsQualification.UnitTests/Services/ContentfulContentFilterServiceTests.cs @@ -370,6 +370,43 @@ public async Task GetFilteredQualifications_ContentfulClientThrowsException_Retu filteredQualifications.Should().BeEmpty(); } + [TestMethod] + public async Task GetFilteredQualifications_DataContainsInvalidDateFormat_LogsError() + { + var results = new ContentfulCollection + { + Items = new[] + { + new Qualification( + "EYQ-123", + "test", + "NCFE", + 4, + "Sep15", // We expect Mmm-yy, e.g. "Sep-15" + "Aug-19", + "abc/123/987", + "requirements") + } + }; + + var mockContentfulClient = new Mock(); + mockContentfulClient.Setup(x => x.GetEntries( + It.IsAny>(), + It.IsAny())) + .ReturnsAsync(results); + + var mockQueryBuilder = new MockQueryBuilder(); + var mockLogger = new Mock>(); + var filterService = new ContentfulContentFilterService(mockContentfulClient.Object, mockLogger.Object) + { + QueryBuilder = mockQueryBuilder + }; + + await filterService.GetFilteredQualifications(4, 5, 2016); + + mockLogger.VerifyError("Qualification date Sep15 has unexpected format"); + } + [TestMethod] public async Task GetFilteredQualifications_DataContainsInvalidMonth_LogsError() { From eb02d0cd3dcf5631da6a5e365e2464106045d795 Mon Sep 17 00:00:00 2001 From: RobertGHippo Date: Wed, 3 Jul 2024 19:11:28 +0100 Subject: [PATCH 7/7] Show this has been thought about, it's a bit unusual. --- .../Services/ContentfulContentFilterService.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Dfe.EarlyYearsQualification.Content/Services/ContentfulContentFilterService.cs b/src/Dfe.EarlyYearsQualification.Content/Services/ContentfulContentFilterService.cs index 2db4ec9e..dc31d4a6 100644 --- a/src/Dfe.EarlyYearsQualification.Content/Services/ContentfulContentFilterService.cs +++ b/src/Dfe.EarlyYearsQualification.Content/Services/ContentfulContentFilterService.cs @@ -94,6 +94,7 @@ private List FilterQualificationsByDate(int startDateMonth, int s else if (qualificationStartDate is null && qualificationEndDate is not null // ReSharper disable once MergeSequentialChecks + // ...reveals the intention more clearly this way && enteredStartDate <= qualificationEndDate) { // if qualification start date is null, check entered start date is <= ToWhichYear & add to results