EES-5238 - adding new HTTP trigger and orchestration for completing the next data set version import #5038

duncan-at-hiveit · 2024-07-09T12:33:23Z

Overview

This PR:

adds an HTTP trigger for kicking off the completion of the next data set version import after mapping is complete.
adds an orchestration that is triggered by the HTTP trigger. Currently the additional logic to use the mappings to complete the import is not implemented, but will be added into this workflow as part of EES-4954.
adds validation and tests around the new trigger and orchestration.

Other miscellaneous changes

Reusing and renaming CreateDataSetResponseViewModel

All 3 of the HTTP trigger Functions were returning the same response details, so I renamed CreateDataSetResponseViewModel to ProcessDataSetVersionResponseViewModel and reused across all 3. This has led to a few little changes in other miscellaneous files.

New "ManualMapping" import stage

As the import process is effectively "paused" at this stage of processing until the user finishes the manual mapping, I added another stage to reflect this. Upon triggering this new orchestration, the import will "unpause" and continue to completion.

A new InstanceId will be assigned to the import entity so as not to confuse a new orchestration with an already-run one!

It didn't have to be done this way - we could also have completed the 1st import entity and then created a 2nd import entity to start off the second stage of the import, but to me personally this felt like more of an intuitive way to model it.

jack-hive · 2024-07-11T09:52:23Z

...on.ExploreEducationStatistics.Public.Data.Processor.Requests/DataSetVersionProcessRequest.cs

Wondering if a name of CompleteNextDataSetVersionImportRequest might be more accurate? I know it's more verbose but this request is specific to this trigger I think? So might be useful to make it very explicit

I had originally planned for it to be reusable for any orchestrations that simply require a DataSetVersionId to operate, but since this is the only one so far, I'm happy to rename it.

In keeping with the existing naming convention, I've gone with NextDataSetVersionCompleteRequest alongside its counterpart NextDataSetVersionCreateMappingsRequest, which I've renamed from NextDataSetVersionCreateRequest.

jack-hive · 2024-07-11T09:59:01Z

...cationStatistics.Public.Data.Processor/Functions/CompleteNextDataSetVersionImportFunction.cs

+        if (nextVersion is null)
+        {
+            return CreateDataSetVersionIdError(
+                message: ValidationMessages.DataSetVersionNotFound,
+                dataSetVersionId: request.DataSetVersionId);
+        }


a 404 might be more appropriate here? But I guess our 400s are more descriptive than our empty 404s. But I reckon I'd have 404'd it if I'd have written it myself. But I can be swayed either way on this one!

Added a new ValidationUtils.NotFoundResult() method and equivalent assertion ActionResultTestExtensions.AssertNotFoundWithValidationProblem() method for testing the generated errors.

jack-hive · 2024-07-11T10:23:16Z

...cationStatistics.Public.Data.Processor/Functions/CompleteNextDataSetVersionImportFunction.cs

+            .Validate(request, cancellationToken)
+            .OnSuccess(_ => GetNextDataSetVersionInMappingStatus(request, cancellationToken))
+            .OnSuccessDo(_ => GetCompletedDataSetVersionMapping(request, cancellationToken))
+            .OnSuccessCombineWith(nextDataSetVersion =>
+                GetImportInManualMappingStage(request, nextDataSetVersion))
+            .OnSuccess(async versionAndImport =>
+            {
+                var (nextVersion, importToContinue) = versionAndImport;
+
+                importToContinue.InstanceId = Guid.NewGuid();
+                publicDataDbContext.DataSetVersionImports.Update(importToContinue);
+                await publicDataDbContext.SaveChangesAsync(cancellationToken);
+
+                await ProcessCompletionOfNextDataSetVersionImport(
+                    client,
+                    dataSetVersionId: nextVersion.Id,
+                    instanceId: importToContinue.InstanceId,
+                    cancellationToken);
+
+                return new ProcessDataSetVersionResponseViewModel
+                {
+                    DataSetId = nextVersion.DataSetId,
+                    DataSetVersionId = nextVersion.Id,
+                    InstanceId = importToContinue.InstanceId
+                };
+            })
+            .HandleFailuresOr(result => new OkObjectResult(result));


Not really thought about this until this PR, but I'm wondering if it would be better to put all this logic in a service layer, and treat this like a controller such that it isn't responsible for the business logic. But I realise we haven't been doing this everywhere currently.

Suggesting that I absorb this change into 4954 if that's OK?

jack-hive · 2024-07-11T10:27:05Z

...cationStatistics.Public.Data.Processor/Functions/CompleteNextDataSetVersionImportFunction.cs

+        CancellationToken cancellationToken)
+    {
+        const string orchestratorName =
+            nameof(ProcessCompletionOfNextDataSetVersionFunction.CompleteNextDataSetVersionImportProcessing);


should this be nameof(ProcessCompletionOfNextDataSetVersionFunction.ProcessCompletionOfNextDataSetVersion)?

or are we waiting until 4954 to switch it out?

Nope you're right, it's the incorrect name here. Changed, thanks

jack-hive · 2024-07-11T10:39:49Z

...stics.Public.Data.Processor.Tests/Functions/CompleteNextDataSetVersionImportFunctionTests.cs

+        }
+
+        [Fact]
+        public async Task DataSetVersionNotInMappingStatus_ReturnsValidationProblem()


could turn this into a [Theory] and test every other Status

What a fine idea, changed to a theory ta.

jack-hive · 2024-07-11T10:40:45Z

...stics.Public.Data.Processor.Tests/Functions/CompleteNextDataSetVersionImportFunctionTests.cs

+        }
+
+        [Fact]
+        public async Task DataSetVersionImportInManualMappingStateNotFound_ReturnsValidationProblem()


jack-hive · 2024-07-11T10:43:56Z

...stics.Public.Data.Processor.Tests/Functions/CompleteNextDataSetVersionImportFunctionTests.cs

+        : CompleteNextDataSetVersionImportFunctionTests(fixture)
+    {
+        [Fact]
+        public async Task Success()


Maybe we could add some checks for:

the other viewmodel properties of responseViewModel, to make sure they've all been set properly

grab the import from the DB, and make sure the status has been updated to Completing

grab the data set version from the DB, and make sure the status has been updated to Draft

So this test doesn't actually run any of the functions in the orchestration - it just verifies that the orchestration runs through the correct list of Functions, and any updates prior to running the orchestration have been made (e.g. creating an empty DataSet before running the "initial data set version" orchestration), so there are no other db updates that would be looked for in this particular test.

I have however added in checks for the other viewmodel fields as suggested, ta.

jack-hive · 2024-07-11T10:48:56Z

...c.Data.Processor.Tests/Functions/ProcessCompletionOfNextDataSetVersionImportFunctionTests.cs

+
+namespace GovUk.Education.ExploreEducationStatistics.Public.Data.Processor.Tests.Functions;
+
+public abstract class ProcessCompletionOfNextDataSetVersionImportFunctionTests(


I think we're missing some tests for ProcessCompletionOfNextDataSetVersionFunction.CompleteNextDataSetVersionImportProcessing

Haha yup, I have updated ProcessInitialDataSetVersionFunctionTests to be CompleteNextDataSetVersionImportProcessingTests, and testing the correct function method now thanks!

jack-hive · 2024-07-11T10:49:23Z

...c.Data.Processor.Tests/Functions/ProcessCompletionOfNextDataSetVersionImportFunctionTests.cs

+        }
+    }
+
+    public class CompleteInitialDataSetVersionProcessingTests(


I think these tests should be in ProcessInitialDataSetVersionFunctionTests.cs?

Haha nope, this was supposed to be CompleteNextDataSetVersionImportProcessingTests testing the completion stage from ProcessCompletionOfNextDataSetVersionFunction.

I have now updated thanks!

jack-hive

Look great!

…on and renaming orchestration for creating next data set version mappings, prior to adding new orchestrations

…leting processing of the next data set version.

…ion is ready to be imported after mapping is complete. Added tests around new validation and success scenarios.

…when principal entities do not exist when processing requests in the Processor project. Various renames of request classes.

duncan-at-hiveit force-pushed the EES-5238-add-new-orchestration-for-completing-next-data-set-version-import branch 4 times, most recently from 46d6687 to 1379b40 Compare July 9, 2024 13:05

Base automatically changed from EES-5238-break-out-shared-functions-prior-to-adding-new-orchestration to dev July 9, 2024 13:07

duncan-at-hiveit force-pushed the EES-5238-add-new-orchestration-for-completing-next-data-set-version-import branch from 1379b40 to 1b304fe Compare July 9, 2024 13:08

jack-hive requested changes Jul 11, 2024

View reviewed changes

duncan-at-hiveit force-pushed the EES-5238-add-new-orchestration-for-completing-next-data-set-version-import branch 2 times, most recently from 68471fe to 1e1a27b Compare July 15, 2024 14:21

jack-hive approved these changes Jul 16, 2024

View reviewed changes

duncan-at-hiveit added 6 commits July 16, 2024 12:50

EES-5238 - breaking out shared ImportDataFunction and WriteDataFuncti…

5615740

…on and renaming orchestration for creating next data set version mappings, prior to adding new orchestrations

EES-5238 - added new HTTP trigger function and orchestration for comp…

267ce4a

…leting processing of the next data set version.

EES-5238 - adding state validation to ensure the targeted DataSetVers…

2809f05

…ion is ready to be imported after mapping is complete. Added tests around new validation and success scenarios.

EES-5238 - tidy-up prior to raising PR

691d2bb

EES-5238 - responding to various PR comments. Swapping 400s for 404s …

ffc5b96

…when principal entities do not exist when processing requests in the Processor project. Various renames of request classes.

EES-5238 - correcting name of orchestration and adding missing test

ad59299

duncan-at-hiveit force-pushed the EES-5238-add-new-orchestration-for-completing-next-data-set-version-import branch from 1e1a27b to ad59299 Compare July 16, 2024 11:51

duncan-at-hiveit merged commit a899c30 into dev Jul 16, 2024
2 of 7 checks passed

duncan-at-hiveit deleted the EES-5238-add-new-orchestration-for-completing-next-data-set-version-import branch July 16, 2024 11:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EES-5238 - adding new HTTP trigger and orchestration for completing the next data set version import #5038

EES-5238 - adding new HTTP trigger and orchestration for completing the next data set version import #5038

duncan-at-hiveit commented Jul 9, 2024 •

edited

Loading

jack-hive Jul 11, 2024

duncan-at-hiveit Jul 12, 2024 •

edited

Loading

jack-hive Jul 11, 2024

duncan-at-hiveit Jul 15, 2024

jack-hive Jul 11, 2024

duncan-at-hiveit Jul 15, 2024

jack-hive Jul 11, 2024

jack-hive Jul 11, 2024

duncan-at-hiveit Jul 15, 2024

jack-hive Jul 11, 2024

duncan-at-hiveit Jul 15, 2024

jack-hive Jul 11, 2024

duncan-at-hiveit Jul 15, 2024

jack-hive Jul 11, 2024

duncan-at-hiveit Jul 15, 2024

jack-hive Jul 11, 2024

duncan-at-hiveit Jul 15, 2024

jack-hive Jul 11, 2024

duncan-at-hiveit Jul 15, 2024

jack-hive left a comment


		namespace GovUk.Education.ExploreEducationStatistics.Public.Data.Processor.Tests.Functions;

		public abstract class ProcessCompletionOfNextDataSetVersionImportFunctionTests(

EES-5238 - adding new HTTP trigger and orchestration for completing the next data set version import #5038

EES-5238 - adding new HTTP trigger and orchestration for completing the next data set version import #5038

Conversation

duncan-at-hiveit commented Jul 9, 2024 • edited Loading

Overview

Other miscellaneous changes

Reusing and renaming CreateDataSetResponseViewModel

New "ManualMapping" import stage

Choose a reason for hiding this comment

duncan-at-hiveit Jul 12, 2024 • edited Loading

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

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

jack-hive left a comment

Choose a reason for hiding this comment

duncan-at-hiveit commented Jul 9, 2024 •

edited

Loading

duncan-at-hiveit Jul 12, 2024 •

edited

Loading