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): Account for workflow timezone in Wait node datetime #8578

Merged
merged 4 commits into from
Feb 14, 2024

Conversation

ivov
Copy link
Contributor

@ivov ivov commented Feb 7, 2024

@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 Feb 7, 2024
@ivov ivov changed the title fix(Wait node): Account for workflow timezone in Wait node datetime fix(Wait Node): Account for workflow timezone in Wait node datetime Feb 7, 2024
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.

Ugh this does not seem to be actually solving the problem. Actually it's starting to look worse 😅

waitTill = new Date(new Date().getTime() + waitAmount);
const original = new Date(new Date().getTime() + waitAmount);

waitTill = DateTime.fromISO(original.toISOString().slice(0, -1)).toUTC().toJSDate();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth considering moving the toUtcDate to the nodes package as well as its tests? WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I just tested it now, and it looks real bad. It now takes into account the timezone, so it seems it startec picking up America/New York so instead of my workflows waiting 10 minutes, they now way 5 hours and 10 minutes, which is even worse 😓

But to make things even more interesting, I changed the workflow's timezone, and it's also being completely ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate? This is how I tested:

  • Selected 12:00 with workflow set at UTC-5 → Stored as 17:00 UTC in DB
  • Selected +2h with workflow set at 10:40 UTC-5 → Stored as 17:40 UTC in DB

These are what I'd expect to be correct. What am I missing?

packages/core/src/NodeExecuteFunctions.ts Show resolved Hide resolved

waitTill = new Date(dateTime);
waitTill = DateTime.fromFormat(dateTimeStr, "yyyy-MM-dd'T'HH:mm:ss", {
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix timezone only for specific points in time, where 4pm is something in UK vs Berlin.

@krynble krynble marked this pull request as ready for review February 14, 2024 14:35
Copy link
Contributor

✅ All Cypress E2E specs passed

Copy link

cypress bot commented Feb 14, 2024

2 flaky tests on run #4102 ↗︎

0 341 5 0 Flakiness 2

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 ivov 🗃️ e2e/*
Project: n8n Commit: 5dc2ef9b21
Status: Passed Duration: 03:30 💡
Started: Feb 14, 2024 3:07 PM Ended: Feb 14, 2024 3:10 PM
Flakiness  5-ndv.cy.ts • 1 flaky test

View Output Video

Test Artifacts
NDV > should not retrieve remote options when required params throw errors Test Replay Screenshots Video
Flakiness  13-pinning.cy.ts • 1 flaky test

View Output Video

Test Artifacts
Data pinning > Should be able to reference paired items in a node located before pinned data Test Replay Screenshots Video

Review all test suite changes for PR #8578 ↗︎

@krynble krynble merged commit 1116a28 into master Feb 14, 2024
30 checks passed
@krynble krynble deleted the pay-1341-wait-node-not-working-as-expected branch February 14, 2024 15:12
@github-actions github-actions bot mentioned this pull request Feb 15, 2024
@janober
Copy link
Member

janober commented Feb 15, 2024

Got released with [email protected]

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.

4 participants