[CP Staging] input of '[' and other special characters fixed by HardikChoudhary24 · Pull Request #30559 · Expensify/App

HardikChoudhary24 · 2023-10-29T18:47:32Z

Details

This PR fixes the input of special characters as mentioned in #30521 comment.

Fixed Issues

$ #28149
$ #30578
PROPOSAL:

Tests

Open any chat and paste an RTL text in the compose box, for example " مثال "
Continue typing some content
Verify that the content gets displayed from left to right
Verify that emoji/mention suggestions are displayed when triggered.
Verify that input of special character is possible.

Verify that no errors appear in the JS console

Offline tests

QA Steps

Open any chat and paste an RTL text in the compose box, for example " مثال "
Continue typing some content.
Verify that the content gets displayed from left to right.
Verify that Emoji/Mention suggestions are showing

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

MacOS: Chrome

chrome.mov

MacOS: Desktop

desktop.mov

Android: mWeb Chrome

WhatsApp.Video.2023-10-29.at.11.17.55.PM.mp4

iOS: Native

ios.mov

iOS: mWeb Safari

iosSafari.mov

MacOS: Safari

safari.mov

Android: Native

android.mp4

melvin-bot · 2023-10-29T18:47:36Z

@burczu 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]

HardikChoudhary24 · 2023-10-29T18:51:04Z

@neil-marcellini @burczu Apologies for the inconvenience caused. In my last change, I used the .match method to check for the presence of the CONST.UNICODE.LTR character, which was causing the issue. Now, I have rectified it and tested it on all platforms to ensure the reliability of my PR for the following cases:

RTL text is displayed from left to right.
User mentions.
Input of emojis and native emoji.
Links.
Special characters.
Draft comments.

Please review my changes and let me know if any other use case needs to be tested. Thank you.

aimane-chnaif · 2023-10-29T18:59:18Z

src/libs/convertToLTRForComposer/index.ts

        // If text is empty string return empty string to avoid an empty draft due to special character.
-        if (text === '' || CONST.UNICODE.LTR.match(text)) {
+        if (text === '' || text.startsWith(CONST.UNICODE.LTR)) {
            return '';
        }
        return text;


Suggested change

// If text is empty string return empty string to avoid an empty draft due to special character.

if (text === '' || CONST.UNICODE.LTR.match(text)) {

if (text === '' || text.startsWith(CONST.UNICODE.LTR)) {

return '';

}

return text;

return text.replace(CONST.UNICODE.LTR, '');

Anyway, when does this happen?

What's the root cause of this bug? Why only [ not working but all other special characters work?

This is because [ has a special meaning in regular expressions and CONST.UNICODE.LTR.match('[') gives an error because [ is not properly escaped. If we do 'CONST.UNICODE.LTR.match(/\[/) it will give null which we require but when we are passing a string we are just passing [. This also happens for ? , + due to the same reason.

Makes sense. Can you think of adding automated test cases? As you see, this might easily break composer input and affect bad user experience.

Sure I will add automated test cases. Should I raise a new PR for that?

The next deploy will be tomorrow (Monday) so if you can add them before that, that would be great.
Otherwise, we need to merge this before deploy to staging and work on unit test as a follow-up.

I won't be able to complete the task before tomorrow as my current workload won't permit me to meet that deadline. Can we do it as a follow up?

I am fine with it. cc: @neil-marcellini

aimane-chnaif

Looks good.
If requires reviewer checklist, I can do that. cc: @burczu

aimane-chnaif · 2023-10-30T14:13:43Z

Going ahead checklist since it's now deploy blocker

aimane-chnaif · 2023-10-30T14:44:08Z

@HardikChoudhary24 while testing android, I noticed that مثال is displayed in RTL

android.mov

aimane-chnaif · 2023-10-30T14:48:24Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

android.mov

Android: mWeb Chrome

mchrome.mov

iOS: Native

ios.mov

iOS: mWeb Safari

msafari.mov

MacOS: Chrome / Safari

web.mov

MacOS: Desktop

desktop.mov

HardikChoudhary24 · 2023-10-30T14:51:01Z

@HardikChoudhary24 while testing android, I noticed that مثال is displayed in RTL

android.mov

Yes it will be in RTL for android as mentioned in comment also this can be seen for other apps like linkedin where the web version is having the desired result and there android app is not showing it in LTR format

aimane-chnaif · 2023-10-30T14:51:34Z

ok, we're good then.
@neil-marcellini all yours

HardikChoudhary24 · 2023-10-30T14:54:13Z

@aimane-chnaif I have added the test cases for this can you please review them once and please let me know if any other test case is required.

aimane-chnaif · 2023-10-30T14:55:52Z

@aimane-chnaif I have added the test cases for this can you please review them once and please let me know if any other test case is required.

Mix of LTR and RTL characters please. Also lint failing

aimane-chnaif · 2023-10-30T15:04:46Z

tests/unit/ConvertToLTRForComposerTest.js

@@ -0,0 +1,71 @@
+import convertToLTRForComposer from '@libs/convertToLTRForComposer';
+import CONST from '@src/CONST';
+import {platform} from 'os';


Seems like new pattern. Not sure if it's the correct way to import platform in unit test

Since we are having a android specific implementation for convertToLTRForComposer where we are returning text without any modification is there any other way around to check for it in the test cases ?

@aimane-chnaif I have fixed the lint issue it was due to the import of platform. Also I have removed the check for platform. I am not sure if it is correct or not let me know if something needs to be updated.

neil-marcellini

Thanks for the quick fix and especially for the tests.

HardikChoudhary24 · 2023-10-30T18:08:37Z

@neil-marcellini since we have different implementation of convertToLTRForComposer in android so is there a check for platform required in the test also? I am not sure about this thus I have not added any check for platform

neil-marcellini · 2023-10-30T18:11:00Z

Is it possible to use platforms specific test files? It's not a big deal if we don't test Android, since it does nothing right?

OSBotify · 2023-10-30T18:13:15Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

HardikChoudhary24 · 2023-10-30T18:14:47Z

Yes even though it just returns the same text that is being passed but in the tests I have checked for specific cases like when we pass a RTL character we should get back a LTR marker concatenated at the start this might fail in case of android. However I ran the convertToLTRForComposerTest without doing any check for platform in my local and there was no issue but I am not sure as I have not worked with tests before.

Beamanator · 2023-10-31T08:44:42Z

CPing fix to staging 👍

input of '[' and other special characters fixed (cherry picked from commit d651cab)

OSBotify · 2023-10-31T09:17:30Z

🚀 Cherry-picked to staging by https://github.com/Beamanator in version: 1.3.93-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

Beamanator · 2023-10-31T10:14:15Z

Tests looking good in staging!

OSBotify · 2023-10-31T21:34:35Z

🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.93-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-11-02T10:40:26Z

🚀 Deployed to staging by https://github.com/neil-marcellini in version: 1.3.95-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-11-06T22:10:29Z

🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.95-9 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

input of '[' and other special characters fixed

7bbc841

HardikChoudhary24 requested a review from a team as a code owner October 29, 2023 18:47

melvin-bot bot removed the request for review from a team October 29, 2023 18:47

melvin-bot bot requested a review from burczu October 29, 2023 18:47

HardikChoudhary24 mentioned this pull request Oct 29, 2023

emoji/mention suggestion are displayed #30521

Merged

59 tasks

lint error fixed

c145333

aimane-chnaif reviewed Oct 29, 2023

View reviewed changes

suggested changes applied

c4cc971

aimane-chnaif approved these changes Oct 29, 2023

View reviewed changes

melvin-bot bot requested a review from neil-marcellini October 29, 2023 19:59

ishpaul777 mentioned this pull request Oct 30, 2023

[HOLD for payment 2023-11-09] Can't write some symbols in newDot #30578

Closed

grgia mentioned this pull request Oct 30, 2023

Chat - Unable to type a few symbols and console error shows up #30583

Closed

6 tasks

HardikChoudhary24 added 2 commits October 30, 2023 20:11

Merge remote-tracking branch 'upstream/main' into fix/input-error

fbc466b

test for convertToLTRForComposer added

7054c51

test case added

a2c2025

aimane-chnaif reviewed Oct 30, 2023

View reviewed changes

HardikChoudhary24 added 4 commits October 30, 2023 21:11

lint fixed

c449904

lint fixed

9bd0b5f

OS check removed

f6eede0

lint fixed

d28b633

neil-marcellini approved these changes Oct 30, 2023

View reviewed changes

neil-marcellini merged commit d651cab into Expensify:main Oct 30, 2023

Beamanator mentioned this pull request Oct 31, 2023

Deploy Checklist: New Expensify 2023-10-30 #30560

Closed

Beamanator changed the title ~~input of '[' and other special characters fixed~~ [CP Staging] input of '[' and other special characters fixed Oct 31, 2023

OSBotify pushed a commit that referenced this pull request Oct 31, 2023

Merge pull request #30559 from HardikChoudhary24/fix/input-error

0416a66

input of '[' and other special characters fixed (cherry picked from commit d651cab)

melvin-bot bot mentioned this pull request Oct 31, 2023

[HOLD for payment 2023-11-09] [$500] Chat - Compose box - Wrong cursor position for RTL texts and inverted arrow keys functionality #28149

Closed

6 tasks

OSBotify mentioned this pull request Nov 2, 2023

Deploy Checklist: New Expensify 2023-11-02 #30752

Closed

Conversation

HardikChoudhary24 commented Oct 29, 2023 • edited by neil-marcellini Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

melvin-bot bot commented Oct 29, 2023

Uh oh!

HardikChoudhary24 commented Oct 29, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aimane-chnaif left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aimane-chnaif commented Oct 30, 2023

Uh oh!

aimane-chnaif commented Oct 30, 2023

Uh oh!

aimane-chnaif commented Oct 30, 2023

Reviewer Checklist

Screenshots/Videos

Uh oh!

HardikChoudhary24 commented Oct 30, 2023

Uh oh!

aimane-chnaif commented Oct 30, 2023

Uh oh!

HardikChoudhary24 commented Oct 30, 2023

Uh oh!

aimane-chnaif commented Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neil-marcellini left a comment

Choose a reason for hiding this comment

Uh oh!

HardikChoudhary24 commented Oct 30, 2023

Uh oh!

neil-marcellini commented Oct 30, 2023

Uh oh!

OSBotify commented Oct 30, 2023

Uh oh!

HardikChoudhary24 commented Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Beamanator commented Oct 31, 2023

Uh oh!

OSBotify commented Oct 31, 2023

Uh oh!

Beamanator commented Oct 31, 2023

Uh oh!

OSBotify commented Oct 31, 2023

Uh oh!

OSBotify commented Nov 2, 2023

Uh oh!

HardikChoudhary24 commented Oct 29, 2023 •

edited by neil-marcellini

Loading

aimane-chnaif left a comment •

edited

Loading

aimane-chnaif commented Oct 30, 2023 •

edited

Loading

HardikChoudhary24 commented Oct 30, 2023 •

edited

Loading