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] Incorrect display of "Reply in Direct Message" in MessageAction #17968

Merged
merged 4 commits into from
Feb 18, 2021
Merged

[FIX] Incorrect display of "Reply in Direct Message" in MessageAction #17968

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
61df42a
Fix bug in MessageAction reply to DM display
abrom Jun 19, 2020
eba34bc
Add integration tests to validate Reply in DM fix
abrom Jun 19, 2020
a264333
Merge branch 'develop' into patch-1
MartinSchoeler Jan 20, 2021
a3ff927
Update MessageAction.js
MartinSchoeler Jan 20, 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
12 changes: 10 additions & 2 deletions app/ui-utils/client/lib/MessageAction.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { Session } from 'meteor/session';
import { messageArgs } from './messageArgs';
import { roomTypes, canDeleteMessage } from '../../../utils/client';
import { Messages, Rooms, Subscriptions } from '../../../models/client';
import { hasAtLeastOnePermission } from '../../../authorization/client';
import { hasAtLeastOnePermission, hasPermission } from '../../../authorization/client';
import { modal } from './modal';

const call = (method, ...args) => new Promise((resolve, reject) => {
Expand Down Expand Up @@ -172,13 +172,21 @@ Meteor.startup(async function() {
reply: msg._id,
});
},
condition({ subscription, room }) {
condition({ subscription, room, msg, u }) {
if (subscription == null) {
return false;
}
if (room.t === 'd' || room.t === 'l') {
return false;
}

// Check if we already have a DM started with the message user (not ourselves) or we can start one
const dmRoom = Rooms.findOne({ _id: [u._id, msg.u._id].sort().join('') });
const canAccessDM = dmRoom && Subscriptions.findOne({ rid: dmRoom._id, 'u._id': u._id });
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe before doing multiple findOnes, we can check if the user lack the permission, so we can avoid calling these methods when we don't need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting something like this?

// Don't show reply option if we're messaging someone else and we don't have
// permission to start a new DM with them
if (u._id !== msg.u._id && !hasPermission('create-d')) {
  const dmRoom = Rooms.findOne({ _id: [u._id, msg.u._id].sort().join('') });
  if (!dmRoom || !Subscriptions.findOne({ rid: dmRoom._id, 'u._id': u._id })) {
    return false;
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MartinSchoeler are you suggesting something like ☝️ ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MartinSchoeler thoughts RE my suggested fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @MartinSchoeler still awaiting your feedback.

if (u._id !== msg.u._id && !canAccessDM && !hasPermission('create-d')) {
return false;
}

return true;
},
order: 0,
Expand Down
130 changes: 125 additions & 5 deletions tests/cypress/integration/06-messaging.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,13 @@ import { username, email, password } from '../../data/user.js';
import { publicChannelName, privateChannelName } from '../../data/channel.js';
import { targetUser, imgURL } from '../../data/interactions.js';
import { checkIfUserIsValid, publicChannelCreated, privateChannelCreated, directMessageCreated, setPublicChannelCreated, setPrivateChannelCreated, setDirectMessageCreated } from '../../data/checks';

import { updatePermission } from '../../data/permissions.helper';
import { api, getCredentials, credentials, request } from '../../data/api-data';
import { createUser, login } from '../../data/users.helper';

// Test data
const message = `message from ${ username }`;
let testDMUsername;

function messagingTest(currentTest) {
describe('Normal message:', () => {
Expand Down Expand Up @@ -88,7 +91,63 @@ function messagingTest(currentTest) {
});
}

function messageActionsTest(currentTest) {
function grantCreateDPermission() {
return new Promise((resolve) => {
getCredentials(() => {
updatePermission('create-d', ['user']).then(resolve);
});
});
}

function revokeCreateDPermission() {
return new Promise((resolve) => {
getCredentials(() => {
updatePermission('create-d', []).then(resolve);
});
});
}

function toggleOpenMessageActionMenu() {
mainContent.popoverWrapper.click();
mainContent.openMessageActionMenu();
}

function createDMUserAndPost(testChannel, done) {
getCredentials(() => {
createUser().then((createdUser) => {
testDMUsername = createdUser.username;

request.post(api('users.setActiveStatus'))
.set(credentials)
.send({
activeStatus: true,
userId: createdUser._id,
}).then(() => {
login(testDMUsername, password).then((userCredentials) => {
request.post(api('chat.postMessage'))
.set(userCredentials)
.send({
channel: testChannel,
text: 'Message from Test DM user',
})
.end(done);
});
});
});
});
}

function leaveTestDM() {
// Leave the existing DM
const dmElement = sideNav.getChannelFromList(testDMUsername).scrollIntoView();
dmElement.closest('.sidebar-item').find('.sidebar-item__menu').invoke('show').click();
sideNav.popOverHideOption.click();

Global.modal.should('be.visible');
Global.modalConfirm.click();
}

function messageActionsTest(currentTest, testChannel) {
describe('[Actions]', () => {
before(() => {
mainContent.sendMessage('Message for Message Actions Tests');
Expand Down Expand Up @@ -139,6 +198,66 @@ function messageActionsTest(currentTest) {
it('it should not show the mark as unread action', () => {
mainContent.messageUnread.should('not.be.visible');
});

if (currentTest === 'direct') {
it('it should not show the Reply to DM action', () => {
mainContent.messageReplyInDM.should('not.be.visible');
});
} else if (currentTest !== 'private') {
context('when the channel last message was posted by someone else', () => {
before((done) => {
revokeCreateDPermission().then(() => {
createDMUserAndPost(testChannel, done);
});
});

it('it should not show the Reply to DM action', () => {
toggleOpenMessageActionMenu();

// We don't have the test DM user in a DM channel or have the `create-d` permission
mainContent.messageReplyInDM.should('not.be.visible');
});

context('when the user has permission to create DMs', () => {
before(() => grantCreateDPermission());
after(() => revokeCreateDPermission());

it('it should show the Reply to DM action', () => {
toggleOpenMessageActionMenu();

mainContent.messageReplyInDM.should('be.visible');
});
});

context('when the user already has a created DM', () => {
// Grant Create DM permission, create a DM, then revoke the permission
before(() => grantCreateDPermission());

before(() => {
mainContent.popoverWrapper.click();
sideNav.spotlightSearchIcon.click();
sideNav.searchChannel(testDMUsername);
});

before(() => revokeCreateDPermission());

before(() => {
sideNav.openChannel(testChannel);
mainContent.openMessageActionMenu();
});

after(() => {
mainContent.popoverWrapper.click();
leaveTestDM();
mainContent.openMessageActionMenu();
});

it('it should show the Reply to DM action', () => {
mainContent.messageReplyInDM.should('be.visible');
});
});
});
}
});

describe('[Usage]', () => {
Expand Down Expand Up @@ -252,7 +371,7 @@ describe('[Message]', () => {
sideNav.searchChannel('general');
});
messagingTest('general');
messageActionsTest('general');
messageActionsTest('general', 'general');
});

describe('[Public Channel]', () => {
Expand All @@ -265,7 +384,7 @@ describe('[Message]', () => {
sideNav.openChannel(publicChannelName);
});
messagingTest('public');
messageActionsTest('public');
messageActionsTest('public', publicChannelName);
});

describe('[Private Channel]', () => {
Expand All @@ -278,7 +397,7 @@ describe('[Message]', () => {
sideNav.openChannel(privateChannelName);
});
messagingTest('private');
messageActionsTest('private');
messageActionsTest('private', privateChannelName);
});

describe('[Direct Message]', () => {
Expand All @@ -292,5 +411,6 @@ describe('[Message]', () => {
sideNav.openChannel(targetUser);
});
messagingTest('direct');
messageActionsTest('direct');
});
});
2 changes: 2 additions & 0 deletions tests/cypress/pageobjects/main-content.page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ class MainContent extends Page {

get messageUnread() { return browser.element('[data-id="mark-message-as-unread"][data-type="message-action"]'); }

get messageReplyInDM() { return browser.element('[data-id="reply-directly"][data-type="message-action"]'); }

// get messageReaction() { return browser.element('.message-actions__button[data-message-action="reaction-message"]'); }
get messagePin() { return browser.element('[data-id="pin-message"][data-type="message-action"]'); }
// get messageClose() { return browser.element('[data-id="rc-popover-close"][data-type="message-action"]'); }
Expand Down
2 changes: 2 additions & 0 deletions tests/cypress/pageobjects/side-nav.page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ class SideNav extends Page {

get popOverContent() { return browser.element('.rc-popover__content'); }

get popOverHideOption() { return browser.element('.rc-popover__content [data-id="hide"][data-type="sidebar-item"]'); }

get statusOnline() { return browser.element('.rc-popover__item--online'); }

get statusAway() { return browser.element('.rc-popover__item--away'); }
Expand Down