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

fix(editor): Prevent saving workflow while another save is in progress #9048

Merged
merged 5 commits into from Apr 4, 2024

Conversation

MiloradFilipovic
Copy link
Contributor

Summary

This PR fixes a bug where users would end up with duplicate workflows when clicking Save button straight from the name input.

Related tickets and issues

Fixes ADO-2077

Review / Merge checklist

  • PR title and summary are descriptive. Remember, the title automatically goes into the changelog. Use (no-changelog) otherwise. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.

    A bug is not considered fixed, unless a test is added to prevent it from happening again.
    A feature is not complete without tests.

@MiloradFilipovic MiloradFilipovic self-assigned this Apr 3, 2024
@n8n-assistant n8n-assistant bot added n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels Apr 3, 2024
const saved = await this.workflowHelpers.saveCurrentWorkflow({ name });
if (saved) {
this.isNameEditEnabled = false;
}
this.uiStore.removeActiveAction('workflowSaving');
Copy link
Contributor

Choose a reason for hiding this comment

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

Would probably be good to do this in a try ... finally to make sure we are never left in an inconsistent state

WorkflowPage.actions.visit();
WorkflowPage.getters.workflowNameInput().invoke('val').then((oldName) => {
WorkflowPage.getters.workflowNameInputContainer().click();
WorkflowPage.getters.workflowNameInput().type('{selectall}');
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this some Cypress magic to select all the text?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tomi Yeah, there is a few of those handy options. You can see them here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👌

* master:
  fix(core): Ensure `status` on Axios errors is available to the BE (#9015)
  fix(editor): Make Webhook node pinnable (#9047)
  fix(core): Upgrade express to address CVE-2024-29041 (no-changelog) (#9021)
  fix(editor): Add fallback for expression resolution in multi-output case (#9045)
  ci(core): Upgrade express-openapi-validator (no-changelog) (#9022)
  refactor(core): Delete all auth exclusion config and checks (no-changelog) (#9044)
  fix(editor): Make share modal content scrollable (#9025)
tomi
tomi previously approved these changes Apr 4, 2024
Copy link
Contributor

@tomi tomi left a comment

Choose a reason for hiding this comment

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

👌

if (saved) {
this.isNameEditEnabled = false;
}
// Exception is handled in the helper function
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I didn't realize that saveCurrentWorkflow already handles exceptions 🙈 In that case the try..finally is probably not needed. Feel free to remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was a bit surprised by try request but thought you saw something I didn't :D Let's remove it since it's cleaner without it

Copy link
Contributor

github-actions bot commented Apr 4, 2024

✅ All Cypress E2E specs passed

Copy link

cypress bot commented Apr 4, 2024

2 flaky tests on run #4550 ↗︎

0 349 12 0 Flakiness 2

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 MiloradFilipovic 🗃️ e2e/*
Project: n8n Commit: b088bdf94c
Status: Passed Duration: 04:34 💡
Started: Apr 4, 2024 7:50 AM Ended: Apr 4, 2024 7:54 AM
Flakiness  cypress/e2e/5-ndv.cy.ts • 2 flaky tests

View Output Video

Test Artifacts
NDV > should not retrieve remote options when required params throw errors Screenshots Video
NDV > Stop listening for trigger event from NDV Screenshots Video

Review all test suite changes for PR #9048 ↗︎

@MiloradFilipovic MiloradFilipovic merged commit 3c9a1d2 into master Apr 4, 2024
28 checks passed
@MiloradFilipovic MiloradFilipovic deleted the ADO-2077-prevent-saving-while-saving branch April 4, 2024 07:59
MiloradFilipovic added a commit that referenced this pull request Apr 4, 2024
* master:
  fix(core): Ensure only leader handles waiting executions (#9014)
  fix(editor): Fix execution with wait node (#9051)
  fix(editor): Issue showing Auth2 callback section when all properties are overriden (#8999)
  fix(editor): Rerun failed nodes in manual executions (#9050)
  fix(editor): Canvas showing error toast when clicking outside of "import workflow by url" modal (#9001)
  fix: Workflows executed from other workflows not stopping (#9010)
  fix: Fix missing input panel in node details view (#9043)
  fix(editor): Prevent saving workflow while another save is in progress (#9048)
@github-actions github-actions bot mentioned this pull request Apr 4, 2024
@janober
Copy link
Member

janober commented Apr 5, 2024

Got released with n8n@1.36.1

@github-actions github-actions bot mentioned this pull request Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
n8n team Authored by the n8n team Released ui Enhancement in /editor-ui or /design-system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants