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.

Sign up for GitHub

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: Unable to select members after adding them to the workspace #36713

Merged
merged 10 commits into from
Mar 4, 2024
Merged

fix: Unable to select members after adding them to the workspace #36713

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
707faaa
fix: Unable to select members after adding them to the workspace
tienifr Feb 16, 2024
3877a67
Merge branch 'main' into fix/34828
tienifr Feb 21, 2024
6c4a680
fix: update owner optimistically
tienifr Feb 21, 2024
3f74e86
fix conflicts
tienifr Feb 23, 2024
58f5090
Merge branch 'main' into fix/34828
tienifr Feb 24, 2024
343c32e
Merge branch 'main' into fix/34828
tienifr Feb 28, 2024
1646e58
subcribe policy in component
tienifr Mar 1, 2024
982c20e
Merge branch 'main' into fix/34828
tienifr Mar 4, 2024
27e2b64
bump rn-onyx version 2.0.10
tienifr Mar 4, 2024
3c56150
Merge branch 'main' into fix/34828
tienifr Mar 4, 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
14 changes: 0 additions & 14 deletions src/libs/ReportUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4474,19 +4474,6 @@ function getReportOfflinePendingActionAndErrors(report: OnyxEntry<Report>): Repo
return {reportPendingAction, reportErrors};
}

function getPolicyExpenseChatReportIDByOwner(policyOwner: string): string | null {
const policyWithOwner = Object.values(allPolicies ?? {}).find((policy) => policy?.owner === policyOwner);
if (!policyWithOwner) {
return null;
}

const expenseChat = Object.values(allReports ?? {}).find((report) => isPolicyExpenseChat(report) && report?.policyID === policyWithOwner.id);
if (!expenseChat) {
return null;
}
return expenseChat.reportID;
}

/**
* Check if the report can create the request with type is iouType
*/
Expand Down Expand Up @@ -5168,7 +5155,6 @@ export {
getReportOfflinePendingActionAndErrors,
isDM,
getPolicy,
getPolicyExpenseChatReportIDByOwner,
getWorkspaceChats,
shouldDisableRename,
hasSingleParticipant,
Expand Down
29 changes: 28 additions & 1 deletion src/libs/actions/User.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import playSound, {SOUNDS} from '@libs/Sound';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
import type {FrequentlyUsedEmoji} from '@src/types/onyx';
import type {FrequentlyUsedEmoji, Policy} from '@src/types/onyx';
import type Login from '@src/types/onyx/Login';
import type {OnyxServerUpdate} from '@src/types/onyx/OnyxUpdatesFromServer';
import type OnyxPersonalDetails from '@src/types/onyx/PersonalDetails';
Expand Down Expand Up @@ -70,6 +70,14 @@ Onyx.connect({
},
});

let allPolicies: OnyxCollection<Policy>;

Onyx.connect({
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not really a fan of subscribing to all policies in this file, can we subscribe to them in ContactMethodDetailsPage instead and use a selector to only get the policies owned by the user? then pass them to setContactMethodAsDefault

Copy link
Contributor Author

@tienifr tienifr Feb 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we should get all policies in ContactMethodDetailsPage because it would be re-rendered even with the small change of policies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@youssef-lr wdyt ^?

Copy link
Contributor

Choose a reason for hiding this comment

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

Which is why I mentioned using a selector! You can select only the policy IDs owned by the user which is all we care about here and there won't be any re-renders as the ID will never change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about creating the function in libs/actions/Policy.ts to return all policyIDs? I think it's the best way @youssef-lr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm checking...

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@youssef-lr Here's my PR fix on Onyx repo: Expensify/react-native-onyx#482

Why this change? With collection, the returned type of selector shouldn't be the type we defined in ContactMethodDetailsPageOnyxProps a.k.a TOnyxProps[TOnyxProp] since the returned type of selector is just a single entry and TOnyxProps[TOnyxProp] is the collection.

As its name selector, this returned type should be Partial<KeyValueMapping[TOnyxKey]>.

What do you think? cc @sobitneupane

Copy link
Contributor

Choose a reason for hiding this comment

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

Replied in the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tienifr @youssef-lr Please have a look at this comment.

key: ONYXKEYS.COLLECTION.POLICY,
waitForCollectionCallback: true,
callback: (value) => (allPolicies = value),
});

/**
* Attempt to close the user's accountt
*/
Expand Down Expand Up @@ -855,6 +863,25 @@ function setContactMethodAsDefault(newDefaultContactMethod: string) {
},
];

Object.values(allPolicies ?? {}).forEach((policy) => {
if (policy?.ownerAccountID !== currentUserAccountID) {
return;
}
optimisticData.push({
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.POLICY}${policy.id}`,
value: {
owner: newDefaultContactMethod,
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, this PR didn't address the need to optimistically update the policy employeeList by removing the previous contact method and adding the new one. More details can be found here: #44705 (comment)

},
});
failureData.push({
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.POLICY}${policy.id}`,
value: {
owner: policy.owner,
},
});
});
const parameters: SetContactMethodAsDefaultParams = {
partnerUserID: newDefaultContactMethod,
};
Expand Down
5 changes: 2 additions & 3 deletions src/pages/workspace/WorkspaceMembersPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,8 @@ function WorkspaceMembersPage({policyMembers, personalDetails, route, policy, se
*/
const validateSelection = useCallback(() => {
const newErrors: Errors = {};
const ownerAccountID = PersonalDetailsUtils.getAccountIDsByLogins(policy?.owner ? [policy.owner] : [])[0];
selectedEmployees.forEach((member) => {
if (member !== ownerAccountID && member !== session?.accountID) {
if (member !== policy?.ownerAccountID && member !== session?.accountID) {
return;
}
newErrors[member] = translate('workspace.people.error.cannotRemove');
Expand Down Expand Up @@ -321,7 +320,7 @@ function WorkspaceMembersPage({policyMembers, personalDetails, route, policy, se
isSelected: selectedEmployees.includes(accountID),
isDisabled:
accountID === session?.accountID ||
details.login === policy?.owner ||
accountID === policy?.ownerAccountID ||
policyMember.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE ||
!isEmptyObject(policyMember.errors),
text: formatPhoneNumber(PersonalDetailsUtils.getDisplayNameOrDefault(details)),
Expand Down
Loading