Add self chat feature by rarira · Pull Request #474 · hyochan/hackatalk

rarira · 2021-08-26T10:07:05Z

Specify project

Both Server and Client

Description

Show UserListItem of mySelf in the top of the Friend Tab, just like KakaoTalk does.
- My thumbnail is intended to be larger than others
Open ProfileModal of myself if UserListItem mentioned above is pressed.
ProfileModal of myself only renders ProfileUpdate button and Chat with myself button:
- Block, Report button removed
- Delete Friend button removed
Chat with myself button finds already opened self chatroom and navigate to that self chat room.
If failed, it creates ChannelType.self chatroom and navigate to newly created room.
You can also create the self chat room by selecting only yourself after pressing FAB Button in the ChannelCreate
Show small circle view including text(getString('Me')) in front of users name to distinguish a self chat room from others

Additional Comments:

below PRs are preferred to be merged before this and i'll make new commit to prevent possible conflicts:
Add result message when updating profile #470
Add result message when updating profile #470
lots of updated relay generated files included

Related Issues

Create empty channel (Self Chat?) #459
- You can't create channel without selecting more than one user
- If you select only yourself, it means you want self chat room
- If you select multiple users, yourself is ignored always: ['yourID', 'otherId'] => ['otherId']
Self chat feature #311
- This is the main issue I've solved

Tests

I added the following tests:
Add almost 100% of test codes for the lines newly added

Checklist

Before you create this PR confirms that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the [CLA].
Run yarn lint && yarn tsc
Run yarn test or yarn test -u if you need to update snapshot.
I am willing to follow-up on review comments in a timely manner.

[wehack2021] - [cloud]

codecov · 2021-08-26T13:52:20Z

Codecov Report

Merging #474 (28081a8) into master (758429c) will increase coverage by 1.60%.
The diff coverage is 71.91%.

@@            Coverage Diff             @@
##           master     #474      +/-   ##
==========================================
+ Coverage   63.05%   64.66%   +1.60%     
==========================================
  Files         110      110              
  Lines        2550     2595      +45     
  Branches      420      429       +9     
==========================================
+ Hits         1608     1678      +70     
+ Misses        810      790      -20     
+ Partials      132      127       -5

Impacted Files	Coverage Δ
...omponents/navigations/MainStackNavigator/index.tsx	`3.44% <ø> (ø)`
client/src/components/pages/MainChannel.tsx	`74.28% <ø> (+2.85%)`	⬆️
server/src/resolvers/Friend/query.ts	`35.71% <12.50%> (-19.85%)`	⬇️
...ts/navigations/MainStackNavigator/ProfileModal.tsx	`58.41% <44.44%> (+2.62%)`	⬆️
server/src/services/ChannelService.ts	`90.90% <57.14%> (-9.10%)`	⬇️
client/src/components/pages/ChannelCreate.tsx	`56.33% <60.00%> (+19.02%)`	⬆️
server/src/resolvers/Channel/mutation.ts	`86.20% <62.50%> (-0.55%)`	⬇️
client/src/components/pages/MainFriend.tsx	`87.50% <100.00%> (+4.16%)`	⬆️
client/src/components/uis/ChannelListItem.tsx	`100.00% <100.00%> (+34.54%)`	⬆️
client/src/components/uis/UserListItem.tsx	`87.87% <100.00%> (+9.30%)`	⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 758429c...28081a8. Read the comment docs.

JongtaekChoi

It seems really good!

0916dhkim

@rarira Fabulous work! Also, the test cases look awesome 👍
I approve this PR with a lot of suggestions below 😄

0916dhkim · 2021-08-29T01:15:11Z

+      if (channelType !== ChannelType.self)
+        await createMemberships(channel.id, filteredPeerUserIds);


We don't need to resolve this in this PR, but I think we need some refactoring in this file. Functions are getting quite big.

0916dhkim · 2021-08-29T01:20:22Z

+export const getChannelType = (
+  userId: string,
+  peerUserIds: string[],
+): ChannelType => {
+  if (peerUserIds.length === 1 && peerUserIds[0] === userId)
+    return ChannelType.self;
+
+  return ChannelType.private;
+};


I like how this logic is extracted into a separate function; however, I think the name can be more clear. How about we name this isSelfChannel(userId, memberIds): boolean?

I intended this function to return ChannelType not boolean because there might be new feature added to server 'public' type channel. But now I know that in that case, we should create totally new different mutation function which is not findOrCreatePrivateChannel..

So I'll make change as you suggested, thanks always

I had changed codes as you suggested but it brings 3 test case failures in server side Channel.test.ts. I think it is because old createNewChannel function was used even when creating public channel..

So I had to revert that commit.

I think we should refactor all related codes when we introduce public channel feature.

I see. Thanks for trying 👍

rarira · 2021-08-29T11:46:20Z

Changed all excludeMe: true to excludeMe:false to update store properly. it's finished, I hope.

0916dhkim · 2021-08-30T01:57:30Z

@rarira Just making sure you did not miss it. There are 19 hidden comments in my first review. Github automatically folds when there are too many comments in one review 😅

rarira · 2021-08-30T16:25:51Z

@rarira When useAuthContext hook is mocked, AuthContextProvider is not necessary here (thus you don't need to export it either). You can check how useNavigation hook is mocked here:
https://github.com/dooboolab/hackatalk/blob/18eef96705434ad25b9ae72aeef463f83f9ded2b/client/src/components/pages/__tests__/ChangePw.test.tsx

I'll try once again later. Thanks.

hyochan#459

Co-authored-by: Donghyeon Kim <0916dhkim@gmail.com>

This reverts commit 3a7f471.

It should be changed update store properly

Co-authored-by: Donghyeon Kim <0916dhkim@gmail.com>

rarira · 2021-08-31T04:04:24Z

@rarira When useAuthContext hook is mocked, AuthContextProvider is not necessary here (thus you don't need to export it either). You can check how useNavigation hook is mocked here:
https://github.com/dooboolab/hackatalk/blob/18eef96705434ad25b9ae72aeef463f83f9ded2b/client/src/components/pages/__tests__/ChangePw.test.tsx

@0916dhkim
I find that createTestElement function in the testUtils.tsx already mocks context and provide user through wrapping testElement with <AuthProvider initialAuthUser={mockContext?.user}> .
So I think mocking useAuthContext here in test code is unnecessary. I just provide test user as mockContext.user as below and the test passed without a issue.

 it('renders [UserListItem]', async () => {
    const user: AuthProviderMeQueryResponse['me'] = {
      id: 'user-1',
      verified: true,
      email: 'test@email.com',
      profile: null,
    };

    const component = createTestElement(<ChannelCreate />, {
      environment: mockEnvironment,
     /// here I provide test user
      user,
    });
    const screen = render(component);

    const nickname = await screen.findByText('jdoe1234');

    const userListItemWrapper = await screen.getByTestId(
      'userListItem-wrapper',
    );

    expect(nickname).toBeTruthy();
    expect(userListItemWrapper.props.isMyself).toBeTruthy();
  });

0916dhkim

@rarira Thanks for the followup! I think we are almost there. Just one more iteration and we'll be good to merge 👍

0916dhkim · 2021-08-31T12:57:29Z

 };

-const ProfileModalContext = createContext<ProfileModalContext>({
+export const ProfileModalContext = createContext<ProfileModalContext>({


So I think mocking useAuthContext here in test code is unnecessary.

@rarira Thank you for addressing AuthProvider related comments 👍
The point was avoid exporting AuthContextProvider. Can you take similar approach for ProfileModalContext and remove export here?

0916dhkim · 2021-08-31T13:03:53Z

-      const screen = render(component);
-      const btn = await screen.findByTestId('list-item-0');
+    const screen = render(component);
+    const channelItemBtn = await screen.findByTestId('list-item-0');


I think adding accessibility label on any icon-only button is a good idea. It helps people who use screen reader.

0916dhkim · 2021-08-31T13:22:16Z

+class FilteredMemberships {
+  constructor(
+    private memberships: Maybe<Membership[]> | undefined,
+    private user: AuthProviderMeQueryResponse['me'] | null,
+    private channelType: string,
+  ) {
+    this.memberships = memberships;
+    this.channelType = channelType;
+    this.user = user;
+  }
+
+  private get filtered(): (Maybe<Membership> | undefined)[] | undefined {
+    return this.memberships?.filter((member) => {
+      if (this.channelType !== 'self')
+        return member?.user?.id !== this.user?.id;
+
+      return true;
+    });
+  }
+
+  get users(): (Maybe<User> | undefined)[] | undefined {
+    return this.filtered?.map((membership) => membership?.user);
+  }
+
+  get userNames(): string[] | undefined {
+    return this.users?.map((v) => v?.nickname || v?.name || '');
+  }
+  calculatedPhotoUrls(): (Maybe<string> | undefined)[] | undefined {
+    return this.filtered?.map(
+      (membership) => membership?.user?.thumbURL || membership?.user?.photoURL,
+    );
+  }
+
+  calculateOnlineStatus(): Maybe<Boolean> | undefined {
+    return this.filtered?.[0]?.user?.isOnline;
+  }
+
+  calculateDisplayName(): Maybe<string> | undefined {
+    return this.users?.length === 1
+      ? this.users?.[0]?.nickname ||
+          this.users?.[0]?.name ||
+          getString('NO_NAME')
+      : this.userNames?.join(', ');
+  }
+}


I think functions are simpler than a class. Something like this:

const calculateUsers = (memberships: Membership[], channelType: string, authUserId): (Maybe<User>)[] => memberships.filter((member) => channelType !== 'self' && member?.user?.id !== authUserId); const calculatePhotoUrls = (memberships: Membership[]): (Maybe<string>)[] => memberships.map((member) => member?.user?.thumbURL || member?.user?.photoURL); const calculateOnlineStatus = (users: (Maybe<User>)[]): boolean => users[0]?.isOnline ?? false;

Indeed, calculatePhotoUrls also needs 'filtered by channel type' memberships passed as an argument, So there needs to be filteredMemberships calculated first and the calculated filteredMemberships is only used for this functions/class method.

if someone pass normal memberships array to calculatePhotoUrls function which is okay in tsc check but we can have unwanted results So I think the reusability of that function is very low.

0916dhkim · 2021-08-31T13:25:20Z

+  });
+
+  it('renders [StyledMeCircleView] when channelType is "self"', () => {
+    TEST_CHANNEL.channelType = 'self';


I think you forgot to copy this TEST_CHANNEL

0916dhkim · 2021-08-31T13:27:50Z

+      if (includeMe) {
+        const myProfile = await prisma.user.findUnique({
+          where: {
+            id: userId,
+          },
+        });


but it had some problem with sorting, I mean, I wanted to display 'ME' always at the top

@rarira It makes sense. Let's leave it like this for now.

rarira · 2021-08-31T14:28:47Z

@0916dhkim

#474 (comment)

I don't know why I can't reply this review comment..

I'm afraid I can't take similar approach to ProfileModalProvider as AuthProvider
Because createTestElement in testUtils return

 <DeviceProvider>
      <ThemeProvider
        initialThemeType={mockContext?.themeType ?? 'dark'}
        customTheme={{light, dark}}>
        <ResettableRelayProvider
          environment={mockContext?.environment ?? createMockEnvironment()}
          createRelayEnvironment={createMockEnvironment}>
          <Suspense fallback={<Text>TEST FALLBACK</Text>}>
            <AuthProvider initialAuthUser={mockContext?.user}>
              <ProfileModalProvider>
                <TestSafeAreaProvider>{child}</TestSafeAreaProvider>
              </ProfileModalProvider>
            </AuthProvider>
          </Suspense>
        </ResettableRelayProvider>
      </ThemeProvider>
    </DeviceProvider>

AuthProvider has prop which can be used to pass mockContext?.user but ProfileModalProvider doesn't have such a prop. Of course I can add new prop like initialModalState to ProfileModalProvider. But I don't know which one is better to add export or new prop only for testing.

0916dhkim

@rarira Thank you for quick followup!

But I don't know which one is better to add export or new prop only for testing.

I suggest adding new prop for testing. Both approaches break encapsulation for testing as you mentioned, but it is less likely to make mistake with the prop than with exports. If you are concerned, we can name the prop something like testModalState (as opposed to initialModalState) so other developers do not get confused about the purpose of that prop.

0916dhkim · 2021-08-31T16:59:54Z

+const calculatePhotoUrls = (
+  memberships: (Maybe<Membership> | undefined)[] | undefined,
+): (Maybe<string> | undefined)[] | undefined =>
+  memberships?.map(
+    (membership) => membership?.user?.thumbURL || membership?.user?.photoURL,
+  );
+
+const calculateOnlineStatus = (
+  memberships: (Maybe<Membership> | undefined)[] | undefined,
+): Maybe<Boolean> | undefined => memberships?.[0]?.user?.isOnline;


calculatePhotoUrls & calculateOnlineStatus can take users parameter like calculateDisplayNames function does. You can then combine calculateFilteredMemberships & calculateUsers functions into one.

I finally find jest.spyOn example Message.test.tsx and update MainFriend.test.tsx
and update those calculate functions as suggested.. now it looks simple :)

hyochan

@rarira Superb work on this~~!! I've just made some comments here that can be enhanced!

hyochan · 2021-09-01T08:52:04Z

+          {modalState.isMyself ? (
+            <TouchableOpacity
+              testID="profile-update-button"
+              style={{padding: 8}}


hyochan

@rarira You are a rock star ⭐

* Fix rendering issue in [UserListItem] * Hotfix - statusMessage rendering throw error on empty string * Remove the borderColor in profile `viewBtn`

hyochan added the 🎯 feature New feature label Aug 27, 2021

hyochan requested review from 0916dhkim and JongtaekChoi August 27, 2021 10:47

JongtaekChoi approved these changes Aug 27, 2021

View reviewed changes

chloe-codes1 reviewed Aug 27, 2021

View reviewed changes

Comment thread client/src/components/pages/__tests__/MainChannel.test.tsx Outdated

0916dhkim approved these changes Aug 29, 2021

View reviewed changes

rarira added 20 commits August 31, 2021 12:09

Update friends query to return users including me conditionally

7dbbbcb

Update UI for my UserListItem

5ee7dff

Update UserUpdateMutation to automatically update local cache

c7272eb

Add isMyself to ProfileModal and conditional render

6ee942f

Change ProfileModal buttons when isMyself: true

88e55f7

Fix error StatusMessageView

dec8255

Add ChannelType.self to Schema

7b22310

fix all the things

75dbe1c

Fix Server able to deal with Self Channel

a707279

Deal with the ChannelListItem

59620f2

Change CreateChannel with no selectedUsers impossible

193e220

hyochan#459

Enable ChannelCreate with myself included

33f2edc

Update findOrCreatePrivateChannel mutationh

d30bea2

Fix eslint errors

8644ba1

Add interaction test code for MainFriend

9217bca

Add interaction test code for MainChannel

c529acc

Add UserListItem render test for ChannelCreate

b326550

Cover ChannelListItem test codes 100%

1b5ff83

Add ProfileModal test codes when modalState.isMyself true

2bf59eb

Update RelayGenerated files

6832c4b

rarira and others added 13 commits August 31, 2021 12:09

Update client/src/components/pages/MainFriend.tsx

301566b

Co-authored-by: Donghyeon Kim <0916dhkim@gmail.com>

Delete unnecessary function literal

22a2c85

Update ChannelCreate.test.txt using useAuthContext mocking

165cffe

Revert "Change service function name"

1f22f24

This reverts commit 3a7f471.

Change all excludeMe to false

adc6df9

It should be changed update store properly

Refactor filteredMemberships related codes using Class

3d91761

Update client/src/components/uis/UserListItem.tsx

01b637f

Co-authored-by: Donghyeon Kim <0916dhkim@gmail.com>

Change all ternary operators which return null to &&

abef26c

Update test codes MainChannel.test.tsx

cd29312

Update client/src/components/navigations/__tests__/ProfileModal.test.tsx

97e2682

Co-authored-by: Donghyeon Kim <0916dhkim@gmail.com>

Update tests codes for better readability

0319224

Fix eslint/prettier error

0ad1ca3

Update ChangeCreate.test.tsx

c94fcf2

rarira force-pushed the feature_self_chat_new branch from 7055403 to c94fcf2 Compare August 31, 2021 03:58

hyochan requested a review from 0916dhkim August 31, 2021 04:16

hyochan linked an issue Aug 31, 2021 that may be closed by this pull request

Create empty channel (Self Chat?) #459

Closed

0916dhkim reviewed Aug 31, 2021

View reviewed changes

rarira added 2 commits August 31, 2021 23:38

Update as suggested

44b654e

Refactor Class to functions

f8bd1c0

0916dhkim reviewed Aug 31, 2021

View reviewed changes

Update test codes and calculate functions

1a73ef9

hyochan suggested changes Sep 1, 2021

View reviewed changes

Update codes as suggested

28081a8

hyochan approved these changes Sep 1, 2021

View reviewed changes

hyochan merged commit 5bdebc2 into hyochan:master Sep 1, 2021

hyochan mentioned this pull request Sep 1, 2021

Hotfix minor error in #474 #480

Merged

5 tasks

hyochan added a commit that referenced this pull request Sep 1, 2021

Fix some rendering issues in #474

51b3f9f

* Fix rendering issue in [UserListItem] * Hotfix - statusMessage rendering throw error on empty string * Remove the borderColor in profile `viewBtn`

rarira deleted the feature_self_chat_new branch September 1, 2021 13:03

		if (channelType !== ChannelType.self)
		await createMemberships(channel.id, filteredPeerUserIds);

Uh oh!

Conversation

rarira commented Aug 26, 2021

Specify project

Description

Related Issues

Tests

Checklist

Uh oh!

codecov Bot commented Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JongtaekChoi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

0916dhkim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rarira commented Aug 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0916dhkim commented Aug 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rarira commented Aug 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rarira commented Aug 31, 2021

Uh oh!

0916dhkim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rarira Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rarira commented Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0916dhkim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hyochan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov Bot commented Aug 26, 2021 •

edited

Loading

rarira commented Aug 29, 2021 •

edited

Loading

0916dhkim commented Aug 30, 2021 •

edited

Loading

rarira commented Aug 30, 2021 •

edited

Loading

rarira Aug 31, 2021 •

edited

Loading

rarira commented Aug 31, 2021 •

edited

Loading