Do not render whispers targeted to others by cristipaval · Pull Request #35943 · Expensify/App

cristipaval · 2024-02-06T18:53:02Z

Details

To unblock the launch of comment linking, we decided to return the whispered report actions to all report participants and treat them regularly in Onyx, but hide them from the front end. This is a temporary solution until we enable the end-to-end encryption that will allow only the targeted users to decrypt the whispers.

The problem that blocks the comment linking launch is the fact that the whispers are currently provided to the targeted users only, which makes previousReportActionID inappropriately set for the report actions succeeding the whisper.

Fixed Issues

#34982
PROPOSAL:

Tests

Test all whisper use cases to make sure they work as expected.

Whispers sent when the users join a public room
Whispers sent when employees are added to workspace chats
Whispers sent when the user scans a receipt and the SmartScan is in progress

Verify that no errors appear in the JS console

Offline tests

Same as tests.

QA Steps

Same as tests.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

d-40e1-aaa5-6b422df31690">

…targeted-to-others

melvin-bot · 2024-02-08T13:02:54Z

@ Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

ishpaul777 · 2024-02-08T15:33:45Z

hii @cristipaval, I am seeing 'Heads up, isn't a member of this room" message in LHN in the invited user Account, while it should not be visible to that user do you that is related to some backend change?

Left screen is Staging web
Right is development desktop

I invited dev. account from the staging account in a room using whisper message.

Expected Result: Whisper message is not visible in dev. account
Actual Result: Whisper message is visible in dev. account in LHN message

Screen.Recording.2024-02-08.at.8.53.28.PM.mov

ishpaul777 · 2024-02-08T15:48:42Z

Reviewer Checklist

Screenshots/Videos

Android: Native

VIDEO-2024-02-09-01-43-41.mp4

Android: mWeb Chrome

VIDEO-2024-02-09-01-54-44.mp4

iOS: Native

Tested Web macos and Ios native most cases in single video

1ec3957a-a3ee-4b58-9ba8-e88a76087e33.mp4

iOS: mWeb Safari

Whispers sent when the users join a public room

Screen.Recording.2024-02-09.at.1.09.00.AM.mov

Whispers sent when employees are added to workspace chats

Screen.Recording.2024-02-09.at.1.03.39.AM.mov

Whispers sent when the user scans a receipt and the SmartScan is in progress

Screen.Recording.2024-02-09.at.1.07.43.AM-1.mov

MacOS: Chrome / Safari

Tested Web macos and Ios native most cases in single video

1ec3957a-a3ee-4b58-9ba8-e88a76087e33.mp4

MacOS: Desktop

Whispers sent when the users join a public room

Screen.Recording.2024-02-08.at.8.53.28.PM.mov

Whispers sent when employees are added to workspace chats

Screen.Recording.2024-02-08.at.8.48.28.PM.mov

Whispers sent when the user scans a receipt and the SmartScan is in progress

Screen.Recording.2024-02-08.at.9.28.34.PM.mov

Screen.Recording.2024-02-08.at.9.37.34.PM.mov

cristipaval · 2024-02-08T15:52:35Z

Great finding @ishpaul777! Let me investigate a bit.

cristipaval · 2024-02-08T18:39:49Z

@ishpaul777 ready again for review

cristipaval · 2024-02-08T18:42:04Z

Ah, forgot to mention that the bug that you found in the sidebar also needs a fix in the backend

ishpaul777 · 2024-02-08T19:19:07Z

reviewing now

cristipaval · 2024-02-08T19:21:54Z

reviewing now

just pushed a new change to make tests pass

ishpaul777 · 2024-02-08T19:24:29Z

Ah, forgot to mention that the bug that you found in the sidebar also needs a fix in the backend

Does that mean we can ignore this for now ?

cristipaval · 2024-02-08T19:26:13Z

Ah, forgot to mention that the bug that you found in the sidebar also needs a fix in the backend

Does that mean we can ignore this for now ?

yes, because currently, there's no whisper to hide until we deploy the backend that also includes the fix

ishpaul777 · 2024-02-08T19:43:35Z

I am just noting now, I no longer recieve "invited < user > " message when inviting user from whisper message (on production also) is it something known?

(ignore simulator screen)

Screen.Recording.2024-02-09.at.1.10.12.AM.mov

melvin-bot · 2024-02-08T20:27:10Z

🎯 @ishpaul777, thanks for reviewing and testing this PR! 🎉

An E/App issue has been created to issue payment here: #36177.

cristipaval · 2024-02-08T20:28:07Z

I am just noting now, I no longer recieve "invited < user > " message when inviting user from whisper message (on production also) is it something known?

yes I think we removed it

ishpaul777 · 2024-02-08T20:28:54Z

Looks good, just a request can you add videos for all platforms or can this item from checklist be skipped?

I checked that screenshots or videos are included for tests on all platforms

cristipaval · 2024-02-09T15:54:00Z

ready again for review @roryabraham

Beamanator

Lookin good! I think I'd consider all my comments NAB so feel free to address or not :D

Co-authored-by: Alex Beaman <alexbeaman@expensify.com>

Beamanator

LGTM! Thanks for the changes! Feel free to self merge once the checks pass

…targeted-to-others

youssef-lr

LGTM!

Rory's comments have been addressed!

OSBotify · 2024-02-13T03:33:15Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

marcaaron

Can we confirm that no change is needed here?

App/src/libs/actions/Report.ts

Lines 1929 to 1982 in 148733c

    
           function shouldShowReportActionNotification(reportID: string, action: ReportAction | null = null, isRemote = false): boolean { 
        
               const tag = isRemote ? '[PushNotification]' : '[LocalNotification]'; 
        
               // Due to payload size constraints, some push notifications may have their report action stripped 
        
               // so we must double check that we were provided an action before using it in these checks. 
        
               if (action && ReportActionsUtils.isDeletedAction(action)) { 
        
                   Log.info(`${tag} Skipping notification because the action was deleted`, false, {reportID, action}); 
        
                   return false; 
        
               } 
        
               if (!ActiveClientManager.isClientTheLeader()) { 
        
                   Log.info(`${tag} Skipping notification because this client is not the leader`); 
        
                   return false; 
        
               } 
        
               // We don't want to send a local notification if the user preference is daily, mute or hidden. 
        
               const notificationPreference = allReports?.[reportID]?.notificationPreference ?? CONST.REPORT.NOTIFICATION_PREFERENCE.ALWAYS; 
        
               if (notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.ALWAYS) { 
        
                   Log.info(`${tag} No notification because user preference is to be notified: ${notificationPreference}`); 
        
                   return false; 
        
               } 
        
               // If this comment is from the current user we don't want to parrot whatever they wrote back to them. 
        
               if (action && action.actorAccountID === currentUserAccountID) { 
        
                   Log.info(`${tag} No notification because comment is from the currently logged in user`); 
        
                   return false; 
        
               } 
        
               // If we are currently viewing this report do not show a notification. 
        
               if (reportID === Navigation.getTopmostReportId() && Visibility.isVisible() && Visibility.hasFocus()) { 
        
                   Log.info(`${tag} No notification because it was a comment for the current report`); 
        
                   return false; 
        
               } 
        
               const report = currentReportData?.[reportID]; 
        
               if (!report || (report && report.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE)) { 
        
                   Log.info(`${tag} No notification because the report does not exist or is pending deleted`, false); 
        
                   return false; 
        
               } 
        
               // If this notification was delayed and the user saw the message already, don't show it 
        
               if (action && report?.lastReadTime && report.lastReadTime >= action.created) { 
        
                   Log.info(`${tag} No notification because the comment was already read`, false, {created: action.created, lastReadTime: report.lastReadTime}); 
        
                   return false; 
        
               } 
        
               // Only show notifications for supported types of report actions 
        
               if (!ReportActionsUtils.isNotifiableReportAction(action)) { 
        
                   Log.info(`${tag} No notification because this action type is not supported`, false, {actionName: action?.actionName}); 
        
                   return false; 
        
               } 
        
               return true; 
        
           }

If we start sending pusher updates for whispers then the actions could show up in browser notifications (which would be confusing as they are hidden from the UI otherwise)

OSBotify · 2024-02-15T05:24:40Z

🚀 Deployed to production by https://github.com/thienlnam in version: 1.4.41-12 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

cristipaval added 4 commits February 6, 2024 16:24

Function that checks if report action is an invisible whisper

600ca09

Merge remote-tracking branch 'origin/main' into cristi_hide-whispers-…

42a526c

…targeted-to-others

Do not display a whisper reportAction that is targeted to someone else

ab601c0

Revert initial change

42c0be3

cristipaval self-assigned this Feb 6, 2024

cristipaval added 2 commits February 6, 2024 20:54

Revert initial change

53b63db

Pass param to the function when is called

386408b

cristipaval mentioned this pull request Feb 6, 2024

[CRITICAL] Incorrect previousReportActionID for whispers from workspace invite #34982

Closed

roryabraham self-requested a review February 7, 2024 09:23

Merge remote-tracking branch 'origin/main' into cristi_hide-whispers-…

7d849cf

…targeted-to-others

cristipaval marked this pull request as ready for review February 8, 2024 13:02

cristipaval requested a review from a team as a code owner February 8, 2024 13:02

melvin-bot Bot removed the request for review from a team February 8, 2024 13:02

cristipaval requested a review from ishpaul777 February 8, 2024 13:04

ishpaul777 reviewed Feb 8, 2024

View reviewed changes

Comment thread src/pages/home/report/ReportActionItem.js Outdated

cristipaval added 2 commits February 8, 2024 20:37

Fix lastMessageText

8beeaf6

Add comment

fccc1ef

Change condition check to make tests pass

e6a3925

ishpaul777 approved these changes Feb 8, 2024

View reviewed changes

melvin-bot Bot mentioned this pull request Feb 8, 2024

[Payment due 2/20] Payment issue for E/App PR#35943 #36177

Closed

roryabraham previously requested changes Feb 8, 2024

View reviewed changes

Comment thread src/libs/ReportActionsUtils.ts Outdated

Comment thread src/pages/home/report/ReportActionItem.js Outdated

cristipaval added 3 commits February 9, 2024 16:14

use existing isWhisperAction function

ca48f41

Fix isWhisperActionTargetedToOthers check

02c3aac

Run prettier

c8c6319

cristipaval requested a review from roryabraham February 9, 2024 15:53

Beamanator previously approved these changes Feb 12, 2024

View reviewed changes

Comment thread src/libs/ReportActionsUtils.ts Outdated

Comment thread src/libs/ReportActionsUtils.ts Outdated

Comment thread src/libs/ReportActionsUtils.ts

Comment thread src/pages/home/report/ReportActionItem.js Outdated

Update src/libs/ReportActionsUtils.ts

db2c374

Co-authored-by: Alex Beaman <alexbeaman@expensify.com>

cristipaval dismissed Beamanator’s stale review via db2c374 February 12, 2024 15:50

cristipaval and others added 3 commits February 12, 2024 17:50

Update src/pages/home/report/ReportActionItem.js

9f6c311

Co-authored-by: Alex Beaman <alexbeaman@expensify.com>

Fix nab

e615147

Add function doc.

183ac5d

Beamanator previously approved these changes Feb 12, 2024

View reviewed changes

cristipaval added 4 commits February 12, 2024 18:17

Merge remote-tracking branch 'origin/main' into cristi_hide-whispers-…

00026db

…targeted-to-others

Merge remote-tracking branch 'origin/main' into cristi_hide-whispers-…

9a40318

…targeted-to-others

Merge remote-tracking branch 'origin/main' into cristi_hide-whispers-…

7dc7dd1

…targeted-to-others

Fix perf-tests.

cef6f1f

cristipaval dismissed Beamanator’s stale review via cef6f1f February 12, 2024 21:59

youssef-lr approved these changes Feb 13, 2024

View reviewed changes

youssef-lr merged commit c4856c9 into main Feb 13, 2024

youssef-lr deleted the cristi_hide-whispers-targeted-to-others branch February 13, 2024 03:28

github-actions Bot mentioned this pull request Feb 13, 2024

Deploy Checklist: New Expensify 2024-02-13 #36429

Closed

marcaaron reviewed Feb 14, 2024

View reviewed changes

	function shouldShowReportActionNotification(reportID: string, action: ReportAction \| null = null, isRemote = false): boolean {
	const tag = isRemote ? '[PushNotification]' : '[LocalNotification]';

	// Due to payload size constraints, some push notifications may have their report action stripped
	// so we must double check that we were provided an action before using it in these checks.
	if (action && ReportActionsUtils.isDeletedAction(action)) {
	Log.info(`${tag} Skipping notification because the action was deleted`, false, {reportID, action});
	return false;
	}

	if (!ActiveClientManager.isClientTheLeader()) {
	Log.info(`${tag} Skipping notification because this client is not the leader`);
	return false;
	}

	// We don't want to send a local notification if the user preference is daily, mute or hidden.
	const notificationPreference = allReports?.[reportID]?.notificationPreference ?? CONST.REPORT.NOTIFICATION_PREFERENCE.ALWAYS;
	if (notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.ALWAYS) {
	Log.info(`${tag} No notification because user preference is to be notified: ${notificationPreference}`);
	return false;
	}

	// If this comment is from the current user we don't want to parrot whatever they wrote back to them.
	if (action && action.actorAccountID === currentUserAccountID) {
	Log.info(`${tag} No notification because comment is from the currently logged in user`);
	return false;
	}

	// If we are currently viewing this report do not show a notification.
	if (reportID === Navigation.getTopmostReportId() && Visibility.isVisible() && Visibility.hasFocus()) {
	Log.info(`${tag} No notification because it was a comment for the current report`);
	return false;
	}

	const report = currentReportData?.[reportID];
	if (!report \|\| (report && report.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE)) {
	Log.info(`${tag} No notification because the report does not exist or is pending deleted`, false);
	return false;
	}

	// If this notification was delayed and the user saw the message already, don't show it
	if (action && report?.lastReadTime && report.lastReadTime >= action.created) {
	Log.info(`${tag} No notification because the comment was already read`, false, {created: action.created, lastReadTime: report.lastReadTime});
	return false;
	}

	// Only show notifications for supported types of report actions
	if (!ReportActionsUtils.isNotifiableReportAction(action)) {
	Log.info(`${tag} No notification because this action type is not supported`, false, {actionName: action?.actionName});
	return false;
	}

	return true;
	}

Conversation

cristipaval commented Feb 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

melvin-bot Bot commented Feb 8, 2024

Uh oh!

ishpaul777 commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ishpaul777 commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

cristipaval commented Feb 8, 2024

Uh oh!

Uh oh!

cristipaval commented Feb 8, 2024

Uh oh!

cristipaval commented Feb 8, 2024

Uh oh!

ishpaul777 commented Feb 8, 2024

Uh oh!

cristipaval commented Feb 8, 2024

Uh oh!

ishpaul777 commented Feb 8, 2024

Uh oh!

cristipaval commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ishpaul777 commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

melvin-bot Bot commented Feb 8, 2024

Uh oh!

cristipaval commented Feb 8, 2024

Uh oh!

ishpaul777 commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cristipaval commented Feb 9, 2024

Uh oh!

Beamanator left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Beamanator left a comment

Choose a reason for hiding this comment

Uh oh!

youssef-lr left a comment

Choose a reason for hiding this comment

Uh oh!

OSBotify commented Feb 13, 2024

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

OSBotify commented Feb 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cristipaval commented Feb 6, 2024 •

edited

Loading

ishpaul777 commented Feb 8, 2024 •

edited

Loading

ishpaul777 commented Feb 8, 2024 •

edited

Loading

cristipaval commented Feb 8, 2024 •

edited

Loading

ishpaul777 commented Feb 8, 2024 •

edited

Loading

ishpaul777 commented Feb 8, 2024 •

edited

Loading