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

Follow Method and database updates #2

Open
wants to merge 13 commits into
base: newsfeed
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
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
Empty file added app/follow/README.md
Empty file.
Empty file added app/follow/client/index.js
Empty file.
Empty file added app/follow/index.js
Empty file.
Empty file added app/follow/server/index.js
Empty file.
58 changes: 31 additions & 27 deletions app/lib/lib/roomTypes/newsfeed.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import { Meteor } from 'meteor/meteor';

import { openRoom } from '../../../ui-utils';
import { ChatRoom, ChatSubscription } from '../../../models';
import { openRoom } from '../../../ui-utils';
import { settings } from '../../../settings';
import { hasAtLeastOnePermission } from '../../../authorization';
import { getUserPreference, RoomTypeConfig, RoomTypeRouteConfig, RoomSettingsEnum, UiTextContext } from '../../../utils';
import { hasAtLeastOnePermission, hasPermission } from '../../../authorization';
import { getUserPreference, RoomSettingsEnum, RoomTypeConfig, RoomTypeRouteConfig, UiTextContext, roomTypes } from '../../../utils';
import { getRoomAvatarURL } from '../../../utils/lib/getRoomAvatarURL';
import { getAvatarURL } from '../../../utils/lib/getAvatarURL';

export class NewsfeedRoomRoute extends RoomTypeRouteConfig {
Expand Down Expand Up @@ -43,6 +44,7 @@ export class NewsfeedRoomType extends RoomTypeConfig {
t: 'n',
name: identifier,
};

return ChatRoom.findOne(query);
}

Expand All @@ -53,68 +55,58 @@ export class NewsfeedRoomType extends RoomTypeConfig {
if (settings.get('UI_Allow_room_names_with_special_chars')) {
return roomData.fname || roomData.name;
}

return roomData.name;
}

condition() {
const groupByType = getUserPreference(Meteor.userId(), 'sidebarGroupByType');
return groupByType && (hasAtLeastOnePermission(['view-c-room', 'view-joined-room']) || settings.get('Accounts_AllowAnonymousRead') === true);
}

showJoinLink(roomId) {
return !!ChatRoom.findOne({ _id: roomId, t: 'n' });
}

includeInRoomSearch() {
return true;
return groupByType && hasPermission('view-p-room');
}

isGroupChat() {
return true;
}

canAddUser(room) {
return hasAtLeastOnePermission(['add-user-to-any-c-room', 'add-user-to-joined-room'], room._id);
return hasAtLeastOnePermission(['add-user-to-any-p-room', 'add-user-to-joined-room'], room._id);
}

canSendMessage(roomId) {
const room = ChatRoom.findOne({ _id: roomId, t: 'n' }, { fields: { prid: 1 } });
if (room.prid) {
return true;
}

// TODO: remove duplicated code
return ChatSubscription.find({
rid: roomId,
}).count() > 0;
}

enableMembersListProfile() {
return true;
}

allowRoomSettingChange(room, setting) {
switch (setting) {
case RoomSettingsEnum.JOIN_CODE:
return false;
case RoomSettingsEnum.BROADCAST:
return room.broadcast;
case RoomSettingsEnum.READ_ONLY:
return !room.broadcast;
case RoomSettingsEnum.REACT_WHEN_READ_ONLY:
return !room.broadcast && room.ro;
case RoomSettingsEnum.E2E:
return false;
case RoomSettingsEnum.SYSTEM_MESSAGES:
case RoomSettingsEnum.E2E:
return settings.get('E2E_Enable') === true;
default:
return true;
}
}

enableMembersListProfile() {
return true;
}

getUiText(context) {
switch (context) {
case UiTextContext.HIDE_WARNING:
return 'Hide_Room_Warning';
return 'Hide_Group_Warning';
case UiTextContext.LEAVE_WARNING:
return 'Leave_Room_Warning';
return 'Leave_Group_Warning';
default:
return '';
}
Expand All @@ -123,6 +115,18 @@ export class NewsfeedRoomType extends RoomTypeConfig {
getAvatarPath(roomData) {
// TODO: change to always get avatar from _id when rooms have avatars

return getAvatarURL({ username: `@${ this.roomName(roomData) }` });
// if room is not a discussion, returns the avatar for its name
if (!roomData.prid) {
return getAvatarURL({ username: `@${ this.roomName(roomData) }` });
}

// if discussion's parent room is known, get his avatar
const proom = ChatRoom.findOne({ _id: roomData.prid }, { reactive: false });
if (proom) {
return roomTypes.getConfig(proom.t).getAvatarPath(proom);
}

// otherwise gets discussion's avatar via _id
return getRoomAvatarURL(roomData.prid);
}
}
4 changes: 3 additions & 1 deletion app/newsfeed/server/deinitialize.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@
//
// import { settings } from '../../settings';
import { deleteRoom } from '../../lib/server/functions';
import { Rooms } from '../../models';
import { Rooms, Users } from '../../models';

function deinitializeNewsfeed() {
const roomList = Rooms.findByType('n').fetch();
roomList.forEach((room) => {
deleteRoom(room._id);
});
Users.update({}, { $unset: { followers: 1 } }, { multi: true });
Users.update({}, { $unset: { following: 1 } }, { multi: true });
kb0304 marked this conversation as resolved.
Show resolved Hide resolved
}

export { deinitializeNewsfeed };
45 changes: 45 additions & 0 deletions app/ui-flextab/client/tabs/userActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,19 @@ export const getActions = ({ user, directActions, hideAdminControls }) => {
}
};


const hasAlreadyFollowed = (username) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about we name it / make a generic "canFollow" method? Which can be later extended to support other use cases (if any)?

Meteor.call('hasAlreadyFollowed', username, function(error, result) {
if (error) {
console.log(error);
return false;
}
Session.set('hasFollowed', result);
return result;
});
};


const actions = [
{
icon: 'message',
Expand All @@ -111,6 +124,38 @@ export const getActions = ({ user, directActions, hideAdminControls }) => {
},
},

function() {
if (isSelf(this.username) || !directActions) {
return;
}
hasAlreadyFollowed(this.username);

if (Session.get('hasFollowed')) {
return {
icon: 'plus',
name: t('Unfollow'),
action: prevent(getUser, ({ username }) =>
Meteor.call('unfollowUser', username, success(() => toastr.success(TAPi18n.__('You_have_unfollowed__username_', { username }))))
),
condition() {
return settings.get('Newsfeed_enabled');
},
};
}


return {
icon: 'plus',
name: t('Follow'),
action: prevent(getUser, ({ username }) =>
Meteor.call('followUser', username, success(() => toastr.success(TAPi18n.__('You_have_followed__username_', { username }))))
),
condition() {
return settings.get('Newsfeed_enabled');
},
};
},

function() {
if (isSelf(this.username) || !directActions) {
return;
Expand Down
1 change: 1 addition & 0 deletions client/importPackages.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,3 +109,4 @@ import '../app/action-links';
import '../app/reactions/client';
import '../app/livechat/client';
import '../app/newsfeed/client';
import '../app/follow/client';
3 changes: 3 additions & 0 deletions packages/rocketchat-i18n/i18n/en.i18n.json
Original file line number Diff line number Diff line change
Expand Up @@ -2969,6 +2969,7 @@
"unarchive-room_description": "Permission to unarchive channels",
"Unblock_User": "Unblock User",
"Unfavorite": "Unfavorite",
"Unfollow": "Unfollow",
"Unfollow_message": "Unfollow message",
"Unignore": "Unignore",
"Uninstall": "Uninstall",
Expand Down Expand Up @@ -3231,10 +3232,12 @@
"You_can_use_webhooks_to_easily_integrate_livechat_with_your_CRM": "You can use webhooks to easily integrate livechat with your CRM.",
"You_cant_leave_a_livechat_room_Please_use_the_close_button": "You can't leave a livechat room. Please, use the close button.",
"You_have_been_muted": "You have been muted and cannot speak in this room",
"You_have_followed__username_": "You have followed __username__",
"You_have_n_codes_remaining": "You have __number__ codes remaining.",
"You_have_not_verified_your_email": "You have not verified your email.",
"You_have_successfully_unsubscribed": "You have successfully unsubscribed from our Mailling List.",
"You_have_to_set_an_API_token_first_in_order_to_use_the_integration": "You have to set an API token first in order to use the integration.",
"You_have_unfollowed__username_": "You have unfollowed __username__",
"You_must_join_to_view_messages_in_this_channel": "You must join to view messages in this channel",
"You_need_confirm_email": "You need to confirm your email to login!",
"You_need_install_an_extension_to_allow_screen_sharing": "You need install an extension to allow screen sharing",
Expand Down
1 change: 1 addition & 0 deletions server/importPackages.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,3 +117,4 @@ import '../app/action-links';
import '../app/reactions/server';
import '../app/livechat/server';
import '../app/newsfeed/server';
import '../app/follow/server';
3 changes: 3 additions & 0 deletions server/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,14 @@ import './methods/createDirectMessage';
import './methods/deleteFileMessage';
import './methods/deleteUser';
import './methods/eraseRoom';
import './methods/followUser';
import './methods/getAvatarSuggestion';
import './methods/getRoomById';
import './methods/getRoomIdByNameOrId';
import './methods/getRoomNameById';
import './methods/getTotalChannels';
import './methods/getUsersOfRoom';
import './methods/hasAlreadyFollowed';
import './methods/hideRoom';
import './methods/ignoreUser';
import './methods/loadHistory';
Expand Down Expand Up @@ -62,6 +64,7 @@ import './methods/setAvatarFromService';
import './methods/setUserActiveStatus';
import './methods/setUserPassword';
import './methods/toogleFavorite';
import './methods/unfollowUser';
import './methods/unmuteUserInRoom';
import './methods/userSetUtcOffset';
import './publications/activeUsers';
Expand Down
43 changes: 43 additions & 0 deletions server/methods/followUser.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import { Meteor } from 'meteor/meteor';

import { Users } from '../../app/models';
import { settings } from '../../app/settings';


Meteor.methods({
followUser(username) {
if (settings.get('Newsfeed_enabled')) {

Choose a reason for hiding this comment

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

Please, never put all your code inside an if, use the if first to stop the process:

if (!settings.get('Newsfeed_enabled')) {
    return false;
}

It will make the code cleaner and more understandable.

// Update followers keys
let userObject = Users.findOneByUsername(username);
if ('followers' in userObject) {
const followersObject = userObject.followers;

Choose a reason for hiding this comment

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

Here you could prevent the code duplication by deciding like:

const followersObject = userObject.followers || {};

const { _id } = Users.findOneByUsername(Meteor.user().username);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can directly use Meteor.userId()

Choose a reason for hiding this comment

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

Meteor.userId() is the best approach here, but just for your knowledge you could have used Meteor.user()._id since Meteor.user() will return the entire user's object, so on that case you are requesting the entire object twice.

And, if you want just one field from a query, please always pass that information to the query to not transfer unnecessary data from db to rocket.chat:

Users.findOneByUsername(Meteor.user().username, {fields: {_id: 1}});

followersObject[_id] = '';
Users.update(userObject._id, { $set: { followers: followersObject } });

Choose a reason for hiding this comment

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

Why not use a direct update?

Users.update(userObject._id, { $set: { `followers.${ _id }`: '' } });

} else {
const followersObject = new Object();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not validate at server startup and create this object there for every user to maintain consistency across all?

const { _id } = Users.findOneByUsername(Meteor.user().username);
followersObject[_id] = '';
Users.update(userObject._id, { $set: { followers: followersObject } });
}

// Update following keys
userObject = Users.findOneByUsername(Meteor.user().username);
if ('following' in userObject) {
const followingObject = userObject.following;
Copy link
Collaborator

Choose a reason for hiding this comment

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

As the number of Following/Followers increase, it would increase in size of each user object considerably. As the User object is perhaps the most commonly fetched object for number of other purposes in all other areas of the application, we might want to structure our schema as described here. Thoughts? @bizzbyster @dtoth
Check this

Choose a reason for hiding this comment

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

@kb0304 @fliptrail I am not an expert on the server side architecture but I tend to agree with Karan based on the link he sent. If you are still unsure we can try to get some inputs from others on the RC team. But in my opinion this seems like a good approach.

const { _id } = Users.findOneByUsername(username);
followingObject[_id] = '';
Users.update(userObject._id, { $set: { following: followingObject } });
} else {
const followingObject = new Object();
const { _id } = Users.findOneByUsername(username);
followingObject[_id] = '';
Users.update(userObject._id, { $set: { following: followingObject } });
}

return true;
}

return false;
},
});
17 changes: 17 additions & 0 deletions server/methods/hasAlreadyFollowed.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { Meteor } from 'meteor/meteor';

import { Users } from '../../app/models';
import { settings } from '../../app/settings';

Meteor.methods({
hasAlreadyFollowed(username) {
if (settings.get('Newsfeed_enabled')) {
if ('followers' in Users.findOneByUsername(username)) {
if (`${ Meteor.user()._id }` in Users.findOneByUsername(username).followers) {
return true;
}
}
}
return false;
},
});
33 changes: 33 additions & 0 deletions server/methods/unfollowUser.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import { Meteor } from 'meteor/meteor';

import { Users } from '../../app/models';
import { settings } from '../../app/settings';


Meteor.methods({
unfollowUser(username) {
if (settings.get('Newsfeed_enabled')) {
// Update followers keys
let userObject = Users.findOneByUsername(username);
if ('followers' in userObject) {
const followersObject = userObject.followers;
const { _id } = Users.findOneByUsername(Meteor.user().username);
delete followersObject[_id];
Users.update(userObject._id, { $set: { followers: followersObject } });
}

// Update following keys
userObject = Users.findOneByUsername(Meteor.user().username);
if ('following' in userObject) {
const followingObject = userObject.following;
const { _id } = Users.findOneByUsername(username);
delete followingObject[_id];
Users.update(userObject._id, { $set: { following: followingObject } });
}

return true;
}

return false;
},
});