-
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(editor): Connect up new project viewer role to the FE #9913
fix(editor): Connect up new project viewer role to the FE #9913
Conversation
…-new-project-viewer-role-to-the-fe # Conflicts: # packages/editor-ui/src/components/CredentialEdit/CredentialConfig.vue # packages/editor-ui/src/components/CredentialEdit/CredentialEdit.vue
Test summaryRun details
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Cloud |
…-new-project-viewer-role-to-the-fe
…-new-project-viewer-role-to-the-fe # Conflicts: # packages/editor-ui/src/views/CredentialsView.vue # packages/editor-ui/src/views/VariablesView.vue # packages/editor-ui/src/views/WorkflowsView.vue
…-new-project-viewer-role-to-the-fe
…-new-project-viewer-role-to-the-fe
…-new-project-viewer-role-to-the-fe
…-new-project-viewer-role-to-the-fe
…-new-project-viewer-role-to-the-fe
…-new-project-viewer-role-to-the-fe # Conflicts: # cypress/composables/projects.ts
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.
Left a few small remarks. I'm mainly concerned about porting the NodeView changes to the new canvas.
Other than that, looking great! Thanks for migrating the components to composition api as well.
/> | ||
<N8nTooltip :disabled="!buttonDisabled"> | ||
<template #content> | ||
<slot name="disabledButtonTooltip"></slot> |
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.
I think this slot should be usable for non-disabled buttons as well. Can you name it buttonTooltip
instead?
return getWorkflowPermissions(workflowsStore.getWorkflowById(props.workflow.id)); | ||
}); | ||
const workflowPermissions = computed( | ||
() => getResourcePermissions(workflowsStore.getWorkflowById(props.workflow.id)?.scopes).workflow, |
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.
Can you extract the workflow
( workflowsStore.getWorkflowById(props.workflow.id)
) into a separate computed
property? Otherwise the value won't be cached.
@@ -23,7 +23,7 @@ | |||
placeholder="Select Execution Order" | |||
size="medium" | |||
filterable | |||
:disabled="readOnlyEnv" | |||
:disabled="readOnlyEnv || !workflowPermissions.update" |
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.
This condition seems to be repeated quite a lot. Could you extract it into a computed prop?
@@ -344,7 +352,9 @@ async function onAutoRefreshToggle(value: boolean) { | |||
:key="execution.id" | |||
:execution="execution" | |||
:workflow-name="getExecutionWorkflowName(execution)" | |||
:workflow-permissions="getExecutionWorkflowPermissions(execution)" |
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.
We should avoid calling functions in the template. Could the permissions for all rendered executions get cached in a key-value computed prop? (with execution id as key, permissions as value)
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.
You'll need to port these changes to the new canvas as well, otherwise permissions will be out of sync.
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.
Changes mentioned above will be done separately.
✅ All Cypress E2E specs passed |
* master: (98 commits) feat(core): Allow overriding npm registry for community packages (#10325) feat(core): Upgrade DB drivers (no-changelog) (#10370) fix(editor): Fix bug causing workflow debugging to not work in new canvas (no-changelog) (#10384) fix: Fix issue with some errors not being handled correctly (no-changelog) (#10371) fix(core): Filter out prototype and constructor lookups in expressions (#10382) fix(editor): Connect up new project viewer role to the FE (#9913) refactor(core): Move queue recovery to scaling service (no-changelog) (#10368) fix(core): Account for owner when filtering by project ID in `GET /workflows` in Public API (#10379) fix(editor): Fix rendering of SVG icons in public chat on iOS (#10381) fix: Require mfa code to disable mfa (#10345) ci: Disable turbo cache when running tests for coverage collection (no-changelog) (#10380) refactor(editor): Add typed event bus (no-changelog) (#10367) refactor(core): Remove unused constants in Redis channels (no-changelog) (#10369) fix(editor): Revert change that hid swagger docs in the ui (#10350) fix(Okta Node): Add missing codex file (no-changelog) (#10372) fix(core): Fix worker shutdown errors when active executions (#10353) refactor(core): Rename ActiveWebhooks to LiveWebhooks (no-changelog) (#10355) fix(n8n Form Trigger Node): Fix issue preventing v1 node from working (#10364) feat(editor): Upgrade markdown-it to address AIKIDO-2024-10034 (no-changelog) (#10358) ci: Upgrade axios to address CVE-2024-39338 (no-changelog) (#10365) ... # Conflicts: # packages/design-system/package.json
@@ -39,6 +39,7 @@ | |||
"@types/xml2js": "catalog:" | |||
}, | |||
"dependencies": { | |||
"@n8n/permissions": "workspace:*", |
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.
This change is breaking some CI workflows now because we did not update a few other places that need to be updated when we add new dependencies like this.
we really need to start breaking up the workflow package instead of continuing to keep making the dependency tree more convoluted.
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.
I feel like some kind of shared frontend/backend types packages would go a long way.
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.
agreed
Got released with |
Summary
Introducing the new
viewer
role in projects.Users can now select this role when inviting users to projects.
As the name suggests members with this role in the project can only view resources (workflows, credentials, executions) and not edit them.
Related Linear tickets, Github issues, and Community forum posts
PAY-1659
Review / Merge checklist