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

[CP Staging] Fix issue where displayed selected date is a day before actually selected date #30810

Merged
merged 3 commits into from
Nov 2, 2023

Conversation

jasperhuangg
Copy link
Contributor

@jasperhuangg jasperhuangg commented Nov 2, 2023

Details

We recently migrated this over from momentjs to vanilla JS date objects, and since vanilla JS date objects take into account the timezone when you construct dates when passing an ISO string, we need to use parseISO instead so that doesn't happen.

Fixed Issues

$ #30807
PROPOSAL:

Tests/QA

  1. Go to any chat.
  2. Tap on "+" button.
  3. Tap on "Request money".
  4. Select "Manual".
  5. Enter any amount and tap "Next".
  6. Tap on "Show more".
  7. Tap on the date field.
  8. Tap on any enabled day.
  9. The displayed selected date should be the same as the one you selected.

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 / Chrome
    • iOS / native
    • iOS / 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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2023-11-02.at.4.25.22.PM.mov
MacOS: Desktop

@jasperhuangg jasperhuangg self-assigned this Nov 2, 2023
@jasperhuangg jasperhuangg marked this pull request as ready for review November 2, 2023 23:37
@jasperhuangg jasperhuangg requested a review from a team as a code owner November 2, 2023 23:37
@melvin-bot melvin-bot bot removed the request for review from a team November 2, 2023 23:37
Copy link

melvin-bot bot commented Nov 2, 2023

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

@melvin-bot melvin-bot bot requested a review from situchan November 2, 2023 23:37
@jasperhuangg jasperhuangg removed the request for review from situchan November 2, 2023 23:38
@jasperhuangg jasperhuangg changed the title Fix issue where displayed selected date is a day before actually selected date [CP Staging] Fix issue where displayed selected date is a day before actually selected date Nov 2, 2023
@NikkiWines NikkiWines self-requested a review November 2, 2023 23:53
@NikkiWines
Copy link
Contributor

NikkiWines commented Nov 2, 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

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2023-11-02.at.16.54.27.mov
MacOS: Desktop
Screen.Recording.2023-11-02.at.16.54.27.mov

Copy link
Contributor

@NikkiWines NikkiWines left a comment

Choose a reason for hiding this comment

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

Looks good!

@NikkiWines NikkiWines merged commit bd1ecb3 into main Nov 2, 2023
12 checks passed
@NikkiWines NikkiWines deleted the jasper-fixDateRegression branch November 2, 2023 23:58
@OSBotify
Copy link
Contributor

OSBotify commented Nov 3, 2023

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

OSBotify pushed a commit that referenced this pull request Nov 3, 2023
[CP Staging] Fix issue where displayed selected date is a day before actually selected date

(cherry picked from commit bd1ecb3)
@OSBotify
Copy link
Contributor

OSBotify commented Nov 3, 2023

🚀 Cherry-picked to staging by https://github.com/jasperhuangg in version: 1.3.95-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.

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

github-actions bot commented Nov 3, 2023

Performance Comparison Report 📊

Significant Changes To Duration

Name Duration
App start TTI 1064.447 ms → 1225.626 ms (+161.179 ms, +15.1%) 🔴
App start runJsBundle 724.810 ms → 847.661 ms (+122.850 ms, +16.9%) 🔴
Show details
Name Duration
App start TTI Baseline
Mean: 1064.447 ms
Stdev: 35.461 ms (3.3%)
Runs: 1015.0473970000166 1015.8291189998854 1016.519922999898 1022.0414670000318 1022.6069469999056 1023.0283590001054 1026.3610789999366 1028.270259999903 1029.0741040001158 1029.6408720000181 1030.7424029998947 1032.616672999924 1032.749562999932 1033.804944000207 1038.6330830000807 1038.7367140001152 1039.5922360001132 1040.490617999807 1041.437857999932 1041.8528209999204 1044.1092179999687 1048.2507559999358 1051.2067259999458 1051.3765420001 1053.241738999961 1055.2868170000147 1055.4350459999405 1055.5459610000253 1055.6394849999342 1058.4305199999362 1059.2932369997725 1064.7374019999988 1065.3333089998923 1065.4601080000866 1065.6514109999407 1066.4031849999446 1068.5427590000909 1069.1505470001139 1069.6234110000078 1069.7488619999494 1075.388995999936 1081.393849000102 1081.6516700000502 1083.5458269999363 1086.8922639999073 1087.2298230000306 1092.0072709999513 1093.9298799999524 1096.3322970001027 1100.5084949999582 1102.5222300000023 1114.768928000005 1135.5741260000505 1136.2992799999192 1137.1855480000377 1141.4837480001152 1147.0892829999793 1152.5784730000887

Current
Mean: 1225.626 ms
Stdev: 32.727 ms (2.7%)
Runs: 1151.8584790001623 1159.4986020000651 1166.7976810000837 1194.3820130000822 1195.2165580000728 1195.3429049998522 1195.9629719997756 1196.8149709999561 1198.1715310001746 1198.3274539997801 1198.3659279998392 1199.2596289999783 1199.6720650000498 1203.4502440001816 1203.631421000231 1204.12760599982 1204.202227000147 1210.3057120000012 1211.5429940000176 1213.1322369999252 1214.1757390000857 1215.3481239997782 1215.5012929998338 1218.0993659999222 1218.666071999818 1219.117591000162 1219.8254689997993 1220.0580119998194 1220.0713410000317 1220.2188499998301 1224.2688629999757 1226.2265570000745 1229.1891990001313 1231.577169999946 1232.6868590000086 1234.2121749999933 1235.4302550000139 1237.2160979998298 1238.4322709999979 1244.1254540001974 1247.5065299998969 1247.969568000175 1248.7790649998933 1249.7373689999804 1250.139119000174 1259.1635599997826 1261.2843200000934 1273.253634000197 1274.548427999951 1280.161758999806 1280.7206489997916 1283.3149879998527 1294.4386430000886 1318.2945289998315
App start runJsBundle Baseline
Mean: 724.810 ms
Stdev: 27.011 ms (3.7%)
Runs: 686 686 687 689 691 692 693 693 694 694 694 695 702 705 707 707 708 708 709 709 714 714 715 717 718 718 718 718 719 720 720 721 722 726 728 729 730 731 732 734 738 738 740 740 740 744 751 753 755 759 762 763 763 767 768 784 786 795

Current
Mean: 847.661 ms
Stdev: 23.849 ms (2.8%)
Runs: 789 793 811 818 819 820 821 826 827 829 830 830 832 833 833 833 834 834 835 835 836 841 841 841 842 843 845 846 846 847 847 847 848 850 851 853 853 854 854 857 859 859 859 869 870 871 877 879 879 880 885 886 889 892 895 896

Meaningless Changes To Duration

Show entries
Name Duration
App start nativeLaunch 20.914 ms → 22.055 ms (+1.141 ms, +5.5%)
App start regularAppStart 0.017 ms → 0.015 ms (-0.001 ms, -7.8%)
Open Search Page TTI 710.006 ms → 705.118 ms (-4.888 ms, -0.7%)
Show details
Name Duration
App start nativeLaunch Baseline
Mean: 20.914 ms
Stdev: 1.534 ms (7.3%)
Runs: 18 19 19 19 19 19 19 19 19 20 20 20 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 21 21 22 22 22 22 22 22 22 22 23 23 23 23 23 24 24 25 25

Current
Mean: 22.055 ms
Stdev: 1.313 ms (6.0%)
Runs: 20 20 20 20 21 21 21 21 21 21 21 21 21 21 21 21 21 21 21 21 22 22 22 22 22 22 22 22 22 22 22 22 22 22 22 22 22 22 22 22 23 23 23 23 23 23 23 24 24 24 24 24 24 26 26
App start regularAppStart Baseline
Mean: 0.017 ms
Stdev: 0.001 ms (5.9%)
Runs: 0.014405000023543835 0.015339999925345182 0.015421000076457858 0.015421000076457858 0.015462000155821443 0.015544000081717968 0.015706999925896525 0.015746999997645617 0.01574800000526011 0.01599099999293685 0.016112999990582466 0.016112999990582466 0.016153999837115407 0.01615400006994605 0.01615400006994605 0.016193999908864498 0.016276000067591667 0.016276000067591667 0.016316999914124608 0.01631700014695525 0.016398000065237284 0.016399000072851777 0.016438999911770225 0.016519999830052257 0.01656099990941584 0.016601999988779426 0.016642000060528517 0.016642000060528517 0.016764000058174133 0.016764000058174133 0.016765000065788627 0.016765000065788627 0.016765000065788627 0.016927000135183334 0.016967999981716275 0.017008000053465366 0.017048999899998307 0.017048999899998307 0.01704900013282895 0.01721199997700751 0.01729299989528954 0.01729299989528954 0.017293000128120184 0.017455999739468098 0.017497000051662326 0.017537000123411417 0.017578000202775 0.0179449999704957 0.017985000042244792 0.018065999960526824 0.01847400004044175 0.0185139998793602 0.01900200010277331 0.019164999946951866 0.019409999949857593

Current
Mean: 0.015 ms
Stdev: 0.001 ms (4.2%)
Runs: 0.013957000337541103 0.014119999948889017 0.014160000253468752 0.014525999780744314 0.014607999939471483 0.014649000018835068 0.014689999632537365 0.014730000402778387 0.014810999855399132 0.014851999934762716 0.014892000239342451 0.014933999627828598 0.014973999932408333 0.015015000011771917 0.015054999850690365 0.015054999850690365 0.015054999850690365 0.015135999768972397 0.015136000234633684 0.015136000234633684 0.015217999927699566 0.01525900000706315 0.015298999845981598 0.01537999976426363 0.015381000004708767 0.015381000004708767 0.015381000004708767 0.015421000309288502 0.015422000084072351 0.015422000084072351 0.015463000163435936 0.015542999841272831 0.015544000081717968 0.015625 0.015625 0.015625 0.015625 0.015625 0.015746999997645617 0.015746999997645617 0.01582799991592765 0.01590899983420968 0.015910000074654818 0.015910000074654818 0.015949999913573265 0.01599099999293685 0.016032000072300434 0.016194999683648348 0.01623500045388937 0.0163569999858737 0.016398000065237284 0.016478999983519316 0.0165200000628829 0.016561000142246485 0.016601999755948782 0.016642000060528517 0.016682999674230814
Open Search Page TTI Baseline
Mean: 710.006 ms
Stdev: 43.667 ms (6.2%)
Runs: 638.2527679996565 643.1054690000601 645.353637999855 646.4010830000043 646.4360759998672 648.0929769999348 659.431478000246 661.8139239996672 662.4362790002488 666.8572599999607 668.1463220003061 668.5993660003878 668.7554119997658 675.8383790003136 675.9767669998109 676.0263269999996 676.8789880000986 678.0350350001827 678.1145029999316 683.2846279996447 683.8438319996931 686.5254730000161 686.860514999833 687.3117680000141 689.0483810002916 690.5345049998723 691.4700930002145 692.1749680000357 697.0515950000845 698.1483559999615 699.3313400000334 705.9494630000554 707.7703050002456 709.6604010001756 715.2089840001427 715.890788000077 718.1862790002488 723.6699629998766 724.2384440000169 731.9071049997583 732.748292000033 733.7575689996593 743.3117680000141 746.209838999901 747.2919919998385 749.1116950004362 749.1680910000578 753.8118090000935 754.400349999778 755.4398190001957 756.5757659999654 761.4288330003619 761.7317709997296 764.3034669999033 767.2394619998522 770.4761959998868 772.9909679996781 774.2862559999339 775.3263759999536 791.6703699999489 826.472413000185

Current
Mean: 705.118 ms
Stdev: 41.489 ms (5.9%)
Runs: 619.8350419998169 623.2923590000719 623.3253170000389 635.9208180001006 638.4473069999367 639.2728679999709 640.2255049999803 654.9811610002071 656.7845869995654 658.8946539992467 662.8798829996958 663.9446210004389 671.4463710002601 673.0600179992616 673.9973550001159 674.5061449999921 674.7663169996813 676.4911710000597 679.6255700001493 679.7229819996282 686.3865569997579 688.7121179997921 689.3264570003375 692.3830570001155 696.6082769995555 697.1433509998024 697.3684079991654 698.1780190002173 698.554036999587 713.0859789997339 713.9771320000291 714.0240890001878 714.5751540008932 722.9499109992757 723.3074139999226 724.0832519996911 724.2772630001418 725.0200190003961 727.2279049996287 727.3074139999226 728.406534999609 731.2493899995461 731.3037109998986 735.8000090001151 736.6898609995842 737.5062669999897 738.1608889997005 738.2458910001442 740.5477710003033 743.1898600002751 743.404378999956 744.5534669999033 748.291259999387 753.9045819998719 757.0330809997395 758.9053960004821 760.1292730001733 760.6970219993964 761.8894450003281 768.3638920001686 798.0117999999784

Copy link
Contributor

github-actions bot commented Nov 3, 2023

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

@OSBotify
Copy link
Contributor

OSBotify commented Nov 6, 2023

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

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.97-7 🚀

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.

3 participants