-
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
Refactor: Create libs/ApiUtils #15178
Conversation
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
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.
I've pointed out some usages of URL_API_ROOT
that might be confusing or be set by mistake to the production API root, because URL_API_ROOT
always points there
cc @tgolen @marcaaron if you would want to review this too 🙌 |
@Santhosh-Sellavel @ctkochan22 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] |
Added description, tests and QA tests and screenshots ✅ Ready for Review We still need to decide what to do about the remaining direct usages of |
Just found there is this PR as well which is a bit stepping on our toes here https://github.com/Expensify/App/pull/14944/files |
@mountiny Are we still reviewing this PR? Or wait until yours is merged? |
@kidroca Can you please update the test steps here, instead of links? |
Done, but I think we should wait for the resolution of the PR here (which might call for additional changes on my PR): |
Cool, then can you update the title HOLD For the PR? |
Put this on hold, @ctkochan22 sorry was ooo on monday and tuesday |
@mountiny Looks like you got a handle on this review. I'm going to unassign and defer to you! |
d34a426
to
bb0b788
Compare
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.
This PR is ready for review and testing ✅
Made some changes to be compatible with other PRs touching the same functionality
It was possible to remove the platform specific implementations for ApiUtils
sub functionality
We are no longer blocked by:
- Use app environment to determine olddot URL and update staging env #14944 - no overlapping changes
- [Staging API] To Use Staging Server or not to use staging server #15517 - agreed to move with my PR first on slack
Updated PR description and test sections to indicated the "Staging toggle" can be used in DEV
It would cause CORS issues (only on DEV), because the proxy server changes are not there yet. They will be handled in the follow of #15517
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.
Looks pretty good. Just left some small ideas for improvements.
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.
Added a few detailed explanations, sorry if it's a bit too long
I can address most of the suggestions right now
But for a few I'll need a response from @marcaaron
src/libs/ApiUtils.js
Outdated
} | ||
|
||
if (typeof stagingServerToggleState !== 'boolean') { | ||
return ENV_NAME === CONST.ENVIRONMENT.STAGING; |
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.
Yes, though see this
https://github.com/Expensify/App/blob/bb0b7883210a9aba510adf682363004d536e29b4/src/libs/ApiUtils.js#L10-L16
we want the default value for staging to be true
, but we can't simply do this
let stagingServerToggleState = ENV_NAME === CONST.ENVIRONMENT.STAGING;
because the real ENV_NAME is resolved a few moments later
We can only do this:
let stagingServerToggleState = false;
Onyx.connect({
key: ONYXKEYS.USER,
callback: (val) => {
const defaultValue = ENV_NAME === CONST.ENVIRONMENT.STAGING;
stagingServerToggleState = lodashGet(val, 'shouldUseStagingServer', defaultValue);
},
});
It's probably fine, but ENV_NAME is obtained async and it might not be updated in the Onyx.connect callback
IMO the safest is what we have right now
What do you think?
5e1b11b
to
802f97b
Compare
@kidroca Is this test "Verify absolute urls are resolved from API ROOT" really required? The attachments urls will always be in a different (and correct) format due to the use of |
Besides that, tests well!, checklist in the way... |
Reviewer Checklist
Screenshots/VideosWebweb.mp4Desktopdesktop.mp4 |
This comment was marked as off-topic.
This comment was marked as off-topic.
I'm not sure / I don't know - This PR if a followup to make
I'm not sure how did you test that
Screen.Recording.2023-03-12.at.11.53.20.mov
That's correct, the followup to fix this is waiting on the current PR |
I think it's related to how the browser handle requests while offline, check the video for Web from the checklist above, I went offline differently than how you did. I think that's outside the scope of this PR since same behaviour is on staging and it may be actually a browser thing so nothing to worry about here. Just to be sure can you do the steps I have and confirm the same results |
@kidroca Yeah, sorry my bad, I tend to disable cache for testing. So everything is actually as expected, as can be seen on the Desktop video. Edit: I have updated the Web video |
Move the logic inside the Onyx callback
Co-authored-by: Abdelhafidh Belalia <16493223+s77rt@users.noreply.github.com>
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.
LGTM! 🚀
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.
@kidroca one comment, thank I will merge this, thanks!
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.
Lets do this
@mountiny Do we track payment for this in the linked issue or in another separated issue? |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/mountiny in version: 1.2.87-0 🚀
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.2.87-1 🚀
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.2.87-1 🚀
|
Details
ApiUtils
andApiUtils.getApiRoot
Why?
https://expensify.slack.com/archives/C01GTK53T8Q/p1676320010253979
We build native apps using the PROD env config
The API is resolved dynamically at run time
But we also have the constant
URL_API_ROOT
which cannot always point to the correct API (it points to PROD)Some ENVs (staging) allow for toggling between different APIs and a function like
getApiRoot
encapsulatesthe checks we have to make when we want to use the root url
The staging toggle now works correctly on DEV and it can switch between the PRIMARY and STAGING APIs
Fixed Issues
$ #15087
PROPOSAL: N/A
Tests
Verify staging and prod urls are resolved from API ROOT
Verify absolute urls are resolved from API ROOT
At the moment the backend is not yet saving attachments using the absolute path, so we need to simulate the result
We need to inspect and modify Onyx data
Find an attachment action and modify the
html
of the message so that the<img src="https://{env}.expensify.com/chat-attachments/{path}" />
part becomes<img src="/chat-attachments/{path}" />
Example
In Chrome dev console:Now refresh the browser. The (modified) attachment should resolve and load successfully from API ROOT
Verify all attachment usages are covered. All these places should resolve the attachment from API ROOT
Settings / Preferences / Test Preferences - staging toggle works correctly on DEV and STAGING
Using the "Staging toggle" changes the attachment URLs
staging.expensify.com/chat-attachments/...
(or whatever STAGING_EXPENSIFY_URL address is configured in .env)Pusher functionality should be unaffected
AddressSearch
component functionality was refactored, verifyGooglePlacesAutocomplete
has no regression regarding therequestUrl
parameter. The fields below should show autocomplete suggestionsOffline tests
Expected Offline behavior
Expected slow internet behavior
QA Steps
Testing on STAGING
staging.expensify.com/chat-attachments/...
www.expensify.com/chat-attachments/...
AddressSearch
component functionality was refactored, verify address can still be autocompleteTesting on PRODUCTION
After PR is merged to PROD
www.expensify.com/chat-attachments/...
AddressSearch
component functionality was refactored, verify address can still be autocompletePR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)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.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android