-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Fix parsing short mentions when sending message to backend #65237
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
Changes from all commits
4688d4e
bae5225
4f95d5a
1c98cee
542e3b5
33f879b
b34ce59
ba75db6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| # Mentions highlighting in Composer input and chat messages | ||
|
|
||
| ## Glossary | ||
| **Full mention** - called more simply `userMention` is the full email of a user. In Expensify app _every_ correct full email gets highlighted in text. | ||
| When parsed via ExpensiMark into html, this mention is described with tag `<mention-user>`. | ||
| #### Examples of user mentions: `@john.doe@company.org`, `@vit@expensify.com` | ||
|
|
||
| **Short mention** - a special type of mention that contains only the login part of a user email **AND** the email domain has to be the same as our email domain. Any other `@mention` will not get highlighted if domains don't match | ||
| When parsed via ExpensiMark into html, it is described with tag `<mention-short>`. | ||
|
|
||
| #### Examples of short mentions: | ||
| - `@vit` - **IF** my domain is `expensify.com` **AND** there exists a user with email `vit@expensify.com` - it will get highlighted ✅ | ||
| - `@mateusz` - **IF** my domain is `expensify.com` **AND** there is **NO** user with email `mateusz@expensify.com`, but there is for example `mateusz@company.org` - it will NOT get highlighted ❌ | ||
|
|
||
| **ExpensiMark** - parser that we are using, which allows for parsing between markdown <---> html formats. Imported from `expensify-common` package. | ||
|
|
||
| ## tl;dr - the most important part | ||
| - there are 2 slightly different flows of handling mentions - one is inside the Composer Input and the other outside of it | ||
| - both are complex and need to support both userMentions and shortMentions - See **FAQ** | ||
|
|
||
| ## Parsing mentions inside Composer/Input (LiveMarkdown) | ||
| Our `Composer` component uses `react-native-live-markdown` for writing and editing markdown and handling mentions. When discussing how mentions work **inside** the composer input always look for answers in this [library](https://github.com/Expensify/react-native-live-markdown). | ||
|
|
||
| ### Mention parsing flow in live-markdown | ||
| 1. User types in some text | ||
| 2. `RNMarkdownTextInput` will handle the text by calling `parseExpensiMark`, which is an internal function of live-markdown: https://github.com/Expensify/react-native-live-markdown/blob/main/src/parseExpensiMark.ts | ||
| 3. `parseExpensiMark` will use `ExpensiMark` for parsing, then do several extra operations so that the component can work correctly | ||
| 4. When `ExpensiMark` parses the text, any full email will get parsed to `<mention-user>...</mention-user>` and any `@phrase` will get parsed to `<mention-short>...</mention-short>` | ||
| 5. `userMentions` are ready to use as they are so they require no further modification, however for `shortMentions` we need to check if they actually should get the highlighting | ||
| 5. We use the `parser` prop of `<MarkdownTextInput>` to pass custom parsing logic - this allows us to do some extra processing after `parseExpensiMark` runs. | ||
| 6. Our custom logic will go over every `<mention-short>` entry and verify if this login is someone that exists in userDetails data, then transform this into a full mention which gets highlighting | ||
|
|
||
| **NOTE:** this entire process takes part only "inside" Composer input. This is what happens between user typing in some text and user seeing the markdown/highlights in real time. | ||
|
|
||
| ## Parsing mentions outside of Composer/Input | ||
| When a user types in a message and wants to send it, we need to process the message text and then call the appropriate backend command. | ||
| However, backend only accepts text in html format. This means that text payload sent to backend has to be parsed via ExpensiMark. In addition, api **will not** accept `<mention-short>` tag - it only accepts full user mention with email. Frontend needs to process every `shortMention` into a `userMention` or stripping it completely from text. | ||
|
|
||
| ### Mention processing flow when sending a message | ||
| 1. After typing in some text user hits ENTER or presses the send button | ||
| 2. Several functions are called but ultimately `addActions(...)` is the one that will prepare backend payload and make the Api call. | ||
| 3. The function solely responsible for getting the correctly parsed text is `getParsedComment()` - it should return the string that is safe to send to backend. | ||
| 4. We **do not** have access to `parseExpensiMark` or any functions that worked in worklets, as we are outside of `live-markdown` but we need to process `shortMentions` regardless. | ||
| 5. The processing is done in `getParsedMessageWithShortMentions`: we parse via `ExpensiMark` with options very similar to what happens inside `parseExpensiMark` in `live-markdown`. (this is similar to Step 5. from previous flow). | ||
| 6. We then find every `<mention-short>...</mention-short>` and try to see if the specific mention exists in userDetails data. | ||
|
|
||
| ## FAQ | ||
| ### Q: Why can't we simply use `parseExpensiMark` in both cases?! | ||
| We cannot call `parseExpensiMark` in both cases, because `parseExpensiMark` returns a special data structure, called `MarkdownRange` which is both created and consumed by `react-native-live-markdown`. | ||
|
|
||
| Expensify API only accepts HTML and not markdown range. | ||
|
|
||
| Useful graph: | ||
| ``` | ||
| ExpensiMark: (raw text with markdown markers) ----> (HTML) | ||
| parseExpensiMark: (raw text with markdown markers) ----> MarkdownRange[] structure | ||
| ``` | ||
| ``` | ||
| <MarkdownTextInput> accepts MarkdownRange[] | ||
| Expensify Api call accepts HTML | ||
| ``` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -99,6 +99,7 @@ | |
| import Navigation, {navigationRef} from './Navigation/Navigation'; | ||
| import {rand64} from './NumberUtils'; | ||
| import Parser from './Parser'; | ||
| import {getParsedMessageWithShortMentions} from './ParsingUtils'; | ||
| import Permissions from './Permissions'; | ||
| import { | ||
| getAccountIDsByLogins, | ||
|
|
@@ -110,7 +111,6 @@ | |
| getPersonalDetailsByIDs, | ||
| getShortMentionIfFound, | ||
| } from './PersonalDetailsUtils'; | ||
| import {addSMSDomainIfPhoneNumber} from './PhoneNumber'; | ||
| import { | ||
| arePaymentsEnabled, | ||
| canSendInvoiceFromWorkspace, | ||
|
|
@@ -814,6 +814,11 @@ | |
| }; | ||
|
|
||
| type ParsingDetails = { | ||
| /** | ||
| * this param is deprecated | ||
| * Currently there are no calls/reference that use this param | ||
| * This should be removed after https://github.com/Expensify/App/issues/50724 as a followup | ||
| */ | ||
| shouldEscapeText?: boolean; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Kicu This might be problematic since we want to escape html tags directly present in message before parsing.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test this message as input It's get rendered as
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I'm not sure I understand. Like what you described sounds exactly correct - a user should never see the tag Also later down the line we
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. discussed on slack and solved |
||
| reportID?: string; | ||
| policyID?: string; | ||
|
|
@@ -886,7 +891,7 @@ | |
| let conciergeReportID: OnyxEntry<string>; | ||
| Onyx.connect({ | ||
| key: ONYXKEYS.CONCIERGE_REPORT_ID, | ||
| callback: (value) => { | ||
| conciergeReportID = value; | ||
| }, | ||
| }); | ||
|
|
@@ -894,7 +899,7 @@ | |
| const defaultAvatarBuildingIconTestID = 'SvgDefaultAvatarBuilding Icon'; | ||
| Onyx.connect({ | ||
| key: ONYXKEYS.SESSION, | ||
| callback: (value) => { | ||
| // When signed out, val is undefined | ||
| if (!value) { | ||
| return; | ||
|
|
@@ -912,7 +917,7 @@ | |
| let currentUserPersonalDetails: OnyxEntry<PersonalDetails>; | ||
| Onyx.connect({ | ||
| key: ONYXKEYS.PERSONAL_DETAILS_LIST, | ||
| callback: (value) => { | ||
| if (currentUserAccountID) { | ||
| currentUserPersonalDetails = value?.[currentUserAccountID] ?? undefined; | ||
| } | ||
|
|
@@ -924,14 +929,14 @@ | |
| let allReportsDraft: OnyxCollection<Report>; | ||
| Onyx.connect({ | ||
| key: ONYXKEYS.COLLECTION.REPORT_DRAFT, | ||
| waitForCollectionCallback: true, | ||
| callback: (value) => (allReportsDraft = value), | ||
| }); | ||
|
|
||
| let allPolicies: OnyxCollection<Policy>; | ||
| Onyx.connect({ | ||
| key: ONYXKEYS.COLLECTION.POLICY, | ||
| waitForCollectionCallback: true, | ||
| callback: (value) => (allPolicies = value), | ||
| }); | ||
|
|
||
|
|
@@ -939,7 +944,7 @@ | |
| let reportsByPolicyID: ReportByPolicyMap; | ||
| Onyx.connect({ | ||
| key: ONYXKEYS.COLLECTION.REPORT, | ||
| waitForCollectionCallback: true, | ||
| callback: (value) => { | ||
| allReports = value; | ||
| UnreadIndicatorUpdaterHelper().then((module) => { | ||
|
|
@@ -976,14 +981,14 @@ | |
| let allBetas: OnyxEntry<Beta[]>; | ||
| Onyx.connect({ | ||
| key: ONYXKEYS.BETAS, | ||
| callback: (value) => (allBetas = value), | ||
| }); | ||
|
|
||
| let allTransactions: OnyxCollection<Transaction> = {}; | ||
| let reportsTransactions: Record<string, Transaction[]> = {}; | ||
| Onyx.connect({ | ||
| key: ONYXKEYS.COLLECTION.TRANSACTION, | ||
| waitForCollectionCallback: true, | ||
| callback: (value) => { | ||
| if (!value) { | ||
| return; | ||
|
|
@@ -1009,7 +1014,7 @@ | |
| let allReportActions: OnyxCollection<ReportActions>; | ||
| Onyx.connect({ | ||
| key: ONYXKEYS.COLLECTION.REPORT_ACTIONS, | ||
| waitForCollectionCallback: true, | ||
| callback: (actions) => { | ||
| if (!actions) { | ||
| return; | ||
|
|
@@ -1022,7 +1027,7 @@ | |
| const allReportMetadataKeyValue: Record<string, ReportMetadata> = {}; | ||
| Onyx.connect({ | ||
| key: ONYXKEYS.COLLECTION.REPORT_METADATA, | ||
| waitForCollectionCallback: true, | ||
| callback: (value) => { | ||
| if (!value) { | ||
| return; | ||
|
|
@@ -5498,44 +5503,6 @@ | |
| return !isEmptyObject(report?.errorFields?.reportName); | ||
| } | ||
|
|
||
| /** | ||
| * Adds a domain to a short mention, converting it into a full mention with email or SMS domain. | ||
| * @param mention The user mention to be converted. | ||
| * @returns The converted mention as a full mention string or undefined if conversion is not applicable. | ||
| */ | ||
| function addDomainToShortMention(mention: string): string | undefined { | ||
| if (!Str.isValidEmail(mention) && currentUserPrivateDomain) { | ||
| const mentionWithEmailDomain = `${mention}@${currentUserPrivateDomain}`; | ||
| if (allPersonalDetailLogins.includes(mentionWithEmailDomain)) { | ||
| return mentionWithEmailDomain; | ||
| } | ||
| } | ||
| if (Str.isValidE164Phone(mention)) { | ||
| const mentionWithSmsDomain = addSMSDomainIfPhoneNumber(mention); | ||
| if (allPersonalDetailLogins.includes(mentionWithSmsDomain)) { | ||
| return mentionWithSmsDomain; | ||
| } | ||
| } | ||
| return undefined; | ||
| } | ||
|
|
||
| /** | ||
| * Replaces all valid short mention found in a text to a full mention | ||
| * | ||
| * Example: | ||
| * "Hello \@example -> Hello \@example\@expensify.com" | ||
| */ | ||
| function completeShortMention(text: string): string { | ||
| return text.replace(CONST.REGEX.SHORT_MENTION, (match) => { | ||
| if (!Str.isValidMention(match)) { | ||
| return match; | ||
| } | ||
| const mention = match.substring(1); | ||
| const mentionWithDomain = addDomainToShortMention(mention); | ||
| return mentionWithDomain ? `@${mentionWithDomain}` : match; | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * For comments shorter than or equal to 10k chars, convert the comment from MD into HTML because that's how it is stored in the database | ||
| * For longer comments, skip parsing, but still escape the text, and display plaintext for performance reasons. It takes over 40s to parse a 100k long string!! | ||
|
|
@@ -5556,16 +5523,21 @@ | |
| } | ||
| } | ||
|
|
||
| const textWithMention = completeShortMention(text); | ||
| const rules = disabledRules ?? []; | ||
|
|
||
| return text.length <= CONST.MAX_MARKUP_LENGTH | ||
| ? Parser.replace(textWithMention, { | ||
| shouldEscapeText: parsingDetails?.shouldEscapeText, | ||
| disabledRules: isGroupPolicyReport ? [...rules] : ['reportMentions', ...rules], | ||
| extras: {mediaAttributeCache: mediaAttributes}, | ||
| }) | ||
| : lodashEscape(text); | ||
| if (text.length > CONST.MAX_MARKUP_LENGTH) { | ||
| return lodashEscape(text); | ||
| } | ||
|
|
||
| return getParsedMessageWithShortMentions({ | ||
| text, | ||
| availableMentionLogins: allPersonalDetailLogins, | ||
| userEmailDomain: currentUserPrivateDomain, | ||
| parserOptions: { | ||
| disabledRules: isGroupPolicyReport ? [...rules] : ['reportMentions', ...rules], | ||
| extras: {mediaAttributeCache: mediaAttributes}, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| function getUploadingAttachmentHtml(file?: FileObject): string { | ||
|
|
@@ -5616,6 +5588,10 @@ | |
| return Parser.htmlToText(policy.description); | ||
| } | ||
|
|
||
| /** | ||
| * Fixme the `shouldEscapeText` arg is never used (it's always set to undefined) | ||
| * it should be removed after https://github.com/Expensify/App/issues/50724 gets fixed as a followup | ||
| */ | ||
| function buildOptimisticAddCommentReportAction( | ||
| text?: string, | ||
| file?: FileObject, | ||
|
|
@@ -11087,8 +11063,6 @@ | |
| } | ||
|
|
||
| export { | ||
| addDomainToShortMention, | ||
| completeShortMention, | ||
| areAllRequestsBeingSmartScanned, | ||
| buildOptimisticAddCommentReportAction, | ||
| buildOptimisticApprovedReportAction, | ||
|
|
||
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.
@Kicu We need to make sure this does not get trigger inside code block. Can you please check for that?
Uh oh!
There was an error while loading. Please reload this page.
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.
@shubham1206agra this line runs after
Parser.replace. This means the new regex runs on the code that ExpensiMark had already parsed.So it's
ExpensiMarkparser that guarantees that<mention-short(or any other mention) will not appear inside code block. I have verified with @Skalakid that this can not happen.However even if it would ever happen - then we'd fix it inside
ExpensiMark. The code that runs in this line does not require any extra checks.