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.

Sign up for GitHub

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(MM-49742): broken user avatar #8356

Merged
merged 5 commits into from
Dec 12, 2024
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2cd52f5
fix(MM-49742): broken user avatar
rahimrahman Nov 18, 2024
950235b
add border for both focused and unfocused for profile photo on the bo…
rahimrahman Dec 4, 2024
56e1d90
revert initial commit
rahimrahman Dec 4, 2024
5c62019
add unit test for Account
rahimrahman Dec 5, 2024
60d426d
Merge branch 'main' into fix/MM-49742-blank-user-avatar
mattermost-build Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions app/constants/view.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ import {Platform} from 'react-native';

export const BOTTOM_TAB_HEIGHT = 52;
export const BOTTOM_TAB_ICON_SIZE = 31.2;
export const BOTTOM_TAB_PROFILE_PHOTO_SIZE = 22;
rahimrahman marked this conversation as resolved.
Show resolved Hide resolved
export const BOTTOM_TAB_STATUS_SIZE = 12;

export const PROFILE_PICTURE_SIZE = 32;
export const PROFILE_PICTURE_EMOJI_SIZE = 28;

Expand Down
93 changes: 93 additions & 0 deletions app/screens/home/tab_bar/account.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
// See LICENSE.txt for license information.
import React from 'react';

import {BOTTOM_TAB_PROFILE_PHOTO_SIZE, BOTTOM_TAB_STATUS_SIZE} from '@constants/view';
import {renderWithEverything, renderWithIntl} from '@test/intl-test-helper';
import TestHelper from '@test/test_helper';
import {changeOpacity} from '@utils/theme';

import {Account, default as AccountWithDatabaseObservable} from './account';

import type {UserModel} from '@database/models/server';
import type Database from '@nozbe/watermelondb/Database';

jest.mock('@components/profile_picture', () => 'ProfilePicture');

const currentUser = {
id: 'user1',
username: 'testuser',
} as UserModel;

const theme = {
buttonBg: '#000',
centerChannelColor: '#fff',
} as Theme;

describe('Account', () => {
const centerChannelColorWithOpacity = changeOpacity(theme.centerChannelColor, 0.48);

it('should render correctly when focused', () => {
const wrapper = renderWithIntl(
<Account
currentUser={currentUser}
isFocused={true}
theme={theme}
/>,
);

const container = wrapper.getByTestId('account-container');
expect(container.props.style).toContainEqual({borderColor: theme.buttonBg});
});

it('should render correctly when unfocused', () => {
const {getByTestId} = renderWithIntl(
<Account
currentUser={currentUser}
isFocused={false}
theme={theme}
/>,
);

const container = getByTestId('account-container');
expect(container.props.style).toContainEqual({borderColor: centerChannelColorWithOpacity});
});

it('should render ProfilePicture with correct props', () => {
const {getByTestId} = renderWithIntl(
<Account
currentUser={currentUser}
isFocused={true}
theme={theme}
/>,
);

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

expect(profilePicture.props.author).toEqual({id: 'user1', username: 'testuser'});
expect(profilePicture.props.showStatus).toBe(true);
expect(profilePicture.props.size).toBe(BOTTOM_TAB_PROFILE_PHOTO_SIZE);
expect(profilePicture.props.statusSize).toBe(BOTTOM_TAB_STATUS_SIZE);
});
});

describe('Account with database observable', () => {
let database: Database;

beforeAll(async () => {
const server = await TestHelper.setupServerDatabase();
database = server.database;
});

it('should render with the correct fake user when observing data from the database', () => {
const wrapper = renderWithEverything(
<AccountWithDatabaseObservable
theme={theme}
isFocused={true}
/>, {database});

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

expect(profilePicture.props.author.email).toContain('@simulator.amazonses.com');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

});
});
23 changes: 17 additions & 6 deletions app/screens/home/tab_bar/account.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@ import React from 'react';
import {View} from 'react-native';

import ProfilePicture from '@components/profile_picture';
import {BOTTOM_TAB_PROFILE_PHOTO_SIZE, BOTTOM_TAB_STATUS_SIZE} from '@constants/view';
import {observeCurrentUser} from '@queries/servers/user';
import {makeStyleSheetFromTheme} from '@utils/theme';
import {changeOpacity, makeStyleSheetFromTheme} from '@utils/theme';

import type {WithDatabaseArgs} from '@typings/database/database';
import type UserModel from '@typings/database/models/servers/user';
Expand All @@ -19,22 +20,32 @@ type Props = {
}

const getStyleSheet = makeStyleSheetFromTheme((theme: Theme) => ({
selected: {
container: {
borderWidth: 2,
borderColor: theme.buttonBg,
borderRadius: 20,
},
focused: {
borderColor: theme.buttonBg,
},
unfocused: {
borderColor: changeOpacity(theme.centerChannelColor, 0.48),
larkox marked this conversation as resolved.
Show resolved Hide resolved
},
}));

const Account = ({currentUser, isFocused, theme}: Props) => {
export const Account = ({currentUser, isFocused, theme}: Props) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

const style = getStyleSheet(theme);

return (
<View style={isFocused ? style.selected : undefined}>
<View
style={[isFocused ? style.focused : style.unfocused, style.container]}
testID='account-container'
>
<ProfilePicture
testID='account-profile-picture'
author={currentUser}
showStatus={true}
size={28}
size={BOTTOM_TAB_PROFILE_PHOTO_SIZE}
statusSize={BOTTOM_TAB_STATUS_SIZE}
/>
</View>
);
Expand Down