From d17e94eae3cfb94de86c4449057d7e8082361f27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jes=C3=BAs=20Espino?= Date: Mon, 11 May 2020 18:19:38 +0200 Subject: [PATCH] Using new getKnownUsers api to cleanup the users on leave channel (#4220) --- app/actions/websocket.test.js | 94 +++++++++---------- app/actions/websocket.ts | 59 ++++++++---- app/mm-redux/actions/users.ts | 4 +- app/mm-redux/client/client4.ts | 9 ++ app/mm-redux/reducers/entities/channels.ts | 3 +- .../selectors/entities/channels.test.js | 50 ---------- app/mm-redux/selectors/entities/channels.ts | 14 --- app/screens/more_dms/index.js | 6 +- app/screens/more_dms/more_dms.js | 23 ++++- 9 files changed, 127 insertions(+), 135 deletions(-) diff --git a/app/actions/websocket.test.js b/app/actions/websocket.test.js index 01ee420a26..ac1ed429ee 100644 --- a/app/actions/websocket.test.js +++ b/app/actions/websocket.test.js @@ -38,6 +38,12 @@ const mockChanelsRequest = (teamId, channels = []) => { reply(200, channels); }; +const mockGetKnownUsersRequest = (userIds = []) => { + nock(Client4.getBaseRoute()). + get('/users/known'). + reply(200, userIds); +}; + const mockRolesRequest = (rolesToLoad = []) => { nock(Client4.getRolesRoute()). post('/names', JSON.stringify(rolesToLoad)). @@ -901,9 +907,6 @@ describe('Actions.Websocket doReconnect', () => { describe('Actions.Websocket notVisibleUsersActions', () => { configureMockStore([thunk]); - const channel1 = TestHelper.fakeChannelWithId(''); - const channel2 = TestHelper.fakeChannelWithId(''); - const me = TestHelper.fakeUserWithId(); const user = TestHelper.fakeUserWithId(); const user2 = TestHelper.fakeUserWithId(); @@ -911,29 +914,17 @@ describe('Actions.Websocket notVisibleUsersActions', () => { const user4 = TestHelper.fakeUserWithId(); const user5 = TestHelper.fakeUserWithId(); - it('should do nothing if the known users and the profiles list are the same', () => { - const membersInChannel = { - [channel1.id]: { - [user.id]: {channel_id: channel1.id, user_id: user.id}, - [user2.id]: {channel_id: channel1.id, user_id: user2.id}, - }, - [channel2.id]: { - [user3.id]: {channel_id: channel2.id, user_id: user3.id}, - }, - }; - + it('should do nothing if the known users and the profiles list are the same', async () => { const profiles = { [me.id]: me, [user.id]: user, [user2.id]: user2, [user3.id]: user3, }; + Client4.serverVersion = '5.23.0'; const state = { entities: { - channels: { - membersInChannel, - }, users: { currentUserId: me.id, profiles, @@ -941,31 +932,21 @@ describe('Actions.Websocket notVisibleUsersActions', () => { }, }; - const actions = Actions.notVisibleUsersActions(state); + mockGetKnownUsersRequest([user.id, user2.id, user3.id]); + + const actions = await Actions.notVisibleUsersActions(state); expect(actions.length).toEqual(0); }); - it('should do nothing if there are known users in my memberships but not in the profiles list', () => { - const membersInChannel = { - [channel1.id]: { - [user.id]: {channel_id: channel1.id, user_id: user.id}, - [user2.id]: {channel_id: channel1.id, user_id: user2.id}, - }, - [channel2.id]: { - [user3.id]: {channel_id: channel2.id, user_id: user3.id}, - }, - }; - + it('should do nothing if there are known users in my memberships but not in the profiles list', async () => { const profiles = { [me.id]: me, [user3.id]: user3, }; + Client4.serverVersion = '5.23.0'; const state = { entities: { - channels: { - membersInChannel, - }, users: { currentUserId: me.id, profiles, @@ -973,20 +954,13 @@ describe('Actions.Websocket notVisibleUsersActions', () => { }, }; - const actions = Actions.notVisibleUsersActions(state); + mockGetKnownUsersRequest([user.id, user2.id, user3.id]); + + const actions = await Actions.notVisibleUsersActions(state); expect(actions.length).toEqual(0); }); it('should remove the users if there are unknown users in the profiles list', async () => { - const membersInChannel = { - [channel1.id]: { - [user.id]: {channel_id: channel1.id, user_id: user.id}, - }, - [channel2.id]: { - [user3.id]: {channel_id: channel2.id, user_id: user3.id}, - }, - }; - const profiles = { [me.id]: me, [user.id]: user, @@ -995,12 +969,10 @@ describe('Actions.Websocket notVisibleUsersActions', () => { [user4.id]: user4, [user5.id]: user5, }; + Client4.serverVersion = '5.23.0'; const state = { entities: { - channels: { - membersInChannel, - }, users: { currentUserId: me.id, profiles, @@ -1008,15 +980,43 @@ describe('Actions.Websocket notVisibleUsersActions', () => { }, }; + mockGetKnownUsersRequest([user.id, user3.id]); + const expectedAction = [ {type: UserTypes.PROFILE_NO_LONGER_VISIBLE, data: {user_id: user2.id}}, {type: UserTypes.PROFILE_NO_LONGER_VISIBLE, data: {user_id: user4.id}}, {type: UserTypes.PROFILE_NO_LONGER_VISIBLE, data: {user_id: user5.id}}, ]; - const actions = Actions.notVisibleUsersActions(state); + const actions = await Actions.notVisibleUsersActions(state); expect(actions.length).toEqual(3); expect(actions).toEqual(expectedAction); }); + + it('should do nothing if the server version is less than 5.23', async () => { + const profiles = { + [me.id]: me, + [user.id]: user, + [user2.id]: user2, + [user3.id]: user3, + [user4.id]: user4, + [user5.id]: user5, + }; + Client4.serverVersion = '5.22.0'; + + const state = { + entities: { + users: { + currentUserId: me.id, + profiles, + }, + }, + }; + + mockGetKnownUsersRequest([user.id, user3.id]); + + const actions = await Actions.notVisibleUsersActions(state); + expect(actions.length).toEqual(0); + }); }); describe('Actions.Websocket handleUserTypingEvent', () => { @@ -1089,4 +1089,4 @@ describe('Actions.Websocket handleUserTypingEvent', () => { const actionTypes = testStore.getActions().map((action) => action.type); expect(actionTypes).toEqual(expectedActionsTypes); }); -}); \ No newline at end of file +}); diff --git a/app/actions/websocket.ts b/app/actions/websocket.ts index fdfe466520..3e437fede0 100644 --- a/app/actions/websocket.ts +++ b/app/actions/websocket.ts @@ -16,7 +16,6 @@ import { getCurrentChannelStats, getChannelMembersInChannels, isManuallyUnread, - getKnownUsers, } from '@mm-redux/selectors/entities/channels'; import {getConfig} from '@mm-redux/selectors/entities/general'; import {getAllPosts, getPost as selectPost} from '@mm-redux/selectors/entities/posts'; @@ -518,7 +517,7 @@ function handlePostUnread(msg: WebSocketMessage) { } function handleLeaveTeamEvent(msg: Partial) { - return (dispatch: DispatchFunc, getState: GetStateFunc) => { + return async (dispatch: DispatchFunc, getState: GetStateFunc) => { const state = getState(); const teams = getTeamsSelector(state); const currentTeamId = getCurrentTeamId(state); @@ -527,7 +526,7 @@ function handleLeaveTeamEvent(msg: Partial) { if (currentUser.id === msg.data.user_id) { const actions: Array = [{type: TeamTypes.LEAVE_TEAM, data: teams[msg.data.team_id]}]; if (isGuest(currentUser.roles)) { - const notVisible = notVisibleUsersActions(state); + const notVisible = await notVisibleUsersActions(state); if (notVisible.length) { actions.push(...notVisible); } @@ -641,17 +640,31 @@ function handleUserRemovedEvent(msg: WebSocketMessage) { const currentChannelId = getCurrentChannelId(state); const currentTeamId = getCurrentTeamId(state); const currentUser = getCurrentUser(state); - const actions: Array = [{ - type: ChannelTypes.CHANNEL_MEMBER_REMOVED, - data: { - channel_id: msg.broadcast.channel_id, - user_id: msg.data.user_id, - }, - }]; + const actions: Array = []; + let channelId; + let userId; + + if (msg.data.user_id) { + userId = msg.data.user_id; + channelId = msg.broadcast.channel_id; + } else if (msg.broadcast.user_id) { + channelId = msg.data.channel_id; + userId = msg.broadcast.user_id; + } + + if (userId) { + actions.push({ + type: ChannelTypes.CHANNEL_MEMBER_REMOVED, + data: { + channel_id: channelId, + user_id: userId, + }, + }); + } const channel = channels[currentChannelId]; - if (msg.data.user_id !== currentUser.id) { + if (msg.data?.user_id !== currentUser.id) { const members = getChannelMembersInChannels(state); const isMember = Object.values(members).some((member) => member[msg.data.user_id]); if (channel && isGuest(currentUser.roles) && !isMember) { @@ -665,6 +678,7 @@ function handleUserRemovedEvent(msg: WebSocketMessage) { } } + let redirectToDefaultChannel = false; if (msg.broadcast.user_id === currentUser.id && currentTeamId) { const {data: myData}: any = await dispatch(loadChannelsForTeam(currentTeamId, true)); @@ -689,10 +703,10 @@ function handleUserRemovedEvent(msg: WebSocketMessage) { if (msg.data.channel_id === currentChannelId) { // emit the event so the client can change his own state - EventEmitter.emit(General.SWITCH_TO_DEFAULT_CHANNEL, currentTeamId); + redirectToDefaultChannel = true; } if (isGuest(currentUser.roles)) { - const notVisible = notVisibleUsersActions(state); + const notVisible = await notVisibleUsersActions(state); if (notVisible.length) { actions.push(...notVisible); } @@ -706,6 +720,9 @@ function handleUserRemovedEvent(msg: WebSocketMessage) { } dispatch(batchActions(actions, 'BATCH_WS_USER_REMOVED')); + if (redirectToDefaultChannel) { + EventEmitter.emit(General.SWITCH_TO_DEFAULT_CHANNEL, currentTeamId); + } } catch { // do nothing } @@ -1126,8 +1143,18 @@ function handleOpenDialogEvent(msg: WebSocketMessage) { } // Helpers -export function notVisibleUsersActions(state: GlobalState): Array { - const knownUsers = getKnownUsers(state); +export async function notVisibleUsersActions(state: GlobalState): Promise> { + if (!isMinimumServerVersion(Client4.getServerVersion(), 5, 23)) { + return []; + } + let knownUsers: Set; + try { + const fetchResult = await Client4.getKnownUsers(); + knownUsers = new Set(fetchResult); + } catch (err) { + return []; + } + knownUsers.add(getCurrentUserId(state)); const allUsers = Object.keys(getUsers(state)); const usersToRemove = new Set(allUsers.filter((x) => !knownUsers.has(x))); @@ -1151,4 +1178,4 @@ export function userTyping(state: GlobalState, channelId: string, parentPostId: websocketClient.userTyping(channelId, parentPostId); lastTimeTypingSent = t; } -} \ No newline at end of file +} diff --git a/app/mm-redux/actions/users.ts b/app/mm-redux/actions/users.ts index 260105dc47..46317f2d4c 100644 --- a/app/mm-redux/actions/users.ts +++ b/app/mm-redux/actions/users.ts @@ -23,7 +23,7 @@ import {bindClientFunc, forceLogoutIfNecessary, debounce} from './helpers'; import {getMyPreferences, makeDirectChannelVisibleIfNecessary, makeGroupMessageVisibleIfNecessary} from './preferences'; import {Dictionary} from '@mm-redux/types/utilities'; export function checkMfa(loginId: string): ActionFunc { - return async (dispatch: DispatchFunc, getState: GetStateFunc) => { + return async (dispatch: DispatchFunc) => { dispatch({type: UserTypes.CHECK_MFA_REQUEST, data: null}); try { const data = await Client4.checkUserMfa(loginId); @@ -985,7 +985,7 @@ export function stopPeriodicStatusUpdates(): ActionFunc { } export function updateMe(user: UserProfile): ActionFunc { - return async (dispatch: DispatchFunc, getState: GetStateFunc) => { + return async (dispatch: DispatchFunc) => { dispatch({type: UserTypes.UPDATE_ME_REQUEST, data: null}); let data; diff --git a/app/mm-redux/client/client4.ts b/app/mm-redux/client/client4.ts index 926c9894b9..a353d76332 100644 --- a/app/mm-redux/client/client4.ts +++ b/app/mm-redux/client/client4.ts @@ -448,6 +448,15 @@ export default class Client4 { ); } + getKnownUsers = async () => { + this.trackEvent('api', 'api_get_known_users'); + + return this.doFetch( + `${this.getUsersRoute()}/known`, + {method: 'get'}, + ); + } + sendPasswordResetEmail = async (email: string) => { this.trackEvent('api', 'api_users_send_password_reset'); diff --git a/app/mm-redux/reducers/entities/channels.ts b/app/mm-redux/reducers/entities/channels.ts index 2c17cefd76..77ff469099 100644 --- a/app/mm-redux/reducers/entities/channels.ts +++ b/app/mm-redux/reducers/entities/channels.ts @@ -11,6 +11,7 @@ import {Team} from '@mm-redux/types/teams'; function removeMemberFromChannels(state: RelationOneToOne>, action: GenericAction) { const nextState = {...state}; Object.keys(state).forEach((channel) => { + nextState[channel] = {...nextState[channel]}; delete nextState[channel][action.data.user_id]; }); return nextState; @@ -371,7 +372,7 @@ function myMembers(state: RelationOneToOne = {}, act if (sync) { current.forEach((member: ChannelMembership) => { const id = member.channel_id; - if (channelMembers.find((cm: ChannelMembership) => cm.channel_id === id)) { + if (channelMembers.find((cm: ChannelMembership) => cm.channel_id !== id)) { delete nextState[id]; hasNewValues = true; } diff --git a/app/mm-redux/selectors/entities/channels.test.js b/app/mm-redux/selectors/entities/channels.test.js index 3fbf353185..be2b474eda 100644 --- a/app/mm-redux/selectors/entities/channels.test.js +++ b/app/mm-redux/selectors/entities/channels.test.js @@ -135,56 +135,6 @@ describe('Selectors.Channels.getChannelsInCurrentTeam', () => { }); }); -describe('Selectors.Channels.getKnownUsers', () => { - const channel1 = TestHelper.fakeChannelWithId(''); - const channel2 = TestHelper.fakeChannelWithId(''); - - const me = TestHelper.fakeUserWithId(); - const user = TestHelper.fakeUserWithId(); - const user2 = TestHelper.fakeUserWithId(); - const user3 = TestHelper.fakeUserWithId(); - - const membersInChannel = { - [channel1.id]: { - [user.id]: {channel_id: channel1.id, user_id: user.id}, - [user2.id]: {channel_id: channel1.id, user_id: user2.id}, - }, - [channel2.id]: { - [user3.id]: {channel_id: channel2.id, user_id: user3.id}, - }, - }; - - it('should return all members of all my channels', () => { - const testState = deepFreezeAndThrowOnMutation({ - entities: { - users: { - currentUserId: me.id, - }, - channels: { - membersInChannel, - }, - }, - }); - - assert.deepEqual(Selectors.getKnownUsers(testState), new Set([me.id, user.id, user2.id, user3.id])); - }); - - it('should return only me if I have no channels', () => { - const testState = deepFreezeAndThrowOnMutation({ - entities: { - users: { - currentUserId: me.id, - }, - channels: { - membersInChannel: {}, - }, - }, - }); - - assert.deepEqual(Selectors.getKnownUsers(testState), new Set([me.id])); - }); -}); - describe('Selectors.Channels.getMyChannels', () => { const team1 = TestHelper.fakeTeamWithId(); const team2 = TestHelper.fakeTeamWithId(); diff --git a/app/mm-redux/selectors/entities/channels.ts b/app/mm-redux/selectors/entities/channels.ts index e9270fe144..6fd35b4f8c 100644 --- a/app/mm-redux/selectors/entities/channels.ts +++ b/app/mm-redux/selectors/entities/channels.ts @@ -44,20 +44,6 @@ export function getChannelMembersInChannels(state: GlobalState): RelationOneToOn return state.entities.channels.membersInChannel; } -export const getKnownUsers: (a: GlobalState) => Set = createSelector( - getChannelMembersInChannels, - getCurrentUserId, - (channelsMemberships: RelationOneToOne>, currentUserId: string): Set => { - const knownUsers: Set = new Set([currentUserId]); - for (const membersInChannel of Object.values(channelsMemberships)) { - for (const member of Object.values(membersInChannel)) { - knownUsers.add(member.user_id); - } - } - return knownUsers; - } -); - function sortChannelsByRecencyOrAlpha(locale: string, lastPosts: RelationOneToOne, sorting: SortingType, a: Channel, b: Channel) { if (sorting === 'recent') { return sortChannelsByRecency(lastPosts, a, b); diff --git a/app/screens/more_dms/index.js b/app/screens/more_dms/index.js index 2a2ca02f65..272626d51b 100644 --- a/app/screens/more_dms/index.js +++ b/app/screens/more_dms/index.js @@ -12,13 +12,16 @@ import {General} from '@mm-redux/constants'; import {getConfig} from '@mm-redux/selectors/entities/general'; import {getTeammateNameDisplaySetting, getTheme} from '@mm-redux/selectors/entities/preferences'; import {getCurrentTeamId} from '@mm-redux/selectors/entities/teams'; -import {getCurrentUserId, getUsers} from '@mm-redux/selectors/entities/users'; +import {getCurrentUserId, getUsers, getCurrentUser} from '@mm-redux/selectors/entities/users'; + +import {isGuest} from '@utils/users'; import MoreDirectMessages from './more_dms'; function mapStateToProps(state) { const config = getConfig(state); const restrictDirectMessage = config.RestrictDirectMessage === General.RESTRICT_DIRECT_MESSAGE_ANY; + const currentUser = getCurrentUser(state); return { restrictDirectMessage, @@ -27,6 +30,7 @@ function mapStateToProps(state) { theme: getTheme(state), currentDisplayName: state.views.channel.displayName, currentUserId: getCurrentUserId(state), + isGuest: isGuest(currentUser), currentTeamId: getCurrentTeamId(state), isLandscape: isLandscape(state), }; diff --git a/app/screens/more_dms/more_dms.js b/app/screens/more_dms/more_dms.js index 12379813d2..c96188becc 100644 --- a/app/screens/more_dms/more_dms.js +++ b/app/screens/more_dms/more_dms.js @@ -52,6 +52,7 @@ export default class MoreDirectMessages extends PureComponent { currentDisplayName: PropTypes.string, currentTeamId: PropTypes.string.isRequired, currentUserId: PropTypes.string.isRequired, + isGuest: PropTypes.object.isRequired, restrictDirectMessage: PropTypes.bool.isRequired, teammateNameDisplay: PropTypes.string, theme: PropTypes.object.isRequired, @@ -358,6 +359,8 @@ export default class MoreDirectMessages extends PureComponent { ); }; + filterUnknownUsers = (u) => Boolean(this.props.allProfiles[u.id]) + renderLoading = () => { const {theme} = this.props; const {loading} = this.state; @@ -399,7 +402,7 @@ export default class MoreDirectMessages extends PureComponent { render() { const {formatMessage} = this.context.intl; - const {currentUserId, theme, isLandscape} = this.props; + const {isGuest, currentUserId, theme, isLandscape} = this.props; const { loading, profiles, @@ -435,7 +438,7 @@ export default class MoreDirectMessages extends PureComponent { let listType; if (term) { const exactMatches = []; - const results = filterProfilesMatchingTerm(searchResults, term).filter((p) => { + const filterByTerm = (p) => { if (selectedCount > 0 && p.id === currentUserId) { return false; } @@ -446,11 +449,23 @@ export default class MoreDirectMessages extends PureComponent { } return true; - }); + }; + + let results; + if (isGuest) { + results = filterProfilesMatchingTerm(searchResults, term).filter((u) => filterByTerm(u) && this.filterUnknownUsers(u)); + } else { + results = filterProfilesMatchingTerm(searchResults, term).filter(filterByTerm); + } data = [...exactMatches, ...results]; + listType = FLATLIST; } else { - data = createProfilesSections(profiles); + if (isGuest) { + data = createProfilesSections(profiles.filter(this.filterUnknownUsers)); + } else { + data = createProfilesSections(profiles); + } listType = SECTIONLIST; }