EES-5140 Fix DataSetFileMeta / Add TimePeriodRangeMeta #4871

mmyoungman · 2024-05-17T08:16:17Z

Copy of this PR, since I accidentally blitzed my changes 😨

This PR fixes an issue where DataSetFileMeta only stored one TimeIdentifier for a whole data set. This assumption I made is incorrect - a data set can have multiple TimeIdentifier's e.g. months.

File.DataSetFileMeta now has a TimePeriodRangeMeta which stores the data set's starting and ending TimeIdentifier/Year.

We are finished with the DataSetFileMeta migration endpoint, so I repurposed it in this PR to set the new TimePeriodRange field and remove TimeIdentifier and Years.

The TimeIdentifier and Year fields in DataSetFileMeta can be removed after this migration. I've added a note to do so to EES-4918.

mmyoungman · 2024-05-17T08:34:52Z

Nick left a comment on the previous PR: #4868 (comment)

Would change this to string Period to match how we've modelled it in the public API.

We've purposefully done this because singular dates and date ranges are eventually coming down the pipeline ([EES-3109](https://dfedigital.atlassian.net/browse/EES-3109)).

These new time periods are potentially going in the time_period column, so the current assumptions that we make all over that this is a single numeric year are likely to be incorrect in the future. Modelling our meta summary as suggested helps prevent this being an issue later down the line.

I've changed Year to Period in TimePeriodRangeBoundMeta.

For clarity, the Year/Period here is coming from the Stats DB Observation table. Do we intend to migrate all observations so Year is no longer a thing? If not, then maybe it is best left as Year.

I've also added a new override to TimePeriodLabelFormatter.Format that takes a string period and parses it into an int year, which you might want to take a look at. I've had to a similar thing in DataImportService#WriteDataSetFileMeta. Not how these changes work with EES-3109 and/or if there is a better way for me to handle this for the future. If Observation.Year is going to change, it should blow up on that change as it'll try to parse an non-int into an int.

ntsim · 2024-05-17T09:38:26Z

In the future, yes it probably makes sense to do a migration to remove Year as we'd likely want to consolidate these into a single string Period column (or similar).

Will speak with you on Slack about your other changes!

ntsim · 2024-05-17T11:05:33Z

...tion.ExploreEducationStatistics.Content.Api.Tests/Controllers/DataSetFilesControllerTests.cs

+                            Start = new TimePeriodRangeBoundMeta {
+                                TimeIdentifier = TimeIdentifier.CalendarYear,
+                                Period = "2000",
+                            },
+                            End = new TimePeriodRangeBoundMeta {


Formatting for these open braces should be on new lines

ntsim · 2024-05-17T11:14:43Z

...tion.ExploreEducationStatistics.Content.Api.Tests/Controllers/DataSetFilesControllerTests.cs

+                            .WithTimePeriodRange(new TimePeriodRangeMeta
+                            {
+                                Start = new TimePeriodRangeBoundMeta
+                                {
+                                    TimeIdentifier = TimeIdentifier.AcademicYear,
+                                    Period = "2000",
+                                },
+                                End = new TimePeriodRangeBoundMeta
+                                {
+                                    TimeIdentifier = TimeIdentifier.AcademicYear,
+                                    Period = "2002",
+                                },
+                            })


Would be good to have some generator extensions for this to make this a bit nicer e.g. something like this:

_fixture.DefaultTimePeriodRangeMeta() .WithStart("2000", TimeIdentifier.AcademicYear) .WithEnd("2002", TimeIdentifier.AcademicYear)

Similar thing applies in the other test case further down

ntsim · 2024-05-17T11:16:39Z

...xploreEducationStatistics.Content.Model.Tests/Fixtures/DataSetFileMetaGeneratorExtensions.cs

@@ -15,8 +15,11 @@ public static Generator<DataSetFileMeta> WithDefaults(this Generator<DataSetFile
    public static InstanceSetters<DataSetFileMeta> SetDefaults(this InstanceSetters<DataSetFileMeta> setters)
        => setters
            .SetGeographicLevels(new List<string> { "National " })


We can change this and the other lists below to collection expressions i.e.

.SetGeographicLevels(["National"])

ntsim · 2024-05-17T11:26:09Z

src/GovUk.Education.ExploreEducationStatistics.Content.Model/DataSetFileMeta.cs

@@ -11,31 +12,52 @@ public class DataSetFileMeta
    public List<string> GeographicLevels { get; set; } = new();


This would probably be better as List<GeographicLevel> to provide type safety here.

In the public API model, we also store the level codes (the enum values) in the DB JSON e.g. NAT, LA, etc, rather than the labels. The codes are not supposed to be changed often, so they are a better choice for this.

This is a more flexible approach that allows us to change the labels we return in the view models, without having to change them in the DB.

Given that we're already running a new migration, would suggest we also update the geographic levels too to store the codes instead.

I've made the change. I ended up doing it with Newtonsoft.Json - my rational being that's what we are currently using for all other ContentDbContext JSON stuff so this keeps it consistent.

ntsim · 2024-05-17T11:27:01Z

src/GovUk.Education.ExploreEducationStatistics.Content.Model/DataSetFileMeta.cs


-    public List<int> Years { get; set; } = new();
+    public TimePeriodRangeMeta TimePeriodRange { get; set; } = null!;


Mentioned in our chat - we can add required across the model properties here and below and remove any unneeded default values.

ntsim · 2024-05-17T11:36:01Z

...tion.ExploreEducationStatistics.Content.Api.Tests/Controllers/DataSetFilesControllerTests.cs

@@ -1641,12 +1652,15 @@ public async Task DataSetFileMetaCorrectlyReturned_Success()
                dataSetFileMetaViewModel.GeographicLevels
                    .AssertDeepEqualTo(originalMeta!.GeographicLevels);

-                dataSetFileMetaViewModel.TimePeriod.AssertDeepEqualTo(
-                    new DataSetFileTimePeriodViewModel
+                dataSetFileMetaViewModel.TimePeriodRange.AssertDeepEqualTo(


Don't need AssertDeepEqualTo as we're using records, can use standard Assert.Equal

ntsim · 2024-05-17T11:36:15Z

...tion.ExploreEducationStatistics.Content.Api.Tests/Controllers/DataSetFilesControllerTests.cs

@@ -1641,12 +1652,15 @@ public async Task DataSetFileMetaCorrectlyReturned_Success()
                dataSetFileMetaViewModel.GeographicLevels
                    .AssertDeepEqualTo(originalMeta!.GeographicLevels);


As below - don't need deep equals, can just use Assert.Equal

ntsim · 2024-05-17T11:37:32Z

...tion.ExploreEducationStatistics.Content.Api.Tests/Controllers/DataSetFilesControllerTests.cs

+                            originalMeta.TimePeriodRange.Start.TimeIdentifier),
+                        To = TimePeriodLabelFormatter.Format(
+                            originalMeta.TimePeriodRange.End.Period,
+                            originalMeta.TimePeriodRange.End.TimeIdentifier),
                    });

                dataSetFileMetaViewModel.Filters


Similar to above - can just use Assert.Equal.

Same for indicators below as well!

ntsim · 2024-05-17T11:38:22Z

...tion.ExploreEducationStatistics.Content.Api.Tests/Controllers/DataSetFilesControllerTests.cs

@@ -1874,13 +1899,15 @@ public async Task FetchDataSetDetails_Success()
            viewModel.Meta.GeographicLevels
                .AssertDeepEqualTo(dataSetFileMeta!.GeographicLevels);


Can just use Assert.Equal rather than deep equals.

Same thing for the other meta facets below as well.

ntsim · 2024-05-17T11:39:49Z

src/GovUk.Education.ExploreEducationStatistics.Content.ViewModels/DataSetFileMetaViewModel.cs

@@ -3,14 +3,14 @@ namespace GovUk.Education.ExploreEducationStatistics.Content.ViewModels;
 public record DataSetFileMetaViewModel
 {
    public List<string> GeographicLevels { get; init; } = new();


Can add required on all these view model properties and remove any unneeded default value - here and below

ntsim · 2024-05-20T15:10:28Z

....Education.ExploreEducationStatistics.Common/Converters/GeographicLevelsListJsonConverter.cs

+
+namespace GovUk.Education.ExploreEducationStatistics.Common.Converters;
+
+public class GeographicLevelsListJsonConverter : JsonConverter<List<GeographicLevel>>


Might want to change this to IList to make it more generic

ntsim requested changes May 17, 2024

View reviewed changes

mmyoungman force-pushed the EES-5140-2 branch from ecf46be to 0e87344 Compare May 20, 2024 10:45

mmyoungman requested a review from ntsim May 20, 2024 12:51

mmyoungman mentioned this pull request May 20, 2024

EES-4856 Add csv preview, variables, and footnotes to GetDataSetFile #4865

Merged

mmyoungman force-pushed the EES-5140-2 branch from 0e87344 to 79a5812 Compare May 20, 2024 14:01

ntsim reviewed May 20, 2024

View reviewed changes

mmyoungman force-pushed the EES-5140-2 branch from 79a5812 to e7ca82c Compare May 20, 2024 15:30

mmyoungman requested a review from ntsim May 20, 2024 15:37

ntsim approved these changes May 20, 2024

View reviewed changes

mmyoungman added 2 commits May 21, 2024 09:27

EES-5140 Fix DataSetFileMeta / Add TimePeriodRangeMeta

a763e06

EES-5140 Changes made in response to PR comments

f9d5b5a

mmyoungman force-pushed the EES-5140-2 branch from e7ca82c to f9d5b5a Compare May 21, 2024 08:32

mmyoungman merged commit 930c76c into dev May 21, 2024
7 checks passed

mmyoungman deleted the EES-5140-2 branch May 21, 2024 09:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EES-5140 Fix DataSetFileMeta / Add TimePeriodRangeMeta #4871

EES-5140 Fix DataSetFileMeta / Add TimePeriodRangeMeta #4871

mmyoungman commented May 17, 2024

mmyoungman commented May 17, 2024 •

edited

Loading

ntsim commented May 17, 2024

ntsim May 17, 2024

ntsim May 17, 2024

ntsim May 17, 2024

ntsim May 17, 2024

mmyoungman May 20, 2024

ntsim May 17, 2024

ntsim May 17, 2024

ntsim May 17, 2024

ntsim May 17, 2024

ntsim May 17, 2024

ntsim May 17, 2024

ntsim May 20, 2024

		@@ -11,31 +12,52 @@ public class DataSetFileMeta
		public List<string> GeographicLevels { get; set; } = new();


		public List<int> Years { get; set; } = new();
		public TimePeriodRangeMeta TimePeriodRange { get; set; } = null!;

		@@ -1641,12 +1652,15 @@ public async Task DataSetFileMetaCorrectlyReturned_Success()
		dataSetFileMetaViewModel.GeographicLevels
		.AssertDeepEqualTo(originalMeta!.GeographicLevels);

		@@ -1874,13 +1899,15 @@ public async Task FetchDataSetDetails_Success()
		viewModel.Meta.GeographicLevels
		.AssertDeepEqualTo(dataSetFileMeta!.GeographicLevels);


		namespace GovUk.Education.ExploreEducationStatistics.Common.Converters;

		public class GeographicLevelsListJsonConverter : JsonConverter<List<GeographicLevel>>

EES-5140 Fix DataSetFileMeta / Add TimePeriodRangeMeta #4871

EES-5140 Fix DataSetFileMeta / Add TimePeriodRangeMeta #4871

Conversation

mmyoungman commented May 17, 2024

mmyoungman commented May 17, 2024 • edited Loading

ntsim commented May 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmyoungman commented May 17, 2024 •

edited

Loading