-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Display a warning when an unverified user's identity changes #28211
Display a warning when an unverified user's identity changes #28211
Conversation
ada3de0
to
6fa98f8
Compare
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.
In a first time, we should move UserIdentifyWarning
into a functional component and break down its behaviour into multiple hooks. Let me know if you need help to do it
There are differences with the figma design too.
I think that I've addressed all of @florianduros 's concerns. The test failure looks like possible flakiness, since it's testing components that I haven't touched. |
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 are in the right direction! Now, we can refactor this hook into multiple custom hooks seperate by logic concern et will be ready to go.
About the test failure, yes it's a flackiness which has been adresse in another PR
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.
It's looking better! Now we can move theses hooks and their logic into separate custom hooks in order to encapsulate their behaviour and make the component easier to maintain.
In test:
- use
jest.spyOn
in test to handle the mocking of the client or the crypto api. - use https://testing-library.com/docs/queries/about/ to get the dom element. We want to favour the
role
query in order to make it sure that we are also accessible - In case of async behaviour when we need to wait for a rerender,
waitFor
is doing for us the retry/waiting.
test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx
Outdated
Show resolved
Hide resolved
test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx
Outdated
Show resolved
Hide resolved
test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx
Outdated
Show resolved
Hide resolved
[cli, room, updateCurrentPrompt], | ||
); | ||
|
||
// Check if the user's identity needs approval, and if so, add them to the |
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.
// Check if the user's identity needs approval, and if so, add them to the | |
// For each user in the list, check if their identity needs approval, and if so, add them to the |
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.
I'd have quite liked to see a test for the join-leave-join scenario, but we should probably just ship this.
LGTM other than the below, well done for persevering with it!
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Strange. One of the tests is now failing in CI, even though I only made a comment change. And I can't reproduce the failure locally. Will need to investigate. |
Fixes element-hq/element-meta#2513
Checklist
public
/exported
symbols have accurate TSDoc documentation.