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): Fix parameter reset on credential change in Discord node #9137

Conversation

elsmr
Copy link
Member

@elsmr elsmr commented Apr 12, 2024

Summary

Resource and operation parameters should not reset when credentials is changed.

The code that is looking for mismatched collection parameter options was also resetting options parameters.

Related to #8114

Related tickets and issues

https://linear.app/n8n/issue/NODE-1089/discord-actions-not-being-seleted-after-selecting-them-in-nodes-panel

Review / Merge checklist

  • PR title and summary are descriptive. Remember, the title automatically goes into the changelog. Use (no-changelog) otherwise. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.

    A bug is not considered fixed, unless a test is added to prevent it from happening again.
    A feature is not complete without tests.

Copy link
Contributor

@MiloradFilipovic MiloradFilipovic left a comment

Choose a reason for hiding this comment

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

Code looks ok but I have two comments on the solution:

  1. This doesn't solve the issue from the linear ticket (which I cannot reproduce so if the issue is not valid anymore it would be good to update the ticket or create a new one just so we keep linear tickets and PR in sync)
  2. I'm still encountering the issue where parameters are reset after changing the credentials:
Screen.Recording.2024-04-12.at.16.23.14.mov

@elsmr
Copy link
Member Author

elsmr commented Apr 12, 2024

@MiloradFilipovic Updated the linear issue description. The original issue is indeed no longer reproducible, see comment thread in Linear with Jon and Niklas.

I think I missed the operation also being reset because I was testing with Message/Send (Send being the default operation), fixed now! 🎉

@n8n-assistant n8n-assistant bot added community Authored by a community member ui Enhancement in /editor-ui or /design-system labels Apr 16, 2024
@Joffcom Joffcom added n8n team Authored by the n8n team and removed community Authored by a community member labels Apr 16, 2024
Copy link
Contributor

@MiloradFilipovic MiloradFilipovic left a comment

Choose a reason for hiding this comment

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

Looking good 🚀

Copy link

cypress bot commented Apr 17, 2024

2 flaky tests on run #4705 ↗︎

0 355 12 0 Flakiness 2

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 elsmr 🗃️ e2e/*
Project: n8n Commit: fc1d3265e3
Status: Passed Duration: 04:25 💡
Started: Apr 17, 2024 12:50 PM Ended: Apr 17, 2024 12:55 PM
Flakiness  5-ndv.cy.ts • 1 flaky test

View Output Video

Test Artifacts
NDV > Stop listening for trigger event from NDV Screenshots Video
Flakiness  24-ndv-paired-item.cy.ts • 1 flaky test

View Output Video

Test Artifacts
NDV > resolves expression with default item when input node is not parent, while still pairing items Test Replay Screenshots Video

Review all test suite changes for PR #9137 ↗︎

Copy link
Contributor

✅ All Cypress E2E specs passed

@elsmr elsmr merged commit 135ef75 into master Apr 18, 2024
28 checks passed
@elsmr elsmr deleted the node-1089-discord-actions-not-being-seleted-after-selecting-them-in branch April 18, 2024 12:56
@github-actions github-actions bot mentioned this pull request Apr 18, 2024
@janober
Copy link
Member

janober commented Apr 18, 2024

Got released with n8n@1.38.1

@github-actions github-actions bot mentioned this pull request Apr 24, 2024
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.

None yet

4 participants