Always request version.json from network (no cache)

[kidroca]

Details

As explained in the issue comment the only usage for HttpUtils.download was in the version check here
Instead of modifying existing code to make HttpUtils.download support a cache/no-cache options, we use fetch directly
Since now HttpUtils.download has no more usages it can be deleted
A problem with the webpack dev config was fixed so that the fix can be tested locally

Fixed Issues

$ #7856

Tests

To test locally modify src/setup/platformSetup/index.website.js so that this

App/src/setup/platformSetup/index.website.js

Lines 63 to 66 in 1c4add7

	// When app loads, get current version (production only)
	if (Config.IS_IN_PRODUCTION) {
	checkForUpdates(webUpdater());
	}

becomes:

    // When app loads, get current version (production only)
    checkForUpdates(webUpdater());

1. dist/version.json is created automatically by the webpack dev server - you should have this file after running npm run web
2. Background the app or switch to another tap
3. Switch back to App

• Verify that no errors appear in the JS console
• Verify a network request is being made to fetch ./version.json
• Verify nothing happens since the fetched version is the same as the currentVersion

1. Modify dist/version.json to be a greater version than currentVersion
2. Background the app or switch to another tap
3. Switch back to App

• Verify that no errors appear in the JS console
• Verify a network request is being made to fetch ./version.json
  • since we expect App to refresh, because a new version is available we should, toggle to preserve Network log between refreshes
  • alternatively you can put a breakpoint inside the code that receive the new version, and code should be paused there after you switch back to the tab. This would also help catch the "Update Available" prompt, since now App would be in the foreground
• Depending on how fast you switch back and forth between tabs, App can either refresh on the background, or prompt you to confirm refreshing. See the videos under Web that showcase both cases
  • Switching between tabs fast should show you an "Update available prompt like this"
    
  • Backgrounding the app and waiting for a few seconds should initiate a refresh in the background - you should be able to see a spinner briefly appearing on the collapsed 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

I think this ticket can only be tested after deploying to staging, because we need to be on older version and a new version be released in the meantime

We're primarily verifying there are no regressions and the update check works as expected
All the scenarios below are existing flows and not introduced in the current PR
We're verifying they work as expected

On web/mobile-web

• Verify new version is detected during background the app

  1. Have App opened and foregrounded before an update is released
  2. Wait for update to be released and then background App - open a new tab
  3. Observer App's tab is refreshing (little spinning circle appears on the tab in Chrome)
  4. Switching back to App's tab you should be on the update version

• Verify new version is detected during foregrounding the app

  • This happens only if you're background very quickly (see attached video)
  1. Have App opened and in the background before an update is released
  2. Wait for update to be released and then foreground App
  3. You should see a prompt like this:
     
  4. Pressing "OK" should refresh the page
  5. You should now be on the updated version

• Verify new version is detected during foregrounding the app - alternative

  • This happens only if you're background very quickly (see attached video)
  • Do the 1,2,3 from above, but instead of pressing "OK" on the prompt press "Cancel"
  • The page should not refresh
  • If you background App now it should refresh in the background and the next time you bring it up it would have the updated version

• Verify problems due to cache are fixed (the bug in the linked ticket)

  1. Shortly (1-2 minutes, up to 30) after an update have been published (on staging or prod) open the web page
  2. Verify you're on the latest version
  3. Verify that opening a new tab or backgrounding the app, does not result in App refresh - app doesn't incorrectly think there's an update
  4. Verify that opening a new tab or backgrounding the app, doesn't result in showing the "Update available prompt"

Screenshots

Web

• Note: I've setup my local environment to simulate there's always a new version

New version refreshes in the background

Screen.Recording.2022-03-30.at.3.11.01.mov

New version prompts to refresh page on the foreground

Screen.Recording.2022-03-30.at.3.17.45.mov

Mobile Web

New version available and refreshes App

Screen.Recording.2022-03-30.at.3.19.43.mov

No new version available - no page refresh

Screen.Recording.2022-03-30.at.3.20.28.mov

Android Chrome

Android.Emulator.-.Pixel_2_API_29_5554.2022-03-30.03-32-40.mp4

Desktop

iOS

Android

[kidroca]

I haven't checked some PR check items, because I'm not certain what I should do.
Waiting to be clarified in this slack thread: https://expensify.slack.com/archives/C01GTK53T8Q/p1648583058174529

[luacmartins]

Hmm I'm following the test steps, I changed the version in dist/version.json from {"version":"1.1.46-3"} to {"version":"1.1.47-3"} but I don't get the prompt to update the app nor do I see a new request for version.json. Am I missing something?

test.mov

[kidroca]

@luacmartins

Hmm I'm following the test steps, I changed the version in dist/version.json from {"version":"1.1.46-3"} to {"version":"1.1.47-3"} but I don't get the prompt to update the app nor do I see a new request for version.json. Am I missing something?

About the prompt it doesn't always appear - I've updated the test steps for QA, and I thought I did for DEV, but apparently not

Regarding the request not appearing in the network tab - it's because App refreshes in the background and you have to enable the [x] preserve log toggle here:

1. App sees there's a newer version
2. App sees you've backgrounded it and it refreshes (which "applies" the update)
3. Refreshing clears the Network tab

I'll update the Test steps, to match QA's

[kidroca]

@luacmartins
I've updated the description and the Test steps

[luacmartins]

Thanks for the updated testing steps! Left a small comment.

[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 @luacmartins in version: 1.1.51-0 🚀

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

[mvtglobally]

@kidroca @luacmartins @roryabraham
We had hard time validating this issue as we dont see the exact time the deploy is happening and team tried our best to stop the build changes across devices.
So far we could only verify Desktop and it looks pass. Web, mWeb and Native apps did not have proper transition and it may be due to the build distribution.
Would you be able to cross check this internally?

[roryabraham]

First off – this only affects web AFAIK not other platforms

[roryabraham]

I went ahead and ran this snippet on staging web:

And honestly, that's good enough for me. I'm going to check this off on the checklist.

[roryabraham]

We'll go ahead and assume that the documentation for fetch is accurate, and that it's so fundamental to JavaScript that we don't need to do anything more to validate that it's working.

[kidroca]

Yes, this should only affect web
I think the only way to test it in prod is to wait for an update, if there's some schedule or someone you can contact to synchronize the test it would help
Otherwise I would just keep a incognito browser open, then wait to be notified that a staging/prod build was deployed (#8350 (comment)) and go to that incognito window

If there's a slack channel that posts a message when a build is starting it can be used to open web while the old version is still actual and then do the test steps after the deploy finishes

[OSBotify]

🚀 Deployed to production by @roryabraham in version: 1.1.51-0 🚀

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

[OSBotify]

🚀 Deployed to production by @roryabraham in version: 1.1.51-0 🚀

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