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

Fix: email pattern not recognised when sending a comment #30260

Merged

Conversation

eh2077
Copy link
Contributor

@eh2077 eh2077 commented Oct 24, 2023

Details

Fixed Issues

$ #28629
PROPOSAL: #28629 (comment)

Tests

  1. Go to a chat and add a comment
`code`test@gmail.com
  1. Verify that email is displayed as hyperlink
  • Verify that no errors appear in the JS console

Offline tests

Same as tests

QA Steps

Same as tests

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover 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 tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
    • MacOS: Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (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
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • 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 followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • 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
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • 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
    • If we are not using the full Onyx data that we loaded, I've added the proper selector in order to ensure the component only re-renders when the data it is using changes
    • 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 author checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native
0-android.mp4
Android: mWeb Chrome
0-mobile-chrome.mp4
iOS: Native
0-ios.mp4
iOS: mWeb Safari
0-mobile-safari.mp4
MacOS: Chrome / Safari
0-web.mp4
MacOS: Desktop
0-desktop.mp4

@eh2077 eh2077 requested a review from a team as a code owner October 24, 2023 14:57
@melvin-bot melvin-bot bot requested review from burczu and removed request for a team October 24, 2023 14:58
@melvin-bot
Copy link

melvin-bot bot commented Oct 24, 2023

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

@burczu
Copy link
Contributor

burczu commented Oct 25, 2023

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

Web
Screen.Recording.2023-10-25.at.14.10.11.mov
Mobile Web - Chrome
Screen.Recording.2023-10-25.at.14.07.01.mov
Mobile Web - Safari
Screen.Recording.2023-10-25.at.14.07.44.mov
Desktop
Screen.Recording.2023-10-25.at.14.09.27.mov
iOS
Screen.Recording.2023-10-25.at.14.00.44.mov
Android
Screen.Recording.2023-10-25.at.14.04.52.mov

Copy link
Contributor

@burczu burczu left a comment

Choose a reason for hiding this comment

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

LGTM

@neil-marcellini neil-marcellini merged commit 98654ad into Expensify:main Oct 25, 2023
16 checks passed
@OSBotify
Copy link
Contributor

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

@github-actions github-actions bot added the DeployBlockerCash This issue or pull request should block deployment label Oct 25, 2023
@github-actions
Copy link
Contributor

Performance Comparison Report 📊

Significant Changes To Duration

Name Duration
App start TTI 1173.044 ms → 1315.951 ms (+142.908 ms, +12.2%) 🔴
App start runJsBundle 816.567 ms → 932.655 ms (+116.089 ms, +14.2%) 🔴
Show details
Name Duration
App start TTI Baseline
Mean: 1173.044 ms
Stdev: 43.672 ms (3.7%)
Runs: 1063.138911000453 1111.314939999953 1113.6909490004182 1117.2763269999996 1119.0950400000438 1123.53385000024 1124.941858000122 1128.900520999916 1129.0152709996328 1130.4348900001496 1131.8563609998673 1132.6180090000853 1133.632508000359 1135.7848410001025 1138.6129249995574 1139.3878480000421 1139.7939780000597 1144.5425270004198 1144.669614000246 1145.1141550000757 1147.895859000273 1149.0094050001353 1150.6619389997795 1155.5867980001494 1157.502423999831 1159.5374440001324 1161.0928589999676 1161.682807000354 1163.5548040000722 1164.9875469999388 1167.0493339998648 1169.3280990002677 1169.6984689999372 1173.1918970001861 1176.5874450001866 1180.0553109999746 1180.1007909998298 1182.7430670000613 1182.9561919998378 1190.7096079997718 1194.4505650000647 1197.8873290000483 1203.7112379996106 1204.467465000227 1208.8889549998567 1209.3660340001807 1210.9362580003217 1211.803938000463 1214.1299489997327 1215.3329530004412 1216.2707110000774 1225.6489329999313 1227.8882780000567 1230.7841910002753 1239.075206999667 1239.4397590002045 1247.3285609995946 1256.0242320001125 1256.1247340003029 1281.7800660002977

Current
Mean: 1315.951 ms
Stdev: 59.988 ms (4.6%)
Runs: 1203.5271939998493 1233.3918220000342 1237.5378109999 1245.4820349998772 1247.1761850002222 1248.1265070000663 1252.1928570000455 1256.639432999771 1257.3036449998617 1258.0616489998065 1259.9200149998069 1260.7880850001238 1262.003213999793 1263.9659070000052 1277.6486760000698 1280.301128000021 1281.7923189997673 1283.965152000077 1284.1619090000167 1286.1188170001842 1287.252568999771 1290.1760559999384 1299.72328499984 1299.758429999929 1300.827806999907 1304.717929000035 1304.9612790001556 1305.4122999999672 1306.053617999889 1307.1324390000664 1310.6098819999024 1315.7201649998315 1321.8742410000414 1322.6288200002164 1322.8152060001157 1325.1392149999738 1332.9890089998953 1333.8423850000836 1335.2516569998115 1336.900020999834 1337.196758999955 1338.9354770001955 1340.1206959998235 1342.8714439999312 1356.9847679999657 1366.6413010000251 1367.825511999894 1374.0787169998512 1385.8770280000754 1386.836043999996 1389.2407390000299 1412.6868870002218 1435.2935429997742 1470.0352139999159 1470.699634999968 1474.059305000119
App start runJsBundle Baseline
Mean: 816.567 ms
Stdev: 36.678 ms (4.5%)
Runs: 747 748 765 765 765 768 768 773 778 781 782 788 788 792 792 795 795 796 796 797 797 797 799 800 804 807 808 808 809 811 812 814 817 818 821 821 821 823 826 829 830 830 832 834 838 842 845 849 852 855 861 862 865 866 868 872 875 883 900 914

Current
Mean: 932.655 ms
Stdev: 48.869 ms (5.2%)
Runs: 858 865 866 869 873 874 880 881 884 884 888 889 894 894 895 899 903 904 905 907 907 909 909 909 912 919 921 923 926 928 928 929 932 933 935 941 941 945 947 947 947 948 953 955 956 959 968 970 975 980 994 1006 1016 1037 1039 1041 1047 1050

Meaningless Changes To Duration

Show entries
Name Duration
Open Search Page TTI 722.567 ms → 744.944 ms (+22.377 ms, +3.1%)
App start nativeLaunch 21.125 ms → 23.034 ms (+1.909 ms, +9.0%)
App start regularAppStart 0.014 ms → 0.017 ms (+0.003 ms, +19.4%) 🟡
Show details
Name Duration
Open Search Page TTI Baseline
Mean: 722.567 ms
Stdev: 50.561 ms (7.0%)
Runs: 649.2878019995987 654.1669519999996 655.265666000545 657.912882999517 660.3965249992907 668.004313999787 668.4434409998357 669.7729489998892 671.0334070008248 674.3115650005639 675.4390059998259 677.0511879995465 677.1049399999902 681.6400149995461 682.4777830000967 682.5558679997921 684.2145589999855 686.5173749998212 688.2310389997438 688.2644450003281 688.8482260005549 690.1889239996672 691.1502689998597 695.5706790005788 702.9954019999132 703.5152179999277 704.4849450001493 708.0609949994832 708.3769540004432 710.7819830002263 714.3975830003619 716.1405440000817 721.1772459996864 721.239990000613 721.5088710002601 728.6586920004338 732.2104900004342 733.6979170003906 734.2477219998837 735.44511000067 740.9165039993823 741.0978189995512 745.8172610001639 750.8343509994447 752.0852859998122 758.2954100007191 764.5785730006173 767.6599540002644 771.9611010001972 773.7358809998259 775.46643100027 779.5333249997348 781.3066819999367 785.2742520002648 785.7349049998447 790.3415530007333 821.4266359992325 842.1762290000916 849.4565030001104 861.5067549999803

Current
Mean: 744.944 ms
Stdev: 47.174 ms (6.3%)
Runs: 673.0734860002995 685.2005210001953 689.4327799999155 690.8137619998306 694.6304940003902 696.3861899999902 696.6959640001878 697.6629640003666 700.4320080000907 702.1671149996109 703.3695069998503 704.9486899999902 706.5643309997395 707.9578860001639 708.2064219997264 708.4275320000015 709.1242680000141 709.4559329999611 710.4062910000794 712.3275149995461 713.7467050002888 714.4405519999564 714.5265309996903 719.2371419998817 720.8120940001681 722.9823410003446 723.5070799998939 732.7120780004188 735.1322440002114 735.5225840001367 739.8896090001799 740.4072679998353 741.0255950000137 744.4784350004047 745.3216969999485 746.6328529999591 747.3234860002995 757.7123219999485 757.9493410000578 758.0187590001151 758.8423270001076 761.6310220002197 764.2452400000766 766.7962239999324 772.4355480000377 775.37552900007 776.5800370001234 777.1419679997489 792.7471929998137 803.6379800001159 804.8031419999897 813.7679449999705 813.991699999664 814.5173339997418 822.3831790001132 822.4045819998719 844.8782959999517 870.2607009997591 876.578451000154
App start nativeLaunch Baseline
Mean: 21.125 ms
Stdev: 1.852 ms (8.8%)
Runs: 18 19 19 19 19 19 19 19 19 19 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 21 21 21 21 21 21 21 21 21 21 21 21 22 22 22 22 22 22 22 23 23 23 23 23 23 24 24 25 25 25 27

Current
Mean: 23.034 ms
Stdev: 2.197 ms (9.5%)
Runs: 19 19 20 20 20 20 20 21 21 21 21 21 21 21 21 22 22 22 22 22 22 22 22 22 22 23 23 23 23 23 23 23 23 23 23 23 24 24 24 24 24 24 24 25 25 25 25 25 25 25 25 26 26 26 26 28 28 29
App start regularAppStart Baseline
Mean: 0.014 ms
Stdev: 0.001 ms (5.2%)
Runs: 0.01253300067037344 0.012613999657332897 0.012736000120639801 0.012816999107599258 0.01285799965262413 0.01285799965262413 0.012980000115931034 0.013020999729633331 0.013141999952495098 0.013224000111222267 0.013224000111222267 0.013264999724924564 0.013386999256908894 0.013427999801933765 0.0134680001065135 0.013469000346958637 0.013590000569820404 0.013590999878942966 0.013590999878942966 0.013631000183522701 0.013631000183522701 0.013631000183522701 0.013632000423967838 0.013712000101804733 0.01375299971550703 0.013794000260531902 0.013794000260531902 0.013915999792516232 0.013915999792516232 0.013915999792516232 0.013956000097095966 0.013956000097095966 0.013956999406218529 0.014078999869525433 0.014078999869525433 0.014078999869525433 0.014119000174105167 0.014240999706089497 0.014281999319791794 0.014322999864816666 0.014363999478518963 0.014364000409841537 0.014403999783098698 0.014403999783098698 0.014527000486850739 0.014566999860107899 0.01460800040513277 0.014810999855399132 0.014810999855399132 0.014810999855399132 0.014852000400424004 0.014933000318706036 0.014933000318706036 0.0152580002322793 0.015298999845981598 0.015381000004708767 0.015381000004708767

Current
Mean: 0.017 ms
Stdev: 0.002 ms (9.3%)
Runs: 0.014038000255823135 0.014241000171750784 0.014241000171750784 0.014444999862462282 0.014567000325769186 0.01464799977838993 0.01464799977838993 0.014648000244051218 0.014851999934762716 0.014934000093489885 0.015056000091135502 0.015135999768972397 0.015137000009417534 0.015176999848335981 0.01525900000706315 0.015299999620765448 0.015422000084072351 0.015422000084072351 0.015503000002354383 0.015584000386297703 0.01582799991592765 0.015868999995291233 0.015949999913573265 0.01607199991121888 0.01615400006994605 0.016195000149309635 0.016195000149309635 0.016234999988228083 0.016316999681293964 0.016559999901801348 0.0166830001398921 0.01672299997881055 0.01680499967187643 0.016805000137537718 0.016846000216901302 0.017048999667167664 0.017130000051110983 0.017171999905258417 0.017211999744176865 0.017212000209838152 0.017374000046402216 0.017577999737113714 0.017619000282138586 0.017740999814122915 0.017821999732404947 0.017862999811768532 0.017862999811768532 0.01794400019571185 0.01798499980941415 0.01822900027036667 0.018432999961078167 0.018472999799996614 0.018553999718278646 0.0186769999563694 0.018798999954015017 0.0188400000333786 0.019327000249177217 0.0194089999422431 0.019897999707609415 0.020304000005126

@github-actions
Copy link
Contributor

@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker.

@OSBotify
Copy link
Contributor

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

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.92-4 🚀

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

@OSBotify
Copy link
Contributor

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

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

@OSBotify
Copy link
Contributor

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DeployBlockerCash This issue or pull request should block deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants