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.

Sign up for GitHub

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): Allow sharee to use workflows with http request node without credential access #8841

Merged
merged 1 commit into from
Mar 11, 2024
+9 −2
Merged

fix(editor): Allow sharee to use workflows with http request node without credential access #8841

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions packages/editor-ui/src/composables/useNodeHelpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,8 +373,15 @@ export function useNodeHelpers() {
node.credentials !== undefined
) {
const stored = credentialsStore.getCredentialsByType(nodeCredentialType);

if (selectedCredsDoNotExist(node, nodeCredentialType, stored)) {
// Prevents HTTP Request node from being unusable if a sharee does not have direct
Copy link
Contributor

Choose a reason for hiding this comment

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

@krynble I'm not sure but it feels like this case is handled a couple of lines down and the problem is rather with the condition of this block.

Doesn't the selectedCredsAreUnusable function do something similar?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually the block of the next condition is doing the same
Couldn't they be merged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my understanding, the current block is checking whether the selected credential does not exist.

But the "exist" part is wrong from what I can tell, because it's ignoring the usedCredentials attribute that highlights credentials owned by other sharees of the workflow.

The next block seems to be looking at credentials that are set, but are of invalid type (say for whatever reason a credential to Hubspot was set to a Pipedrive node).

Then below there's a for loop that I don't quite understand what it does for all cases but after debugging I was able to spot a difference in behavior for regular nodes (dedicated to a specific service) vs http request node, hence the change I've made.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just saw these identical blocks and thought maybe there is a chance to simplify
CleanShot 2024-03-08 at 12 50 00

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, there are a few similarities between those blocks, but since they have slightly different checks, for the sake of clarity (which is not great I know, but I would like to avoid making it worse) I think they can remain separate.

// access to a credential
const isCredentialUsedInWorkflow =
workflowsStore.usedCredentials?.[node.credentials?.[nodeCredentialType]?.id as string];

if (
selectedCredsDoNotExist(node, nodeCredentialType, stored) &&
!isCredentialUsedInWorkflow
) {
const credential = credentialsStore.getCredentialTypeByName(nodeCredentialType);
return credential ? reportUnsetCredential(credential) : null;
}
Expand Down
Loading