fix(MM-49742): broken user avatar #8356

rahimrahman · 2024-11-18T20:23:48Z

Summary

Blank user avatar "hides" menu option.

Ticket Link

Fixes: https://mattermost.atlassian.net/browse/MM-49742

Checklist

Added or updated unit tests (required for all new features)
Has UI changes
Includes text changes and localization file updates
Have tested against the 5 core themes to ensure consistency between them.
Have run E2E tests by adding label E2E iOS tests for PR.

Device Information

This PR was tested on: iPhone 15 Simulator, iOS 17.4.1

Screenshots

With Photo	Without Photo

Added unit test to Account:

 PASS  app/screens/home/tab_bar/account.test.tsx
  Account
    ✓ should render correctly when focused (154 ms)
    ✓ should render correctly when unfocused (2 ms)
    ✓ should render ProfilePicture with correct props (1 ms)
  Account with database observable
    ✓ should render with the correct fake user when observing data from the database (2 ms)

A worker process has failed to exit gracefully and has been force exited. This is likely caused by tests leaking due to improper teardown. Try running with --detectOpenHandles to find leaks. Active timers can also cause this, ensure that .unref() was called on them.

=============================== Coverage summary ===============================
Statements   : 100% ( 7/7 )
Branches     : 100% ( 2/2 )
Functions    : 100% ( 3/3 )
Lines        : 100% ( 5/5 )
================================================================================
Test Suites: 1 passed, 1 total
Tests:       4 passed, 4 total
Snapshots:   0 total
Time:        2.032 s
Ran all test suites matching /app\/screens\/home\/tab_bar\/account.test.tsx/i.

Release Note

rahimrahman · 2024-11-18T20:25:51Z

app/components/profile_picture/image.tsx

@@ -44,10 +44,10 @@ const Image = ({author, forwardRef, iconSize, size, source, url}: Props) => {
    const lastPictureUpdateAt = author ? getLastPictureUpdate(author) : 0;
    const fIStyle = useMemo(() => ({
        borderRadius: size / 2,
-        backgroundColor: theme.centerChannelBg,
+        backgroundColor: changeOpacity(theme.centerChannelColor, 0.12),


@matthewbirtch per request

by just adding a 12% opacity center channel color fill

Only helps if the image is transparent. If the image is the same color as centerChannelColor, then you'd have the same problem.

rahimrahman · 2024-11-19T15:33:13Z

I will fix test as soon as I know that this is the right fix.

matthewbirtch · 2024-11-19T20:09:05Z

@rahimrahman are we able to conditionally change the background color? The reason I ask is that it doesn't show up well in all themes on the profile screen. See screenshot below for example.

If it appears on sidebarBg, then we should use 12% sidebarText. If it appears on the centerChannelBg, then it should be 12% centerChannelColor (as you have it).

Is this possible?

rahimrahman · 2024-11-20T20:57:54Z

@matthewbirtch the biggest challenge here is the backgroundcolor for the Image is buried 2-3 children deep.

(GrandParentComponent)
  |
  - ParentComponent
    |
    - ProfilePicture
      |
      - Image

And the Image component is where I'm setting the background. ParentComponent or GrandParentComponent is the one with the backgroundColor for the entire container, so in order for the Image component to distinguish what backgroundColor is being used from the ParentComponent or GrandParentComponent, we would have to pass the prop down a few levels (prop drilling).

I won't be able to use React context either since changing the backgroundColor based on the ParentComponent or GrandParentComponent will cause changes to all profile images.

What if we do what Elias suggested?

We can add a border to the avatar even if it has an image or a blank image, but I don’t know if that is what we want.

This would isolate the issue only on tab bar, but understandable, this might still be an issue in post, etc.

With Photo	Without Photo

matthewbirtch · 2024-11-28T21:25:48Z

Sorry for the delayed reply @rahimrahman. I'd rather not have a border on the profile image at all times. I'd rather go back to what we had previously in the original: #8356 (comment)

rahimrahman · 2024-12-03T16:21:33Z

Sorry for the delayed reply @rahimrahman. I'd rather not have a border on the profile image at all times. I'd rather go back to what we had previously in the original: #8356 (comment)

It's only on the tab bar. Not all the time.

matthewbirtch · 2024-12-04T17:42:33Z

Sorry for the delayed reply @rahimrahman. I'd rather not have a border on the profile image at all times. I'd rather go back to what we had previously in the original: #8356 (comment)

It's only on the tab bar. Not all the time.

Ok, I think this should be ok then. Let's do it

…ttom tab bar * status size reduced to 12 from default 14 * the size of the profile photo has been reduced from 28 to 22 to be inline with other icons on tab bar

app/constants/view.ts

rahimrahman · 2024-12-05T23:28:52Z

app/screens/home/tab_bar/account.test.tsx

+
+        const profilePicture = wrapper.getByTestId('account-profile-picture');
+
+        expect(profilePicture.props.author.email).toContain('@simulator.amazonses.com');


This is one of the few consistent data from the fakeUser method. This is just to make sure the observable is working.

To make the test more "stable" and a bit more intention revealing, you could add to the database a new user, and set the current user id to that user. That way you don't have to rely on "the few consistent data from the fakeUser method".

Make sense too. Thank you for your feedback. Although the goal of this is to ensure that the prop author receive an object of UserModel. I could check that it is not undefined because currentUser can be.

Now that I think about it, I should think about currentUser being undefined in Account, however since I'm mocking ProfilePicture, it will not mean a thing.

I can also check that the author has a few keys, like email, firstName & lastName. What the values are irrelevant.

larkox

Nothing blocking, but a few things to consider.

app/screens/home/tab_bar/account.tsx

larkox · 2024-12-06T10:12:38Z

app/screens/home/tab_bar/account.tsx

 }));

-const Account = ({currentUser, isFocused, theme}: Props) => {
+export const Account = ({currentUser, isFocused, theme}: Props) => {


I am not a big fan of exporting stuff just for testing, since they can be inadvertently imported.

My approach is either test everything on the exported interface, or export a new object (exportsForTest or something like that) with all the elements you want to export only for testing purposes.

It is dirty and ugly, but helps not importing things that shouldn't be imported in other files.

@larkox isn't this the same as creating two separate files, one for Account component and the second one only with database with observables? Account component will be tested separately, and the file with database observables will be test separately as well.

Would that be more ideal? That way I'm not exporting just for testing, but exporting to be imported inside observable.

My approach is either test everything on the exported interface

I like unit testing in isolation and in the most atomic level. 1) Account component and 2) with observable, to show what each parts will do. Thus the reason why I had 2 different describe.

Having two different files seems more "the way we do things". It still opens the door to importing the wrong component, but it is less common (it is easier to notice that you are importing something like: components/myComponent/myComponent).

I don't disagree with your claim of "testing in isolation and in the most atomic level", but it is something to be careful about, since it can be driven to the absurd. For example, you could consider that each callback is an atomic part, and should be tested separately, and your component will look like the following:

export const myCallback1 = (prop1, prop2, state1, state2, setState1, setState2) => {...}; export const myCallback2 = (prop1, state1, setState2) => {...}; const MyComponent = (prop1, prop2) => { const [state1, setState1] = useState(); const [state2, setState2] = useState(); const c1 = useCallback(() => myCallback1(prop1, prop2, state1, state2, setState1, setState2)); const c2 = useCallback(() => myCallback2(prop1, state1, setState2)); return (...); } export default MyComponent;

With your approach here, you are creating a new "implementation detail separation" between the component and the with observable. And what I mean with an "implementation detail separation" is that the component will never be used without the with observable. Even if in code they can be separate, logically they are an inseparable atomic thing.

That being said, that approach can also gotten to the absurd and say that the whole app is an atomic inseparable thing, and we should only the test the app as a whole.

In summary... I don't disagree on making this cut as long as it makes testing easier, but as a rule of thumb I would try to make the test of the whole component (state and component, not state on one side and component on the other).

Understood. I see your points. I have been invited and accepted to join the testing team and will bring this up to see where we can find common grounds. In the meantime, I will leave it as it is, with the knowledge that Account now can be imported as a component without observables, and eventually it should and it could (as part of design components).

larkox · 2024-12-06T10:13:10Z

app/screens/home/tab_bar/account.test.tsx

+
+        const profilePicture = wrapper.getByTestId('account-profile-picture');
+
+        expect(profilePicture.props.author.email).toContain('@simulator.amazonses.com');


To make the test more "stable" and a bit more intention revealing, you could add to the database a new user, and set the current user id to that user. That way you don't have to rely on "the few consistent data from the fakeUser method".

nickmisasi

LGTM

rahimrahman · 2024-12-06T19:56:35Z

@matthewbirtch hi, reminder to review the result of the changes that we agreed upon. Here are two screenshots to make it easier to see.

matthewbirtch

Looks good. Thanks @rahimrahman

yasserfaraazkhan

Hi @rahimrahman
there is a crash occuring when I open channel, on iphone and android device
I'll DM the test server creds

com.mattermost.rnbeta-latest.log

enahum · 2024-12-11T11:48:01Z

Hi @rahimrahman

there is a crash occuring when I open channel, on iphone and android device

I'll DM the test server creds

com.mattermost.rnbeta-latest.log

I'm not sure is related to this PR. @larkox would this be related to the work you did around the components to display in the settings screen or something?

larkox · 2024-12-11T12:21:06Z

Hi @rahimrahman there is a crash occuring when I open channel, on iphone and android device I'll DM the test server creds

com.mattermost.rnbeta-latest.log

This is almost sure something related to the prop validation PR I added. Merging main into this PR should fix it.

larkox · 2024-12-11T12:21:19Z

/update-branch

yasserfaraazkhan

Thank you Rahim.
No blockers on the changes. The bug exists and might have a fix from Daniel. I'll follow up on that.

fix(MM-49742): broken user avatar

2cd52f5

mm-cloud-bot added kind/bug Categorizes issue or PR as related to a bug. release-note labels Nov 18, 2024

rahimrahman requested a review from matthewbirtch November 18, 2024 20:24

rahimrahman commented Nov 18, 2024

View reviewed changes

rahimrahman added 3 commits December 4, 2024 12:58

add border for both focused and unfocused for profile photo on the bo…

950235b

…ttom tab bar * status size reduced to 12 from default 14 * the size of the profile photo has been reduced from 28 to 22 to be inline with other icons on tab bar

revert initial commit

56e1d90

add unit test for Account

5c62019

rahimrahman requested review from a team, nickmisasi and larkox and removed request for a team December 5, 2024 19:31

rahimrahman commented Dec 5, 2024

View reviewed changes

larkox approved these changes Dec 6, 2024

View reviewed changes

nickmisasi approved these changes Dec 6, 2024

View reviewed changes

matthewbirtch approved these changes Dec 6, 2024

View reviewed changes

rahimrahman added 3: QA Review Requires review by a QA tester Build Apps for PR Build the mobile app for iOS and Android to test labels Dec 10, 2024

rahimrahman requested a review from yasserfaraazkhan December 10, 2024 15:44

yasserfaraazkhan requested changes Dec 11, 2024

View reviewed changes

Merge branch 'main' into fix/MM-49742-blank-user-avatar

60d426d

yasserfaraazkhan self-requested a review December 11, 2024 15:52

yasserfaraazkhan approved these changes Dec 11, 2024

View reviewed changes

rahimrahman added QA Review Done and removed 3: QA Review Requires review by a QA tester labels Dec 12, 2024

rahimrahman merged commit 6d94675 into main Dec 12, 2024
12 checks passed

rahimrahman deleted the fix/MM-49742-blank-user-avatar branch December 12, 2024 17:33

amyblais added this to the v2.25.0 milestone Dec 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(MM-49742): broken user avatar #8356

fix(MM-49742): broken user avatar #8356

rahimrahman commented Nov 18, 2024 •

edited

Loading

rahimrahman Nov 18, 2024

rahimrahman commented Nov 19, 2024

matthewbirtch commented Nov 19, 2024 •

edited

Loading

rahimrahman commented Nov 20, 2024 •

edited

Loading

matthewbirtch commented Nov 28, 2024

rahimrahman commented Dec 3, 2024

matthewbirtch commented Dec 4, 2024

rahimrahman Dec 5, 2024

larkox Dec 6, 2024

rahimrahman Dec 6, 2024

larkox left a comment

larkox Dec 6, 2024

rahimrahman Dec 6, 2024

larkox Dec 9, 2024

rahimrahman Dec 9, 2024

larkox Dec 6, 2024

nickmisasi left a comment

rahimrahman commented Dec 6, 2024

matthewbirtch left a comment

yasserfaraazkhan left a comment

enahum commented Dec 11, 2024

larkox commented Dec 11, 2024

larkox commented Dec 11, 2024

yasserfaraazkhan left a comment


		const profilePicture = wrapper.getByTestId('account-profile-picture');

		expect(profilePicture.props.author.email).toContain('@simulator.amazonses.com');

fix(MM-49742): broken user avatar #8356

fix(MM-49742): broken user avatar #8356

Conversation

rahimrahman commented Nov 18, 2024 • edited Loading

Summary

Ticket Link

Checklist

Device Information

Screenshots

Release Note

Choose a reason for hiding this comment

rahimrahman commented Nov 19, 2024

matthewbirtch commented Nov 19, 2024 • edited Loading

rahimrahman commented Nov 20, 2024 • edited Loading

matthewbirtch commented Nov 28, 2024

rahimrahman commented Dec 3, 2024

matthewbirtch commented Dec 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larkox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickmisasi left a comment

Choose a reason for hiding this comment

rahimrahman commented Dec 6, 2024

matthewbirtch left a comment

Choose a reason for hiding this comment

yasserfaraazkhan left a comment

Choose a reason for hiding this comment

enahum commented Dec 11, 2024

larkox commented Dec 11, 2024

larkox commented Dec 11, 2024

yasserfaraazkhan left a comment

Choose a reason for hiding this comment

rahimrahman commented Nov 18, 2024 •

edited

Loading

matthewbirtch commented Nov 19, 2024 •

edited

Loading

rahimrahman commented Nov 20, 2024 •

edited

Loading