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): Disable expression editor modal opening on readonly field #8457

Conversation

cstuncsik
Copy link
Contributor

@cstuncsik cstuncsik commented Jan 26, 2024

Expression fields appear editable when they should be in read only state

m

We need to look at expression fields across all nodes to make sure they are 'read only' when a credential on them is being used that isn't shared with the user currently editing. There are a few different scenarios to consider. One is where you're an instance owner/admin and more things look editiable than they should be. The other is that it looks like even for standard member users, expression fields/text input fields are editable when they shouldn't be

Expected behaviour

When editing the Google sheets node where the credential has not been shared with you, you should not be able to edit any of the fields on that nodes.

When opening up the expression editor in full screen mode, it should be greyed out and in read only mode

Actual behaviour

Expression/text fields on certain nodes (if not all) are still editable. You can't save them but its confusing because it looks like you can update and save them and you won't see any error messages. Any changes will just disappear once you refresh.

For certain nodes, you're unable to edit in the small expressions input but once you open up in full screen you can then edit things in there

A/C

Expressions on readonly (when credentials have not been shared) nodes should be readonly for instance owner, instance admin and member users

When you open up the expression editor in full screen mode on any node, this should also be in read only state.

We should check all nodes are handling expressions/text inputs like this consistently

Examples of where this is happening

GMAIL Node - you can't edit the message in the initial view but once you open up to full screen editor you then can
image

Google Sheets node - You can edit the values to send fields when they should be read only like the fields above
image

@n8n-assistant n8n-assistant bot added n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels Jan 26, 2024
@cstuncsik cstuncsik requested a review from ivov January 26, 2024 13:44
@ivov
Copy link
Contributor

ivov commented Jan 26, 2024

Thanks for fixing this! Added a comment about the requirements on Linear.

@cstuncsik
Copy link
Contributor Author

Thanks @ivov, sorry, I just missed it.
Pushed another fix disabling editing the extended expression editor
Thinking on a good way to test it (possibly not in an E2E test)

@ivov
Copy link
Contributor

ivov commented Jan 29, 2024

So readonly works fine, but I'm still seeing this white instead of greyed out. Let's sync on this when you get a chance.

Capture 2024-01-29 at 12 19 11@2x Capture 2024-01-29 at 12 19 48@2x

Copy link

cypress bot commented Jan 29, 2024

1 flaky test on run #3950 ↗︎

0 339 5 0 Flakiness 1

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 cstuncsik 🗃️ e2e/*
Project: n8n Commit: 1c39f53250
Status: Passed Duration: 03:20 💡
Started: Jan 29, 2024 3:29 PM Ended: Jan 29, 2024 3:32 PM
Flakiness  cypress/e2e/19-execution.cy.ts • 1 flaky test

View Output Video

Test Artifacts
Execution > should test manual workflow stop Test Replay Screenshots Video

Review all test suite changes for PR #8457 ↗︎

Copy link
Contributor

✅ All Cypress E2E specs passed

@cstuncsik cstuncsik merged commit eb27ed0 into master Jan 29, 2024
28 checks passed
@cstuncsik cstuncsik deleted the pay-1000-expression-fields-appear-editable-when-they-should-be-in branch January 29, 2024 15:34
MiloradFilipovic added a commit that referenced this pull request Jan 30, 2024
…error-states

* master:
  fix(editor): Send template id as a number in telemetry events (#8484)
  refactor(core): Replace promisify-d node calls with native promises (no-changelog) (#8464)
  fix(core): Fix stopping and retrying failed executions (#8480)
  feat: Add model parameter to OpenAI embeddings (#8481)
  fix(editor): Disable expression editor modal opening on readonly field (#8457)
  fix(core): Prevent calling internal hook email event if emailing is disabled (#8462)
@github-actions github-actions bot mentioned this pull request Jan 31, 2024
@janober
Copy link
Member

janober commented Feb 2, 2024

Got released with [email protected]

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.

3 participants