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] Get route.params.openOnAdminRoom if exists #14955

Merged
merged 2 commits into from
Feb 8, 2023

Conversation

marcochavezf
Copy link
Contributor

cc @mountiny @madmax330

Details

When we enter a route to validate the secondary login (e.g http://staging.new.expensify.com/v/12116/WSSWMNNMH), the app goes to the ValidateLogin page first, and that page navigates to the home/root directory, which just pops whatever screen is on top (and thus the route.params are lost):

Screenshot 2023-02-08 at 14 05 23

Fixed Issues

$ #14948

Tests

  1. Logout of NewDot
  2. Log into OldDot with any account and add a Secondary Login from Account Settings e.g. [email protected]
  3. Open validate secondary log in email Verify secondary email for Expensify and copy the last part of the URL, i.e. /v/16/WSSWMNNM (Everything after the /v/)
  4. Enter the URL from step 3 in the browser e.g. http://staging.new.expensify.com/v/12116/WSSWMNNMH
  5. Login with the secondary account and verify the app is not crashing
  • Verify that no errors appear in the JS console

Offline tests

N/A this only happens right after the browser loads the validate link

QA Steps

Same as Test section

  • 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 / 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 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 correct English and 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
    • 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 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 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.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
Screen.Recording.2023-02-08.at.14.16.37.mov
Mobile Web - Chrome
Screen.Recording.2023-02-08.at.14.20.35.mov
Mobile Web - Safari
Screen.Recording.2023-02-08.at.14.19.07.mov
Desktop

N/A validate link can only be opened on web

iOS

N/A validate link can only be opened on web

Android

N/A validate link can only be opened on web

@marcochavezf marcochavezf requested a review from a team as a code owner February 8, 2023 20:24
@marcochavezf marcochavezf self-assigned this Feb 8, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2023

⚠️ ⚠️ Heads up! This pull request has the CP Staging label ⚠️ ⚠️
If you applied the CP Staging label before the PR was merged, the PR will be be immediately deployed to staging even if the open StagingDeployCash deploy checklist is locked.
However if you applied the CP Staging after the PR was merged it's possible it won't be CP'ed automatically. If you need it to be CP'ed to staging, tag a member of @Expensify/mobile-deployers to CP it manually, otherwise you can wait for it to go out with the next deploy.

@melvin-bot melvin-bot bot requested review from Gonals and mananjadhav and removed request for a team February 8, 2023 20:25
@MelvinBot
Copy link

@mananjadhav @Gonals One of you needs to 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]

@madmax330
Copy link
Contributor

madmax330 commented Feb 8, 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 / Chrome
    • iOS / native
    • iOS / 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 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 correct English and 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 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 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.
  • 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-02-08.at.9.57.36.PM.mov
Mobile Web - Chrome
Screen.Recording.2023-02-08.at.9.53.29.PM.mov
Mobile Web - Safari
Screen.Recording.2023-02-08.at.9.48.28.PM.mov
Desktop

Same as above

iOS

Same as above

Android

Same as above

@madmax330 madmax330 merged commit 05f5c49 into main Feb 8, 2023
@madmax330 madmax330 deleted the marco-checkRouteParams branch February 8, 2023 22:06
@melvin-bot melvin-bot bot added the Emergency label Feb 8, 2023
@MelvinBot
Copy link

@madmax330 looks like this was merged without a test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

@madmax330
Copy link
Contributor

Tests were passing

OSBotify pushed a commit that referenced this pull request Feb 8, 2023
[CP Staging] Get route.params.openOnAdminRoom if exists

(cherry picked from commit 05f5c49)
OSBotify added a commit that referenced this pull request Feb 8, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2023

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 720.741 ms → 735.958 ms (+15.217 ms, +2.1%)
App start runJsBundle 195.833 ms → 200.469 ms (+4.635 ms, +2.4%)
App start regularAppStart 0.015 ms → 0.018 ms (+0.003 ms, +17.1%) 🟡
App start nativeLaunch 20.097 ms → 20.000 ms (-0.097 ms, ±0.0%)
Open Search Page TTI 615.596 ms → 611.653 ms (-3.943 ms, -0.6%)
Show details
Name Duration
App start TTI Baseline
Mean: 720.741 ms
Stdev: 35.407 ms (4.9%)
Runs: 658.8582250000909 665.9498950000852 667.6902040001005 674.9255280001089 685.3966230000369 688.5323030001018 690.4570190000813 691.0935619999655 693.0969249999616 693.6226870000828 695.6626629999373 696.1158529999666 708.4304329999723 708.5582950001117 715.8283669999801 719.826008999953 722.0219550000038 726.0746520000976 731.4491560000461 731.9589549999218 734.885629999917 736.3363489999902 743.648051999975 744.6075200000778 750.1351520000026 750.5744179999456 751.4521719999611 758.606235000072 768.4881400000304 781.3353069999721 786.9209950000513 791.1667919999454

Current
Mean: 735.958 ms
Stdev: 33.422 ms (4.5%)
Runs: 681.7816820000298 685.9370099999942 688.7276709999423 689.3199489999097 689.6886140001006 693.2103760000318 697.0398319999222 698.7547840001062 715.8413730000611 717.7084500000346 720.970871000085 721.2010719999671 727.154070999939 727.2527940000873 729.136545999907 732.5993099999614 743.9623900000006 746.2883230000734 747.6001919999253 749.574821999995 749.7212169999257 751.7946250000969 759.8764849998988 762.1277209999971 763.3799540000036 763.4149750000797 768.6445730000269 774.3825749999378 779.814810999902 782.4695959999226 785.7417109999806 805.5229080000427
App start runJsBundle Baseline
Mean: 195.833 ms
Stdev: 19.678 ms (10.0%)
Runs: 165 172 172 173 176 178 179 181 181 182 182 185 187 190 190 195 197 198 200 201 204 207 209 210 212 216 229 232 234 238

Current
Mean: 200.469 ms
Stdev: 18.085 ms (9.0%)
Runs: 176 176 178 179 179 181 183 183 186 188 189 190 190 193 193 196 197 199 204 207 208 211 212 215 216 219 219 220 227 229 230 242
App start regularAppStart Baseline
Mean: 0.015 ms
Stdev: 0.001 ms (6.0%)
Runs: 0.013265000190585852 0.013590000104159117 0.013752999948337674 0.01395700010471046 0.01432300009764731 0.014526000013574958 0.014527000021189451 0.014567000092938542 0.014648000011220574 0.01472900016233325 0.0147299999371171 0.0147299999371171 0.014811000088229775 0.0148930000141263 0.01513699977658689 0.015299999853596091 0.015421999851241708 0.015503000002354383 0.015625 0.015666000079363585 0.015746999997645617 0.0157880000770092 0.015909999841824174 0.015910000074654818 0.01611399999819696 0.016235999995842576 0.01631700014695525 0.016357000218704343 0.016642000060528517 0.01692699990235269

Current
Mean: 0.018 ms
Stdev: 0.001 ms (7.7%)
Runs: 0.015217999927699566 0.015421999851241708 0.01603100006468594 0.01607199991121888 0.016275999834761024 0.016276000067591667 0.0165200000628829 0.016764000058174133 0.016804999904707074 0.01688600005581975 0.01688600005581975 0.017089999979361892 0.017374000046402216 0.01741599990054965 0.017578000202775 0.017660000128671527 0.017699999967589974 0.017782000126317143 0.01814799988642335 0.018309999955818057 0.01835100003518164 0.018352000042796135 0.018595000030472875 0.0188400000333786 0.019001999869942665 0.019001999869942665 0.01904299994930625 0.019084000028669834 0.019694000016897917 0.020060000009834766 0.02026300015859306 0.020508000161498785
App start nativeLaunch Baseline
Mean: 20.097 ms
Stdev: 1.907 ms (9.5%)
Runs: 18 18 18 18 18 18 18 19 19 19 19 19 19 19 19 20 20 20 20 20 21 21 21 21 21 22 23 23 23 24 25

Current
Mean: 20.000 ms
Stdev: 1.965 ms (9.8%)
Runs: 18 18 18 18 18 18 19 19 19 19 19 19 19 19 19 19 20 20 20 21 21 21 21 21 22 22 22 25 26
Open Search Page TTI Baseline
Mean: 615.596 ms
Stdev: 19.343 ms (3.1%)
Runs: 586.2425130000338 591.9544280001428 593.68737900001 596.2899169998709 598.2973229999188 598.3474940001033 598.4533689999953 598.6049810000695 600.5206299999263 601.7120769999456 603.5368249998428 603.54776999983 604.2159430000465 604.799885999877 606.1433920001145 609.1542160001118 613.7424720001873 614.4372149999253 615.1918949999381 615.3918459999841 620.4583340000827 621.3538410000037 623.800048999954 631.074300999986 631.4223229999188 631.7755950000137 634.3743900000118 638.0048020000104 647.2349859999958 653.5183110001963 653.5323489999864 658.2458500000648

Current
Mean: 611.653 ms
Stdev: 25.465 ms (4.2%)
Runs: 573.0076909998897 585.5229899999686 586.0803640000522 587.2917889999226 587.4777830000967 587.5653079999611 587.6033530000132 589.206135999877 594.4554850000422 594.847250000108 595.1534019999672 597.3566090001259 597.803588999901 598.7364099998958 600.569376999978 600.7570390000474 604.6424970000517 605.3521729998756 606.9566659999546 608.2419839999638 608.941366000101 614.1683760001324 615.5694989999756 620.6331789998803 622.8704429999925 633.965412999969 636.4223639999982 652.7069510000292 652.7197670000605 653.4074309999123 658.216633999953 660.0594079999719 666.2270520001184

@OSBotify
Copy link
Contributor

OSBotify commented Feb 8, 2023

🚀 Cherry-picked to staging by https://github.com/madmax330 in version: 1.2.67-7 🚀

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.

@OSBotify
Copy link
Contributor

OSBotify commented Feb 8, 2023

🚀 Deployed to production by https://github.com/mountiny in version: 1.2.67-7 🚀

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

@OSBotify
Copy link
Contributor

🚀 Cherry-picked to staging by https://github.com/AndrewGable in version: 1.3.28-2 🚀

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.

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/AndrewGable in version: 1.3.28-5 🚀

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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants