Add new HOC to get policy.connections data#39132
Conversation
| // If the policy doesn't have the connections data when it has the accounting connections feature enabled, | ||
| // call the API to get the data | ||
| openPolicyAccountingPage(policy.id); |
There was a problem hiding this comment.
@aldo-expensify, is it accurate to say that the connections object is empty when the user hasn't connected to any accounting software?
If that is the case, this useEffect will keep calling openPolicyAccountingPage indefinitely for workspaces that have the accounting feature enabled but that haven't connected to any accounting software.
Do you have any suggestion on how we can distinguish the situation when the user has connected to the accounting software but the connections object hasn't populated? This is the only situation when we have to call openPolicyAccountingPage.
There was a problem hiding this comment.
@aldo-expensify, is it accurate to say that the connections object is empty when the user hasn't connected to any accounting software?
I just tested and it appears to me that the connections key is completely missing. I guess what you could do is create a local state with useState so you are sure that you only call openPolicyAccountingPage once. Something like
if (!loadedPolicyConnections) {
openPolicyAccountingPage();
setLoadedPolicyConnections(true);
}
There was a problem hiding this comment.
Aldo's plan would work, but it might result in more API requests than necessary.
An alternate thought I wad was that maybe in the policy summaries that get sent to NewDot we could include a new property like connectCount: 2 so that we know how many connections to expect. That way, you could determine the difference between not having any connections configured, and not having the connections loaded in Onyx yet.
There was a problem hiding this comment.
Thank you for the thoughtful and creative ideas, both 😄
For now, I'll go with @aldo-expensify's suggestion as it's easier to implement. As an optimization to mitigate unnecessary API calls, I'll create a GH issue to implement @tgolen's suggestion.
There was a problem hiding this comment.
Created a GH issue to implement Tim's solution: https://github.com/Expensify/Expensify/issues/383660
There was a problem hiding this comment.
What about instead of using a local state as I suggested, we use a context with a state, so if we open more pages that require the policy's connection settings, we can see that a different page already loaded it.
This is a sound idea. It addresses @tgolen's concerns about unnecessary API calls to a certain extent.
@tgolen, do you have any other concerns besides unnecessary API calls? I'm interested in understanding the potential issues your solution aims to resolve. 😄
There was a problem hiding this comment.
My main concerns are unnecessary API calls, yes. They do not have a zero cost and I think it is a very naive approach there is a better solution. Whatever you do here is going to put a pattern in place that engineers are going to follow from here on out and I think it's a bad pattern.
It sounds like @aldo-expensify is concerned that a piece of policy data would be complex to keep track of, but this is just like any other piece of policy that exists on the policy and there is nothing more complex that needs to be done here that we aren't doing with all of the other policy data.
There was a problem hiding this comment.
I think we should seek more opinions in engineering-chat, but meanwhile... I don't think the problem of "unnecessary API calls" is big enough to introduce this new piece of data. Why? The only cases that would happen are:
- User navigates to the main Accounting page: if
policy.connectionsis missing, it will trigger a fetch, but just one and will get nothing (there was noconnections). - User navigates to deeper Accounting page when there is no
policy.connections: this can only happen if the user is entering directly using the url, which I think is a rare case.
In cases where there is actually a connection, after we load it, we won't do further calls to the API
There was a problem hiding this comment.
Slack discussion: https://expensify.slack.com/archives/C03TQ48KC/p1712185841746469
There was a problem hiding this comment.
We came to the consensus that we should go with the approach to fetch the connections data once.
I'll modify the PR to use a Context as suggested by @aldo-expensify 👍
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
mollfpr
left a comment
mollfpr
left a comment
There was a problem hiding this comment.
Mostly the code looks good to me!
| ); | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
I don't get why we return nothing if the connections are false, I try to use it on the workspace profile page but it shows a blank page. Also, I'm curious about what case the HOC will be used.
There was a problem hiding this comment.
I added some comments in the new commit. It explains the usage of the HOC and the context that I also added in that commit.
There was a problem hiding this comment.
I don't get why we return nothing if the connections are false
@mollfpr, @aldo-expensify, should we return LoadingPage instead of null?
There was a problem hiding this comment.
We might be able to leverage this later. The only case I think we should return null is where we want to navigate the page if there are no connections to avoid rendering a component while animating the transition.
There was a problem hiding this comment.
I think we should should a spinner if policy.isLoadingConnections is true, and if policy.isLoadingConnections is false, hmmm 404? 🤷
|
@aldo-expensify, @mollfpr, I added a context in the new commit. Please review the PR again 🙇 The context was added to implement this suggestion |
|
Reassure test was failing. Merged the main. |
|
I feel like the new implementation with |
I completely agree with you, @tgolen! I decided to add a new Onyx collection key called I also removed the fields I added in previous commits 👍 |
|
@mollfpr, this PR is ready for another review |
mollfpr
left a comment
mollfpr
left a comment
There was a problem hiding this comment.
Mostly looks good to me!
| openPolicyAccountingPage(policy.id); | ||
| }, [hasConnectionsDataBeenFetched, policy]); | ||
|
|
||
| if (status === 'loading' || !hasConnectionsDataBeenFetched) { |
There was a problem hiding this comment.
This will always show the loader when the policy hasn't enabled the accounting feature. Should we let the OpenPolicyAccountingPage API be called even though the policy hasn't enabled the feature?
There was a problem hiding this comment.
Just to confirm, how is this accessible in the UI if accounting isn't enabled?
There was a problem hiding this comment.
@mollfpr, this page won't be acceesible for users who hasn't enabled the accounting feature.
Also, even if the user somehow be able to access this page, it won't cause the infinite loading state. hasConnectionsDataBeenFetched will be true if the fetch is attempted once (even if there is no data returned from the API call)
There was a problem hiding this comment.
also, just to be clear, the connections field might still be empty even if the user has enabled the accounting feature if the user hasn't connected to any accounting software
There was a problem hiding this comment.
This seems broken. I'm getting infinite loader when I try to visit the page again
Screen.Recording.2024-04-18.at.5.48.40.PM.mov
There was a problem hiding this comment.
@s77rt similar problem with going offline and then offline: #39132 (comment)
@hayata-suenaga is working on it here #40375
Co-authored-by: Luthfi <luthfi.ufi14@gmail.com>
|
@roryabraham Please 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] |
| import ONYXKEYS from '@src/ONYXKEYS'; | ||
|
|
||
| function openPolicyAccountingPage(policyID: string) { | ||
| const hasConnectionsDataBeenFetchedKey = `${ONYXKEYS.COLLECTION.POLICY_HAS_CONNECTIONS_DATA_BEEN_FETCHED}${policyID}` as const; |
There was a problem hiding this comment.
Why is as const necessary here?
There was a problem hiding this comment.
It looks like you removed it from a different spot. Is it still necessary here?
There was a problem hiding this comment.
The const is necessary here, otherwise the hasConnectionsDataBeenFetchedKey gets string as a type and then OnyxUpdate is not happy with such key:
There was a problem hiding this comment.
haha I definitely moved the const from a different place but it didn't cause the TS error 😆 for this particular one
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
| const finallyData: OnyxUpdate[] = [ | ||
| { | ||
| onyxMethod: Onyx.METHOD.MERGE, | ||
| key: hasConnectionsDataBeenFetchedKey, | ||
| value: true, | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
@hayata-suenaga After playing with this, I think you missed the failureData setting hasConnectionsDataBeenFetched to false or something like that?
If you do:
- Create a workspace
- Enable
Accounting, but don't got to the accounting settings yet - Disable your internet connection
- Open the
Accountingsettings
I would have expected that an Offline blocker would appear since we haven't loaded the policy connections, but it doesn't because hasConnectionsDataBeenFetched is set true even if OPEN_POLICY_ACCOUNTING_PAGE fails
There was a problem hiding this comment.
Here is the PR to fix the issue. Thank you for letting me know of the issue 🙇
|
🚀 Deployed to staging by https://github.com/aldo-expensify in version: 1.4.63-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.63-21 🚀
|
7 participants
cc: @tgolen @aldo-expensify
Details
This PR does two things:
OpenPolicyAccountingPageAPI commandpolicyobjectpolicy.connectionsis setpolicy.connectionsis not set, invoke the new action functionopenPolicyAccountingPageto get data that populatespolicy.connectionspolicy.connectionsis not set and being fetched, return the loading indicator.policy.connectionsis not set and the device is offline, display the offline screenWe also store a boolean value inside Onyx to remember if the connections data for the given policy has been attempted to be fetched in the current user session. If the data is fetched previously but the connections field of the policy object is
nullorundefined, that means that the data doesn't exist on the backend.Fixed Issues
$ #39108
PROPOSAL: N/A
Tests / Offline Tests / QA Steps
No QA. No test. This PR just adds the HOC component. This PR does not add a code to use the HOC. We can test the HOC in a PR that first uses it.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionBecause there is no way to test this PR, there is no screenshot to add
toggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps./** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop