-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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): Open only one tab with plans page #7377
fix(editor): Open only one tab with plans page #7377
Conversation
Great PR! Please pay attention to the following items before merging: Files matching
Files matching
Files matching
Make sure to check off this list before asking for review. |
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.
Hey @inga-lovinde,
Thank you for your detailed PR and for pinpointing the issues you found.
Your short-term solution is well noted, and I’ll merge it so that we can work towards a more robust, long-term solution.
Regarding the two telemetry calls, this is an interesting point that you’ve raised, and I’ve created a ticket to look into finding other places where this happens.
Given that the component is not currently tested, we’ll merge your PR without tests, but thank you for taking the matter into consideration.
Although speeding up testing and linting is on our radar, we’re not currently addressing the issue. We’ll be tackling developer tool performance after we go through with our current runtime performance improvement plans.
You’re completely right about the editor-ui typecheck issues. We’ve recently migrated to Vue 3, which improved type safety in our components tremendously. The unfortunate downside of this is that we now have a few thousand errors to fix that were previously undetected.
We’ll be working on this during our dedicated technical debt days and we’ll gradually fix the errors.
Once again, thank you for the great remarks!
Thank you for the response, @alexgrozav (Just a small clarification: my problem with tests is not that they're slow, it's that they crash my system with out-of-memory errors if I try to run all tests by Seeing how "Unit tests" workflow still doesn't work, and I don't know if you discuss it internally already (you probably do? Still worth it to mention this here for other external contributors, though): it's very difficult for me to diagnose the issue only by its external appearances, but I looked into things a bit, and created a new issue for that, #7423 |
I think that CI will continue failing until this branch is rebased onto the latest main (or until the latest main changes are merged here). However, unfortunately, I cannot do this right now. UPD: Rebased and force-pushed, but workflows are awaiting approval right now. |
3af95e3
to
0b92947
Compare
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7377 +/- ##
==========================================
- Coverage 33.50% 33.49% -0.01%
==========================================
Files 3390 3390
Lines 207045 207077 +32
Branches 22344 22341 -3
==========================================
- Hits 69369 69366 -3
- Misses 136554 136589 +35
Partials 1122 1122
☔ View full report in Codecov by Sentry. |
# [1.12.0](https://github.com/n8n-io/n8n/compare/n8n@1.11.0...n8n@1.12.0) (2023-10-18) ### Bug Fixes * **core:** Add check that queue is defined and remove cyclic dependency ([#7404](#7404)) ([45f2ef3](45f2ef3)) * **core:** Do not throw when deleting workflows with executions without binary-data ([#7411](#7411)) ([2b6a15e](2b6a15e)) * **core:** Fix expression with paired item with multi-input node ([#7424](#7424)) ([ec14141](ec14141)) * **core:** Fix ignoring crashed executions without event msgs ([#7368](#7368)) ([2f4d91b](2f4d91b)) * **core:** Pg-promise de-initialization fix ([#7417](#7417)) ([7703904](7703904)) * **core:** Prevent false stalled jobs in queue mode from displaying as errored ([#7435](#7435)) ([e01b9e5](e01b9e5)) * **core:** Prevent undefined issues when restoring binary data ([#7419](#7419)) ([46977a2](46977a2)) * **editor:** Fix remote options fetching on every keystroke ([#7320](#7320)) ([367255a](367255a)) * **editor:** Open only one tab with plans page ([#7377](#7377)) ([c599006](c599006)) * **Google Sheets Node:** Update by row_number, restored 'Handling Extra Data Option', updated Cell Format default ([#7357](#7357)) ([d8531a5](d8531a5)) * **Ldap Node:** Fix issue with connections not closing correctly ([#7432](#7432)) ([c3f0be8](c3f0be8)) * **Set Node:** Null should not throw an error ([#7416](#7416)) ([e9b6ab0](e9b6ab0)) * **TheHive 5 Node:** Observable encoding in alert > create fix ([#7450](#7450)) ([a2d2e3d](a2d2e3d)) ### Features * **core:** Make executions pruning interval configurable ([#7439](#7439)) ([40707fa](40707fa)) * **Google Calendar Trigger Node:** Add support for cancelled events ([#7436](#7436)) ([9d241a0](9d241a0)) * **HubSpot Trigger Node:** Add support for ticket related events ([#7156](#7156)) ([57c6093](57c6093)) * **n8n Form Trigger Node:** New node ([#7130](#7130)) ([3ddc176](3ddc176)) * **Spreadsheet File Node:** Improve CSV parsing ([#7448](#7448)) ([79f23fb](79f23fb)) Co-authored-by: netroy <netroy@users.noreply.github.com>
Got released with |
## Issue In community edition, clicking on "View plans" button on "Settings" -> "Usage and plan" page (e.g. http://127.0.0.1:5678/settings/usage) opens two new tabs with n8n pricing (one of them with UTM tracking, another without). This was introduced in #6317 , when click handler of "View plans" link container [started calling](https://github.com/n8n-io/n8n/pull/6317/files#diff-0bf26afac8a06e03b3d39d0668f22408859355b585a9ab420800c125e33f0691R109) `uiStore.goToUpgrade(...)` which opens n8n pricing in a new tab, while browser opens another tab for the link URL. The simplest fix, implemented in this PR, is to prevent default event handling (so that, after `onViewPlans` is called, browser will not attempt to process the click additionally as clicking on the link), similarly to how it is prevented on some other pages. It only solves the immediate problem of browser opening two new tabs on clicking "View plans". Note that **I didn't implement any tests for the changed behavior**, because it was not covered by tests before, and I couldn't quite figure out how to cover it now within the existing test approach (considering that testing the fact that only one new tab is open will likely require to write entirely new tests relying on puppeteer; as far as I can see, no existing `editor-ui` tests are doing anything like that). I'll gladly implement tests for the new behavior if you tell me how you would like them to look. The existing tests for `editor-ui` still pass; I didn't run tests for other subpackages (see "additional contribution notes" below). ## Additional notes on the issue. I'm not sure that the change in this PR is the correct long-term solution for the issue, because the URLs for these two methods (custom click handler for link container and default link handling) are slightly different: * Custom click handler calls `useTelemetryStore().track('User clicked upgrade CTA', ...)`; then calls `sendUsageTelemetry('view_plans')` (it feels weird that two calls to telemetry are made); then opens new tab for `https://n8n.io/pricing?utm_campaign=open&source=usage_page` (note that prior to #7316 the second call to telemetry was done after the new tab is opened, not before); * Link itself refers to another page, with slightly different tracking parameters: `https://subscription.n8n.io/?instanceid=[REDACTED]&version=1.10.0&callback=http%3A%2F%2F127.0.0.1%3A5678%2Fsettings%2Fusage&source=usage_page`; but this page redirects to `https://n8n.io/pricing/`. It is not clear which one of the two is the right way of doing things. Although `goToUpgrade` is called in 20 places throughout `editor-ui`, while `viewPlansUrl`, as far as I can see, is used for this button only. Additionally, since Settings pages don't work without JS anyway, I can only think of two separate scenarios where any tab would be opened: * Left-clicking the link (or Ctrl-clicking, or pressing Space or Enter when the link is focused, or tapping): previously, both custom click handler was executed and link's `href` was opened; in this PR, only custom click handler is executed (similarly to how it is done in the other places where `goToUpgrade` is called); * Right-clicking (or long tapping, or opening context menu in any other way) and selecting "open link in new tab" (or similar): opens a new tab for URL from the `href` attribute (and does not send any telemetry at all). I'd say that the better permanent solution would probably be to get rid of one of these methods entirely, and only rely on another in all cases (for me, as an outside contributor, the preferred way would be for custom click handler to only send telemetry, while letting my browser handle the actual navigation). However, that would be a large change, much more than one line in this PR. Additionally, other similar places where `goToUpgrade` is currently called (directly or indirectly) would also need to be adapted for this change. ## Additional contribution notes As a first-time contributor, I've encountered several things I didn't expect; I'm not sure if they should be expected or are issues: 1. Tests for the entire monorepo consume a lot of RAM; 20GB free RAM was not enough, so I couldn't run tests for the entire monorepo and had to only run them for `packages/editor-ui`; 2. Linting is very slow; `pnpm lint` in `packages/editor-ui` takes ten minutes to complete; 3. It seems that types are not checked. Code OSS highlights numerous errors in code files: for example, `'debug'` is incompatible with `CloudUpdateLinkSourceType` expected by `goToUpgrade` here: https://github.com/n8n-io/n8n/blob/3e7a4d3b2cc12fcb1b011fccd0773bb807986884/packages/editor-ui/src/composables/useExecutionDebugging.ts#L128 However, I'm not getting any errors during build. There is a `typecheck` script defined in `package.json`, but `pnpm typecheck` fails with: ``` n8n-toy-demo:~/projects/n8n/packages/editor-ui$ pnpm typecheck > n8n-editor-ui@1.10.0 typecheck /home/inga/projects/n8n/packages/editor-ui > vue-tsc --emitDeclarationOnly error TS5069: Option 'emitDeclarationOnly' cannot be specified without specifying option 'declaration' or option 'composite'. Found 1 error. ELIFECYCLE Command failed with exit code 1. n8n-toy-demo:~/projects/n8n/packages/editor-ui$ ``` Replacing `--emitDeclarationsOnly` with `--noEmit` in `package.json` unblocks typechecking and results in seemingly, at first glance, correct "Found 1924 errors in 306 files" (at least several of the reported errors that I've checked seem to be correct). But maybe I'm missing something and there are not in fact two thousands type errors in `editor-ui`?
# [1.12.0](https://github.com/n8n-io/n8n/compare/n8n@1.11.0...n8n@1.12.0) (2023-10-18) ### Bug Fixes * **core:** Add check that queue is defined and remove cyclic dependency ([#7404](#7404)) ([45f2ef3](45f2ef3)) * **core:** Do not throw when deleting workflows with executions without binary-data ([#7411](#7411)) ([2b6a15e](2b6a15e)) * **core:** Fix expression with paired item with multi-input node ([#7424](#7424)) ([ec14141](ec14141)) * **core:** Fix ignoring crashed executions without event msgs ([#7368](#7368)) ([2f4d91b](2f4d91b)) * **core:** Pg-promise de-initialization fix ([#7417](#7417)) ([7703904](7703904)) * **core:** Prevent false stalled jobs in queue mode from displaying as errored ([#7435](#7435)) ([e01b9e5](e01b9e5)) * **core:** Prevent undefined issues when restoring binary data ([#7419](#7419)) ([46977a2](46977a2)) * **editor:** Fix remote options fetching on every keystroke ([#7320](#7320)) ([367255a](367255a)) * **editor:** Open only one tab with plans page ([#7377](#7377)) ([c599006](c599006)) * **Google Sheets Node:** Update by row_number, restored 'Handling Extra Data Option', updated Cell Format default ([#7357](#7357)) ([d8531a5](d8531a5)) * **Ldap Node:** Fix issue with connections not closing correctly ([#7432](#7432)) ([c3f0be8](c3f0be8)) * **Set Node:** Null should not throw an error ([#7416](#7416)) ([e9b6ab0](e9b6ab0)) * **TheHive 5 Node:** Observable encoding in alert > create fix ([#7450](#7450)) ([a2d2e3d](a2d2e3d)) ### Features * **core:** Make executions pruning interval configurable ([#7439](#7439)) ([40707fa](40707fa)) * **Google Calendar Trigger Node:** Add support for cancelled events ([#7436](#7436)) ([9d241a0](9d241a0)) * **HubSpot Trigger Node:** Add support for ticket related events ([#7156](#7156)) ([57c6093](57c6093)) * **n8n Form Trigger Node:** New node ([#7130](#7130)) ([3ddc176](3ddc176)) * **Spreadsheet File Node:** Improve CSV parsing ([#7448](#7448)) ([79f23fb](79f23fb)) Co-authored-by: netroy <netroy@users.noreply.github.com>
## Issue In community edition, clicking on "View plans" button on "Settings" -> "Usage and plan" page (e.g. http://127.0.0.1:5678/settings/usage) opens two new tabs with n8n pricing (one of them with UTM tracking, another without). This was introduced in #6317 , when click handler of "View plans" link container [started calling](https://github.com/n8n-io/n8n/pull/6317/files#diff-0bf26afac8a06e03b3d39d0668f22408859355b585a9ab420800c125e33f0691R109) `uiStore.goToUpgrade(...)` which opens n8n pricing in a new tab, while browser opens another tab for the link URL. The simplest fix, implemented in this PR, is to prevent default event handling (so that, after `onViewPlans` is called, browser will not attempt to process the click additionally as clicking on the link), similarly to how it is prevented on some other pages. It only solves the immediate problem of browser opening two new tabs on clicking "View plans". Note that **I didn't implement any tests for the changed behavior**, because it was not covered by tests before, and I couldn't quite figure out how to cover it now within the existing test approach (considering that testing the fact that only one new tab is open will likely require to write entirely new tests relying on puppeteer; as far as I can see, no existing `editor-ui` tests are doing anything like that). I'll gladly implement tests for the new behavior if you tell me how you would like them to look. The existing tests for `editor-ui` still pass; I didn't run tests for other subpackages (see "additional contribution notes" below). ## Additional notes on the issue. I'm not sure that the change in this PR is the correct long-term solution for the issue, because the URLs for these two methods (custom click handler for link container and default link handling) are slightly different: * Custom click handler calls `useTelemetryStore().track('User clicked upgrade CTA', ...)`; then calls `sendUsageTelemetry('view_plans')` (it feels weird that two calls to telemetry are made); then opens new tab for `https://n8n.io/pricing?utm_campaign=open&source=usage_page` (note that prior to #7316 the second call to telemetry was done after the new tab is opened, not before); * Link itself refers to another page, with slightly different tracking parameters: `https://subscription.n8n.io/?instanceid=[REDACTED]&version=1.10.0&callback=http%3A%2F%2F127.0.0.1%3A5678%2Fsettings%2Fusage&source=usage_page`; but this page redirects to `https://n8n.io/pricing/`. It is not clear which one of the two is the right way of doing things. Although `goToUpgrade` is called in 20 places throughout `editor-ui`, while `viewPlansUrl`, as far as I can see, is used for this button only. Additionally, since Settings pages don't work without JS anyway, I can only think of two separate scenarios where any tab would be opened: * Left-clicking the link (or Ctrl-clicking, or pressing Space or Enter when the link is focused, or tapping): previously, both custom click handler was executed and link's `href` was opened; in this PR, only custom click handler is executed (similarly to how it is done in the other places where `goToUpgrade` is called); * Right-clicking (or long tapping, or opening context menu in any other way) and selecting "open link in new tab" (or similar): opens a new tab for URL from the `href` attribute (and does not send any telemetry at all). I'd say that the better permanent solution would probably be to get rid of one of these methods entirely, and only rely on another in all cases (for me, as an outside contributor, the preferred way would be for custom click handler to only send telemetry, while letting my browser handle the actual navigation). However, that would be a large change, much more than one line in this PR. Additionally, other similar places where `goToUpgrade` is currently called (directly or indirectly) would also need to be adapted for this change. ## Additional contribution notes As a first-time contributor, I've encountered several things I didn't expect; I'm not sure if they should be expected or are issues: 1. Tests for the entire monorepo consume a lot of RAM; 20GB free RAM was not enough, so I couldn't run tests for the entire monorepo and had to only run them for `packages/editor-ui`; 2. Linting is very slow; `pnpm lint` in `packages/editor-ui` takes ten minutes to complete; 3. It seems that types are not checked. Code OSS highlights numerous errors in code files: for example, `'debug'` is incompatible with `CloudUpdateLinkSourceType` expected by `goToUpgrade` here: https://github.com/n8n-io/n8n/blob/3e7a4d3b2cc12fcb1b011fccd0773bb807986884/packages/editor-ui/src/composables/useExecutionDebugging.ts#L128 However, I'm not getting any errors during build. There is a `typecheck` script defined in `package.json`, but `pnpm typecheck` fails with: ``` n8n-toy-demo:~/projects/n8n/packages/editor-ui$ pnpm typecheck > n8n-editor-ui@1.10.0 typecheck /home/inga/projects/n8n/packages/editor-ui > vue-tsc --emitDeclarationOnly error TS5069: Option 'emitDeclarationOnly' cannot be specified without specifying option 'declaration' or option 'composite'. Found 1 error. ELIFECYCLE Command failed with exit code 1. n8n-toy-demo:~/projects/n8n/packages/editor-ui$ ``` Replacing `--emitDeclarationsOnly` with `--noEmit` in `package.json` unblocks typechecking and results in seemingly, at first glance, correct "Found 1924 errors in 306 files" (at least several of the reported errors that I've checked seem to be correct). But maybe I'm missing something and there are not in fact two thousands type errors in `editor-ui`?
## [1.11.2](https://github.com/n8n-io/n8n/compare/n8n@1.11.1...n8n@1.11.2) (2023-10-23) ### Bug Fixes * **core:** Handle gzip and deflate compressed request payloads ([#7461](#7461)) ([f43ff71](f43ff71)) * **core:** Prevent false stalled jobs in queue mode from displaying as errored ([#7435](#7435)) ([465a952](465a952)) * **core:** Reduce logging overhead for levels that do not output ([#7479](#7479)) ([010aa57](010aa57)) * **editor:** Allow importing the same workflow multiple times ([#7458](#7458)) ([33e3df8](33e3df8)), closes [#7457](#7457) * **editor:** Fix canvas selection breaking after interacting with node actions ([#7466](#7466)) ([90ce8de](90ce8de)) * **editor:** Fix connections disappearing after reactivating canvas and renaming a node ([#7483](#7483)) ([b0bd0d8](b0bd0d8)) * **editor:** Open only one tab with plans page ([#7377](#7377)) ([d14e9cb](d14e9cb)) * **Ldap Node:** Fix issue with connections not closing correctly ([#7432](#7432)) ([60ca02e](60ca02e)) * **MySQL Node:** Resolve expressions in v1 ([#7464](#7464)) ([2b18909](2b18909)) * **TheHive 5 Node:** Observable encoding in alert > create fix ([#7450](#7450)) ([b9547ad](b9547ad)) Co-authored-by: netroy <netroy@users.noreply.github.com>
Issue
In community edition, clicking on "View plans" button on "Settings" -> "Usage and plan" page (e.g. http://127.0.0.1:5678/settings/usage) opens two new tabs with n8n pricing (one of them with UTM tracking, another without).
This was introduced in #6317 , when click handler of "View plans" link container started calling
uiStore.goToUpgrade(...)
which opens n8n pricing in a new tab, while browser opens another tab for the link URL.The simplest fix, implemented in this PR, is to prevent default event handling (so that, after
onViewPlans
is called, browser will not attempt to process the click additionally as clicking on the link), similarly to how it is prevented on some other pages. It only solves the immediate problem of browser opening two new tabs on clicking "View plans".Note that I didn't implement any tests for the changed behavior, because it was not covered by tests before, and I couldn't quite figure out how to cover it now within the existing test approach (considering that testing the fact that only one new tab is open will likely require to write entirely new tests relying on puppeteer; as far as I can see, no existing
editor-ui
tests are doing anything like that). I'll gladly implement tests for the new behavior if you tell me how you would like them to look.The existing tests for
editor-ui
still pass; I didn't run tests for other subpackages (see "additional contribution notes" below).Additional notes on the issue.
I'm not sure that the change in this PR is the correct long-term solution for the issue, because the URLs for these two methods (custom click handler for link container and default link handling) are slightly different:
useTelemetryStore().track('User clicked upgrade CTA', ...)
; then callssendUsageTelemetry('view_plans')
(it feels weird that two calls to telemetry are made); then opens new tab forhttps://n8n.io/pricing?utm_campaign=open&source=usage_page
(note that prior to feat(editor): Support autologin for upgrade path #7316 the second call to telemetry was done after the new tab is opened, not before);https://subscription.n8n.io/?instanceid=[REDACTED]&version=1.10.0&callback=http%3A%2F%2F127.0.0.1%3A5678%2Fsettings%2Fusage&source=usage_page
; but this page redirects tohttps://n8n.io/pricing/
.It is not clear which one of the two is the right way of doing things. Although
goToUpgrade
is called in 20 places throughouteditor-ui
, whileviewPlansUrl
, as far as I can see, is used for this button only.Additionally, since Settings pages don't work without JS anyway, I can only think of two separate scenarios where any tab would be opened:
href
was opened; in this PR, only custom click handler is executed (similarly to how it is done in the other places wheregoToUpgrade
is called);href
attribute (and does not send any telemetry at all).I'd say that the better permanent solution would probably be to get rid of one of these methods entirely, and only rely on another in all cases (for me, as an outside contributor, the preferred way would be for custom click handler to only send telemetry, while letting my browser handle the actual navigation). However, that would be a large change, much more than one line in this PR.
Additionally, other similar places where
goToUpgrade
is currently called (directly or indirectly) would also need to be adapted for this change.Additional contribution notes
As a first-time contributor, I've encountered several things I didn't expect; I'm not sure if they should be expected or are issues:
packages/editor-ui
;pnpm lint
inpackages/editor-ui
takes ten minutes to complete;'debug'
is incompatible withCloudUpdateLinkSourceType
expected bygoToUpgrade
here:n8n/packages/editor-ui/src/composables/useExecutionDebugging.ts
Line 128 in 3e7a4d3
However, I'm not getting any errors during build. There is a
typecheck
script defined inpackage.json
, butpnpm typecheck
fails with:--emitDeclarationsOnly
with--noEmit
inpackage.json
unblocks typechecking and results in seemingly, at first glance, correct "Found 1924 errors in 306 files" (at least several of the reported errors that I've checked seem to be correct).But maybe I'm missing something and there are not in fact two thousands type errors in
editor-ui
?