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(Wait Node): Prevent waiting until invalid date #10523

Conversation

ivov
Copy link
Contributor

@ivov ivov commented Aug 23, 2024

@@ -346,6 +347,7 @@ export class Wait extends Webhook {
},
default: '',
description: 'The date and time to wait for before continuing',
required: true,
Copy link
Contributor Author

@ivov ivov Aug 23, 2024

Choose a reason for hiding this comment

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

A deeper fix would be having this param type fail when required: true and still using a '' default for all nodes.

Since this is a rare edge case and that deeper change might have unexpected side effects, I'd rather prevent this at the nodes-base and core levels for now.

@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team node/improvement New feature or request labels Aug 23, 2024
@@ -3776,7 +3777,9 @@ export function getExecuteFunctions(
);
return dataProxy.getDataProxy();
},
async putExecutionToWait(waitTill: Date): Promise<void> {
async putExecutionToWait(waitTill: unknown): Promise<void> {
assertValidDate(waitTill);
Copy link
Member

Choose a reason for hiding this comment

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

instead of putting this here (where we'll then validate all dates), we should put this check in the wait node only when using a user input date.

packages/nodes-base/nodes/Wait/Wait.node.ts Show resolved Hide resolved
Copy link
Member

@netroy netroy left a comment

Choose a reason for hiding this comment

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

Works as expected 👍🏽

Copy link
Contributor

✅ All Cypress E2E specs passed

Copy link

cypress bot commented Aug 27, 2024

n8n    Run #6625

Run Properties:  status check passed Passed #6625  •  git commit 5349f85ae1: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 ivov 🗃️ e2e/*
Project n8n
Branch Review pay-1838-wait-node-without-specified-date-will-cause-all-executions
Run status status check passed Passed #6625
Run duration 04m 57s
Commit git commit 5349f85ae1: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 ivov 🗃️ e2e/*
Committer Iván Ovejero
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 419
View all changes introduced in this branch ↗︎

@ivov ivov merged commit c0e7620 into master Aug 27, 2024
32 checks passed
@ivov ivov deleted the pay-1838-wait-node-without-specified-date-will-cause-all-executions branch August 27, 2024 14:45
MiloradFilipovic added a commit that referenced this pull request Aug 27, 2024
* master:
  refactor(core): Use `@/databases/` instead of `@db/` (no-changelog) (#10573)
  ci: Fix destroy benchmark env workflow (no-changelog) (#10572)
  feat: Add benchmarking of pooled sqlite (no-changelog) (#10550)
  refactor(editor): User journey link to n8n.io (#10331)
  fix(Wait Node): Prevent waiting until invalid date (#10523)
  refactor(core): Standardize filename casing for controllers and databases (no-changelog) (#10564)
  refactor(core): Allow custom types on getCredentials (no-changelog) (#10567)
  fix(editor): Scale output item selector input width with value (#10555)
  refactor(core): Delete InternalHooks (no-changelog) (#10561)
  fix(core): Make boolean config value parsing backward-compatible (#10560)
  fix(Google Sheets Trigger Node): Show sheet name is too long error (#10542)
  fix(editor): Ensure `Datatable` component renders `All` option (#10525)
  fix(core): Stop explicit redis client disconnect on shutdown (#10551)
  ci: Use correct branch for benchmark docker build workflow (no-changelog) (#10552)
  refactor(core): Separate listeners in scaling service (no-changelog) (#10487)
This was referenced Aug 28, 2024
@janober
Copy link
Member

janober commented Aug 28, 2024

Got released with n8n@1.57.0

MiloradFilipovic added a commit that referenced this pull request Aug 28, 2024
* master: (24 commits)
  feat(core): Switch to MJML for email templates (#10518)
  fix(Gmail Trigger Node): Don't return date instances, but date strings instead (#10582)
  fix(core): Deduplicate sentry events using error stacktraces instead (no-changelog) (#10590)
  feat(editor): Implement new app layout (#10548)
  refactor(core): Standardize filename casing for services and Public API (no-changelog) (#10579)
  🚀 Release 1.57.0 (#10587)
  fix(editor): Add tooltips to workflow history button (#10570)
  ci: Fix provenance generation during NPM publish (no-changelog) (#10586)
  feat(core): Expose queue metrics for Prometheus (#10559)
  refactor(core): Map out pubsub messages (no-changelog) (#10566)
  fix: Fix scenario prefix not being passed (no-changelog) (#10575)
  refactor(core): Convert `verbose` to `debug` logs (#10574)
  refactor(core): Use `@/databases/` instead of `@db/` (no-changelog) (#10573)
  ci: Fix destroy benchmark env workflow (no-changelog) (#10572)
  feat: Add benchmarking of pooled sqlite (no-changelog) (#10550)
  refactor(editor): User journey link to n8n.io (#10331)
  fix(Wait Node): Prevent waiting until invalid date (#10523)
  refactor(core): Standardize filename casing for controllers and databases (no-changelog) (#10564)
  refactor(core): Allow custom types on getCredentials (no-changelog) (#10567)
  fix(editor): Scale output item selector input width with value (#10555)
  ...

# Conflicts:
#	packages/editor-ui/src/stores/assistant.store.ts
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 node/improvement New feature or request Released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants