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 empty verification bottom sheet #7130

Merged
merged 4 commits into from
Sep 16, 2022
Merged

Conversation

bmarty
Copy link
Member

@bmarty bmarty commented Sep 14, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

When the User with otherUserId was not known by the SDK, it was not possible to verify them.

First commit 6d2a9ec is the first part of the fix. otherUserId was null because retrieved from a null MatrixItem.
Second commit 92d7391 ensure that the MatrixItem cannot be null.

The 2nd commit should be sufficient to fix the issue, but the first commit make thing clearer IMHO.

Motivation and context

Be able to Verify every users.

Screenshots / GIFs

Before After
imageimage imageimage

Tests

  • Step 1
  • Step 2
  • Step ...

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

bmarty and others added 2 commits September 14, 2022 19:05
Ensure the User is retrieved from the network, or fallback to a default User object.
@bmarty bmarty requested review from a team and onurays and removed request for a team September 14, 2022 18:31
@@ -126,7 +128,7 @@ class VerificationBottomSheetViewModel @AssistedInject constructor(
}
}

val userItem = session.getUser(initialState.otherUserId)
Copy link
Member Author

Choose a reason for hiding this comment

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

A null userItem was the source of the issue.

@@ -160,7 +162,6 @@ class VerificationBottomSheetViewModel @AssistedInject constructor(

setState {
copy(
otherUserMxItem = userItem?.toMatrixItem(),
Copy link
Member Author

Choose a reason for hiding this comment

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

is set in the constructor and updated with full data in fetchOtherUserProfile

session.getUser(otherUserId)?.toMatrixItem()?.let {
setState {
copy(
otherUserMxItem = it
Copy link
Member Author

Choose a reason for hiding this comment

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

First change to set known data

?.let {
setState {
copy(
otherUserMxItem = it
Copy link
Member Author

Choose a reason for hiding this comment

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

second change to set up to date data, if possible (network available + API is returning data)

@@ -216,12 +239,12 @@ class VerificationBottomSheetViewModel @AssistedInject constructor(

private fun cancelAllPendingVerifications(state: VerificationBottomSheetViewState) {
session.cryptoService()
.verificationService().getExistingVerificationRequest(state.otherUserMxItem?.id ?: "", state.transactionId)?.let {
Copy link
Member Author

@bmarty bmarty Sep 14, 2022

Choose a reason for hiding this comment

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

defaulting to "" everywhere was pretty bad...

@bmarty bmarty requested a review from BillCarsonFr September 14, 2022 18:37
@bmarty
Copy link
Member Author

bmarty commented Sep 14, 2022

Adding @BillCarsonFr as a reviewer, since this is code related to Crypto.

@ganfra
Copy link
Member

ganfra commented Sep 14, 2022

Probably because of the move to worker for fetching users?

Copy link
Member

@BillCarsonFr BillCarsonFr left a comment

Choose a reason for hiding this comment

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

LGTM.
I can see that getUser() is also called in IncomingVerificationRequestHandler.
It should call resolveUser or at least default to a MatriItem with the correct userId?

@bmarty bmarty added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Sep 15, 2022
@bmarty
Copy link
Member Author

bmarty commented Sep 15, 2022

Probably because of the move to worker for fetching users?

I do not think so, because even all attempts was unsuccessful. With this theory, after a while it should work. I think this is not a regression, but an old bug.

@bmarty
Copy link
Member Author

bmarty commented Sep 15, 2022

LGTM. I can see that getUser() is also called in IncomingVerificationRequestHandler. It should call resolveUser or at least default to a MatriItem with the correct userId?

Thanks, let me have a look on it.

@bmarty bmarty closed this Sep 15, 2022
@bmarty bmarty reopened this Sep 15, 2022
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

8.9% 8.9% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@onurays onurays left a comment

Choose a reason for hiding this comment

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

Nice improvement. Statically reviewed, LGTM! It was annoying to fallback to "".

@bmarty bmarty merged commit 73e061e into develop Sep 16, 2022
@bmarty bmarty deleted the feature/bma/fix_verification branch September 16, 2022 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-NextRelease For issues and PRs which should be included in the NextRelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants