Merge pull request #760 from DFE-Digital/development

Release v2.5.0

.github/workflows/deploy-azure-function.yml

@@ -42,6 +42,11 @@ jobs:
           az_client_id: ${{ secrets.AZ_CLIENT_ID }}
           az_client_secret: ${{ secrets.AZ_CLIENT_SECRET }}
 
+      - name: Set storage account name var
+        shell: bash
+        run: echo "STORAGE_ACCOUNT=$(echo ${{ env.AZURE_STORAGEACCOUNT_NAME }} | sed -e "s/-//g")" >> $GITHUB_ENV
+
+
       - name: Get workflow IP address
         id: whats-my-ip
         uses: ./.github/actions/whats-my-ip-address
@@ -51,8 +56,9 @@ jobs:
         # The Azure Storage Account's name contains no hyphens, unlike almost all of our other resources
         # Therefore remove hyphens from the string before using it as the account name
         run: |
+          az storage account update --name $STORAGE_ACCOUNT --resource-group ${{ env.AZURE_RESOURCEGROUP_NAME }} --public-network-access Enabled &> /dev/null
           az storage account network-rule add --resource-group ${{ env.AZURE_RESOURCEGROUP_NAME }} \
-                                              --account-name $(echo ${{ env.AZURE_STORAGEACCOUNT_NAME }} | sed -e "s/-//g") \
+                                              --account-name $STORAGE_ACCOUNT \
                                               --ip-address ${{ steps.whats-my-ip.outputs.ip }} &> /dev/null
 
       - name: Deploy Azure Function App
@@ -66,5 +72,6 @@ jobs:
         if: always()
         run: |
           az storage account network-rule remove --resource-group ${{ env.AZURE_RESOURCEGROUP_NAME }} \
-                                                 --account-name $(echo ${{ env.AZURE_STORAGEACCOUNT_NAME }} | sed -e "s/-//g") \
+                                                 --account-name $STORAGE_ACCOUNT \
                                                  --ip-address ${{ steps.whats-my-ip.outputs.ip }} &> /dev/null
+          az storage account update --name $STORAGE_ACCOUNT --resource-group ${{ env.AZURE_RESOURCEGROUP_NAME }} --public-network-access Disabled &> /dev/null

docs/adr/0037-content-validation-process.md

@@ -0,0 +1,66 @@
+# 0037 - Content Validation Process
+
+* **Status**: proposed
+
+## Context and Problem Statement
+
+Certain fields in Contentful are mandatory, and we currently do not save any content items which are invalid, due to missing such fields.
+This poses a problem with the following content creation process:
+1. A question is created and details filled in
+2. A new answer is added to that question
+3. The answer is incomplete, so invalid, so doesn't save
+4. Therefore no question -> answer relationship is saved
+5. The answer is filled in
+6. It then gets saved to the db on its own
+7. the question -> answer relationship was never re-attempted, so still does not exist
+8. The question then doesn't show this answer in production
+
+This can be resolved by republishing the question, but it is frustrating for content designers to have to repeatedly republish content.
+It also leaves production missing content until someone notices, so we need a way to fix this issue.
+
+## Decision Drivers
+
+- Ease of development
+- Reliability of the solution
+- How testable the solution is
+- Minimising risk of error for the function
+- Minimising risk of error in the web app
+
+## Considered Options
+
+### Overnight/periodic job to run a content validator and retrieve missing items:
+
+- Pros
+    - Minimal regression risk to web app as it can stay exactly as is
+    - Can handle any error that has occured with the function, not just this, including it temporarily going down entirely
+- Cons
+    - In the time between an issue arising and the periodic job running, the app could be broken due to missing relationships, so this may not be adequate
+    - May be resource intensive if run frequently, as it does full validation
+    - Requires developing a solution to fix the issues found by the validator and setting up periodic jobs
+
+### Making all database fields (except primary keys) nullable:
+
+- Pros
+    - Robust approach to the problem, it's highly likely to ensure the correct relationships save to the database every time
+    - Minimal risk of errors within the Azure function
+- Cons
+    - High risk of hitting an error page in the web app when partial content exists, due to it expecting content that isn't there
+    - Would require extensive testing with partially complete content items to be sure of the web app not breaking
+    - Widespread changes within the code base, risk of regression
+
+### Having default values for simple fields, and nullable content references
+Not every field can have a default, for instance a question section requires an interstitial page, which cannot be populated with a default value,
+but we could make every field that can have a default, have a default and the others nullable.
+
+- Pros
+    - Lower risk than making all fields nullable, as it requires fewer changes to the web app
+    - Also a robust approach that is highly likely to work
+    - Relatively low risk of errors within the Azure function
+- Cons
+    - May still be quite a few code changes involved in making all content references nullable, with the same risks as the option above (but less widespread)
+    - Needs careful testing of all types which are made nullable
+    - The defaults will render incomplete content "valid" when it isn't. This risk is small because Contentful doesn't allow publishing content with missing mandatory fields. So only UsePreview environments have a risk of showing blank content.
+
+## Decision Outcome
+
+Option 3. It's lowest risk and involvement whilst effectively resolving the problem. The suggested implementation will be to modify the way the Azure function maps a json object into database entities and add defaults at this stage, to minimise the changes required.

docs/adr/README.md

@@ -11,7 +11,7 @@ General information about architectural decision records is available at <https:
 ## List
 
 | Title                                                                                                       | Status   |
-| ----------------------------------------------------------------------------------------------------------- | -------- |
+|-------------------------------------------------------------------------------------------------------------|----------|
 | [0001 - Web Framework](./0001-web-framework.md)                                                             | Accepted |
 | [0002 - Application Framework](./0002-application-framework.md)                                             | Accepted |
 | [0003 - Build/Test/Deploy Environment](./0002-application-framework.md)                                     | Accepted |
@@ -40,10 +40,11 @@ General information about architectural decision records is available at <https:
 | [0027 - Accesibility Testing](./0027-accesibility-testing.md)                                               | Accepted |
 | [0028 - Command Line Arguments Parsing](./0028-command-line-arguments.md)                                   | Accepted |
 | [0029 - Terraform input variables](./0029-terraform-variable-for-admin-password.md)                         | Accepted |
-| [0030 - Storing CMS Data In Database][./0030-storing-cms-in-db.md]                                          | Accepted |
-| [0031 - CMS DB Structure][./0031-cms-db-structure.md]                                                       | Accepted |
-| [0032 - Classes/Code Structure for DB Entities][./0032-classes-for-db-entities.md]                          | Accepted |
+| [0030 - Storing CMS Data In Database](./0030-storing-cms-in-db.md)                                          | Accepted |
+| [0031 - CMS DB Structure](./0031-cms-db-structure.md)                                                       | Accepted |
+| [0032 - Classes/Code Structure for DB Entities](./0032-classes-for-db-entities.md)                          | Accepted |
 | [0033 - Test data generation](./0033-test-data-generation.md)                                               | Accepted |
 | [0034 - Entity mapping](./0034-entity-mapping.md)                                                           | Accepted |
 | [0035 - Caching](./0035-caching.md)                                                                         | Accepted |
-| [0036 - DB Unit Tests](./0036-db-unit-tests.md)                                                             | Proposed |
+| [0036 - DB Unit Tests](./0036-db-unit-tests.md)                                                             | Proposed |
+| [0037 - Content Validation Process](./0037-content-validation-process.md)                                   | Proposed |

src/Dfe.PlanTech.Application/Submissions/Queries/GetSubmissionStatusesQuery.cs

@@ -17,19 +17,20 @@ public async Task<List<SectionStatusDto>> GetSectionSubmissionStatuses(string ca
         return await db.ToListAsync(db.GetSectionStatuses(categoryId, establishmentId));
     }
 
-    public async Task<SectionStatusNew> GetSectionSubmissionStatusAsync(int establishmentId,
+    public async Task<SectionStatus> GetSectionSubmissionStatusAsync(int establishmentId,
                                                                          ISectionComponent section,
                                                                          bool completed,
                                                                          CancellationToken cancellationToken)
     {
-        var sectionStatus = db.GetSubmissions.Where(submission => submission.EstablishmentId == establishmentId &&
-                                                 submission.SectionId == section.Sys.Id && !submission.Deleted);
+        var sectionStatus = db.GetSubmissions.Where(submission => submission.EstablishmentId == establishmentId
+                                                                  && submission.SectionId == section.Sys.Id
+                                                                  && !submission.Deleted);
 
         var groupedAndLatest = GetLatestSubmissionStatus(sectionStatus, completed);
 
         var result = await db.FirstOrDefaultAsync(groupedAndLatest, cancellationToken);
 
-        return result ?? new SectionStatusNew()
+        return result ?? new SectionStatus()
         {
             SectionId = section.Sys.Id,
             Completed = false,
@@ -46,8 +47,8 @@ public async Task<SectionStatusNew> GetSectionSubmissionStatusAsync(int establis
     /// <param name="submissionStatuses"></param>
     /// <param name="completed"></param>
     /// <returns></returns>
-    private static IQueryable<SectionStatusNew> GetLatestSubmissionStatus(IQueryable<Submission> submissionStatuses, bool completed)
-    => submissionStatuses.Select(submission => new SectionStatusNew()
+    private static IQueryable<SectionStatus> GetLatestSubmissionStatus(IQueryable<Submission> submissionStatuses, bool completed)
+    => submissionStatuses.Select(submission => new SectionStatus()
     {
         DateCreated = submission.DateCreated,
         Completed = submission.Completed,
@@ -56,8 +57,5 @@ private static IQueryable<SectionStatusNew> GetLatestSubmissionStatus(IQueryable
         Status = submission.Completed ? Status.Completed : submission.Responses.Count != 0 ? Status.InProgress : Status.NotStarted,
     })
     .GroupBy(submission => submission.SectionId)
-    .Select(grouping => grouping
-        .OrderByDescending(status => completed && status.Completed)
-        .ThenByDescending(status => status.DateCreated)
-        .First());
+    .Select(grouping => grouping.OrderByDescending(status => completed && status.Completed).ThenByDescending(status => status.DateCreated).First());
 }

src/Dfe.PlanTech.Application/Submissions/Queries/SubmissionStatusProcessor.cs

@@ -24,7 +24,7 @@ public ISectionComponent Section
     }
     public IUser User { get; init; }
     public Question? NextQuestion { get; set; }
-    public SectionStatusNew? SectionStatus { get; private set; }
+    public SectionStatus? SectionStatus { get; private set; }
     public SubmissionStatus Status { get; set; }
 
     public SubmissionStatusProcessor(IGetSectionQuery getSectionQuery,

src/Dfe.PlanTech.DatabaseUpgrader/Scripts/2024/20240827_1056_CreateGetCurrentSubmissionIdProcedure.sql

@@ -0,0 +1,15 @@
+CREATE OR ALTER PROCEDURE GetCurrentSubmissionId
+    @sectionId NVARCHAR(50),
+    @establishmentId INT,
+    @submissionId INT OUTPUT
+AS
+
+SELECT @submissionId = (
+    SELECT TOP 1 Id
+    FROM [dbo].[submission]
+    WHERE completed = 0
+        AND deleted = 0
+        AND sectionId = @sectionId
+        AND establishmentId = @establishmentId
+    ORDER BY dateCreated DESC
+)

src/Dfe.PlanTech.DatabaseUpgrader/Scripts/2024/20240827_1058_UpdateSelectOrInsertSubmissionIdProcedure.sql

@@ -0,0 +1,29 @@
+ALTER PROCEDURE SelectOrInsertSubmissionId
+    @sectionId NVARCHAR(50),
+    @sectionName NVARCHAR(50),
+    @establishmentId INT,
+    @submissionId INT OUTPUT
+AS
+
+BEGIN TRY
+    BEGIN TRAN
+        EXEC GetCurrentSubmissionId
+            @sectionid=@sectionId,
+            @establishmentId=@establishmentId,
+            @submissionId=@submissionId OUTPUT
+
+        IF @submissionId IS NULL
+            BEGIN
+                INSERT INTO [dbo].[submission]
+                    (establishmentId, completed, sectionId, sectionName)
+                OUTPUT INSERTED.ID
+                VALUES
+                    (@establishmentId, 0, @sectionId, @sectionName)
+
+                SELECT @submissionId = SCOPE_IDENTITY()
+            END
+    COMMIT TRAN
+END TRY
+BEGIN CATCH
+    ROLLBACK TRAN
+END CATCH

src/Dfe.PlanTech.DatabaseUpgrader/Scripts/2024/20240827_1102_UpdateDeleteCurrentSubmissionProcedure.sql

@@ -0,0 +1,25 @@
+CREATE OR ALTER PROCEDURE DeleteCurrentSubmission
+    @sectionId NVARCHAR(50),
+    @sectionName NVARCHAR(50),
+    @establishmentId INT
+AS
+    BEGIN TRY
+        DECLARE @submissionId INT
+
+        EXEC GetCurrentSubmissionId
+            @sectionid=@sectionId,
+            @establishmentId=@establishmentId,
+            @submissionId=@submissionId OUTPUT
+
+        BEGIN TRAN
+            UPDATE S
+            SET deleted = 1
+            FROM [dbo].[submission] S
+            WHERE S.id = @submissionId
+        COMMIT TRAN
+
+    END TRY
+    BEGIN CATCH
+        ROLLBACK TRAN
+    END CATCH
+GO

src/Dfe.PlanTech.Domain/Interfaces/IGetSubmissionStatusesQuery.cs

@@ -7,7 +7,7 @@ public interface IGetSubmissionStatusesQuery
 {
     Task<List<SectionStatusDto>> GetSectionSubmissionStatuses(string categoryId);
 
-    Task<SectionStatusNew> GetSectionSubmissionStatusAsync(int establishmentId,
+    Task<SectionStatus> GetSectionSubmissionStatusAsync(int establishmentId,
                                                            ISectionComponent section,
                                                            bool completed,
                                                            CancellationToken cancellationToken);

src/Dfe.PlanTech.Domain/Submissions/Interfaces/ISubmissionStatusProcessor.cs

@@ -17,7 +17,7 @@ public interface ISubmissionStatusProcessor
     public SubmissionStatus Status { get; set; }
     public Question? NextQuestion { get; set; }
     public ISectionComponent Section { get; }
-    public SectionStatusNew? SectionStatus { get; }
+    public SectionStatus? SectionStatus { get; }
 
     Task GetJourneyStatusForSection(string sectionSlug, CancellationToken cancellationToken);
     Task GetJourneyStatusForSectionRecommendation(string sectionSlug, CancellationToken cancellationToken);

src/Dfe.PlanTech.Domain/Submissions/Models/SectionStatus.cs

@@ -2,20 +2,7 @@
 
 namespace Dfe.PlanTech.Domain.Submissions.Models;
 
-public class SectionStatusDto
-{
-    public string SectionId { get; set; } = null!;
-
-    public bool Completed { get; set; }
-
-    public string? LastMaturity { get; set; }
-
-    public DateTime DateCreated { get; set; }
-
-    public DateTime DateUpdated { get; set; }
-}
-
-public record SectionStatusNew
+public record SectionStatus
 {
     public string SectionId { get; set; } = null!;
 

src/Dfe.PlanTech.Domain/Submissions/Models/SectionStatusDto.cs

@@ -0,0 +1,14 @@
+namespace Dfe.PlanTech.Domain.Submissions.Models;
+
+public class SectionStatusDto
+{
+    public string SectionId { get; set; } = null!;
+
+    public bool Completed { get; set; }
+
+    public string? LastMaturity { get; set; }
+
+    public DateTime DateCreated { get; set; }
+
+    public DateTime DateUpdated { get; set; }
+}