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: Resend invite operation on users list #11351

Merged

Conversation

RicardoE105
Copy link
Contributor

@RicardoE105 RicardoE105 commented Oct 22, 2024

Summary

Pending users were added into the store with the property globalRole, but when we return the users we save them in the store with the property role instead.

image

Before

See video on ticket -> https://linear.app/n8n/issue/HELP-645/resend-invitation-link-does-not-work-anymore

Now

CleanShot 2024-10-22 at 16 10 25

Related Linear tickets, Github issues, and Community forum posts

https://linear.app/n8n/issue/HELP-645/resend-invitation-link-does-not-work-anymore

Review / Merge checklist

  • PR title and summary are descriptive. (conventions)
  • Tests included.
  • PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

@RicardoE105 RicardoE105 changed the title fix resend invite operation fix(editor): Resend invite operation Oct 22, 2024
@RicardoE105 RicardoE105 changed the title fix(editor): Resend invite operation fix(editor): Resend invite operation on users list Oct 22, 2024
@RicardoE105 RicardoE105 added the tests-needed This PR needs additional tests label Oct 22, 2024
netroy
netroy previously requested changes Oct 22, 2024
Copy link
Member

@netroy netroy left a comment

Choose a reason for hiding this comment

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

since users.store already has a test file, can we please add a test case to cover for this? that way we can avoid the same regression in the future.

@n8n-assistant n8n-assistant bot added n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels Oct 22, 2024
@RicardoE105 RicardoE105 removed the tests-needed This PR needs additional tests label Oct 22, 2024
Copy link
Contributor

@despairblue despairblue left a comment

Choose a reason for hiding this comment

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

The test wasn't preventing a regression. The API mock contained the role, but in reality this happens because the API does not return the role, right?

Which begs the question if the store code could be simplified by changing the API?

packages/editor-ui/src/stores/users.store.test.ts Outdated Show resolved Hide resolved
@RicardoE105
Copy link
Contributor Author

RicardoE105 commented Oct 22, 2024

@despairblue yes, adding the role there is an overlooked. Just committed your change. Adding the the ID and not returning the role still works.

@despairblue
Copy link
Contributor

@despairblue yes, adding the role there is an overlooked. Just committed your change. Adding the the ID and not returning the role still works.

Would it be cleaner to add the role property to the API response?

This makes sure the role is in the user store in the frontend without
the FE needing to do manual remapping.
@despairblue
Copy link
Contributor

@RicardoE105 this is what I mean:

a6565d3

Just wondering if that is cleaner, than having the FE retain data it sends to the API and merge it with the response.

@RicardoE105
Copy link
Contributor Author

RicardoE105 commented Oct 22, 2024

@despairblue gotcha now. Yes, let's do that. Will include that change if this PR if you do not mind.

@RicardoE105 RicardoE105 requested review from despairblue and removed request for netroy October 22, 2024 17:46
@RicardoE105 RicardoE105 changed the title fix(editor): Resend invite operation on users list fix: Resend invite operation on users list Oct 22, 2024
Copy link

cypress bot commented Oct 23, 2024

n8n    Run #7508

Run Properties:  status check passed Passed #7508  •  git commit 1cd0284f2b: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 RicardoE105 🗃️ e2e/*
Project n8n
Branch Review help-645-resend-invitation-link-does-not-work-anymore
Run status status check passed Passed #7508
Run duration 04m 27s
Commit git commit 1cd0284f2b: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 RicardoE105 🗃️ e2e/*
Committer Ricardo Espinoza
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 3
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 458
View all changes introduced in this branch ↗︎

Copy link
Contributor

✅ All Cypress E2E specs passed

@netroy netroy dismissed their stale review October 23, 2024 08:22

approved already

@RicardoE105 RicardoE105 merged commit e4218de into master Oct 23, 2024
35 checks passed
@RicardoE105 RicardoE105 deleted the help-645-resend-invitation-link-does-not-work-anymore branch October 23, 2024 08:23
@github-actions github-actions bot mentioned this pull request Oct 24, 2024
@janober
Copy link
Member

janober commented Oct 24, 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.

4 participants