Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/service task #745

Open
wants to merge 36 commits into
base: main
Choose a base branch
from
Open

Feat/service task #745

wants to merge 36 commits into from

Conversation

bjorntore
Copy link
Contributor

Implementing basic support for service task in bpmn.

Moving PDF and eFormidling into service tasks, but also keep legacy implementations and old way of configuring for backwards compatibility for now.

image

Description

  • IServiceTask inherits from IProcessTask and behaves in the same way in most cases, except it also has a method called Execute which must be implemented.
  • The execute method is called in ProcessNext, right next to where we call user action handlers.
    <bpmn:task id="Task_1" name="Utfylling">
      <bpmn:incoming>SequenceFlow_1n56yn5</bpmn:incoming>
      <bpmn:outgoing>SequenceFlow_1oot28q</bpmn:outgoing>
      <bpmn:extensionElements>
        <altinn:taskExtension>
          <altinn:taskType>data</altinn:taskType>
        </altinn:taskExtension>
      </bpmn:extensionElements>
    </bpmn:task>
    <bpmn:serviceTask id="Task_2" name="PDF">
        <bpmn:incoming>SequenceFlow_1oot28q</bpmn:incoming>
        <bpmn:outgoing>SequenceFlow_4ooasd</bpmn:outgoing>
      <bpmn:extensionElements>
        <altinn:taskExtension>
          <altinn:taskType>pdf</altinn:taskType>
          <altinn:pdfConfig>
            <altinn:filename>some.text.resource.binding</altinn:filename>
            <altinn:tasksToIncludeInPdf>
              <altinn:task>Task_1</altinn:task>
            </altinn:tasksToIncludeInPdf>
          </altinn:pdfConfig>
        </altinn:taskExtension>
      </bpmn:extensionElements>
    </bpmn:serviceTask>
    <bpmn:serviceTask id="Task_3" name="eFormidling">
      <bpmn:incoming>SequenceFlow_4ooasd</bpmn:incoming>
      <bpmn:outgoing>SequenceFlow_5assd2s</bpmn:outgoing>
      <bpmn:extensionElements>
        <altinn:taskExtension>
          <altinn:taskType>eFormidling</altinn:taskType>
        </altinn:taskExtension>
      </bpmn:extensionElements>
    </bpmn:serviceTask>

Related Issue(s)

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)

Copy link

sonarcloud bot commented Sep 4, 2024

Comment on lines 201 to 213
catch (Exception ex)
{
serviceTaskActivity?.Errored(ex);

var result = new ProcessChangeResult()
{
Success = false,
ErrorMessage = $"Server action {altinnTaskType} failed!",
ErrorType = ProcessErrorType.Internal
};
activity?.SetProcessChangeResult(result);
return result;
}

Check notice

Code scanning / CodeQL

Generic catch clause Note

Generic catch clause.
Copy link
Contributor

@martinothamar martinothamar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API and stuff LGTM. We discussed

  • Validation error for trying to invoke actions when current task is a service task
  • Clear/simple migration story (both old and new apps work) - can we just not run the legacy tasks if a PDF service task refers to the current task for example
  • Service tasks failing should probably not have 500 status code for the user/client?
  • Talk to frontenders and Studio team on how the task should be configured

…Add failing test for rejecting PDF service task.
…t for the current task. Done in order to be able to evaluate expressions in gateway while current task is a service task with no layout-set.
…ayout-set for the current task. Done in order to be able to evaluate expressions in gateway while current task is a service task with no layout-set."

This reverts commit ef36453.
_pdfService.VerifyNoOtherCalls();
_appModel.VerifyNoOtherCalls();
}
Mock<ILogger<PdfServiceTask>> loggerMock = new();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any benefit to using it in this test?

Copy link

sonarcloud bot commented Oct 23, 2024

# Conflicts:
#	src/Altinn.App.Core/Features/Validation/Default/DefaultTaskValidator.cs
#	src/Altinn.App.Core/Internal/Process/ProcessEngine.cs
#	test/Altinn.App.Api.Tests/Altinn.App.Api.Tests.csproj
#	test/Altinn.App.Core.Tests/Internal/Process/EventHandlers/ProcessTask/EndTaskEventHandlerTests.cs
#	test/Altinn.App.Core.Tests/Internal/Process/ProcessEngineTest.cs
#	test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/PdfServiceTaskTests.cs
@bjorntore bjorntore added the feature Label Pull requests with new features. Used when generation releasenotes label Nov 25, 2024
Copy link

sonarcloud bot commented Nov 25, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Label Pull requests with new features. Used when generation releasenotes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants