Skip to content

[FIX] Show direct rooms as readonly when one of the users is deactivated #21684

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

Merged
sampaiodiego merged 4 commits into from
Apr 21, 2021
Merged

[FIX] Show direct rooms as readonly when one of the users is deactivated #21684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6717954
Show direct rooms as readonly when one of the users is deactivated
KevLehman Apr 20, 2021
e377901
Update app/models/server/models/Rooms.js
KevLehman Apr 20, 2021
25e6467
Merge branch 'develop' into fix/show-room-as-read-only-when-user-deac…
sampaiodiego Apr 21, 2021
9b0c95d
Remove extra validation
sampaiodiego Apr 21, 2021
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
4 changes: 3 additions & 1 deletion app/lib/server/functions/setUserActiveStatus.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { check } from 'meteor/check';
import { Accounts } from 'meteor/accounts-base';

import * as Mailer from '../../../mailer';
import { Users, Subscriptions } from '../../../models';
import { Users, Subscriptions, Rooms } from '../../../models';
import { settings } from '../../../settings';
import { relinquishRoomOwnerships } from './relinquishRoomOwnerships';
import { shouldRemoveOrChangeOwner, getSubscribedRoomsForUserWithDetails } from './getRoomsWithSingleOwner';
Expand Down Expand Up @@ -39,8 +39,10 @@ export function setUserActiveStatus(userId, active, confirmRelinquish = false) {

if (active === false) {
Users.unsetLoginTokens(userId);
Rooms.setReadOnlyByUserId(userId, true, false);
} else {
Users.unsetReason(userId);
Rooms.setReadOnlyByUserId(userId, false, false);
}
if (active && !settings.get('Accounts_Send_Email_When_Activating')) {
return true;
Expand Down
16 changes: 16 additions & 0 deletions app/models/server/models/Rooms.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,22 @@ export class Rooms extends Base {
return this.update(query, update);
}

setReadOnlyByUserId(_id, readOnly, reactWhenReadOnly) {
const query = {
uids: _id,

@sampaiodiego sampaiodiego Apr 20, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this will probably make event multiple DM rooms as read only.. which I'm not sure is the desired behavior 🤔

@KevLehman KevLehman Apr 20, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Well, it should mark all the direct conversations with the deactivated user as readonly 🤔

@sampaiodiego sampaiodiego Apr 20, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

right, is that behavior expected? like if I'm talking to 5 different people, if one of them gets deactivated, should the whole conversation be locked up? this might be already better than what we have today though =)

@sampaiodiego sampaiodiego Apr 20, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

oh, and I just realised, what if a user get re-activated? this update will make room available again even if there are other users still not active on that multi DM..

let's say there is a multi DM with users: userA, userB and userC.. if userA is deactivated, it will change that DM to read only.. then after that userB get deactivated as well, the room stays read only.. if userA is activated again, the DM should keep as read only, since userB is still not active.

@KevLehman KevLehman Apr 21, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤔 I think we can refine to the point where if a conversation is between 2 and one is locked, the whole conversation should be locked. But if it's between more people, then it shouldn't be locked away, even if there are deactivated members,

Regarding the second, yes 😬 I didn't consider multi dms for this haha I thought that was a different type of room.

t: 'd',
};

const update = {
$set: {
ro: readOnly,
reactWhenReadOnly,
},
};

return this.update(query, update, { multi: true });
}

setAllowReactingWhenReadOnlyById = function(_id, allowReacting) {
const query = {
_id,
Expand Down
1 change: 1 addition & 0 deletions packages/rocketchat-i18n/i18n/en.i18n.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3397,6 +3397,7 @@
"Room_has_been_unarchived": "Room has been unarchived",
"Room_Info": "Room Information",
"room_is_blocked": "This room is blocked",
"room_account_deactivated": "This account is deactivated",
"room_is_read_only": "This room is read only",
"room_name": "room name",
"Room_name_changed": "Room name changed to: <em>__room_name__</em> by <em>__user_by__</em>",
Expand Down