Open on admin room

[madmax330]

Details

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/229659
PROPOSAL: GH_LINK_ISSUE(COMMENT)

Tests

1. Create a new account on OldDot
2. When you first login go to Settings > Policies > Group
3. Click on "Select" on the free plan card
4. You should be take to newDot with the new workspace settings open in the sidebar and the admins room chat focused in the background like so:

• Verify that no errors appear in the JS console

Offline tests

QA Steps

• Verify that no errors appear in the JS console
  Same as tests. Testing on Web should be enough since you cannot have oldDot easily on mobile

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

See tests steps

Mobile Web - Chrome

Screen.Recording.2023-02-03.at.7.21.06.PM.mov

Here we just make sure that we're brought to the workspace settings since there is no background chat shown

Mobile Web - Safari

Same as iOS, safari doesn't let you link to newDot

Desktop

Web doesn't link to desktop

iOS

Deep links apparently don't work in safari and I can't get chrome on the iOS simulator

https://github.com/Expensify/Web-Expensify/blob/7285e2e43820216804d80b91607732d1229448cd/homepage/js/signin.jsx#L1399

Android

Web doesn't deep link to android

[madmax330]

Discussing here whether or not there is a preferred way to do this: https://expensify.slack.com/archives/C03TQ48KC/p1664879283534009

[amyevans]

👍 I'll hold off on reviewing for now

[madmax330]

Alright @amyevans this is finally ready for review 😅

[madmax330]

This change is needed because if we don't have it, when the owner email is null it creates some console errors

[amyevans]

@madmax330 Sorry I'm a bit behind on PR reviews, I'll review tomorrow!

[amyevans]

Question: I see the openOnAdminRoom parameter is only passed when the workspace does not yet exist. Is there a reason we don't want to always land admins in the #admins room?

[madmax330]

Is there a reason we don't want to always land admins in the #admins room?

If it already exists then you've probably already been on newDot. So whatever chats you visited while you were there will take precedence over this parameter. Which is the expected behavior.

[amyevans]

The code LGTM. Testing was a journey, I essentially only had Web working, although that's where you landed too looks like 😄

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

Mobile Web - Chrome

N/A

Mobile Web - Safari

N/A

Desktop

N/A

iOS

Unless you toggle the setting to Allow popups, the UX of this is not good. Although I think it's more likely for user to access the deep link from the old app instead of mWeb. I tried to test in staging on the old app but to the user the link just appears unresponsive. Anyway, this isn't the concern of this PR directly, just an observation.

Android

N/A

[OSBotify]

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

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
App start TTI	685.509 ms → 698.224 ms (+12.715 ms, +1.9%)
App start runJsBundle	190.710 ms → 191.531 ms (+0.822 ms, ±0.0%)
App start regularAppStart	0.015 ms → 0.015 ms (-0.000 ms, -1.4%)
App start nativeLaunch	19.586 ms → 19.107 ms (-0.479 ms, -2.4%)
Open Search Page TTI	609.334 ms → 603.383 ms (-5.951 ms, -1.0%)

Show details

Name	Duration
App start TTI	Baseline Mean: 685.509 ms Stdev: 33.956 ms (5.0%) Runs: 630.5254669999704 637.7726159999147 642.3896019998938 645.9803969999775 647.61622599978 648.4377350001596 652.0362470000982 657.64848300023 660.1245099999942 660.7515429998748 666.1021360000595 667.2097740001045 670.5181320002303 670.6349590001628 679.0241249999963 684.8826609998941 688.5778780002147 691.7561300001107 693.2283120001666 695.2864859998226 700.9843959999271 704.6936570000835 707.4497710000724 714.3427720000036 714.6454329998232 718.501629000064 718.94319000002 727.6477749999613 747.8618339998648 750.5996090001427 754.6200439999811 Current Mean: 698.224 ms Stdev: 24.873 ms (3.6%) Runs: 657.1707310001366 659.2049270002171 660.6183850001544 662.7616420001723 664.7284439997748 671.7642899998464 673.1614520000294 677.1822709999979 684.7871220000088 687.0225860001519 689.3508939999156 692.3202069997787 697.6594610000029 697.9591879998334 698.4554460002109 702.0992859997787 702.9396959999576 703.703261999879 704.8318900000304 706.3682699999772 710.9727300000377 712.340239000041 714.0983040002175 717.3588769999333 721.1559509998187 725.9299369999208 726.9361849999987 731.0975410002284 732.6887969998643 760.062992000021
App start runJsBundle	Baseline Mean: 190.710 ms Stdev: 22.081 ms (11.6%) Runs: 159 162 163 167 169 170 170 171 172 173 175 176 181 183 183 184 191 194 197 197 197 202 206 210 211 214 219 221 222 228 245 Current Mean: 191.531 ms Stdev: 20.754 ms (10.8%) Runs: 162 163 164 165 168 170 170 171 173 177 180 184 185 185 186 187 188 192 196 198 200 200 206 207 210 212 212 213 214 220 222 249
App start regularAppStart	Baseline Mean: 0.015 ms Stdev: 0.001 ms (9.3%) Runs: 0.012817999813705683 0.013305999804288149 0.013346000108867884 0.013346999883651733 0.0134680001065135 0.013712000101804733 0.013753000181168318 0.013793999794870615 0.013996999710798264 0.014038000255823135 0.014078999869525433 0.014241999946534634 0.014281999785453081 0.014403999783098698 0.014444999862462282 0.01444500032812357 0.014485000167042017 0.014688999857753515 0.01472900016233325 0.014851999934762716 0.014892000239342451 0.015176999848335981 0.015300000086426735 0.015584000386297703 0.01570699969306588 0.015951000154018402 0.016439000144600868 0.017212000209838152 0.017456000205129385 0.017821999732404947 0.018309999722987413 Current Mean: 0.015 ms Stdev: 0.001 ms (6.8%) Runs: 0.012817999813705683 0.013427999801933765 0.013549999799579382 0.01355000026524067 0.01375299971550703 0.013793999794870615 0.01383400009945035 0.0138349998742342 0.013874999713152647 0.01399700017645955 0.014038000255823135 0.014200999867171049 0.014200999867171049 0.014362999703735113 0.01436399994418025 0.014485000167042017 0.014526000246405602 0.014688999857753515 0.015056000091135502 0.01509599993005395 0.015096000395715237 0.015096000395715237 0.015381000004708767 0.015422000084072351 0.015503000002354383 0.015542999841272831 0.015705999918282032 0.016112999990582466 0.0165200000628829 0.017374999821186066
App start nativeLaunch	Baseline Mean: 19.586 ms Stdev: 1.352 ms (6.9%) Runs: 17 18 18 18 18 19 19 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 21 21 21 21 22 22 23 Current Mean: 19.107 ms Stdev: 1.113 ms (5.8%) Runs: 17 18 18 18 18 18 18 18 18 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 20 21 21 22
Open Search Page TTI	Baseline Mean: 609.334 ms Stdev: 23.762 ms (3.9%) Runs: 572.9414059999399 577.9859219999053 578.9807130000554 579.7051999997348 580.1993420002982 586.4326980002224 591.5865879999474 594.1936040003784 596.1190590001643 596.6075440002605 597.3440350000747 597.682618000079 598.6924230000004 600.1178390001878 600.2060139998794 600.3279220000841 601.3910730001517 603.2956140004098 607.8378510000184 608.2508139996789 611.01147400029 611.1825360003859 623.35730100004 624.1156009999104 625.669637999963 627.8288579997607 630.4492999999784 632.9443770004436 635.7576089999638 636.4719639997929 643.5627040001564 663.9541009999812 671.8132319999859 Current Mean: 603.383 ms Stdev: 16.228 ms (2.7%) Runs: 574.3457439998165 581.615601000376 581.9508870001882 587.4599609998986 587.956828000024 588.2260340000503 588.2455649999902 588.3404540000483 588.4113360000774 589.9152830000967 590.3538819998503 593.0178629998118 596.652710000053 596.9397379998118 598.7132159997709 601.1324050002731 601.1391189997084 604.1133630000986 606.5072829998098 607.7293699998409 608.2007240001112 608.9543459997512 609.8949380000122 611.8609620002098 614.124836999923 615.0015460001305 615.9666750002652 618.1891680001281 625.1722419997677 626.8408619998954 629.022828000132 632.5450029997155 643.0964770000428

[OSBotify]

🚀 Deployed to staging by https://github.com/amyevans in version: 1.2.67-0 🚀

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

[OSBotify]

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

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