diff --git a/.github/workflows/deploy-azure-function.yml b/.github/workflows/deploy-azure-function.yml index 227e4fbc9..de4043e1a 100644 --- a/.github/workflows/deploy-azure-function.yml +++ b/.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 diff --git a/contentandsupport b/contentandsupport index 68822846c..a350cbe2f 160000 --- a/contentandsupport +++ b/contentandsupport @@ -1 +1 @@ -Subproject commit 68822846c353c103a79c334eca60e8040ee7d943 +Subproject commit a350cbe2f45375a4b2b96857cc071d98cd158de4 diff --git a/docs/adr/0037-content-validation-process.md b/docs/adr/0037-content-validation-process.md new file mode 100644 index 000000000..2d950375a --- /dev/null +++ b/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. diff --git a/docs/adr/README.md b/docs/adr/README.md index f55db6ced..0f7b5daae 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -11,7 +11,7 @@ General information about architectural decision records is available at > GetSectionSubmissionStatuses(string ca return await db.ToListAsync(db.GetSectionStatuses(categoryId, establishmentId)); } - public async Task GetSectionSubmissionStatusAsync(int establishmentId, + public async Task 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 GetSectionSubmissionStatusAsync(int establis /// /// /// - private static IQueryable GetLatestSubmissionStatus(IQueryable submissionStatuses, bool completed) - => submissionStatuses.Select(submission => new SectionStatusNew() + private static IQueryable GetLatestSubmissionStatus(IQueryable submissionStatuses, bool completed) + => submissionStatuses.Select(submission => new SectionStatus() { DateCreated = submission.DateCreated, Completed = submission.Completed, @@ -56,8 +57,5 @@ private static IQueryable 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()); } diff --git a/src/Dfe.PlanTech.Application/Submissions/Queries/SubmissionStatusProcessor.cs b/src/Dfe.PlanTech.Application/Submissions/Queries/SubmissionStatusProcessor.cs index 1258f3b33..7ff300295 100644 --- a/src/Dfe.PlanTech.Application/Submissions/Queries/SubmissionStatusProcessor.cs +++ b/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, diff --git a/src/Dfe.PlanTech.DatabaseUpgrader/Scripts/2024/20240827_1056_CreateGetCurrentSubmissionIdProcedure.sql b/src/Dfe.PlanTech.DatabaseUpgrader/Scripts/2024/20240827_1056_CreateGetCurrentSubmissionIdProcedure.sql new file mode 100644 index 000000000..f67e5c7da --- /dev/null +++ b/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 +) \ No newline at end of file diff --git a/src/Dfe.PlanTech.DatabaseUpgrader/Scripts/2024/20240827_1058_UpdateSelectOrInsertSubmissionIdProcedure.sql b/src/Dfe.PlanTech.DatabaseUpgrader/Scripts/2024/20240827_1058_UpdateSelectOrInsertSubmissionIdProcedure.sql new file mode 100644 index 000000000..c6f97bf5f --- /dev/null +++ b/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 diff --git a/src/Dfe.PlanTech.DatabaseUpgrader/Scripts/2024/20240827_1102_UpdateDeleteCurrentSubmissionProcedure.sql b/src/Dfe.PlanTech.DatabaseUpgrader/Scripts/2024/20240827_1102_UpdateDeleteCurrentSubmissionProcedure.sql new file mode 100644 index 000000000..cb4e6f4df --- /dev/null +++ b/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 diff --git a/src/Dfe.PlanTech.Domain/Interfaces/IGetSubmissionStatusesQuery.cs b/src/Dfe.PlanTech.Domain/Interfaces/IGetSubmissionStatusesQuery.cs index fdcf7ba06..4444729ba 100644 --- a/src/Dfe.PlanTech.Domain/Interfaces/IGetSubmissionStatusesQuery.cs +++ b/src/Dfe.PlanTech.Domain/Interfaces/IGetSubmissionStatusesQuery.cs @@ -7,7 +7,7 @@ public interface IGetSubmissionStatusesQuery { Task> GetSectionSubmissionStatuses(string categoryId); - Task GetSectionSubmissionStatusAsync(int establishmentId, + Task GetSectionSubmissionStatusAsync(int establishmentId, ISectionComponent section, bool completed, CancellationToken cancellationToken); diff --git a/src/Dfe.PlanTech.Domain/Submissions/Interfaces/ISubmissionStatusProcessor.cs b/src/Dfe.PlanTech.Domain/Submissions/Interfaces/ISubmissionStatusProcessor.cs index 5407ded4d..7c0afd85c 100644 --- a/src/Dfe.PlanTech.Domain/Submissions/Interfaces/ISubmissionStatusProcessor.cs +++ b/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); diff --git a/src/Dfe.PlanTech.Domain/Submissions/Models/SectionStatus.cs b/src/Dfe.PlanTech.Domain/Submissions/Models/SectionStatus.cs index 679fa36c6..d0381f9a7 100644 --- a/src/Dfe.PlanTech.Domain/Submissions/Models/SectionStatus.cs +++ b/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!; diff --git a/src/Dfe.PlanTech.Domain/Submissions/Models/SectionStatusDto.cs b/src/Dfe.PlanTech.Domain/Submissions/Models/SectionStatusDto.cs new file mode 100644 index 000000000..a4dd9f90f --- /dev/null +++ b/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; } +} diff --git a/src/Dfe.PlanTech.Web.Node/styles/scss/overrides.scss b/src/Dfe.PlanTech.Web.Node/styles/scss/overrides.scss index 59c145712..471d07f69 100644 --- a/src/Dfe.PlanTech.Web.Node/styles/scss/overrides.scss +++ b/src/Dfe.PlanTech.Web.Node/styles/scss/overrides.scss @@ -1,81 +1,85 @@ @use "../../node_modules/dfe-frontend/packages/dfefrontend.scss"; * { - font-family: BlinkMacSystemFont, "Segoe UI", Roboto, Oxygen-Sans, Ubuntu, - Cantarell, "Helvetica Neue", sans-serif !important; + font-family: BlinkMacSystemFont, "Segoe UI", Roboto, Oxygen-Sans, Ubuntu, + Cantarell, "Helvetica Neue", sans-serif !important; } .logo { - display: block; - max-height: 32px; - max-width: 100%; + display: block; + max-height: 32px; + max-width: 100%; } .govuk-table { - margin-top: 30px; - margin-bottom: 30px; + margin-top: 30px; + margin-bottom: 30px; } a.govuk-home-link { - position: relative; - display: inline-block; - margin-top: 15px; - margin-bottom: 15px; + position: relative; + display: inline-block; + margin-top: 15px; + margin-bottom: 15px; } //Remove the padding from paragraphs within lists li { - p.govuk-body { - margin-top: 0px; - margin-bottom: 0px; - } + p.govuk-body { + margin-top: 0px; + margin-bottom: 0px; + } } div.govuk-width-container { - @extend .dfe-width-container; + @extend .dfe-width-container; } strong.govuk-tag { - max-width: fit-content; - vertical-align: middle; + max-width: fit-content; + vertical-align: middle; - &:first-letter { - text-transform: uppercase; - } + &:first-letter { + text-transform: uppercase; + } } @media (min-width: 40.0625em) { - .dfe-self-assessment-list { - > .govuk-summary-list__row { - height: 4rem; + .dfe-self-assessment-list { + > .govuk-summary-list__row { + height: 4rem; + } } - } } .govuk-summary-list__key, .govuk-summary-list__value { - vertical-align: middle; - - > .govuk-button { vertical-align: middle; - } + + > .govuk-button { + vertical-align: middle; + } } .dfe-header { - border-bottom: 10px solid #347ca9 + border-bottom: 10px solid #347ca9; } .share-html-img { - width: 100px; - float: left; - padding-right: 10px; + width: 100px; + float: left; + padding-right: 10px; } //Hide GTM iframe iframe { - display: none; + display: none; } .whitespace-preline { - white-space: pre-line; + white-space: pre-line; +} + +button.govuk-button--secondary { + background-color: white; } diff --git a/src/Dfe.PlanTech.Web/Views/Shared/_Footer.cshtml b/src/Dfe.PlanTech.Web/Views/Shared/_Footer.cshtml index f2e52c8d1..f990657e8 100644 --- a/src/Dfe.PlanTech.Web/Views/Shared/_Footer.cshtml +++ b/src/Dfe.PlanTech.Web/Views/Shared/_Footer.cshtml @@ -2,7 +2,7 @@