-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[No QA] Use Ping endpoint for internet reachability instead of API root #18460
Conversation
@mananjadhav @cristipaval 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] |
Removing C+ review because it's not possible for @mananjadhav to test this |
# Conflicts: # src/libs/NetworkConnection.js
Conflicts resolved and PR updated. |
Looks like this should have been on hold for https://github.com/Expensify/Web-Expensify/pull/37294 😅 |
Actually I don't understand how this calls the Ping endpoint so I'm not sure anymore 😅 |
@@ -82,7 +82,16 @@ function subscribeToNetInfo() { | |||
// When App is served locally (or from Electron) this address is always reachable - even offline | |||
// Using the API url ensures reachability is tested over internet | |||
reachabilityUrl: `${CONFIG.EXPENSIFY.DEFAULT_API_ROOT}api`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn this have the Ping command in the ULR?
🚀 Deployed to staging by https://github.com/cristipaval in version: 1.3.14-1 🚀
|
sorry for merging it. I did it because I saw that the title was updated and the hold was removed 😬 |
No worries, we're merging things manically for EC3 at the moment and it's not ideal. |
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.3.14-14 🚀
|
Shit, took this off HOLD too early. |
HOLD on https://github.com/Expensify/Web-Expensify/pull/37294
Details
Pings a new API endpoint that makes sure our database is reachable in addition to just our API's webserver. Note: This doesn't seem to affect Android. From the react-native-community/netinfo docs:
The native reachability APIs are generally much faster and more reliable than the polling mechanism used by web, so I'm not sure we should set
useNativeReachability=false
Fixed Issues
$ #9211
Tests
This must be tested by an Expensify engineer.
true
internetReachability
requests go toapi?command=Ping
. They should respond with ajsonCode
of200
. You should also see astatus
field ofUP
(though we're not using that.sudo systemctl stop auth
Ping
API commands should start responding with ajsonCode
of 502, and the app should switch to offline mode.sudo systemctl restart auth
Ping
should go back to responding with200
and the app should go back online.Offline tests
This is kind of all related to online/offline.
QA Steps
None
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
iOS.mov
Android