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: Save new version of the workflow instead of the previous (no-changelog) #7428

Merged
merged 5 commits into from
Oct 23, 2023

Conversation

valya
Copy link
Contributor

@valya valya commented Oct 12, 2023

Github issue / Community forum post (link here to close automatically):

@github-actions
Copy link
Contributor

Great PR! Please pay attention to the following items before merging:

Files matching packages/**:

  • If fixing bug, added test to cover scenario.
  • If addressing forum or Github issue, added link to description.

Files matching packages/**/*.ts:

  • Added unit tests to cover new or updated functionality.

Make sure to check off this list before asking for review.

@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (519680c) 33.55% compared to head (89581df) 33.55%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #7428    +/-   ##
========================================
  Coverage   33.55%   33.55%            
========================================
  Files        3398     3399     +1     
  Lines      207597   207453   -144     
  Branches    22428    22417    -11     
========================================
- Hits        69652    69621    -31     
+ Misses     136823   136704   -119     
- Partials     1122     1128     +6     
Files Coverage Δ
...blicApi/v1/handlers/workflows/workflows.handler.ts 85.34% <100.00%> (+2.46%) ⬆️
...lows/workflowHistory/workflowHistory.service.ee.ts 96.42% <100.00%> (+10.71%) ⬆️
packages/cli/src/workflows/workflows.controller.ts 68.59% <100.00%> (+7.05%) ⬆️
packages/cli/src/workflows/workflows.services.ts 57.14% <100.00%> (+1.09%) ⬆️

... and 54 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Oct 12, 2023
@cstuncsik cstuncsik requested a review from krynble October 13, 2023 08:08
Copy link
Contributor

@krynble krynble left a comment

Choose a reason for hiding this comment

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

We might have missed it in the previous PR, but can we add the tests for those? Just to make sure that the function is being called?

krynble
krynble previously approved these changes Oct 23, 2023
Copy link
Contributor

@krynble krynble left a comment

Choose a reason for hiding this comment

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

LGTM, just a few nitpics

@cypress
Copy link

cypress bot commented Oct 23, 2023

1 flaky test on run #2550 ↗︎

0 258 0 0 Flakiness 1

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 valya 🗃️ e2e/*
Project: n8n Commit: 89581df3ee
Status: Passed Duration: 09:06 💡
Started: Oct 23, 2023 2:13 PM Ended: Oct 23, 2023 2:22 PM
Flakiness  cypress/e2e/28-debug.cy.ts • 1 flaky test

View Output Video

Test Artifacts
Debug > should be able to debug executions Output Screenshots Video

Review all test suite changes for PR #7428 ↗︎

@github-actions
Copy link
Contributor

✅ All Cypress E2E specs passed

@github-actions
Copy link
Contributor

✅ All Cypress E2E specs passed

@krynble krynble merged commit 41236b7 into master Oct 23, 2023
18 checks passed
@krynble krynble deleted the fix-workflowhistory-save-new-version branch October 23, 2023 14:30
netroy pushed a commit that referenced this pull request Oct 24, 2023
…ngelog) (#7428)

Github issue / Community forum post (link here to close automatically):
@janober
Copy link
Member

janober commented Oct 24, 2023

Got released with [email protected]

@@ -301,8 +301,8 @@ export class WorkflowsService {
);
}

if (isWorkflowHistoryLicensed()) {
await Container.get(WorkflowHistoryService).saveVersion(user, shared.workflow);
if (isWorkflowHistoryLicensed() && workflow.versionId !== shared.workflow.versionId) {
Copy link
Member

Choose a reason for hiding this comment

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

@valya @krynble
This change is breaking workflow activation/deactivation when isWorkflowHistoryLicensed returns true.

MiloradFilipovic added a commit that referenced this pull request Oct 25, 2023
* master: (30 commits)
  🚀 Release 1.14.0 (#7514)
  ci: Fix oclif manifest generation
  feat(Switch Node): Add support for infinite Switch outputs (#7499)
  🚀 Release 1.13.0 (#7512)
  fix(core): Ensure nodes post-processors run in the correct order (#7500)
  feat(editor): Add PH tracking to event (#7511)
  fix(core): Fix workflow activation with history and workflow history for EE (no-changelog) (#7508)
  refactor(core): Make executions pruning more resilient (#7480)
  fix(Spreadsheet File Node): Fix include empty cells not working with v2 (#7505)
  fix(core): Create instance settings directory recursively (no-changelog) (#7506)
  fix(Microsoft SQL Node): Prevent SQL injection (#7467)
  refactor(core): Make pruning via lifecycle configuration in S3 mode mandatory (#7482)
  fix(core): Always derive `instanceId` from the encryption key (no-changlog) (#7501)
  fix(MQTT Trigger Node): Fix node causing a start up hang when active (#7498)
  feat: Collect usage metrics on license renewal (no-changelog) (#7486)
  fix(core): Fix `frontend.settings` external hook execution (#7496)
  fix(Redis Node): Fix adding sets data types (#7444)
  fix: Save new version of the workflow instead of the previous (no-changelog) (#7428)
  refactor(core): Abstract away InstanceSettings and `encryptionKey` into injectable services (no-changelog) (#7471)
  fix(Customer.io Node): Fix api endpoint when using EU region (#7485)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team Released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants