refactor: Reactivate workflow locking #4770
Conversation
n8n-assistant
bot
added
core
| @@ -206,6 +207,17 @@ export class WorkflowsService { | |||
| ); | |||
| } | |||
|
|
|||
| // Update the workflow's hash | |||
| workflow.hash = uuid(); | |||
There was a problem hiding this comment.
If we use @BeforeUpdate() to set this in WorkflowEntity, then we ca be certain that this get updated on every update, irrespective of where it comes from in the code.
There was a problem hiding this comment.
Fair point, but we have another PR to be merged (#4200 ) and it will break if we do so. We'll be updating the workflow_entity on other stuff than only workflow editions, leading to more false positives. For this reason I think it's best to only update the hash when actual changes happen.
There was a problem hiding this comment.
No issues so far on manual testing.
Two UX questions:
- Blocker message is way too long - display duration does not allow to read it in full at normal pace, even when expecting it. And even if we extend duration, blocker message is still probably too long for a simple toast message. Also in the blocker message it would be nice to include a link to open the workflow in a separate tab.
- I noticed at
/workflows, if a user activates a workflow, a sharee is still able to freely activate/deactivate that same workflow at/workflows, and vice versa. Is this expected? Should they not be blocked?
packages/cli/src/databases/migrations/mysqldb/1669739707125-AddWorkflowHashColumn.ts
Outdated
Show resolved
Hide resolved
| `); | ||
|
|
||
| workflowIds.map(({ id }) => { | ||
| const [updateQuery, updateParams] = queryRunner.connection.driver.escapeQueryWithParameters( |
There was a problem hiding this comment.
Some UUIDs might need escaping? Or why do we need this?
There was a problem hiding this comment.
No special reason, it's just good practice to send queries through the escape mechanism
There was a problem hiding this comment.
doesn't the query-runner do that automatically in the next line?
| /** | ||
| * Hash of editable workflow state. | ||
| */ | ||
| @Column({ length: 36 }) |
There was a problem hiding this comment.
We are no longer hashing, so versionId instead of hash?
| @@ -213,7 +216,7 @@ EEWorkflowController.patch( | |||
| '/:id(\\d+)', | |||
| ResponseHelper.send(async (req: WorkflowRequest.Update) => { | |||
| const { id: workflowId } = req.params; | |||
| // const forceSave = req.query.forceSave === 'true'; // disabled temporarily - tests were also disabled | |||
| const forceSave = req.query.forceSave === 'true'; | |||
There was a problem hiding this comment.
If you can please unskip and update the relevant tests, and ensure they pass on all three DBs.
There was a problem hiding this comment.
Thanks for reminding me this, tests uncommented and fixed.
| if ( | ||
| !forceSave && | ||
| workflow.versionId !== '' && | ||
| workflow.versionId !== shared.workflow.versionId | ||
| ) { |
There was a problem hiding this comment.
Current error message below does not seem relevant anymore?
Since client-side we now have a confirmation modal, should the message in the service here simply state that the attempted change was blocked because of interim updates without force saving?
| @@ -41,8 +41,8 @@ abstract class ResponseError extends Error { | |||
| } | |||
|
|
|||
| export class BadRequestError extends ResponseError { | |||
| constructor(message: string) { | |||
| super(message, 400); | |||
| constructor(message: string, errorCode?: number) { | |||
There was a problem hiding this comment.
To prevent confusion, since "error code 100" already means something else:
| constructor(message: string, errorCode?: number) { | |
| constructor(message: string, internalErrorCode?: number) { |
* master: fix: Handle error when workflow does not exist or is inaccessible (#4831) feat(editor): Schema view (#4615) fix: Enable source-maps on WorkflowRunnerProcess in `own` mode (#4832) ci: Fix linting on master (no-changelog) (#4830) feat: Add message for readonly nodes. Improve foreign credentials handling (#4759) feat(KoBoToolbox Node): Add support for Media file API (#4578) fix(Local File Trigger Node): Fix issue that causes a crash if the ignore field is empty (#4824) (#4825) refactor: Codex updates for XML and HtmlExtract Nodes (#4801) refactor: Reactivate workflow locking (#4770) # Conflicts: # packages/editor-ui/src/Interface.ts
|
Got released with |
No description provided.