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

Handle properly when getUser returns null - prefer using getUserOrDefault #7372

Merged
merged 8 commits into from
Oct 19, 2022

Conversation

bmarty
Copy link
Member

@bmarty bmarty commented Oct 14, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

getUser can return null, since the loading of profile is now async. It may lead to unexpected issue and a crash, fixed by this commit: 0a6d620

Other commits fixes similar things. Also when retrieved, the profile is now stored into DB for future usage.

Motivation and context

Screenshots / GIFs

Tests

To reproduce the crash (but this is not 100% reproducible due to race condition)

  • Do an initial sync on a big account
  • Quickly open a room a start verifying a user.

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

@bmarty bmarty added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Oct 14, 2022
userItem = it,
isMine = userId == session.myUserId
)
} ?: return super.initialState(viewModelContext)
Copy link
Member Author

Choose a reason for hiding this comment

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

Defaulting to super.initialState(viewModelContext) make the app crash because there is no default constructor for DeviceListViewState

@bmarty bmarty changed the title Handle properly when getUser return null - prefer using getUserOrDefault Handle properly when getUser returns null - prefer using getUserOrDefault Oct 14, 2022
@bmarty
Copy link
Member Author

bmarty commented Oct 14, 2022

(not compiling, I will iterate)

@bmarty bmarty requested review from a team and mnaturel and removed request for a team October 17, 2022 10:24
@sonarcloud
Copy link

sonarcloud bot commented Oct 17, 2022

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

20.3% 20.3% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@mnaturel mnaturel 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 would like to test that it has fixed the crash. Is it possible you add some description in the PR description in the Tests section to do that?

@bmarty
Copy link
Member Author

bmarty commented Oct 18, 2022

LGTM. I would like to test that it has fixed the crash. Is it possible you add some description in the PR description in the Tests section to do that?

Yes, done, sorry the previous description was not complete, and I did not create a dedicated issue about the crash. A few reports with this crash are coming from the PlayStore, and it can hard to reproduce locally.

Copy link
Contributor

@mnaturel mnaturel left a comment

Choose a reason for hiding this comment

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

LGTM.

@bmarty bmarty merged commit f5fe5cc into develop Oct 19, 2022
@bmarty bmarty deleted the feature/bma/fix_getUser_null branch October 19, 2022 06:40
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.

2 participants