implements default signing task validator #1046

cammiida · 2025-01-20T11:50:40Z

Description

Default validator for all signing tasks. Runs by default if not runDefaultValidator="false" in process.bpmn.

Should it be called runDefaultValidations instead? 🤷‍♀️

Verification

Your code builds clean without any errors or warnings
Manual testing done (required)
Relevant automated test added (if you find this hard, leave it and we'll help out)
All tests run green

Documentation

User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)

cammiida · 2025-01-20T14:36:05Z

/publish

…nd added function docs

cammiida · 2025-01-21T11:04:59Z

/publish

github-actions · 2025-01-21T11:05:19Z

PR release:

⚙️ Building...
✅ Done!

danielskovli

LGTM 🥇

src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs

HauklandJ

🙌

HauklandJ · 2025-01-21T13:45:30Z

src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs

+            _processReader.GetAltinnTaskExtension(taskId)?.SignatureConfiguration
+        );
+
+        if (signingConfiguration == null)


Suggested change

if (signingConfiguration == null)

if (signingConfiguration is null)

HauklandJ · 2025-01-21T13:45:34Z

src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs

+        }
+
+        var (getAppMetadataError, appMetadata) = await CatchError(_appMetadata.GetApplicationMetadata());
+        if (getAppMetadataError != null || appMetadata == null)


Suggested change

if (getAppMetadataError != null || appMetadata == null)

if (getAppMetadataError is not null || appMetadata is null)

HauklandJ · 2025-01-21T13:45:54Z

src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs

+        var (getSigneeContextsError, signeeContexts) = await CatchError(
+            _signingService.GetSigneeContexts(cachedDataMutator, signingConfiguration)
+        );
+        if (getSigneeContextsError != null || signeeContexts == null)


Suggested change

if (getSigneeContextsError != null || signeeContexts == null)

if (getSigneeContextsError is not null || signeeContexts is null)

HauklandJ · 2025-01-21T13:46:07Z

src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs

+            return [];
+        }
+
+        var allHaveSigned = signeeContexts.All(signeeContext => signeeContext.SignDocument != null);


Suggested change

var allHaveSigned = signeeContexts.All(signeeContext => signeeContext.SignDocument != null);

var allHaveSigned = signeeContexts.All(signeeContext => signeeContext.SignDocument is not null);

HauklandJ · 2025-01-21T13:47:38Z

src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs

+    /// <summary>
+    /// Catch exceptions from a task and return them as a tuple with the result.
+    /// </summary>
+    private static async Task<Tuple<Exception?, T?>> CatchError<T>(Task<T> task)


Should this be an internal utility?

HauklandJ · 2025-01-21T13:57:49Z

src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs

+
+        AltinnSignatureConfiguration? signingConfiguration = taskConfig?.SignatureConfiguration;
+
+        return signingConfiguration?.RunDefaultValidator == true && taskConfig?.TaskType == "signing";


It would be nice to have a "TaskTypeConst" similar to LanguageConst, but I consider it outside the scope of this pr. I will create and issue for it.

sonarqubecloud · 2025-01-21T14:00:33Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
84.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

ivarne · 2025-01-21T14:32:48Z

src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs

+        }
+        catch (Exception ex)
+        {
+            _logger.LogError(ex, $"Error while fetching signing configuration for task {taskId}");


This log message will be noisy and confusing if a ProcessElement is missing the TaskExtension tag, because this ShouldRunForTask runs on all tasks (also those that are not signature tasks)

ivarne · 2025-01-21T14:37:46Z

src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs

+    /// </summary>
+    public Task<bool> HasRelevantChanges(IInstanceDataAccessor dataAccessor, string taskId, DataElementChanges changes)
+    {
+        throw new NotImplementedException();


We can add the documentation as a message, so that it is easier to understand why this is not implemented.

Suggested change

throw new NotImplementedException();

throw new UnreachableException("HasRelevantChanges should never be called because NoIncrementalValidation is true.");

This might need to import using System.Diagnostics;

ivarne · 2025-01-21T14:39:14Z

src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs

+        if (signingConfiguration == null)
+        {
+            _logger.LogError($"No signing configuration found for task {taskId}");
+            return [];


This was checked by ShouldRunForTask, so throw here as well.

ivarne · 2025-01-21T14:56:19Z

src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs

+        );
+
+        var (getSigneeContextsError, signeeContexts) = await CatchError(
+            _signingService.GetSigneeContexts(cachedDataMutator, signingConfiguration)


You should be able to get the signeeContexts from the dataAccessor instead of instantiating a Mutator in a validator.

You would need to update the signatures in the signing service to use IInstanceDataAccessor

implements default signing task validator

0f20cbc

cammiida added the feature Label Pull requests with new features. Used when generation releasenotes label Jan 20, 2025

cammiida added 3 commits January 20, 2025 14:45

adds and fixes tests

d703796

makes catcherror func private

1661405

screams null instead of whispering. Also removes logger

1e41c73

adds process bpmn param to turn off default signing task validation a…

ac90843

…nd added function docs

cammiida marked this pull request as ready for review January 21, 2025 11:04

cammiida linked an issue Jan 21, 2025 that may be closed by this pull request

Standard implementation of signing task validator #801

Open

danielskovli approved these changes Jan 21, 2025

View reviewed changes

src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs Outdated Show resolved Hide resolved

src/Altinn.App.Core/Features/Validation/Default/SigningTaskValidator.cs Outdated Show resolved Hide resolved

makes SigningTaskValidator sealed and using is instead of double equals

9f46aea

bjorntore approved these changes Jan 21, 2025

View reviewed changes

HauklandJ approved these changes Jan 21, 2025

View reviewed changes

ivarne reviewed Jan 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implements default signing task validator #1046

implements default signing task validator #1046

cammiida commented Jan 20, 2025 •

edited

Loading

cammiida commented Jan 20, 2025

cammiida commented Jan 21, 2025

github-actions bot commented Jan 21, 2025 •

edited

Loading

danielskovli left a comment

HauklandJ left a comment

HauklandJ Jan 21, 2025

HauklandJ Jan 21, 2025

HauklandJ Jan 21, 2025

HauklandJ Jan 21, 2025

HauklandJ Jan 21, 2025

HauklandJ Jan 21, 2025

sonarqubecloud bot commented Jan 21, 2025

ivarne Jan 21, 2025

ivarne Jan 21, 2025

ivarne Jan 21, 2025

ivarne Jan 21, 2025

ivarne Jan 21, 2025

	if (signingConfiguration == null)
	if (signingConfiguration is null)

	if (getAppMetadataError != null \|\| appMetadata == null)
	if (getAppMetadataError is not null \|\| appMetadata is null)

	if (getSigneeContextsError != null \|\| signeeContexts == null)
	if (getSigneeContextsError is not null \|\| signeeContexts is null)

	var allHaveSigned = signeeContexts.All(signeeContext => signeeContext.SignDocument != null);
	var allHaveSigned = signeeContexts.All(signeeContext => signeeContext.SignDocument is not null);


		AltinnSignatureConfiguration? signingConfiguration = taskConfig?.SignatureConfiguration;

		return signingConfiguration?.RunDefaultValidator == true && taskConfig?.TaskType == "signing";

	throw new NotImplementedException();
	throw new UnreachableException("HasRelevantChanges should never be called because NoIncrementalValidation is true.");

implements default signing task validator #1046

Are you sure you want to change the base?

implements default signing task validator #1046

Conversation

cammiida commented Jan 20, 2025 • edited Loading

Description

Verification

Documentation

cammiida commented Jan 20, 2025

cammiida commented Jan 21, 2025

github-actions bot commented Jan 21, 2025 • edited Loading

PR release:

danielskovli left a comment

Choose a reason for hiding this comment

HauklandJ left a 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

sonarqubecloud bot commented Jan 21, 2025

Quality Gate passed

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

cammiida commented Jan 20, 2025 •

edited

Loading

github-actions bot commented Jan 21, 2025 •

edited

Loading