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 web app not recognising device being online on chromebook connected via instant tethering #6500

Merged

Conversation

eVoloshchak
Copy link
Contributor

@eVoloshchak eVoloshchak commented Nov 29, 2021

Details

This is a migration to 7.3.1 version of @react-native-community/netinfo, which accommodates this pull-request.

Fixed Issues

#4910

Tests

  1. Connect a chromebook to a phone via instant tethering
  2. Verify that there is a green dot on user's avatar indicating that user is online

QA Steps

  1. Open expensify app
  2. Verify that there is a green dot on user's avatar indicating that user is online
  3. Disconnect from the network
  4. Verify that there is a grey dot on user's avatar indicating that user is offline
  5. Connect to network via cellular data if applicable
  6. Verify that there is a green dot on user's avatar indicating that user is online

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

HP chromebook 11 G6

Screen.recording.2021-11-29.18.29.26.mp4

Note

We are migrating 2 major versions (from 5 to 7), so there could be some breaking changes. This needs to be properly tested on all platforms.

@eVoloshchak eVoloshchak requested a review from a team as a code owner November 29, 2021 16:21
@MelvinBot MelvinBot requested review from marcaaron and removed request for a team November 29, 2021 16:21
@marcaaron
Copy link
Contributor

This change looks good to me. I'm unsure why the GH actions are failing - but it doesn't look related to this.

@marcaaron
Copy link
Contributor

marcaaron commented Nov 29, 2021

This needs to be properly tested on all platforms. I only tested it for web, verifying that it works as expected on chromebook and in Chrome for Linux.

Would you mind listing out some tests in the removed "QA Steps" section? We have all PRs manually tested by our QA team and they will only test what we explicitly ask them to test. Just noticed it only mentions Chromebook there which I'm not sure they will be able to test. Thanks!

marcaaron
marcaaron previously approved these changes Nov 29, 2021
@eVoloshchak
Copy link
Contributor Author

eVoloshchak commented Nov 29, 2021

Would you mind listing out some tests in the removed "QA Steps" section? We have all PRs manually tested by our QA team and they will only test what we explicitly ask them to test.

I've renamed "Tests" section to "QA Steps". Those are the two steps I used to verify that the problem with instant tethering is resolved.
However, this PR also introduces a whole netinfo library update, which I did not test if works on other platforms and do not know the necessary steps to test properly.

Just noticed it only mentions Chromebook there which I'm not sure they will be able to test. Thanks!

It also has to be a fairly modern chromebook that supports instant tethering, not all of them are. This article is inaccurate.

@marcaaron
Copy link
Contributor

marcaaron commented Nov 29, 2021

I've renamed "Tests" section to "QA Steps". Those are the two steps I used to verify that the problem with instant tethering is resolved.

Tests is what your reviewer should do to verify the changes. QA Steps are for QA. They should not be combined.

this PR also introduces a whole netinfo library update, which I did not test if works on other platforms and do not know the necessary steps to test properly.

Mostly just test that offline is detected? The test is not much different. Just use chrome dev tools network tab, turn off wifi, turn off cell data, etc.

@eVoloshchak
Copy link
Contributor Author

Tests is what your reviewer should do to verify the changes. QA Steps are for QA. They should not be combined.

Added a separate QA steps entry

Mostly just test that offline is detected? The test is not much different. Just use chrome dev tools network tab, turn off wifi, turn off cell data, etc.

I've added these steps to QA. I've also tested it on mobile web, I don't have access to other platforms.

@marcaaron
Copy link
Contributor

Ah we need to run pod install on these changes to commit the updates there. Are you able to do this from your end?

@eVoloshchak
Copy link
Contributor Author

Ah we need to run pod install on these changes to commit the updates there. Are you able to do this from your end?

No, I don't have a mac
If that is critical, I can insall macOS inside a VM, but that would take a couple days to download

@marcaaron
Copy link
Contributor

Ok, are you able to test on Android? Let us know if we can help get you set up there.

I would be happy to cover the iOS testing and changes for you once this is merged.

@marcaaron
Copy link
Contributor

Tested on Desktop and iOS and verified everything is still working.

@eVoloshchak
Copy link
Contributor Author

Ok, are you able to test on Android? Let us know if we can help get you set up there.

I can do that, i'll need a couple of hours to set-up the environment

I would be happy to cover the iOS testing and changes for you once this is merged.

Thanks

@eVoloshchak
Copy link
Contributor Author

Tested on Android, everything works as expected

@marcaaron
Copy link
Contributor

Thanks @eVoloshchak let's :shipit:

@marcaaron marcaaron self-requested a review November 30, 2021 21:27
@marcaaron marcaaron merged commit 9c9b7c0 into Expensify:main Nov 30, 2021
@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.

@OSBotify
Copy link
Contributor

OSBotify commented Dec 7, 2021

🚀 Deployed to staging by @marcaaron in version: 1.1.17-8 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Dec 8, 2021

🚀 Deployed to production by @roryabraham in version: 1.1.18-3 🚀

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.

None yet

3 participants