-
Notifications
You must be signed in to change notification settings - Fork 272
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
feat: allow the user to change membership role #515
feat: allow the user to change membership role #515
Conversation
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.
typo in file name:)
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.
@mskwierczynski Also please add some feedback to the user as we spoke
import { render } from '../../../tests/utils/rendering'; | ||
import { MembershipEntry } from './membershipEntry.component'; | ||
import { updateTenantMembershipMutation } from './membershipEntry.graphql'; | ||
import { membershipFactory, tenantFactory } from '@sb/webapp-tenants/tests/factories/tenant'; |
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.
use ../../../tests/factories/tenant
;
|
||
describe('MembershipEntry: Component', () => { | ||
it('should commit update mutation', async () => { | ||
jest.mock('react-router-dom', () => ({ |
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 con't need to use this. You can use the tenant id from the tenant passed to currentUserFactory
.
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.
oh yes, that's right
})); | ||
|
||
render( | ||
<CurrentTenantProvider> |
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.
Also there should be <CurrentTenantProvider/>
already in test providers included so you don't need to wrap the component here.
004844c
to
b629d52
Compare
...rc/components/tenantMembersList/membershipEntry/__tests__/membershipEntry.component.spec.tsx
Show resolved
Hide resolved
data, | ||
})); | ||
|
||
render(<MembershipEntry membership={membership} />, { |
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 are used to create a const Component = () => <MembershipEntry.../>
in every test to we could easily reuse the component props between tests in each test file. Also would be good to add refetch
param and check below if it is called after mutation
|
||
export type MembershipEntryProps = { | ||
className?: string; | ||
membership: TenantMembershipType; | ||
refetch?: () => void; |
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.
What do you think about changing this name to something more clear? Like onAfterUpdate
?
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.
sure. will do
...ebapp-tenants/src/components/tenantMembersList/membershipEntry/membershipEntry.component.tsx
Show resolved
Hide resolved
b1f430a
to
17e08eb
Compare
7e80efb
to
e27d9db
Compare
Please check if the PR fulfills these requirements
What kind of change does this PR introduce?
New feature: membership role update.
What is the current behavior?
What is the new behavior?
Closes #514