-
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
[HOLD for C+ manual request payment] [$500] Chat - Underscore doesn't work in emoji suggestions (works fine in search) #29081
Comments
Job added to Upwork: https://www.upwork.com/jobs/~01f81536e559725a51 |
Triggered auto assignment to @kadiealexander ( |
Bug0 Triage Checklist (Main S/O)
|
ProposalPlease re-state the problem that we are trying to solve in this issue.underscore doesn't work in emoji suggestions in composer but works fine in emoji picker What is the root cause of that problem?Lines 1277 to 1279 in 389d7b0
The issue is with regex that is used to test the input from composer,. App/src/pages/home/report/ReportActionCompose/SuggestionEmoji.js Lines 20 to 24 in c0fe505
the above code is splitting the inputText based upon the regex which also includes special characters including underscore(_) and then select the last from the leftWords array, after using underscore the last word is next to underscore which causes this behaviour What changes do you think we should make in order to solve the problem?First of all change the isEmojiCode function to like this below const isEmojiCode = (str, pos) => {
const after_ = str.slice(str.indexOf(':'));
const leftWords = after_.slice(0, pos).split(CONST.REGEX.SPECIAL_CHAR_OR_EMOJI);
const leftWord = _.last(leftWords);
return CONST.REGEX.HAS_COLON_ONLY_AT_THE_BEGINNING.test(leftWord) && leftWord.length > 2;
}; and remove the underscore from the regex and change the regex to this /[\n\s,/?"{}[\]()&~^%\\;`$=#<>!*\p{Extended_Pictographic}\u200d\u{1f1e6}-\u{1f1ff}\u{1f3fb}-\u{1f3ff}\u{e0020}-\u{e007f}\u20E3\uFE0F]|[#*0-9]\uFE0F?\u20E3/gu, Resultscreen-recording-2023-10-07-at-73636-pm_zHolOi5f.mp4What alternative solutions did you explore? (Optional)N/A |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mollfpr ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Placing underscore in an emoji disables the emoji selection What is the root cause of that problem?in the following regex: we unmatch the Lines 1277 to 1279 in 389d7b0
What changes do you think we should make in order to solve the problem?we should match any underscore after the colon but not before the colon: /[\n\s,/?"{}[\]()&~^%\\;`$=#<>!*\p{Extended_Pictographic}\u200d\u{1f1e6}-\u{1f1ff}\u{1f3fb}-\u{1f3ff}\u{e0020}-\u{e007f}\u20E3\uFE0F]|[#*0-9]\uFE0F?\u20E3|:.+/gu; also we need to use const leftWords = str.slice(0, pos).match(CONST.REGEX.SPECIAL_CHAR_OR_EMOJI); POCScreen.Recording.2023-10-08.at.2.46.03.AM.mov |
ProposalPlease re-state the problem that we are trying to solve in this issue.Underscore doesn’t work in emoji suggestions (works fine in search) What is the root cause of that problem?The main problem is that, unlike search, we have an additional check
As a result, a regular expression is used that does not allow us to use _ and several other characters Lines 1277 to 1280 in 389d7b0
What changes do you think we should make in order to solve the problem?We can update this regular like What alternative solutions did you explore? (Optional)We can don’t use isCurrentlyShowingEmojiSuggestion to make the same behavior as in search |
ProposalPlease re-state the problem that we are trying to solve in this issue.Underscore doesn't work in emoji suggestions (works fine in search) What is the root cause of that problem?The root cause of this issue is that regex split string by underscore Lines 1277 to 1280 in 619aee6
For example, if we type App/src/pages/home/report/ReportActionCompose/SuggestionEmoji.js Lines 20 to 24 in 619aee6
So emoji suggestion is not shown, see App/src/pages/home/report/ReportActionCompose/SuggestionEmoji.js Lines 169 to 181 in 619aee6
What changes do you think we should make in order to solve the problem?The character underscore To fix this issue, we can remove /[\n\s,/?"{}[\]()&~^%\\;`$=#<>!*\p{Extended_Pictographic}\u200d\u{1f1e6}-\u{1f1ff}\u{1f3fb}-\u{1f3ff}\u{e0020}-\u{e007f}\u20E3\uFE0F]|[#*0-9]\uFE0F?\u20E3|(_\b(?!$))/gu, So, text Screen.Recording.2023-10-09.at.9.35.09.PM.movWhat alternative solutions did you explore? (Optional)N/A |
@saranshbalyan-1234 Your solution does not working on
@abzokhattab Why is that? @ZhenjaHorbach Your regex doesn't work. |
@mollfpr |
Proposal updated #29081 (comment) |
all the conditions should now be handled with the updated proposal #29081 (comment) |
changed all tests are passing I think we need to take this issue into consideration when reviewing the proposals |
Running out of time, I'll get this reviewed in the morning 🙏 |
Thank you all for the proposals! I see a lot of ways to solve this issue, and I would like to have the solution simpler. The proposal from @eh2077 seems to have done the job, and it's not making a lot of change. I don't see any regression from the updated regex. 🎀 👀 🎀 C+ reviewed! @kadiealexander Could you add the engineering label? Thank you! |
Triggered auto assignment to @tgolen ( |
Thanks @mollfpr I agree with that regex change proposal. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.86-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2023-10-25. 🎊 After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
For reference, here are some details about the assignees on this issue:
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
https://github.com/Expensify/App/pull/27984/files#r1371846486
The regression step should be enough.
|
Payouts due:
Eligible for 50% #urgency bonus? Yes Upwork job is here. |
Thanks @kadiealexander for the payment summary. I'll do manual request in NewDot. |
I will withdraw the offer I just sent 😂 Thanks @mollfpr |
Not overdue. |
$750 payment approved for @mollfpr based on this summary. |
Can this be closed out now? |
I think yes. |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
App should display emoji suggestions for text with underscore as many emojis have underscore in them
Actual Result:
App does not display emoji suggestions for text with underscore
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.79.3
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
android.native.underscore.does.not.work.suggestion.mov
Recording.4897.mp4
mac.chrome.desktop.underscore.not.allowed.in.emoji.suggestion.mov
ios.safari.native.underscore.not.allowed.in.emoji.suggestion.mov
android.chrome.underscore.breaks.suggestions.mp4
windows.chrome.underscore.breaks.suggestions.mp4
android.native.timezone.position.error.1.mov
Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1696680165185589
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @kadiealexanderThe text was updated successfully, but these errors were encountered: