Conversation
…of empty start event
This comment has been minimized.
This comment has been minimized.
| const { definitionId, version } = req.params; | ||
| const engine = management.getEngineWithDefinitionId(definitionId); | ||
|
|
||
| if (!engine) { |
There was a problem hiding this comment.
One could potentially argue that a missing engine means that the version is in the state of "not active". In that case you could theoretically return a success here.
Maybe you should just check if the version is deployed to this engine. If not return an error and if yes return active as false if no engine is found or when the specific version is not loaded in the engine.
This way we get the semantic meaning that any version that was deployed to a machine is either in an active or inactive state and only querying for the active state of a non-existing version is considered an error.
The way this is currently implemented you get errors in the MS backend when the version is not loaded into an engine.
Steps to reproduce:
- Deploy a process with a timer to an engine
- Stop the engine (e.g. Ctrl+C in the terminal)
- Start the engine again
- Open the deployment view for the process
- Look in to the log of the ms backend
| }; | ||
| } | ||
|
|
||
| const process = engine._versionProcessMapping[version]; |
There was a problem hiding this comment.
I think I wouldn't use these kinds of access to "private" members of the engine outside the file in which engine is defined. Maybe create a new function "isProcessVersionDeployed" similar to "undeployProcessVersion" in "engine.js" and check in there if the version is deployed in the execution environment.
|
|
||
| const process = engine._versionProcessMapping[version]; | ||
|
|
||
| if (!process) { |
There was a problem hiding this comment.
See my comment about what might be considered "not active" above.
| import useInstanceVariables from './use-instance-variables'; | ||
| import { inlineScript, inlineUserTaskData } from '@proceed/user-task-helper'; | ||
| import { toBpmnObject, getStartEvents, getElementById } from '@proceed/bpmn-helper'; | ||
| import { useParams } from 'next/navigation'; |
There was a problem hiding this comment.
This import does not seem to be used anymore.
|
|
||
| if (!engines.length) return false; | ||
|
|
||
| const result = await getDeploymentActivation(engines[0], definitionId, version); |
There was a problem hiding this comment.
The assignment to "result" is not necessary since you are just returning it in the next step.
| } | ||
| toBpmnObject(currentVersion.bpmn) | ||
| .then(async (bpmnObj) => { | ||
| const startEventIds = await getStartEvents(bpmnObj); |
There was a problem hiding this comment.
You could skip a step by doing const startEvents = await getElementsByTagName(bpmnObj, 'bpmn:StartEvent') instead.
| setHasPlainStartEvents(false); | ||
| return; | ||
| } | ||
| toBpmnObject(currentVersion.bpmn) |
There was a problem hiding this comment.
I think I would find it a bit better to use an async function instead of a promise and then a "then".
Basically
useEffect(() => {
...
async function initStartEventInfo() {
try {
const bpmnObj = await toBpmnObject(currentVersion.bpmn);
...
} catch (_) {}
}
initStartEventInfo();
}, [currentVersion?.bpmn]);
There was a problem hiding this comment.
I usually prefer to use promises. I am not sure why, but they just look cleaner and better to me :) Nevertheless, I have updated it to use an async function according to your suggestion.
| useEffect(() => { | ||
| if (!currentVersion) return; | ||
| setIsActivationLoading(true); | ||
| getProcessActivationStatus(processId, activeSpaceId, currentVersion.versionId) |
There was a problem hiding this comment.
Similar as above. I think it would be nicer to use an async function instead of a promise.
| setIsActivationLoading(true); | ||
| getProcessActivationStatus(processId, activeSpaceId, currentVersion.versionId) | ||
| .then(setIsProcessActivated) | ||
| .catch(() => setIsProcessActivated(true)) |
There was a problem hiding this comment.
Is a default of "true" for the process being activated better than "false"?
There was a problem hiding this comment.
I was also very confused about this. I think a better approach would be to use wrapServerCall here and then show the error message in case of error and then setting the value to false. Please have a look.
| <Tooltip | ||
| title={instanceIsPausing ? 'Abort pausing the instance' : 'Resume the instance'} | ||
| > | ||
| {/* New middle group: Start + Activate/Deactivate */} |
There was a problem hiding this comment.
I think I would remove the "New".
This comment has been minimized.
This comment has been minimized.
|
✅ Successfully created Preview Deployment. |
2 participants
Summary
Instance select dropdown now shows the sorted instance (always new instance on top)
Reorganized the Start/Resume/Pause/Stop buttons:
Start new instance button is now conditionally shown based on whether the process has a plain start event.
Added new button with spinning green icon when process is active, static black when inactive. This Button only appears when the current process version has at least one Timer Start Event
Made the Resume/Pause/Stop group to a 2-icon-group. The Resume icon (Play symbol) instead of the Pause icon is now shown if the process is paused.
Added
GET /process/:definitionId/versions/:version/activeendpoint to retrieve the real activation state of a deployed process version from the engineActivation state is fetched from the backend on every page load so navigating away and back always reflects the real state