-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
base: main
Are you sure you want to change the base?
Ex4 chat masking #51790
Conversation
@srikarparsi 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] |
Hi @LCOleksii, is this for a specific issue? Please let me know and I can reopen this PR |
hm @srikarparsi why did you close this PR? |
Oh sorry, I didn't know if it was a legit one because I didn't see a linked issue and there was a bunch of lint errors when it was ready for review. But re-opening since you seem to suggest it's legit? |
Yep, it is legit. |
Re-assigning you @danieldoglas if that's okay |
edf55ae
to
ad88466
Compare
src/libs/ReportUtils.ts
Outdated
const baseRegexp = new RegExp(`${CONST.EMAIL.EXPENSIFY_EMAIL_DOMAIN}$`); | ||
const teamRegexp = new RegExp(`${CONST.EMAIL.EXPENSIFY_TEAM_EMAIL_DOMAIN}$`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove the regex testing and change this to use endsWith()
UNMASK: Chats between Non-Expensify users and Expensify users (e.g. Concierge) MASK: Chats between Expensify users and other Expensify users Chats between non-Expensify users that do NOT include any Expensify users Exposed PersonalDetailsProvider from OnyxProvider Added CONST that represents email domain for support team: - EXPENSIFY_TEAM_EMAIL_DOMAIN Updated @lib/Fullstory to evaluate MASK/UNMASK based on provided report context Added isExpensifyAndCustomerChat exported function to ReportUtils to handle report type evaluation.
src/libs/Fullstory/index.ts
Outdated
function getChatFSAttributes(context: OnyxEntry<PersonalDetailsList>, name: string, report: OnyxInputOrEntry<Report>, prefix: boolean): string { | ||
let componentPrefix = prefix ? `${name},` : ''; | ||
let componentSuffix = name ? `,fs-${name}` : ''; | ||
let fsAttrValue = ''; | ||
|
||
if (isConciergeChatReport(report)) { | ||
componentPrefix = prefix ? `${CONCIERGE}-${name},` : ''; | ||
componentSuffix = name ? `,fs-${name}` : ''; | ||
/* | ||
concierge-chatMessage,fs-unmask,fs-chatMessage | ||
*/ | ||
fsAttrValue = `${componentPrefix}${UNMASK}${componentSuffix}`; | ||
} else if (isExpensifyAndCustomerChat(context, report)) { | ||
componentPrefix = prefix ? `${CUSTOMER}-${name},` : ''; | ||
componentSuffix = name ? `,fs-${name}` : ''; | ||
/* | ||
customer-chatMessage,fs-unmask,fs-chatMessage | ||
*/ | ||
fsAttrValue = `${componentPrefix}${UNMASK}${componentSuffix}`; | ||
} else { | ||
componentPrefix = prefix ? `${OTHER}-${name},` : ''; | ||
componentSuffix = name ? `,fs-${name}` : ''; | ||
/* | ||
other-chatMessage,fs-mask,fs-chatMessage | ||
*/ | ||
fsAttrValue = `${componentPrefix}${MASK}${componentSuffix}`; | ||
} | ||
|
||
return fsAttrValue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this logic can be simplified as
function getChatFSAttributes(context: OnyxEntry<PersonalDetailsList>, name: string, report: OnyxInputOrEntry<Report>, prefix: boolean): string { | |
let componentPrefix = prefix ? `${name},` : ''; | |
let componentSuffix = name ? `,fs-${name}` : ''; | |
let fsAttrValue = ''; | |
if (isConciergeChatReport(report)) { | |
componentPrefix = prefix ? `${CONCIERGE}-${name},` : ''; | |
componentSuffix = name ? `,fs-${name}` : ''; | |
/* | |
concierge-chatMessage,fs-unmask,fs-chatMessage | |
*/ | |
fsAttrValue = `${componentPrefix}${UNMASK}${componentSuffix}`; | |
} else if (isExpensifyAndCustomerChat(context, report)) { | |
componentPrefix = prefix ? `${CUSTOMER}-${name},` : ''; | |
componentSuffix = name ? `,fs-${name}` : ''; | |
/* | |
customer-chatMessage,fs-unmask,fs-chatMessage | |
*/ | |
fsAttrValue = `${componentPrefix}${UNMASK}${componentSuffix}`; | |
} else { | |
componentPrefix = prefix ? `${OTHER}-${name},` : ''; | |
componentSuffix = name ? `,fs-${name}` : ''; | |
/* | |
other-chatMessage,fs-mask,fs-chatMessage | |
*/ | |
fsAttrValue = `${componentPrefix}${MASK}${componentSuffix}`; | |
} | |
return fsAttrValue; | |
} | |
function getChatFSAttributes(context: OnyxEntry<PersonalDetailsList>, name: string, report: OnyxInputOrEntry<Report>, prefix: boolean): string { | |
const baseName = name ? `,fs-${name}` : ''; | |
let componentPrefix = ''; | |
let maskType = MASK; | |
if (isConciergeChatReport(report)) { | |
componentPrefix = CONCIERGE; | |
maskType = UNMASK; | |
} else if (isExpensifyAndCustomerChat(context, report)) { | |
componentPrefix = CUSTOMER; | |
maskType = UNMASK; | |
} else { | |
componentPrefix = OTHER; | |
} | |
const prefixPart = prefix ? `${componentPrefix}-${name},` : ''; | |
return `${prefixPart}${maskType}${baseName}`; | |
} |
src/libs/Fullstory/index.ts
Outdated
return fsAttrValue; | ||
} | ||
|
||
function getChatFSAttributes(context: OnyxEntry<PersonalDetailsList>, name: string, report: OnyxInputOrEntry<Report>, prefix: boolean): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great to add test on all this functions
* Extract values from non-scraped at build time attribute WEB_PROP_ATTR, | ||
* reevaluate "fs-class". | ||
*/ | ||
function parseFSAttributes(): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
e4717a5
to
85e7858
Compare
src/libs/ReportUtils.ts
Outdated
const participantAccountIDs = new Set(Object.keys(report.participants)); | ||
if (participantAccountIDs.size > 2) { | ||
return false; | ||
} |
There was a problem hiding this comment.
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.
src/libs/ReportUtils.ts
Outdated
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; | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, got ot.
src/libs/ReportUtils.ts
Outdated
// 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; | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/libs/ReportUtils.ts
Outdated
if (contextAccountData) { | ||
const login = contextAccountData.login ?? ''; | ||
|
||
if (login.endsWith(CONST.EMAIL.EXPENSIFY_EMAIL_DOMAIN) ?? login.endsWith(CONST.EMAIL.EXPENSIFY_TEAM_EMAIL_DOMAIN)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What it means here the use of ??
? shouldn't be the OR operator? ||
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this case could be caught with tests, we should add some
const participantAccountIDs = new Set(Object.keys(report.participants)); | ||
if (participantAccountIDs.size > 2) { | ||
return false; | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 [`${formattedName},${UNMASK}`, `${formattedName}`]; | ||
} | ||
if (shouldUnmaskChat(context, report)) { | ||
const formattedName = `${CUSTOMER}-${name}`; | ||
return [`${formattedName},${UNMASK}`, `${formattedName}`]; | ||
} | ||
|
||
const formattedName = `${OTHER}-${name}`; | ||
return [`${formattedName},${MASK}`, `${formattedName}`]; |
There was a problem hiding this comment.
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]
There was a problem hiding this comment.
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
const [reportActionsListTestID, reportActionsListFSClass] = getChatFSAttributes(participantsContext, 'ReportActionsList', report); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
@LCOleksii please merge main into this |
# Conflicts: # src/libs/ReportUtils.ts # src/pages/home/report/ReportActionsList.tsx
There is a failing test |
Any news here? |
@LCOleksii bump on te test fix |
@gedu ready for your eyes again |
Cool, I will take a look tomorrow |
@gedu this is still being worked on, correct? Who are you waiting on? |
Sorry, something with more priority shows up, tomorrow first thing in the morning I will check it |
LGTM |
please assign me for review |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
/** | ||
* Placeholder function for Mobile-Web compatibility. | ||
*/ | ||
function parseFSAttributes(): void { | ||
// pass | ||
} |
There was a problem hiding this comment.
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 ?
const WEB_PROP_ATTR = 'data-testid'; | ||
const MASK = 'fs-mask'; | ||
const UNMASK = 'fs-unmask'; | ||
const CUSTOMER = 'customer'; |
There was a problem hiding this comment.
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 ?
return true; | ||
} | ||
|
||
if (isThread(report) && report.chatType && report.chatType === CONST.REPORT.CHAT_TYPE.POLICY_EXPENSE_CHAT) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) { |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as ^
} | ||
|
||
if (participantsContext) { | ||
// by email participants |
There was a problem hiding this comment.
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 ?
@@ -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); |
There was a problem hiding this comment.
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?
Explanation of Change
UNMASK:
Chats between Non-Expensify users and Expensify users (e.g. Concierge)
MASK:
Chats between Expensify users and other Expensify users
Chats between non-Expensify users that do NOT include any Expensify users
Exposed PersonalDetailsProvider from OnyxProvider
Added CONST that represents email domain for support team:
Updated @lib/Fullstory to evaluate MASK/UNMASK based on provided report context
Added isExpensifyAndCustomerChat exported function to ReportUtils
to handle report type evaluation.
Fixed Issues
$ #52271
PROPOSAL:
Improve MASK/UNMASK process based on report context.
No visual changes. Component attributes added.
Tests
QA:
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
$https://drive.google.com/file/d/1KoI9jJIAkUD80-na2Xzf35iFoI1wuLER/view?usp=drive_link
iOS: Native
$https://drive.google.com/file/d/10QZ6PG__dkNqF5RbQVFbSbhc4yzbi8Dv/view?usp=drive_link
iOS: mWeb Safari
MacOS: Chrome / Safari
$https://drive.google.com/file/d/1ZP5xIH7hcxV_LS_iA2PpbZDUgNRVaNjg/view?usp=sharing
MacOS: Desktop