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

Ex4 chat masking #51790

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions src/CONST.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1623,6 +1623,7 @@ const CONST = {
STUDENT_AMBASSADOR: '[email protected]',
SVFG: '[email protected]',
EXPENSIFY_EMAIL_DOMAIN: '@expensify.com',
EXPENSIFY_TEAM_EMAIL_DOMAIN: '@team.expensify.com',
},

CONCIERGE_DISPLAY_NAME: 'Concierge',
Expand Down
3 changes: 2 additions & 1 deletion src/components/OnyxProvider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import createOnyxContext from './createOnyxContext';
// Set up any providers for individual keys. This should only be used in cases where many components will subscribe to
// the same key (e.g. FlatList renderItem components)
const [withNetwork, NetworkProvider, NetworkContext] = createOnyxContext(ONYXKEYS.NETWORK);
const [, PersonalDetailsProvider, , usePersonalDetails] = createOnyxContext(ONYXKEYS.PERSONAL_DETAILS_LIST);
const [, PersonalDetailsProvider, PersonalDetailsContext, usePersonalDetails] = createOnyxContext(ONYXKEYS.PERSONAL_DETAILS_LIST);
const [withCurrentDate, CurrentDateProvider] = createOnyxContext(ONYXKEYS.CURRENT_DATE);
const [, BlockedFromConciergeProvider, , useBlockedFromConcierge] = createOnyxContext(ONYXKEYS.NVP_BLOCKED_FROM_CONCIERGE);
const [, BetasProvider, BetasContext, useBetas] = createOnyxContext(ONYXKEYS.BETAS);
Expand Down Expand Up @@ -55,6 +55,7 @@ export {
PreferredThemeContext,
useBetas,
useFrequentlyUsedEmojis,
PersonalDetailsContext,
PreferredEmojiSkinToneContext,
useBlockedFromConcierge,
useSession,
Expand Down
50 changes: 48 additions & 2 deletions src/libs/Fullstory/index.native.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,15 @@
import FullStory, {FSPage} from '@fullstory/react-native';
import type {OnyxEntry} from 'react-native-onyx';
import {isConciergeChatReport, isExpensifyAndCustomerChat} from '@libs/ReportUtils';
import CONST from '@src/CONST';
import * as Environment from '@src/libs/Environment/Environment';
import type {UserMetadata} from '@src/types/onyx';
import type {OnyxInputOrEntry, PersonalDetailsList, Report, UserMetadata} from '@src/types/onyx';

const MASK = 'fs-mask';
const UNMASK = 'fs-unmask';
const CUSTOMER = 'customer';
const CONCIERGE = 'concierge';
const OTHER = 'other';

/**
* Fullstory React-Native lib adapter
Expand Down Expand Up @@ -63,5 +70,44 @@ const FS = {
},
};

/**
* Placeholder function for Mobile-Web compatibility.
*/
function parseFSAttributes(): void {
// pass
}
Comment on lines +73 to +78
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay considering this placeholder, I will overlook it, but what is the use of this ?


/*
prefix? if component name should be used as a prefix,
in case data-test-id attribute usage,
clean component name should be preserved in data-test-id.
*/
function getFSAttributes(name: string, mask: boolean, prefix: boolean): string {
if (!name && !prefix) {
return `${mask ? MASK : UNMASK}`;
}
// prefixed for Native apps should contain only component name
if (prefix) {
return name;
}

return `${name},${mask ? MASK : UNMASK}`;
}

function getChatFSAttributes(context: OnyxEntry<PersonalDetailsList>, name: string, report: OnyxInputOrEntry<Report>): string[] {
if (!name) {
return ['', ''];
}
if (isConciergeChatReport(report)) {
const formattedName = `${CONCIERGE}-${name}`;
return [`${formattedName}`, `${UNMASK},${formattedName}`];
}
if (isExpensifyAndCustomerChat(context, report)) {
const formattedName = `${CUSTOMER}-${name}`;
return [`${formattedName}`, `${UNMASK},${formattedName}`];
}
const formattedName = `${OTHER}-${name}`;
return [`${formattedName}`, `${MASK},${formattedName}`];
}
export default FS;
export {FSPage};
export {FSPage, parseFSAttributes, getFSAttributes, getChatFSAttributes};
84 changes: 81 additions & 3 deletions src/libs/Fullstory/index.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,18 @@
import {FullStory, init, isInitialized} from '@fullstory/browser';
import type {OnyxEntry} from 'react-native-onyx';
import {isConciergeChatReport, isExpensifyAndCustomerChat} from '@libs/ReportUtils';
import CONST from '@src/CONST';
import * as Environment from '@src/libs/Environment/Environment';
import type {UserMetadata} from '@src/types/onyx';
import type {OnyxInputOrEntry, PersonalDetailsList, Report, UserMetadata} from '@src/types/onyx';
import type NavigationProperties from './types';

const WEB_PROP_ATTR = 'data-testid';
const MASK = 'fs-mask';
const UNMASK = 'fs-unmask';
const CUSTOMER = 'customer';
Copy link
Contributor

Choose a reason for hiding this comment

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

I see these values repeated again from native and web files, this breaks the checklist checkbox which wants our code to be DRY, can you define these values in CONST.ts and use here and in native file ?

const CONCIERGE = 'concierge';
const OTHER = 'other';

// Placeholder Browser API does not support Manual Page definition
class FSPage {
private pageName;
Expand All @@ -16,7 +24,9 @@ class FSPage {
this.properties = properties;
}

start() {}
start() {
parseFSAttributes();
}
}

/**
Expand Down Expand Up @@ -92,5 +102,73 @@ const FS = {
init: (_value: OnyxEntry<UserMetadata>) => {},
};

/**
* Extract values from non-scraped at build time attribute WEB_PROP_ATTR,
* reevaluate "fs-class".
*/
function parseFSAttributes(): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

I’m concerned about this function; it seems to be executed multiple times in a row. Each execution tests approximately 300 elements, and this occurs when switching reports, even if switching between the same reports.

Screenshot 2024-11-06 at 19 11 58

Would be great to avoid it, given that I felt the report switching lagging

Copy link
Contributor

Choose a reason for hiding this comment

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

I also suggest to add test on this function, which seems complex and any future update should be cover by tests

window?.document?.querySelectorAll(`[${WEB_PROP_ATTR}]`).forEach((o) => {
const attr = o.getAttribute(WEB_PROP_ATTR) ?? '';
if (!/fs-/gim.test(attr)) {
return;
}

const fsAttrs = attr.match(/fs-[a-zA-Z0-9_-]+/g) ?? [];
o.setAttribute('fs-class', fsAttrs.join(','));

let cleanedAttrs = attr;
fsAttrs.forEach((fsAttr) => {
cleanedAttrs = cleanedAttrs.replace(fsAttr, '');
});

cleanedAttrs = cleanedAttrs
.replace(/,+/g, ',')
.replace(/\s*,\s*/g, ',')
.replace(/^,+|,+$/g, '')
.replace(/\s+/g, ' ')
.trim();

if (cleanedAttrs) {
o.setAttribute(WEB_PROP_ATTR, cleanedAttrs);
} else {
o.removeAttribute(WEB_PROP_ATTR);
}
});
}

/*
prefix? if component name should be used as a prefix,
in case data-test-id attribute usage,
clean component name should be preserved in data-test-id.
*/
function getFSAttributes(name: string, mask: boolean, prefix: boolean): string {
if (!name) {
return `${mask ? MASK : UNMASK}`;
}

if (prefix) {
return `${name},${mask ? MASK : UNMASK}`;
}

return `${name}`;
}

function getChatFSAttributes(context: OnyxEntry<PersonalDetailsList>, name: string, report: OnyxInputOrEntry<Report>): string[] {
if (!name) {
return ['', ''];
}
if (isConciergeChatReport(report)) {
const formattedName = `${CONCIERGE}-${name}`;
return [`${formattedName},${UNMASK}`, `${formattedName}`];
}
if (isExpensifyAndCustomerChat(context, report)) {
const formattedName = `${CUSTOMER}-${name}`;
return [`${formattedName},${UNMASK}`, `${formattedName}`];
}

const formattedName = `${OTHER}-${name}`;
return [`${formattedName},${MASK}`, `${formattedName}`];
}

export default FS;
export {FSPage};
export {FSPage, parseFSAttributes, getFSAttributes, getChatFSAttributes};
47 changes: 47 additions & 0 deletions src/libs/ReportUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8407,6 +8407,52 @@ function isExpenseReportWithoutParentAccess(report: OnyxEntry<Report>) {
return isExpenseReport(report) && report?.hasParentAccess === false;
}

function isExpensifyAndCustomerChat(participantsContext: OnyxEntry<PersonalDetailsList>, report: OnyxInputOrEntry<Report>): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
function isExpensifyAndCustomerChat(participantsContext: OnyxEntry<PersonalDetailsList>, report: OnyxInputOrEntry<Report>): boolean {
function isExpensifyAndCustomerChat(participantsContext: OnyxEntry<PersonalDetailsList>, report: OnyxInputOrEntry<Report>): boolean {

I don't think this is a correct name for this function. We're also checking if the chat is policyExpenseChat and returning true, and in those cases, it's not a expensify and customer chat. We should probably rename this to something like shouldUnmaskChat or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you.
Done changed @danieldoglas

if (!report?.participants) {
return true;
}

const participantAccountIDs = new Set(Object.keys(report.participants));
if (participantAccountIDs.size > 2) {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should come after the other checks. A police expense chat can have 10s of people, and we would want to unmask those too. this should be added after line 8426.


if (isThread(report) && report.chatType && report.chatType === CONST.REPORT.CHAT_TYPE.POLICY_EXPENSE_CHAT) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (isThread(report) && report.chatType && report.chatType === CONST.REPORT.CHAT_TYPE.POLICY_EXPENSE_CHAT) {
if (isThread(report) && report?.chatType && report?.chatType === CONST.REPORT.CHAT_TYPE.POLICY_EXPENSE_CHAT) {

I worry that the app will crash if we don't have any of those, won't it ? do you think that this is unnecessary ?

return true;
}

if (isThread(report) && report.type === CONST.REPORT.TYPE.EXPENSE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same as ^

return true;
}

if (participantsContext) {
// by email participants
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if i understand this comment, can you be more specific here ?

for (const participantAccountID of participantAccountIDs) {
const id = Number(participantAccountID);
const contextAccountData = participantsContext[id];
if (contextAccountData) {
const login = contextAccountData.login ?? '';
if (login.endsWith(CONST.EMAIL.EXPENSIFY_EMAIL_DOMAIN)) {
return true;
}

if (login.endsWith(CONST.EMAIL.EXPENSIFY_TEAM_EMAIL_DOMAIN)) {
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is wrong. If we have 1:1 chats between expensify users only, we don't want to unmask those. Since you'll only have 2 emails here, we only want to return true if one of the emails is from expensify AND the other isnt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, got ot.

}
}
}

// System users communication
const expensifyTeam = new Set(Object.values(CONST.ACCOUNT_ID));
const hasIntersection = [...participantAccountIDs].some((id) => expensifyTeam.has(Number(id)));
if (hasIntersection) {
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused on this part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set.Intersection is not available in the TS version you are using.
this is alternative to intersection check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed this bit as this was not part of original ticket.


return false;
}

export {
addDomainToShortMention,
completeShortMention,
Expand Down Expand Up @@ -8599,6 +8645,7 @@ export {
isClosedExpenseReportWithNoExpenses,
isCompletedTaskReport,
isConciergeChatReport,
isExpensifyAndCustomerChat,
isControlPolicyExpenseChat,
isControlPolicyExpenseReport,
isCurrentUserSubmitter,
Expand Down
13 changes: 11 additions & 2 deletions src/pages/home/report/ReportActionsList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import {useIsFocused, useRoute} from '@react-navigation/native';
import type {RouteProp} from '@react-navigation/native';
// eslint-disable-next-line lodash/import-scope
import type {DebouncedFunc} from 'lodash';
import React, {memo, useCallback, useEffect, useMemo, useRef, useState} from 'react';
import React, {memo, useCallback, useContext, useEffect, useMemo, useRef, useState} from 'react';
import {DeviceEventEmitter, InteractionManager, View} from 'react-native';
import type {LayoutChangeEvent, NativeScrollEvent, NativeSyntheticEvent, StyleProp, ViewStyle} from 'react-native';
import {useOnyx} from 'react-native-onyx';
Expand All @@ -20,13 +20,15 @@ import useResponsiveLayout from '@hooks/useResponsiveLayout';
import useThemeStyles from '@hooks/useThemeStyles';
import useWindowDimensions from '@hooks/useWindowDimensions';
import DateUtils from '@libs/DateUtils';
import {getChatFSAttributes} from '@libs/Fullstory';
import Navigation from '@libs/Navigation/Navigation';
import * as ReportActionsUtils from '@libs/ReportActionsUtils';
import * as ReportUtils from '@libs/ReportUtils';
import Visibility from '@libs/Visibility';
import type {AuthScreensParamList} from '@navigation/types';
import variables from '@styles/variables';
import * as Report from '@userActions/Report';
import {PersonalDetailsContext} from '@src/components/OnyxProvider';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
Expand Down Expand Up @@ -165,6 +167,7 @@ function ReportActionsList({

const [reportNameValuePairs] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${report?.reportID ?? -1}`);
const [accountID] = useOnyx(ONYXKEYS.SESSION, {selector: (session) => session?.accountID});
const participantsContext = useContext(PersonalDetailsContext);

useEffect(() => {
const unsubscriber = Visibility.onVisibilityChange(() => {
Expand Down Expand Up @@ -709,13 +712,19 @@ function ReportActionsList({
// When performing comment linking, initially 25 items are added to the list. Subsequent fetches add 15 items from the cache or 50 items from the server.
// This is to ensure that the user is able to see the 'scroll to newer comments' button when they do comment linking and have not reached the end of the list yet.
const canScrollToNewerComments = !isLoadingInitialReportActions && !hasNewestReportAction && sortedReportActions.length > 25 && !isLastPendingActionIsDelete;
const [reportActionsListTestID, reportActionsListFSClass] = getChatFSAttributes(participantsContext, 'ReportActionsList', report);
Copy link
Contributor

Choose a reason for hiding this comment

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

was comment from @gedu addressed here ? they mentioned that ReportActionsList was rendered many times?


Comment on lines +725 to +726
Copy link
Contributor

Choose a reason for hiding this comment

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

ReportActionsList is being rerender many times, when switching and when sending/receiving messages, I would recommend to wrap it in a useMemo and the dependency array would be [report.reportID]
Screenshot 2024-11-13 at 11 44 20

Screenshot 2024-11-13 at 11 45 56

Copy link
Contributor

Choose a reason for hiding this comment

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

Please address this comment too

return (
<>
<FloatingMessageCounter
isActive={(isFloatingMessageCounterVisible && !!unreadMarkerReportActionID) || canScrollToNewerComments}
onClick={scrollToBottomAndMarkReportAsRead}
/>
<View style={[styles.flex1, !shouldShowReportRecipientLocalTime && !hideComposer ? styles.pb4 : {}]}>
<View
style={[styles.flex1, !shouldShowReportRecipientLocalTime && !hideComposer ? styles.pb4 : {}]}
testID={reportActionsListTestID}
fsClass={reportActionsListFSClass}
>
<InvertedFlatList
accessibilityLabel={translate('sidebarScreen.listOfChatMessages')}
ref={reportScrollManager.ref}
Expand Down
Loading