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): Connect up new project viewer role to the FE #9913

Merged

Conversation

cstuncsik
Copy link
Contributor

@cstuncsik cstuncsik commented Jul 2, 2024

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

  • PR title and summary are descriptive.
  • Docs updated or follow-up ticket created.
  • Tests included.

@n8n-assistant n8n-assistant bot added n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels Jul 2, 2024
Copy link

cypress bot commented Jul 5, 2024



Test summary

400 0 0 0Flakiness 0


Run details

Project n8n
Status Passed
Commit ad8246f
Started Aug 13, 2024 12:17 PM
Ended Aug 13, 2024 12:21 PM
Duration 04:34 💡
OS Linux Debian -
Browser Electron 118

View run in Cypress Cloud ➡️


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

# Conflicts:
#	packages/editor-ui/src/views/CredentialsView.vue
#	packages/editor-ui/src/views/VariablesView.vue
#	packages/editor-ui/src/views/WorkflowsView.vue
@cstuncsik cstuncsik marked this pull request as ready for review August 7, 2024 10:42
Copy link
Contributor

@alexgrozav alexgrozav left a 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>
Copy link
Contributor

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,
Copy link
Contributor

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"
Copy link
Contributor

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)"
Copy link
Contributor

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)

Copy link
Contributor

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.

@alexgrozav alexgrozav self-requested a review August 13, 2024 12:11
Copy link
Contributor

@alexgrozav alexgrozav left a 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.

Copy link
Contributor

✅ All Cypress E2E specs passed

@alexgrozav alexgrozav merged commit 117e2d9 into master Aug 13, 2024
49 of 50 checks passed
@alexgrozav alexgrozav deleted the pay-1659-connect-up-new-project-viewer-role-to-the-fe branch August 13, 2024 13:45
MiloradFilipovic added a commit that referenced this pull request Aug 14, 2024
* 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:*",
Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Member

Choose a reason for hiding this comment

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

agreed

@janober
Copy link
Member

janober commented Aug 15, 2024

Got released with n8n@1.55.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.

5 participants