EES-5738 Changes in response to PR comments

src/GovUk.Education.ExploreEducationStatistics.Admin/Controllers/Api/DataSetFileGeographicLevelsMigrationController.cs

@@ -4,6 +4,7 @@
 using System.Threading.Tasks;
 using GovUk.Education.ExploreEducationStatistics.Admin.Models;
 using GovUk.Education.ExploreEducationStatistics.Common.Extensions;
+using GovUk.Education.ExploreEducationStatistics.Common.Model;
 using GovUk.Education.ExploreEducationStatistics.Content.Model;
 using GovUk.Education.ExploreEducationStatistics.Content.Model.Database;
 using Microsoft.AspNetCore.Authorization;
@@ -24,14 +25,14 @@ public class MigrationResult
     }
 
     [HttpPut("bau/migrate-datasetfile-geographiclevels")]
-    public async Task<MigrationResult> DataSetFileGeographicLevelsMigration(
+    public async Task<MigrationResult> DataSetFileVersionGeographicLevelsMigration(
         [FromQuery] bool isDryRun = true,
         [FromQuery] int? num = null,
         CancellationToken cancellationToken = default)
     {
         var queryable = contentDbContext.Files
-            .Where(f => f.DataSetFileMeta != null
-                        && f.DataSetFileGeographicLevels.Count == 0);
+            .Where(f => f.Type == FileType.Data
+                        && f.DataSetFileVersionGeographicLevels.Count == 0);
 
         if (num != null)
         {
@@ -53,16 +54,17 @@ public async Task<MigrationResult> DataSetFileGeographicLevelsMigration(
                 continue;
             }
 
-            var dataSetFileGeographicLevels = meta!.GeographicLevels
+            var dataSetFileVersionGeographicLevels = meta!.GeographicLevels
                 .Distinct()
-                .Select(gl => new DataSetFileGeographicLevel
+                .Select(gl => new DataSetFileVersionGeographicLevel
                 {
                     DataSetFileVersionId = file.Id,
                     GeographicLevel = gl,
                 })
                 .ToList();
 
-            contentDbContext.DataSetFileGeographicLevels.AddRange(dataSetFileGeographicLevels);
+            contentDbContext.DataSetFileVersionGeographicLevels.AddRange(
+                dataSetFileVersionGeographicLevels);
 
             numProcessed++;
         }

src/GovUk.Education.ExploreEducationStatistics.Admin/Migrations/ContentMigrations/20241213123442_EES5738_CreateDataSetFileGeographicLevelsTable.cs

@@ -29,6 +29,8 @@ protected override void Up(MigrationBuilder migrationBuilder)
 
             migrationBuilder.Sql("GRANT SELECT ON dbo.DataSetFileGeographicLevels TO [content];");
             migrationBuilder.Sql("GRANT INSERT ON dbo.DataSetFileGeographicLevels TO [importer];");
+            // @MarkFix add for data and publisher users too? And respond to PR comment
+            // @MarkFix recreate the migration
         }
 
         /// <inheritdoc />

src/GovUk.Education.ExploreEducationStatistics.Content.Api.Tests/Controllers/DataSetFilesControllerTests.cs

@@ -101,27 +101,21 @@ public async Task FilterByGeographicLevel_Success()
                 var (publication1, publication2) = _fixture
                     .DefaultPublication()
                     // Publications each have a published release version
-                    .WithReleases(_fixture.DefaultRelease(publishedVersions: 1)
-                        .Generate(1))
+                    .WithReleases([_fixture.DefaultRelease(publishedVersions: 1)])
                     .WithTheme(_fixture.DefaultTheme())
                     .GenerateTuple2();
 
-                var publication1Release1Version1Files = _fixture.DefaultReleaseFile()
+                ReleaseFile publication1Release1Version1File = _fixture.DefaultReleaseFile()
                     .WithReleaseVersion(publication1.ReleaseVersions[0])
-                    .WithFiles(_fixture.DefaultFile(FileType.Data)
-                    .WithDataSetFileMeta(_fixture.DefaultDataSetFileMeta()
-                        .WithGeographicLevels(
-                            [GeographicLevel.Country, GeographicLevel.LocalAuthority, GeographicLevel.Institution]))
-                    .WithDataSetFileGeographicLevels(
-                        [GeographicLevel.Country, GeographicLevel.LocalAuthority, GeographicLevel.Institution])
-                    .GenerateList(1))
-                    .GenerateList();
+                    .WithFile(_fixture.DefaultFile(FileType.Data)
+                        .WithDataSetFileVersionGeographicLevels(
+                            [GeographicLevel.Country, GeographicLevel.LocalAuthority, GeographicLevel.Institution]));
 
                 var publication2Release1Version1Files = GenerateDataSetFilesForReleaseVersion(publication2.ReleaseVersions[0]);
 
                 await TestApp.AddTestData<ContentDbContext>(context =>
                 {
-                    context.ReleaseFiles.AddRange(publication1Release1Version1Files);
+                    context.ReleaseFiles.Add(publication1Release1Version1File);
                     context.ReleaseFiles.AddRange(publication2Release1Version1Files);
                 });
 
@@ -137,7 +131,7 @@ await TestApp.AddTestData<ContentDbContext>(context =>
 
                 pagedResult.AssertHasExpectedPagingAndResultCount(
                     expectedTotalResults: 1);
-                AssertResultsForExpectedReleaseFiles(publication1Release1Version1Files, pagedResult.Results);
+                AssertResultsForExpectedReleaseFiles([publication1Release1Version1File], pagedResult.Results);
             }
 
             [Fact]
@@ -1783,7 +1777,7 @@ private List<ReleaseFile> GenerateDataSetFilesForReleaseVersion(
                 .WithReleaseVersion(releaseVersion)
                 .WithFiles(_fixture.DefaultFile(FileType.Data)
                     .WithDataSetFileMeta(_fixture.DefaultDataSetFileMeta())
-                    .WithDataSetFileGeographicLevels([GeographicLevel.Country])
+                    .WithDataSetFileVersionGeographicLevels([GeographicLevel.Country])
                     .GenerateList(numberOfDataSets))
                 .GenerateList();
         }

src/GovUk.Education.ExploreEducationStatistics.Content.Model.Tests/Fixtures/DataImportGeneratorExtensions.cs

@@ -114,7 +114,7 @@ public static InstanceSetters<DataImport> SetDefaultFilesWithoutMeta(
                 (_, d, context) => context.Fixture
                     .DefaultFile(FileType.Data)
                     .WithDataSetFileMeta(null)
-                    .WithDataSetFileGeographicLevels([])
+                    .WithDataSetFileVersionGeographicLevels([])
                     .WithFilename($"{dataFileName}.csv")
                     .WithSubjectId(d.SubjectId)
             )

src/GovUk.Education.ExploreEducationStatistics.Content.Model.Tests/Fixtures/DataSetFileVersionGeographicLevelGeneratorExtensions.cs

@@ -0,0 +1,51 @@
+using System;
+using GovUk.Education.ExploreEducationStatistics.Common.Model.Data;
+using GovUk.Education.ExploreEducationStatistics.Common.Tests.Fixtures;
+
+namespace GovUk.Education.ExploreEducationStatistics.Content.Model.Tests.Fixtures;
+
+public static class DataSetFileVersionGeographicLevelGeneratorExtensions
+{
+    public static Generator<DataSetFileVersionGeographicLevel> DefaultDataSetFileVersionGeographicLevel(this DataFixture fixture)
+        => fixture.Generator<DataSetFileVersionGeographicLevel>().WithDefaults();
+
+    public static Generator<DataSetFileVersionGeographicLevel> WithDefaults(this Generator<DataSetFileVersionGeographicLevel> generator)
+        => generator.ForInstance(d => d.SetDefaults());
+
+    public static InstanceSetters<DataSetFileVersionGeographicLevel> SetDefaults(
+        this InstanceSetters<DataSetFileVersionGeographicLevel> setters)
+        => setters
+            .SetDefault(p => p.DataSetFileVersionId);
+
+    public static Generator<DataSetFileVersionGeographicLevel> WithDataSetFileVersion(
+        this Generator<DataSetFileVersionGeographicLevel> generator,
+        File dataSetFileVersion)
+        => generator.ForInstance(s => s.SetDataSetFileVersion(dataSetFileVersion));
+
+    public static Generator<DataSetFileVersionGeographicLevel> WithDataSetFileVersionId(
+        this Generator<DataSetFileVersionGeographicLevel> generator,
+        Guid dataSetFileVersionId)
+        => generator.ForInstance(s => s.SetDataSetFileVersionId(dataSetFileVersionId));
+
+    public static Generator<DataSetFileVersionGeographicLevel> WithGeographicLevel(
+        this Generator<DataSetFileVersionGeographicLevel> generator,
+        GeographicLevel geographicLevel)
+        => generator.ForInstance(s => s.SetGeographicLevel(geographicLevel));
+
+    public static InstanceSetters<DataSetFileVersionGeographicLevel> SetDataSetFileVersion(
+        this InstanceSetters<DataSetFileVersionGeographicLevel> setters,
+        File dataSetFileVersion)
+        => setters
+            .Set(f => f.DataSetFileVersion, dataSetFileVersion)
+            .Set(f => f.DataSetFileVersionId, dataSetFileVersion.Id);
+
+    public static InstanceSetters<DataSetFileVersionGeographicLevel> SetDataSetFileVersionId(
+        this InstanceSetters<DataSetFileVersionGeographicLevel> setters,
+        Guid dataSetFileVersionId)
+        => setters.Set(f => f.DataSetFileVersionId, dataSetFileVersionId);
+
+    public static InstanceSetters<DataSetFileVersionGeographicLevel> SetGeographicLevel(
+        this InstanceSetters<DataSetFileVersionGeographicLevel> setters,
+        GeographicLevel geographicLevel)
+        => setters.Set(f => f.GeographicLevel, geographicLevel);
+}

src/GovUk.Education.ExploreEducationStatistics.Content.Model.Tests/Fixtures/FileGeneratorExtensions.cs

@@ -85,10 +85,10 @@ public static Generator<File> WithDataSetFileMeta(
         DataSetFileMeta? dataSetFileMeta)
         => generator.ForInstance(s => s.SetDataSetFileMeta(dataSetFileMeta));
 
-    public static Generator<File> WithDataSetFileGeographicLevels(
+    public static Generator<File> WithDataSetFileVersionGeographicLevels(
         this Generator<File> generator,
         List<GeographicLevel> geographicLevels)
-        => generator.ForInstance(s => s.SetDataSetFileGeographicLevels(geographicLevels));
+        => generator.ForInstance(s => s.SetDataSetFileVersionGeographicLevels(geographicLevels));
 
     public static InstanceSetters<File> SetDefaults(this InstanceSetters<File> setters, FileType? fileType)
         => fileType switch
@@ -117,7 +117,7 @@ public static InstanceSetters<File> SetDataFileDefaults(this InstanceSetters<Fil
             .SetContentType("text/csv")
             .SetDefault(f => f.DataSetFileId)
             .Set(f => f.DataSetFileMeta, (_, _, context) => context.Fixture.DefaultDataSetFileMeta())
-            .SetDataSetFileGeographicLevels([GeographicLevel.Country, GeographicLevel.LocalAuthority, GeographicLevel.LocalAuthorityDistrict]);
+            .SetDataSetFileVersionGeographicLevels([GeographicLevel.Country, GeographicLevel.LocalAuthority, GeographicLevel.LocalAuthorityDistrict]);
 
     public static InstanceSetters<File> SetMetaFileDefaults(this InstanceSetters<File> setters)
         => setters
@@ -208,15 +208,16 @@ public static InstanceSetters<File> SetDataSetFileMeta(
         DataSetFileMeta? dataSetFileMeta)
         => setters.Set(f => f.DataSetFileMeta, dataSetFileMeta);
 
-    public static InstanceSetters<File> SetDataSetFileGeographicLevels(
+    public static InstanceSetters<File> SetDataSetFileVersionGeographicLevels(
         this InstanceSetters<File> setters,
         List<GeographicLevel> geographicLevels)
         => setters.Set(
-            file => file.DataSetFileGeographicLevels,
+            file => file.DataSetFileVersionGeographicLevels,
             (_, file) => geographicLevels.Select(
-                gl => new DataSetFileGeographicLevel
+                gl => new DataSetFileVersionGeographicLevel
                 {
                     DataSetFileVersionId = file.Id,
+                    DataSetFileVersion = file,
                     GeographicLevel = gl,
                 }).ToList());
 }

src/GovUk.Education.ExploreEducationStatistics.Content.Model/DataSetFileGeographicLevels.cs → src/GovUk.Education.ExploreEducationStatistics.Content.Model/DataSetFileVersionGeographicLevel.cs

@@ -4,7 +4,7 @@
 
 namespace GovUk.Education.ExploreEducationStatistics.Content.Model;
 
-public class DataSetFileGeographicLevel
+public class DataSetFileVersionGeographicLevel
 {
     public Guid DataSetFileVersionId { get; set; } // Currently Files.Id, but will become DataSetFileVersion.Id in EES-5105
 

src/GovUk.Education.ExploreEducationStatistics.Content.Model/Database/ContentDbContext.cs

@@ -55,7 +55,7 @@ private void Configure(bool updateTimestamps = true)
         public virtual DbSet<ReleaseStatus> ReleaseStatus { get; set; }
         public virtual DbSet<ReleaseFile> ReleaseFiles { get; set; }
         public virtual DbSet<File> Files { get; set; }
-        public virtual DbSet<DataSetFileGeographicLevel> DataSetFileGeographicLevels { get; set; }
+        public virtual DbSet<DataSetFileVersionGeographicLevel> DataSetFileVersionGeographicLevels { get; set; }
         public virtual DbSet<ContentSection> ContentSections { get; set; }
         public virtual DbSet<ContentBlock> ContentBlocks { get; set; }
         public virtual DbSet<KeyStatistic> KeyStatistics { get; set; }
@@ -111,7 +111,7 @@ protected override void OnModelCreating(ModelBuilder modelBuilder)
             ConfigureReleaseStatus(modelBuilder);
             ConfigureReleaseFile(modelBuilder);
             ConfigureFile(modelBuilder);
-            ConfigureDataSetFileGeographicLevel(modelBuilder);
+            ConfigureDataSetFileVersionGeographicLevel(modelBuilder);
             ConfigureContentBlock(modelBuilder);
             ConfigureContentSection(modelBuilder);
             ConfigureReleaseVersion(modelBuilder);
@@ -454,16 +454,16 @@ private static void ConfigureFile(ModelBuilder modelBuilder)
                         v => v.HasValue
                             ? DateTime.SpecifyKind(v.Value, DateTimeKind.Utc)
                             : null);
-                entity.Property(p => p.DataSetFileMeta) // EES-5666
+                entity.Property(p => p.DataSetFileMeta)
                     .HasConversion(
                         v => JsonConvert.SerializeObject(v),
                         v => JsonConvert.DeserializeObject<DataSetFileMeta>(v));
             });
         }
 
-        private static void ConfigureDataSetFileGeographicLevel(ModelBuilder modelBuilder)
+        private static void ConfigureDataSetFileVersionGeographicLevel(ModelBuilder modelBuilder)
         {
-            modelBuilder.Entity<DataSetFileGeographicLevel>(entity =>
+            modelBuilder.Entity<DataSetFileVersionGeographicLevel>(entity =>
             {
                 entity.HasKey(gl => new
                 {
@@ -472,6 +472,7 @@ private static void ConfigureDataSetFileGeographicLevel(ModelBuilder modelBuilde
                 });
 
                 entity.Property(gl => gl.GeographicLevel)
+                    .HasMaxLength(6)
                     .HasConversion(new EnumToEnumValueConverter<GeographicLevel>());
             });
         }

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

@@ -23,7 +23,7 @@ public class File : ICreatedTimestamp<DateTime?>
 
         public int? DataSetFileVersion { get; set; }
 
-        public List<DataSetFileGeographicLevel> DataSetFileGeographicLevels { get; set; } = [];
+        public List<DataSetFileVersionGeographicLevel> DataSetFileVersionGeographicLevels { get; set; } = [];
 
         public DataSetFileMeta? DataSetFileMeta { get; set; }
 

src/GovUk.Education.ExploreEducationStatistics.Content.Services/DataSetFileService.cs

@@ -487,9 +487,8 @@ internal static IQueryable<ReleaseFile> HavingGeographicLevel(
         GeographicLevel? geographicLevel)
     {
         return geographicLevel.HasValue
-            ? query.Where(rf => rf.File.DataSetFileGeographicLevels.Any(
-                gl => gl.GeographicLevel == geographicLevel
-                      && rf.FileId == gl.DataSetFileVersionId))
+            ? query.Where(rf => rf.File.DataSetFileVersionGeographicLevels.Any(
+                gl => gl.GeographicLevel == geographicLevel))
             : query;
     }
 

src/GovUk.Education.ExploreEducationStatistics.Data.Processor.Tests/Services/DataImportServiceTests.cs

@@ -220,7 +220,7 @@ public async Task WriteDataSetMetaFile_Success()
 
             var file = _fixture.DefaultFile(FileType.Data)
                 .WithDataSetFileMeta(null)
-                .WithDataSetFileGeographicLevels([])
+                .WithDataSetFileVersionGeographicLevels([])
                 .WithSubjectId(subject.Id)
                 .Generate();
 

src/GovUk.Education.ExploreEducationStatistics.Data.Processor/Services/DataImportService.cs

@@ -221,14 +221,14 @@ public async Task WriteDataSetFileMeta(Guid fileId, Guid subjectId)
                              && f.SubjectId == subjectId);
             file.DataSetFileMeta = dataSetFileMeta;
 
-            var dataSetFileGeographicLevels = geographicLevels
-                .Distinct()
-                .Select(gl => new DataSetFileGeographicLevel
+            var dataSetFileVersionGeographicLevels = geographicLevels
+                .Select(gl => new DataSetFileVersionGeographicLevel
                 {
                     DataSetFileVersionId = fileId,
                     GeographicLevel = gl,
                 }).ToList();
-            await contentDbContext.DataSetFileGeographicLevels.AddRangeAsync(dataSetFileGeographicLevels);
+            contentDbContext.DataSetFileVersionGeographicLevels.AddRange(
+                dataSetFileVersionGeographicLevels);
 
             await contentDbContext.SaveChangesAsync();
         }