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(editor): Load credentials for workflow before determining credentials errors #9876

Conversation

RicardoE105
Copy link
Contributor

@RicardoE105 RicardoE105 commented Jun 26, 2024

Summary

We are attempting to get node credentials errors before we load the credentials in the store. This causes the UI to show "issues with the credentials" even though there are none. Loading the credentials as soon as we initiate the view seems to fix the issue.

Specifically, we are trying to get the credentials errors for predefinedCredentialType and here, we attempt to get the type by calling getCredentialsByType, but that method depends on allCredentialsByType which at the same time depends on the state property allCredentials. At the time of calling the credentials have not been loaded in the store, so it returns [] even though it should have returned the credentials type. This empty array causes the logic to show an incorrect error.

The execution does follow:

Mounted -> ready (jsplumb) -> initView - > openWorkflow -> addNodes -> refreshNodeIssues -> getNodeIssue -> getNodeCredentialIssues -> credentialsStore.getCredentialsByType (were we return [] because the store has not loaded yet.) -> at some point later we load the credentials for that workflow in the store (this should have happened before getNodeCredentialIssues)

@cstuncsik Can you please have a look? I'm not too familiar with this side of the code, so I'm not sure whether this would break something else or if it's the best approach to solve the issue. So far, it seems to fix the problem.

Related Linear tickets, Github issues, and Community forum posts

https://linear.app/n8n/issue/ADO-2280/http-request-shows-credentials-not-set-warning-in-ui-despite

Before changes

https://www.loom.com/share/5c07e76288e84041b6b8abdf99a33b30?from_recorder=1&focus_title=1

After the changes

https://www.loom.com/share/7ac13702dced4784bc1bb820a29ed5bc?from_recorder=1&focus_title=1

Review / Merge checklist

  • PR title and summary are descriptive. (conventions)
  • Tests included.
  • PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

@RicardoE105 RicardoE105 changed the title load credentials before determining credentials errors fix(editor): Load credentials before determining credentials errors Jun 26, 2024
@n8n-assistant n8n-assistant bot added n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels Jun 26, 2024
@RicardoE105 RicardoE105 changed the title fix(editor): Load credentials before determining credentials errors fix(editor): Load credentials for workflow before determining credentials errors Jun 26, 2024
Copy link

cypress bot commented Jun 26, 2024

2 flaky tests on run #5688 ↗︎

0 396 0 0 Flakiness 2

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 RicardoE105 🗃️ e2e/*
Project: n8n Commit: e6f8babc50
Status: Passed Duration: 04:44 💡
Started: Jun 27, 2024 8:47 AM Ended: Jun 27, 2024 8:52 AM
Flakiness  5-ndv.cy.ts • 1 flaky test

View Output Video

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

View Output Video

Test Artifacts
Undo/Redo > should undo/redo adding connected nodes Test Replay Screenshots Video

Review all test suite changes for PR #9876 ↗︎

@cstuncsik
Copy link
Contributor

@RicardoE105 The reason the load credentials function was moved down in the init function is when you refresh the browser in the editor or someone opens a workflow in a new tab, there is not yet a workflowId to load the credentials

It looks like we have to find a place to load the credentials after fetching the workflow but before openWorkflow

Copy link
Contributor

✅ All Cypress E2E specs passed

@ivov ivov merged commit 4008c14 into master Jun 27, 2024
26 checks passed
@ivov ivov deleted the ado-2280-http-request-shows-credentials-not-set-warning-in-ui-despite branch June 27, 2024 09:54
@github-actions github-actions bot mentioned this pull request Jun 27, 2024
@janober
Copy link
Member

janober commented Jun 27, 2024

Got released with n8n@1.48.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
n8n team Authored by the n8n team Released ui Enhancement in /editor-ui or /design-system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants