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 linking to report for staging URLs #14487

Merged
merged 4 commits into from
Jan 31, 2023
Merged

Fix linking to report for staging URLs #14487

merged 4 commits into from
Jan 31, 2023

Conversation

jasperhuangg
Copy link
Contributor

@jasperhuangg jasperhuangg commented Jan 23, 2023

Details

The code here handles linking to specific reports and wasn't correctly capturing staging URLs. This updates it to also handle staging URLs.

Fixed Issues

$ #12594

Tests

  1. Copy the contents of .env.staging into .env
  2. Sign out of all your sessions on OldDot and NewDot in your browser.
  3. Run the app (on Desktop, iOS, and Android). Sign with the following credentials:
    • Username: nonphotoblue@pfortunezk.com
    • Password: asdfASDF00
  4. Click the "CHAT WITH YOUR SETUP SPECIALIST" link sent by Concierge.
  5. Verify the report opens in the same client, and doesn't open any sign in links.

Staging QA

  1. Sign with the following credentials.
    • Username: nonphotoblue@pfortunezk.com
    • Password: asdfASDF00
  2. Click the "CHAT WITH YOUR SETUP SPECIALIST" link sent by Concierge.
  3. Verify the report opens in the same client, and doesn't open any sign in links.

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

Only applicable locally on Desktop, iOS, and Android due to CORS errors on Web platforms.

Desktop
Screen.Recording.2023-01-30.at.3.54.10.PM.mov
iOS
Screen.Recording.2023-01-30.at.4.13.12.PM.mov
Android
Screen.Recording.2023-01-30.at.4.17.56.PM.mov

@jasperhuangg jasperhuangg self-assigned this Jan 23, 2023
@jasperhuangg jasperhuangg marked this pull request as ready for review January 23, 2023 22:12
@jasperhuangg jasperhuangg requested a review from a team as a code owner January 23, 2023 22:12
@jasperhuangg jasperhuangg changed the title Fix linking to account for staging URLs Fix linking to report for staging URLs Jan 23, 2023
@melvin-bot melvin-bot bot requested review from amyevans and Santhosh-Sellavel and removed request for a team January 23, 2023 22:13
@melvin-bot
Copy link

melvin-bot bot commented Jan 23, 2023

@Santhosh-Sellavel @amyevans 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]

@jasperhuangg jasperhuangg requested review from alex-mechler and removed request for Santhosh-Sellavel January 23, 2023 22:13
@alex-mechler
Copy link
Contributor

This updates it to also handle NewDot URLs.

Should this say staging?

Copy link
Contributor

@alex-mechler alex-mechler left a comment

Choose a reason for hiding this comment

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

Actually, your QA steps should be updated. If the link is clicked in desktop, it should be opened in the desktop app as well. In addition, please provide videos of you testing this

@Santhosh-Sellavel
Copy link
Collaborator

PR is failing for me, I set up the environment as STAGING in .env.

Still navigates to the old dot

Screen.Recording.2023-01-24.at.10.39.30.PM.mov

@alex-mechler does it passes for you?

cc: @jasperhuangg

@jasperhuangg
Copy link
Contributor Author

Hey guys sorry, I should have left this on draft, was about to test it but something came up last minute. I'm gonna put this back on draft for the time being.

@jasperhuangg jasperhuangg marked this pull request as draft January 24, 2023 17:50
Copy link
Collaborator

@Santhosh-Sellavel Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

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

Adding myself as a reviewer!

@jasperhuangg
Copy link
Contributor Author

I'm testing this locally and I can't seem to get it to navigate me to OldDot (without my changes), it just opens the report in the same client as expected. I'm checking to see if there were any PRs merged that aren't on staging yet (since this behavior still happens on staging).

Screen.Recording.2023-01-30.at.3.40.23.PM.mov

@Santhosh-Sellavel
Copy link
Collaborator

@jasperhuangg try setting environment as staging locally then perform test steps

@jasperhuangg
Copy link
Contributor Author

jasperhuangg commented Jan 30, 2023

Figured it out, thanks @Santhosh-Sellavel I thought I had set up my .env correctly for staging but turns out it wasn't.

Working on screenshots now. I think we should be able to provide screen recordings for this for all platforms except Web since we'll get CORS errors when trying to run the staging env on Web.

@jasperhuangg jasperhuangg marked this pull request as ready for review January 31, 2023 00:19
@jasperhuangg
Copy link
Contributor Author

@Santhosh-Sellavel @alex-mechler thanks for your patience, ready for review!

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Jan 31, 2023

@jasperhuangg Just asking for confirmation should it open admins chat or 1:1 with a dedicated specialist?

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Jan 31, 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

Desktop
Screen.Recording.2023-01-31.at.7.26.17.AM.mov
iOS
Simulator.Screen.Recording.-.iPhone.14.-.2023-01-31.at.07.39.40.mp4
Android
Screen.Recording.2023-01-31.at.7.28.11.AM.mov

Copy link
Collaborator

@Santhosh-Sellavel Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

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

LGTM, tests well!

@jasperhuangg
Copy link
Contributor Author

jasperhuangg commented Jan 31, 2023

@jasperhuangg Just asking for confirmation should it open admins chat or 1:1 with a dedicated specialist?

@Santhosh-Sellavel I'm honestly not entirely sure, I just know it's supposed to navigate directly to the report, I'm not changing how the URL is generated (that's handled in the back-end).

Based on how there's a message from what seems like a setup specialist I think that behavior is expected though. @alex-mechler can you confirm?

@alex-mechler
Copy link
Contributor

Based on how there's a message from what seems like a setup specialist I think that behavior is expected though. @alex-mechler can you confirm?

Correct, the communication between the guide (setup specialist) and the customer will largely be in the #admins room, as then other admins can also see the conversation. It is expected to go to #admins as a result


All you @amyevans

Copy link
Contributor

@amyevans amyevans 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!

@amyevans amyevans merged commit 5759b47 into main Jan 31, 2023
@amyevans amyevans deleted the jasper-fixDeepLinking branch January 31, 2023 18:57
@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
Copy link
Contributor

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 710.717 ms → 737.661 ms (+26.944 ms, +3.8%)
App start runJsBundle 194.806 ms → 199.333 ms (+4.527 ms, +2.3%)
App start nativeLaunch 19.500 ms → 20.069 ms (+0.569 ms, +2.9%)
App start regularAppStart 0.015 ms → 0.016 ms (+0.001 ms, +7.7%)
Open Search Page TTI 612.191 ms → 608.006 ms (-4.184 ms, -0.7%)
Show details
Name Duration
App start TTI Baseline
Mean: 710.717 ms
Stdev: 30.299 ms (4.3%)
Runs: 661.5672210007906 666.397818999365 675.8399669993669 678.4208170007914 679.7005679998547 681.4345559999347 689.1018160004169 689.170252000913 693.459525000304 694.6946150008589 697.2971439994872 697.847712000832 698.3147580008954 699.4979120008647 700.5435869991779 702.8368050009012 704.3935369998217 704.8055489994586 704.8399720005691 711.7771429996938 714.6837109997869 717.3716620001942 719.977949000895 722.7192829996347 732.2157770004123 734.6624529995024 740.606954999268 752.1272950004786 754.2807100005448 754.4277040008456 778.6614900007844 789.2753190007061

Current
Mean: 737.661 ms
Stdev: 33.828 ms (4.6%)
Runs: 688.4626299999654 694.928300999105 695.7525319997221 696.0049770008773 696.1569609995931 697.0496440008283 698.5014880001545 699.3062040004879 707.8459239993244 715.4588820002973 716.7962359990925 721.8938469998538 726.130134999752 727.3707449994981 734.681649999693 737.7826240006834 740.311005000025 740.8067770004272 742.348782999441 745.5448339991271 747.4020920004696 747.6704309992492 751.7031100001186 759.2594859991223 765.0501290000975 767.2161870002747 778.656849000603 779.6657870002091 782.3053830005229 795.957759000361 797.0204809997231 810.1024329997599
App start runJsBundle Baseline
Mean: 194.806 ms
Stdev: 15.863 ms (8.1%)
Runs: 172 173 173 175 178 180 183 184 185 185 187 187 188 188 188 192 193 194 195 199 200 200 203 204 209 211 212 222 225 225 229

Current
Mean: 199.333 ms
Stdev: 17.128 ms (8.6%)
Runs: 167 171 173 182 183 185 186 189 190 193 194 194 197 198 198 199 200 201 202 203 203 204 208 209 210 219 220 224 232 246
App start nativeLaunch Baseline
Mean: 19.500 ms
Stdev: 1.581 ms (8.1%)
Runs: 17 17 18 18 18 18 18 18 18 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 21 21 21 21 22 22 22 24

Current
Mean: 20.069 ms
Stdev: 2.067 ms (10.3%)
Runs: 17 18 18 18 18 19 19 19 19 19 19 19 19 19 19 19 20 20 20 21 21 21 21 21 22 23 23 25 26
App start regularAppStart Baseline
Mean: 0.015 ms
Stdev: 0.001 ms (6.4%)
Runs: 0.012817000970244408 0.013630999252200127 0.013752998784184456 0.013754000887274742 0.013875000178813934 0.013955999165773392 0.014118999242782593 0.014240998774766922 0.014404000714421272 0.01452699862420559 0.014688998460769653 0.01476999931037426 0.014770999550819397 0.014770999550819397 0.014852000400424004 0.014893000945448875 0.01493300125002861 0.015137000009417534 0.01521800085902214 0.01534000039100647 0.015461999922990799 0.015543999150395393 0.015665000304579735 0.015665000304579735 0.015666000545024872 0.015828000381588936 0.016114000231027603 0.016397999599575996 0.01664300076663494 0.0170499999076128

Current
Mean: 0.016 ms
Stdev: 0.001 ms (5.7%)
Runs: 0.01452699862420559 0.014810999855399132 0.014852000400424004 0.014932999387383461 0.015015000477433205 0.01505500078201294 0.015137000009417534 0.015258001163601875 0.015625 0.015665000304579735 0.015706999227404594 0.015828000381588936 0.015951000154018402 0.01607299968600273 0.016114000231027603 0.01615399867296219 0.016154000535607338 0.016234999522566795 0.016315998509526253 0.016439000144600868 0.016439000144600868 0.016642000526189804 0.016642998903989792 0.016682999208569527 0.0167239997535944 0.017130998894572258 0.01753699965775013 0.017537999898195267 0.01769999973475933 0.018147999420762062
Open Search Page TTI Baseline
Mean: 612.191 ms
Stdev: 16.561 ms (2.7%)
Runs: 587.7056480012834 593.0411370005459 593.4158120006323 594.4024659991264 594.9972339998931 597.3501390013844 597.6797690000385 597.8192549999803 598.8074140008539 599.9420579988509 601.5611169990152 603.6812749989331 603.9226480014622 604.6128339990973 605.0991620011628 607.0477700009942 611.1664220001549 613.6553549990058 615.6381839998066 615.8031419999897 617.3416339997202 618.8395180013031 619.6960450001061 619.92264899984 621.3802079986781 621.7088219989091 623.019612999633 630.1419280003756 632.2722980007529 641.910237999633 652.3470869995654 654.1781009994447

Current
Mean: 608.006 ms
Stdev: 14.301 ms (2.4%)
Runs: 582.8473709989339 586.2100830003619 586.4007569998503 591.9512940011919 592.9703379999846 594.0679940003902 594.1788330003619 595.9868980012834 596.2055660001934 596.5658369995654 597.628866000101 597.8050539996475 598.949869999662 603.5952960010618 603.6237389985472 605.5211190003902 606.5373940002173 608.7705079987645 609.8168540000916 615.3111979998648 615.6480299998075 616.3377279993147 617.2036540005356 618.0710450001061 618.2702229991555 620.7429609987885 620.8083500005305 621.578288000077 625.2687179986387 626.297729998827 629.9089770000428 630.1063230000436 639.0251879990101

@OSBotify
Copy link
Contributor

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

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

@OSBotify
Copy link
Contributor

OSBotify commented Feb 1, 2023

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.63-0 🚀

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.

5 participants