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

Add new HOC to get policy.connections data #39132

Merged
merged 37 commits into from
Apr 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
06e5ea4
chore: add parameter type for OpenPolicyAccountingPage
Mar 27, 2024
c945585
chore: add new command
Mar 27, 2024
e0532d5
chore: add new action func to call web-e command
Mar 27, 2024
e6292e3
feat: add new HOC component
Mar 27, 2024
b509d0f
fix: conditional statement & change the parameter from object to sing…
Mar 28, 2024
8e32fde
fix: check if the connections feature is enabled & add comments
Mar 28, 2024
d8139a8
doc: add and improve comments
Mar 29, 2024
9a44aeb
doc: add comment on the purpose of the new state
Mar 29, 2024
cf2cce6
doc: remove unnecessary comments
Mar 29, 2024
2370278
chore: create a context
Apr 4, 2024
db3252e
fix: import react
Apr 4, 2024
0eca9cd
Merge branch 'main' into hayata-add-new-hoc
Apr 5, 2024
b7ab9e0
Merge branch 'main' into hayata-add-new-hoc
Apr 11, 2024
1b09ba0
chore: remove the context
Apr 12, 2024
32e979b
chore: use withPolicyConnectionsHOC
Apr 12, 2024
6f5d49b
feat: add field for loading status inside policy
Apr 15, 2024
14c8e92
feat: add new field to Policy object
Apr 15, 2024
357ce25
chore: update the fetching status field when taking the API request
Apr 15, 2024
d9340cf
chore: check the fetched and loading statuses before making the api r…
Apr 15, 2024
68db14d
refactor: remove unused statuses
Apr 15, 2024
4ba46b9
Merge branch 'main' into hayata-add-new-hoc
Apr 15, 2024
2254f3e
feat: add a new Onyx key
Apr 17, 2024
b1272d4
chore: remove fetch state from the policy object
Apr 17, 2024
d29646d
chore: remove unused field from the policy object
Apr 17, 2024
124f534
fix: delete non-existing variable
Apr 17, 2024
c1e0cac
fix: change the new key to the collections
Apr 17, 2024
515833e
feat: use the fetched state
Apr 17, 2024
f21307a
chore: update the fetched state
Apr 17, 2024
a44ab1c
chore: display loading indicator while data is being fetched
Apr 17, 2024
170cf3a
chore: ensure to display offline screen
Apr 17, 2024
f25a3e5
fix: getting the actual value and status from useOnyx
Apr 17, 2024
31cd1ec
Merge branch 'main' into hayata-add-new-hoc
Apr 17, 2024
9457554
fix: update the prop list
Apr 17, 2024
c4593e7
add missing underscore at the end of the Onyx collection key name
Apr 17, 2024
4804fd1
fix: default policy id to 0
Apr 17, 2024
e19060b
doc: specify the reason why the loading state can't be inside the pol…
Apr 17, 2024
a084e52
fix: remove as const statement
Apr 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/ONYXKEYS.ts
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,11 @@ const ONYXKEYS = {
POLICY_RECENTLY_USED_CATEGORIES: 'policyRecentlyUsedCategories_',
POLICY_TAGS: 'policyTags_',
POLICY_RECENTLY_USED_TAGS: 'nvp_recentlyUsedTags_',
// Whether the policy's connection data was attempted to be fetched in
// the current user session. As this state only exists client-side, it
// should not be included as part of the policy object. The policy
// object should mirror the data as it's stored in the database.
POLICY_HAS_CONNECTIONS_DATA_BEEN_FETCHED: 'policyHasConnectionsDataBeenFetched_',
OLD_POLICY_RECENTLY_USED_TAGS: 'policyRecentlyUsedTags_',
WORKSPACE_INVITE_MEMBERS_DRAFT: 'workspaceInviteMembersDraft_',
WORKSPACE_INVITE_MESSAGE_DRAFT: 'workspaceInviteMessageDraft_',
Expand Down Expand Up @@ -525,6 +530,7 @@ type OnyxCollectionValuesMapping = {
[ONYXKEYS.COLLECTION.POLICY_CATEGORIES]: OnyxTypes.PolicyCategories;
[ONYXKEYS.COLLECTION.POLICY_TAGS]: OnyxTypes.PolicyTagList;
[ONYXKEYS.COLLECTION.POLICY_RECENTLY_USED_CATEGORIES]: OnyxTypes.RecentlyUsedCategories;
[ONYXKEYS.COLLECTION.POLICY_HAS_CONNECTIONS_DATA_BEEN_FETCHED]: boolean;
[ONYXKEYS.COLLECTION.DEPRECATED_POLICY_MEMBER_LIST]: OnyxTypes.PolicyEmployeeList;
[ONYXKEYS.COLLECTION.WORKSPACE_INVITE_MEMBERS_DRAFT]: OnyxTypes.InvitedEmailsToAccountIDs;
[ONYXKEYS.COLLECTION.WORKSPACE_INVITE_MESSAGE_DRAFT]: string;
Expand Down
5 changes: 5 additions & 0 deletions src/libs/API/parameters/OpenPolicyAccountingPageParams.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
type OpenPolicyAccountingPageParams = {
policyID: string;
};

export default OpenPolicyAccountingPageParams;
1 change: 1 addition & 0 deletions src/libs/API/parameters/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -210,3 +210,4 @@ export type {default as ConvertTrackedExpenseToRequestParams} from './ConvertTra
export type {default as ShareTrackedExpenseParams} from './ShareTrackedExpenseParams';
export type {default as CategorizeTrackedExpenseParams} from './CategorizeTrackedExpenseParams';
export type {default as LeavePolicyParams} from './LeavePolicyParams';
export type {default as OpenPolicyAccountingPageParams} from './OpenPolicyAccountingPageParams';
2 changes: 2 additions & 0 deletions src/libs/API/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,7 @@ const READ_COMMANDS = {
OPEN_POLICY_WORKFLOWS_PAGE: 'OpenPolicyWorkflowsPage',
OPEN_POLICY_DISTANCE_RATES_PAGE: 'OpenPolicyDistanceRatesPage',
OPEN_POLICY_MORE_FEATURES_PAGE: 'OpenPolicyMoreFeaturesPage',
OPEN_POLICY_ACCOUNTING_PAGE: 'OpenPolicyAccountingPage',
} as const;

type ReadCommand = ValueOf<typeof READ_COMMANDS>;
Expand Down Expand Up @@ -497,6 +498,7 @@ type ReadCommandParameters = {
[READ_COMMANDS.OPEN_POLICY_WORKFLOWS_PAGE]: Parameters.OpenPolicyWorkflowsPageParams;
[READ_COMMANDS.OPEN_POLICY_DISTANCE_RATES_PAGE]: Parameters.OpenPolicyDistanceRatesPageParams;
[READ_COMMANDS.OPEN_POLICY_MORE_FEATURES_PAGE]: Parameters.OpenPolicyMoreFeaturesPageParams;
[READ_COMMANDS.OPEN_POLICY_ACCOUNTING_PAGE]: Parameters.OpenPolicyAccountingPageParams;
};

const SIDE_EFFECT_REQUEST_COMMANDS = {
Expand Down
38 changes: 38 additions & 0 deletions src/libs/actions/PolicyConnections.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import Onyx from 'react-native-onyx';
import type {OnyxUpdate} from 'react-native-onyx';
import * as API from '@libs/API';
import type {OpenPolicyAccountingPageParams} from '@libs/API/parameters';
import {READ_COMMANDS} from '@libs/API/types';
import ONYXKEYS from '@src/ONYXKEYS';

function openPolicyAccountingPage(policyID: string) {
const hasConnectionsDataBeenFetchedKey = `${ONYXKEYS.COLLECTION.POLICY_HAS_CONNECTIONS_DATA_BEEN_FETCHED}${policyID}` as const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is as const necessary here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix: a084e52

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you removed it from a different spot. Is it still necessary here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The const is necessary here, otherwise the hasConnectionsDataBeenFetchedKey gets string as a type and then OnyxUpdate is not happy with such key:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha I definitely moved the const from a different place but it didn't cause the TS error 😆 for this particular one


const optimisticData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: hasConnectionsDataBeenFetchedKey,
value: false,
},
];
const finallyData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: hasConnectionsDataBeenFetchedKey,
value: true,
},
];
Comment on lines +18 to +24
Copy link
Contributor

@aldo-expensify aldo-expensify Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hayata-suenaga After playing with this, I think you missed the failureData setting hasConnectionsDataBeenFetched to false or something like that?

If you do:

  1. Create a workspace
  2. Enable Accounting, but don't got to the accounting settings yet
  3. Disable your internet connection
  4. Open the Accounting settings

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the PR to fix the issue. Thank you for letting me know of the issue 🙇


const parameters: OpenPolicyAccountingPageParams = {
policyID,
};

API.read(READ_COMMANDS.OPEN_POLICY_ACCOUNTING_PAGE, parameters, {
optimisticData,
finallyData,
});
}

// More action functions will be added later
// eslint-disable-next-line import/prefer-default-export
export {openPolicyAccountingPage};
4 changes: 2 additions & 2 deletions src/pages/workspace/accounting/WorkspaceAccountingPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ import Navigation from '@navigation/Navigation';
import AdminPolicyAccessOrNotFoundWrapper from '@pages/workspace/AdminPolicyAccessOrNotFoundWrapper';
import FeatureEnabledAccessOrNotFoundWrapper from '@pages/workspace/FeatureEnabledAccessOrNotFoundWrapper';
import PaidPolicyAccessOrNotFoundWrapper from '@pages/workspace/PaidPolicyAccessOrNotFoundWrapper';
import withPolicy from '@pages/workspace/withPolicy';
import type {WithPolicyProps} from '@pages/workspace/withPolicy';
import withPolicyConnections from '@pages/workspace/withPolicyConnections';
import type {AnchorPosition} from '@styles/index';
import CONST from '@src/CONST';
import ROUTES from '@src/ROUTES';
Expand Down Expand Up @@ -237,4 +237,4 @@ function WorkspaceAccountingPage({policy}: WithPolicyProps) {

WorkspaceAccountingPage.displayName = 'WorkspaceAccountingPage';

export default withPolicy(WorkspaceAccountingPage);
export default withPolicyConnections(WorkspaceAccountingPage);
69 changes: 69 additions & 0 deletions src/pages/workspace/withPolicyConnections.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import React, {useEffect} from 'react';
import type {ComponentType} from 'react';
import {useOnyx} from 'react-native-onyx';
import FullPageOfflineBlockingView from '@components/BlockingViews/FullPageOfflineBlockingView';
import FullScreenLoadingIndicator from '@components/FullscreenLoadingIndicator';
import useNetwork from '@hooks/useNetwork';
import {openPolicyAccountingPage} from '@libs/actions/PolicyConnections';
import ONYXKEYS from '@src/ONYXKEYS';
import withPolicy from './withPolicy';
import type {WithPolicyProps} from './withPolicy';

type WithPolicyConnectionsProps = WithPolicyProps;

/**
* Higher-order component that fetches the connections data and populates
* the corresponding field of the policy object if the field is empty. It then passes the policy object
* to the wrapped component.
*
* Use this HOC when you need the policy object with its connections field populated.
*
* Only the active policy gets the complete policy data upon app start that includes the connections data.
* For other policies, the connections data needs to be fetched when it's needed.
*/
function withPolicyConnections(WrappedComponent: ComponentType<WithPolicyConnectionsProps>) {
function WithPolicyConnections({policy, policyDraft, route}: WithPolicyConnectionsProps) {
const {isOffline} = useNetwork();
const [hasConnectionsDataBeenFetched, {status}] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY_HAS_CONNECTIONS_DATA_BEEN_FETCHED}${policy?.id ?? '0'}`, {
initWithStoredValues: false,
});

useEffect(() => {
// When the accounting feature is not enabled, or if the connections data already exists,
// there is no need to fetch the connections data.
if (!policy || !policy.areConnectionsEnabled || !!hasConnectionsDataBeenFetched || !!policy.connections) {
return;
}

openPolicyAccountingPage(policy.id);
}, [hasConnectionsDataBeenFetched, policy]);

if (status === 'loading' || !hasConnectionsDataBeenFetched) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm, how is this accessible in the UI if accounting isn't enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@s77rt similar problem with going offline and then offline: #39132 (comment)

@hayata-suenaga is working on it here #40375

if (isOffline) {
return (
<FullPageOfflineBlockingView>
<WrappedComponent
policy={policy}
policyDraft={policyDraft}
route={route}
/>
</FullPageOfflineBlockingView>
);
}

return <FullScreenLoadingIndicator />;
}

return (
<WrappedComponent
policy={policy}
policyDraft={policyDraft}
route={route}
/>
);
}

return withPolicy(WithPolicyConnections);
}

export default withPolicyConnections;
Loading