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

Display a warning when an unverified user's identity changes #28211

Merged

Conversation

uhoreg
Copy link
Member

@uhoreg uhoreg commented Oct 16, 2024

Fixes element-hq/element-meta#2513

image

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • I have licensed the changes to Element by completing the Contributor License Agreement (CLA)

@uhoreg uhoreg requested review from a team and richvdh and removed request for a team October 16, 2024 21:54
@CLAassistant
Copy link

CLAassistant commented Oct 16, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@florianduros florianduros left a 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.

res/css/views/rooms/_UserIdentityWarning.pcss Outdated Show resolved Hide resolved
res/css/views/rooms/_UserIdentityWarning.pcss Outdated Show resolved Hide resolved
res/css/views/rooms/_UserIdentityWarning.pcss Outdated Show resolved Hide resolved
res/css/views/rooms/_UserIdentityWarning.pcss Show resolved Hide resolved
res/css/views/rooms/_UserIdentityWarning.pcss Outdated Show resolved Hide resolved
res/css/views/rooms/_UserIdentityWarning.pcss Show resolved Hide resolved
src/components/views/rooms/UserIdentityWarning.tsx Outdated Show resolved Hide resolved
src/components/views/rooms/UserIdentityWarning.tsx Outdated Show resolved Hide resolved
src/components/views/rooms/UserIdentityWarning.tsx Outdated Show resolved Hide resolved
src/components/views/rooms/UserIdentityWarning.tsx Outdated Show resolved Hide resolved
@uhoreg
Copy link
Member Author

uhoreg commented Oct 23, 2024

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.

Copy link
Member

@florianduros florianduros left a 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

src/components/views/rooms/UserIdentityWarning.tsx Outdated Show resolved Hide resolved
src/components/views/rooms/UserIdentityWarning.tsx Outdated Show resolved Hide resolved
src/components/views/rooms/UserIdentityWarning.tsx Outdated Show resolved Hide resolved
Copy link
Member

@florianduros florianduros left a 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.

src/components/views/rooms/UserIdentityWarning.tsx Outdated Show resolved Hide resolved
src/components/views/rooms/UserIdentityWarning.tsx Outdated Show resolved Hide resolved
src/components/views/rooms/UserIdentityWarning.tsx Outdated Show resolved Hide resolved
src/components/views/rooms/UserIdentityWarning.tsx Outdated Show resolved Hide resolved
src/components/views/rooms/UserIdentityWarning.tsx Outdated Show resolved Hide resolved
src/components/views/rooms/UserIdentityWarning.tsx Outdated Show resolved Hide resolved
src/components/views/rooms/UserIdentityWarning.tsx Outdated Show resolved Hide resolved
src/components/views/rooms/UserIdentityWarning.tsx Outdated Show resolved Hide resolved
src/components/views/rooms/UserIdentityWarning.tsx Outdated Show resolved Hide resolved
[cli, room, updateCurrentPrompt],
);

// Check if the user's identity needs approval, and if so, add them to the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// 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

src/components/views/rooms/UserIdentityWarning.tsx Outdated Show resolved Hide resolved
Copy link
Member

@richvdh richvdh left a 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!

src/components/views/rooms/UserIdentityWarning.tsx Outdated Show resolved Hide resolved
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
@uhoreg
Copy link
Member Author

uhoreg commented Nov 20, 2024

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.

@uhoreg uhoreg added this pull request to the merge queue Nov 21, 2024
Merged via the queue into element-hq:develop with commit c2ce7db Nov 21, 2024
29 checks passed
@uhoreg uhoreg deleted the unverified_user_identity_change_warning branch November 21, 2024 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invisible Crypto: Web: display a warning when an *unverified* user changes identity
5 participants