Open old dot link with openOldDotLink()

[mateusbra]

Details

We are going to use the openOldDotLink() function when we have oldDot links, it will open the oldDot link with our account logged in.

Fixed Issues

$ #7909
$ #8943

Tests

Test 1 - old dot links

1. Launch the app.
2. Open the report with Concierge.
3. Click on the "Request a call" link sent by Concierge.
4. Verify that the link opens in old dot with the user logged in.

Test 2 - new dot links

1. Launch the app.
2. Open any report.
3. Send this link - https://new.expensify.com/statements/123
4. Verify that clicking on the link navigates the app to a screen within the app.

Test 3 - Attachments

1. Launch the app.
2. Open any report.
3. Send a video, photo and pdf attachment.
4. Verify that there are no regressions related to viewing, sending, downloading etc.

Test 4 - Other links

1. Open any report.
2. Send a link eg: http://google.com/
3. The link should open in a new tab.

PR Review Checklist

Contributor (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 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 included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • iOS / native
  • Android / native
  • iOS / Safari
  • Android / Chrome
  • MacOS / Chrome
  • 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 was added in all src/languages/* files
  • I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team 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
• 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 */
  • Any functional components have the displayName property
  • 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
• 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.

PR Reviewer Checklist

• 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 verified tests pass on all platforms & I tested again on:
  • iOS / native
  • Android / native
  • iOS / Safari
  • Android / Chrome
  • MacOS / Chrome
  • 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 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 was added in all src/languages/* files
  • I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team 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 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 */
  • Any functional components have the displayName property
  • 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 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.

QA Steps

Test 1 - old dot links

1. Launch the app.
2. Open the report with Concierge.
3. Click on the "Request a call" link sent by Concierge.
4. Verify that the link opens in old dot with the user logged in.

Test 2 - new dot links

1. Launch the app.
2. Open any report.
3. Send this link - https://new.expensify.com/statements/123
4. Verify that clicking on the link navigates the app to a screen within the app.

Test 3 - Attachments

1. Launch the app.
2. Open any report.
3. Send a video, photo and pdf attachment.
4. Verify that there are no regressions related to viewing, sending, downloading etc.

Test 4 - Other links

1. Open any report.
2. Send a link eg: http://google.com/
3. The link should open in a new tab.

Screenshots

Web

Gravacao.de.Tela.2022-04-26.as.23.00.06.mov

Mobile Web

2022-04-25.17-31-12.mp4

Gravacao.de.Tela.2022-04-27.as.01.38.07.mov

Desktop

Gravacao.de.Tela.2022-04-27.as.00.54.15.mov

iOS

Ios.mov

Android

2022-04-25.17-26-39.mp4

[rushatgabhane]

@mateusbra please change the title of PR to something like 'Open old dot link with openOldDotLink()'

In tests, the video doesn't need to be sent to Concierge. It can be sent to any user.

1. Launch the app
2. Open the report with Concierge <----- this
3. Send a video attachment

[rushatgabhane]

@mateusbra a few issues

Actual: Clicking download on a video on web opens it in a new tab.
Expected: Video starts downloading.

Actual: On mWeb, clicking download on a video does nothing.
Expected: Video start downloading.

screen-20220523-025215.mp4

[mateusbra]

@rushatgabhane I noticed this happenned, I think its a regression from another PR, it was also reproducible running main on dev, could you confirm that for me?

[rushatgabhane]

I noticed this happenned, I think its a regression from another PR

@mateusbra please report the bug on slack if you haven't already.

---

This bug isn't present on main

1. Send a video
2. Right click to open context menu
3. Download

Expected: video downloads
Actual: nothing happens (no API request is made)

Screen.Recording.2022-05-23.at.12.03.50.PM.mov

[mateusbra]

I wasn't able to reproduce this bug from my side.
This video shows what happens to me running openOldDotLinksRegression after merging main(didn't tested this specific bug before merging it), I think its working fine, am I missing something?

2022-05-23.12-25-56.mp4

cc: @rushatgabhane

@mateusbra please report the bug on slack if you haven't already.

I'll report that on slack, thank you.
reported here: https://expensify.slack.com/archives/C01GTK53T8Q/p1653322086360229

[rushatgabhane]

@iwiznia LGTM! 🎉

#### PR Reviewer Checklist

• 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 verified tests pass on all platforms & I tested again on:
  • iOS / native
  • Android / native
  • iOS / Safari
  • Android / Chrome
  • MacOS / Chrome
  • 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 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 was added in all src/languages/* files
  • I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team 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 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 */
  • Any functional components have the displayName property
  • 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 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.

[rushatgabhane]

I wasn't able to reproduce this bug from my side.

@mateusbra yeah you're right. I can't replicate this bug after merging with main.

Also, thanks for logging the regression on slack :)

[rushatgabhane]

@mateusbra could you please reformat the tests and QA steps to something like this? I think it's more easier to read.

Test 1 - old dot links

1. Launch the app.
2. Open the report with Concierge.
3. Click on the "Request a call" link sent by Concierge.
4. Verify that the link opens in old dot with the user logged in.

Test 2- new dot links

1. Open any report
2. Send this link - https://new.expensify.com/statements/123
3. Verify that clicking on the link navigates the app to a screen within the app ().

Test 3 - Attachments

1. Open any report.
2. Send a video, photo and pdf attachment.
3. Verify that there are no regressions related to viewing, sending, downloading etc.

Test 4 - Other links

1. Open any report.
2. Send a link eg: http://google.com
3. The link should open in a new tab.

[mateusbra]

tests updated!

[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.

[OSBotify]

🚀 Deployed to staging by @iwiznia in version: 1.1.66-0 🚀

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

[Julesssss]

Hi @rushatgabhane, @mateusbra -- it looks like this PR caused this regression. Would you mind taking a look?

[rushatgabhane]

Update from #9158 (comment): we aren't sure if it's a deploy blocker.

J. Nam thinks it might be a configuration issue on staging, and that we should re-test it on production. This should be fine since we aren't using statements yet.

My exfy account doesn't have access to wallet, so I see a white screen both on staging and production.

[OSBotify]

🚀 Deployed to production by @chiragsalian in version: 1.1.66-1 🚀

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

[mateusbra]

I don't think its a regression, we only changed how old dot links works in the PR.

[iwiznia]

The timeline also doesn't line up, the issue was opened 2 days ago and says it was reproducible in production, this PR was not in production 2 days ago

[rushatgabhane]

and says it was reproducible in production

@iwiznia I think that's a mistake or some misunderstanding

Because J. Nam cannot reproduce the issue on production - #9158 (comment)

[iwiznia]

Oh... if that's the case, then it is probable this was the cause. @mateusbra can you check it out? Should be easy to test, replicate the bug on main, then try to replicate it with this PR reverted.

[rushatgabhane]

@iwiznia our emails (eg: rushatgabhane [at] gmail.com) don't have access to wallets so we always see a white screen. We can't really test the bug without access to wallets

[iwiznia]

Oh... I don't have access either 😢
Do you need to actually have a wallet set up or just have access to the feature? Because if it's the latter, I can give you temporary access, if it is the former I probably can't because only people with an SSN from USA can enable the feature.

[rushatgabhane]

@iwiznia I have no idea about wallets 😅
@thienlnam can you please help us out with #8980 (comment) (because you have wallets set up)

[mateusbra]

I couldn't reproduce the bug neither on main or this PR reverted on prod. It navigates me to this blank screen(it doesn't open new tab on browser):

On staging it opens on a new tab, on prod it doesn't.

[thienlnam]

I don't think its a regression, we only changed how old dot links works in the PR.

The statements page is an OldDot link btw.

Yeah I've tested this on production again, and haven't run into problems so I think this was just a configuration issue on staging that cause it not to work.

Doesn't seem like there is a regression here

[rushatgabhane]

Doesn't seem like there is a regression here

Thanks a lot for confirming!