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

[Subscribing] Blank display name #5603

Merged
merged 6 commits into from
Mar 28, 2022
Merged

[Subscribing] Blank display name #5603

merged 6 commits into from
Mar 28, 2022

Conversation

Claire1817
Copy link
Contributor

@Claire1817 Claire1817 commented Mar 22, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Add a call to getProfile in the defaultSyncTask and store the result in the DB.

Motivation and context

#5497

Screenshots / GIFs

Screenshot_1647875995

Tests

  • Create an account
  • Open the left panel and check if there is the display name

Tested devices

  • Physical
  • Emulator
  • OS version(s): API30

Checklist

@Claire1817 Claire1817 marked this pull request as draft March 22, 2022 10:28
@github-actions
Copy link

github-actions bot commented Mar 22, 2022

Unit Test Results

106 files  ±0  106 suites  ±0   1m 13s ⏱️ -19s
188 tests ±0  188 ✔️ ±0  0 💤 ±0  0 ±0 
622 runs  ±0  622 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 61299a0. ± Comparison against base commit 794131d.

♻️ This comment has been updated with latest results.

@Claire1817 Claire1817 requested review from a team and ouchadam and removed request for a team March 22, 2022 13:05
@Claire1817 Claire1817 marked this pull request as ready for review March 22, 2022 13:07
val user = tryOrNull { session.getProfile(userId) }
userStore.createOrUpdate(
userId = userId,
displayName = user?.get(ProfileService.DISPLAY_NAME_KEY) as? String,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe take the opportunity to create some helper to avoid doing that everywhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

are there other places we're using this logic, could be a function in the class for now until we have another usage?

or do you mean helpers for fun User.getDisplayName() : String?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ganfra if we add the small part in a private method in SyncTask class is enough for you ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed a similar code in that file
We can probably have a function returning a User object from the given userId and the related profile dictionary?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, @Claire1817 add it to the interface ProfileService please.

Something like:

    suspend fun getProfileAsUser(userId: String): User {
        return getProfile(userId).let { dict ->
            User(
                    userId = userId,
                    displayName = dict[DISPLAY_NAME_KEY] as? String,
                    avatarUrl = dict[AVATAR_URL_KEY] as? String
            )
        }
    }

@ouchadam
Copy link
Contributor

nice catch! 💯 something that was brought it up in the FTUE channel (and we might fix in the future) was to align the drawer with iOS by centering the user id when there's no display name

Copy link
Contributor

@ouchadam ouchadam left a comment

Choose a reason for hiding this comment

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

LGTM! will handover to @ganfra to confirm the small refactor comment

Copy link
Contributor

@Florian14 Florian14 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 let someone from the core team approve your changes

Copy link
Contributor

@ouchadam ouchadam left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the updates! 💯

@Claire1817 Claire1817 merged commit a9b87de into develop Mar 28, 2022
@Claire1817 Claire1817 deleted the cgizard/ISSUE-5497 branch March 28, 2022 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants