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 @@ -1682,6 +1682,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, shouldUnmaskChat} 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 (shouldUnmaskChat(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, shouldUnmaskChat} 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 (shouldUnmaskChat(context, report)) {
const formattedName = `${CUSTOMER}-${name}`;
return [`${formattedName},${UNMASK}`, `${formattedName}`];
}

const formattedName = `${OTHER}-${name}`;
return [`${formattedName},${MASK}`, `${formattedName}`];
Comment on lines +162 to +170
Copy link
Contributor

Choose a reason for hiding this comment

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

there is no need to interpolate formattedName, it is an string already, so the second position can be just
[${formattedName}, ${XXXXX}, formattedName]

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

}

export default FS;
export {FSPage};
export {FSPage, parseFSAttributes, getFSAttributes, getChatFSAttributes};
46 changes: 46 additions & 0 deletions src/libs/ReportUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8478,6 +8478,51 @@ function hasInvoiceReports() {
return allReports.some((report) => isInvoiceReport(report));
}

function shouldUnmaskChat(participantsContext: OnyxEntry<PersonalDetailsList>, report: OnyxInputOrEntry<Report>): boolean {
if (!report?.participants) {
return true;
}

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;
}

const participantAccountIDs = new Set(Object.keys(report.participants));
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 ^

if (participantAccountIDs.size > 2) {
return false;
}
Comment on lines +8494 to +8497
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need to create a Set

const participantAccountIDs = Object.keys(report.participants);
    if (participantAccountIDs.length > 2) {
        return false;
    }
    ```
    that should work 

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


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 ?

let teamInChat = false;
let userInChat = false;
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) || login.endsWith(CONST.EMAIL.EXPENSIFY_TEAM_EMAIL_DOMAIN)) {
teamInChat = true;
} else {
userInChat = true;
}
}
}
// exclude teamOnly chat
if (teamInChat && userInChat) {
return true;
}
}

return false;
}

function getReportMetadata(reportID?: string) {
return allReportMetadata?.[`${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportID}`];
}
Expand Down Expand Up @@ -8675,6 +8720,7 @@ export {
isClosedExpenseReportWithNoExpenses,
isCompletedTaskReport,
isConciergeChatReport,
shouldUnmaskChat,
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 @@ -2,7 +2,7 @@ import type {ListRenderItemInfo} from '@react-native/virtualized-lists/Lists/Vir
import {useIsFocused, useRoute} 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 @@ -19,6 +19,7 @@ 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 isSearchTopmostCentralPane from '@libs/Navigation/isSearchTopmostCentralPane';
import Navigation from '@libs/Navigation/Navigation';
import type {PlatformStackRouteProp} from '@libs/Navigation/PlatformStackNavigation/types';
Expand All @@ -29,6 +30,7 @@ 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 @@ -171,6 +173,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 @@ -719,13 +722,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
9 changes: 9 additions & 0 deletions tests/perf-test/ReportActionsList.perf-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,15 @@ type LazyLoadLHNTestUtils = {

const mockedNavigate = jest.fn();

// Mock Fullstory library dependency
jest.mock('@libs/Fullstory', () => ({
default: {
consentAndIdentify: jest.fn(),
},
getFSAttributes: jest.fn(),
getChatFSAttributes: jest.fn().mockReturnValue(['mockTestID', 'mockFSClass']),
}));

jest.mock('@components/withCurrentUserPersonalDetails', () => {
// Lazy loading of LHNTestUtils
const lazyLoadLHNTestUtils = () => require<LazyLoadLHNTestUtils>('../utils/LHNTestUtils');
Expand Down
Loading