-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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(Notion Node): Fix issue preventing some database page urls from working #10070
fix(Notion Node): Fix issue preventing some database page urls from working #10070
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Joffcom Let me know if you've got any questions on this feedback, or would like an additional review after making changes. Thank you, JJ
packages/nodes-base/nodes/Notion/shared/descriptions/DatabasePageDescription.ts
Outdated
Show resolved
Hide resolved
packages/nodes-base/nodes/Notion/shared/descriptions/DatabasePageDescription.ts
Outdated
Show resolved
Hide resolved
packages/nodes-base/nodes/Notion/shared/descriptions/DatabasePageDescription.ts
Outdated
Show resolved
Hide resolved
packages/nodes-base/nodes/Notion/shared/descriptions/DatabasePageDescription.ts
Outdated
Show resolved
Hide resolved
packages/nodes-base/nodes/Notion/shared/descriptions/DatabasePageDescription.ts
Outdated
Show resolved
Hide resolved
packages/nodes-base/nodes/Notion/shared/descriptions/DatabasePageDescription.ts
Outdated
Show resolved
Hide resolved
@jerzygangi are the recent changes any better? This now follows what Notion documents so for page ids we use a UUIDv4 and for database page you can use any UUID. I have also included your URL example in the tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
✅ All Cypress E2E specs passed |
3 flaky tests on run #6114 ↗︎
Details:
5-ndv.cy.ts • 2 flaky tests
24-ndv-paired-item.cy.ts • 1 flaky test
Review all test suite changes for PR #10070 ↗︎ |
Got released with |
@Joffcom Sorry, I'm a bit late replying. This regex looks solid -- also, the new unit tests with sample uuid's is smart. Can't wait for this commit hit n8n.cloud users. Separately, I wanted offer a 💯 emoji to you for the rapid turnaround time. I occasionally report bugs to startups, and it's amazing how many take months to reply & never fix the problem. After 1 or 2 ignored bug reports, I stop reporting. Conversely, the few engineering teams that do act quickly (like n8n) send a message to the customer that says "We value your time; we're building a quality product; please don't churn; we want your business." Keep up the hard work. |
Hello |
Summary
Fixes an issue with the RLC regex that was incorrectly flagging
https://www.notion.so/fake-instance/x-ID
as invalid. Also added test forextractPageId
that uses the regex pattern first.Related Linear tickets, Github issues, and Community forum posts
https://community.n8n.io/t/notion-update-by-url-not-working-for-guest-added-pages/50095