-
Notifications
You must be signed in to change notification settings - Fork 3.9k
[Release 1] [Domain Control] Revoke admin access #77780
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
Changes from all commits
256a7d6
764ceee
20aa7a6
a5ceb39
6034354
6f0125f
24d8ae8
ef412d8
aedc0fb
102efa9
ab95b79
12160f7
8f24ba7
be00cbc
183ceb8
3317415
fadf6fe
78d1045
33bb57f
0b2e5c3
0e0c8ed
4412cc9
14f0afd
2468563
e28d3bc
462d80e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| type RemoveDomainAdminParams = { | ||
| domainAccountID: number; | ||
| targetAccountID: number; | ||
| }; | ||
|
|
||
| export default RemoveDomainAdminParams; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| import Onyx from 'react-native-onyx'; | ||
| import type {OnyxUpdate} from 'react-native-onyx'; | ||
| import * as API from '@libs/API'; | ||
| import type {AddAdminToDomainParams, SetTechnicalContactEmailParams, ToggleConsolidatedDomainBillingParams} from '@libs/API/parameters'; | ||
| import type {AddAdminToDomainParams, RemoveDomainAdminParams, SetTechnicalContactEmailParams, ToggleConsolidatedDomainBillingParams} from '@libs/API/parameters'; | ||
| import {READ_COMMANDS, SIDE_EFFECT_REQUEST_COMMANDS, WRITE_COMMANDS} from '@libs/API/types'; | ||
| import {getMicroSecondOnyxErrorWithTranslationKey} from '@libs/ErrorUtils'; | ||
| import {getAuthToken} from '@libs/Network/NetworkStore'; | ||
|
|
@@ -625,15 +625,9 @@ function addAdminToDomain(domainAccountID: number, accountID: number, targetEmai | |
| } | ||
|
|
||
| /** | ||
| * Removes an error after trying to add admin | ||
| * Removes an error and pending actions after trying to add admin | ||
| */ | ||
| function clearAddAdminError(domainAccountID: number, accountID: number) { | ||
| const PERMISSION_KEY = `${ONYXKEYS.COLLECTION.EXPENSIFY_ADMIN_ACCESS_PREFIX}${accountID}`; | ||
|
|
||
| Onyx.merge(`${ONYXKEYS.COLLECTION.DOMAIN}${domainAccountID}`, { | ||
| [PERMISSION_KEY]: null, | ||
| }); | ||
|
|
||
| function clearAdminError(domainAccountID: number, accountID: number) { | ||
| Onyx.merge(`${ONYXKEYS.COLLECTION.DOMAIN_ERRORS}${domainAccountID}`, { | ||
| adminErrors: { | ||
| [accountID]: null, | ||
|
|
@@ -646,6 +640,66 @@ function clearAddAdminError(domainAccountID: number, accountID: number) { | |
| }, | ||
| }); | ||
| } | ||
| /** | ||
| * Removes admin access for a domain member | ||
| */ | ||
| function revokeDomainAdminAccess(domainAccountID: number, accountID: number) { | ||
| const optimisticData: OnyxUpdate[] = [ | ||
| { | ||
| onyxMethod: Onyx.METHOD.MERGE, | ||
| key: `${ONYXKEYS.COLLECTION.DOMAIN_PENDING_ACTIONS}${domainAccountID}`, | ||
| value: { | ||
| admin: { | ||
| [accountID]: { | ||
| pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| ]; | ||
| const successData: OnyxUpdate[] = [ | ||
| { | ||
| onyxMethod: Onyx.METHOD.MERGE, | ||
| key: `${ONYXKEYS.COLLECTION.DOMAIN_PENDING_ACTIONS}${domainAccountID}`, | ||
| value: { | ||
| admin: { | ||
| [accountID]: { | ||
| pendingAction: null, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| ]; | ||
|
Comment on lines
+660
to
+672
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we do not want to remove them from the domain in optimistic data, should we remove them in the successData?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I found it a but strange with removing those permissions and optimistics... It does behave as it should without removing it manually in optimistic data so I removed that part. Unless there is sth I'm missing... See the video here
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that looks correct, I think it is just odd as we have the domain_pending_actions concept as separate collection |
||
| const failureData: OnyxUpdate[] = [ | ||
|
mountiny marked this conversation as resolved.
|
||
| { | ||
| onyxMethod: Onyx.METHOD.MERGE, | ||
| key: `${ONYXKEYS.COLLECTION.DOMAIN_PENDING_ACTIONS}${domainAccountID}`, | ||
| value: { | ||
| admin: { | ||
| [accountID]: { | ||
| pendingAction: null, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| onyxMethod: Onyx.METHOD.MERGE, | ||
| key: `${ONYXKEYS.COLLECTION.DOMAIN_ERRORS}${domainAccountID}`, | ||
| value: { | ||
| adminErrors: { | ||
| [accountID]: {errors: getMicroSecondOnyxErrorWithTranslationKey('domain.admins.error.removeAdmin')}, | ||
| }, | ||
| }, | ||
|
mountiny marked this conversation as resolved.
|
||
| }, | ||
| ]; | ||
|
|
||
| const parameters: RemoveDomainAdminParams = { | ||
| domainAccountID, | ||
| targetAccountID: accountID, | ||
| }; | ||
|
|
||
| API.write(WRITE_COMMANDS.REMOVE_DOMAIN_ADMIN, parameters, {optimisticData, successData, failureData}); | ||
| } | ||
|
|
||
| export { | ||
| getDomainValidationCode, | ||
|
|
@@ -666,5 +720,6 @@ export { | |
| toggleConsolidatedDomainBilling, | ||
| clearToggleConsolidatedDomainBillingErrors, | ||
| addAdminToDomain, | ||
| clearAddAdminError, | ||
| clearAdminError, | ||
| revokeDomainAdminAccess, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import {adminAccountIDsSelector, technicalContactSettingsSelector} from '@selectors/Domain'; | ||
| import {adminAccountIDsSelector, adminPendingActionSelector, technicalContactSettingsSelector} from '@selectors/Domain'; | ||
| import React from 'react'; | ||
| import {View} from 'react-native'; | ||
| import Badge from '@components/Badge'; | ||
|
|
@@ -26,7 +26,7 @@ import Navigation from '@navigation/Navigation'; | |
| import type {PlatformStackScreenProps} from '@navigation/PlatformStackNavigation/types'; | ||
| import type {DomainSplitNavigatorParamList} from '@navigation/types'; | ||
| import DomainNotFoundPageWrapper from '@pages/domain/DomainNotFoundPageWrapper'; | ||
| import {clearAddAdminError} from '@userActions/Domain'; | ||
| import {clearAdminError} from '@userActions/Domain'; | ||
| import {getCurrentUserAccountID} from '@userActions/Report'; | ||
| import CONST from '@src/CONST'; | ||
| import ONYXKEYS from '@src/ONYXKEYS'; | ||
|
|
@@ -54,13 +54,21 @@ function DomainAdminsPage({route}: DomainAdminsPageProps) { | |
| canBeMissing: true, | ||
| selector: adminAccountIDsSelector, | ||
| }); | ||
|
|
||
| const [domainErrors] = useOnyx(`${ONYXKEYS.COLLECTION.DOMAIN_ERRORS}${domainAccountID}`, { | ||
| canBeMissing: true, | ||
| }); | ||
|
|
||
| const [domainPendingAction] = useOnyx(`${ONYXKEYS.COLLECTION.DOMAIN_PENDING_ACTIONS}${domainAccountID}`, { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be great to explain in the definition of the pending actions why we need it and why we just dont add it to the domain_ collection that is common for other collections (we dont have Reports_pending_actions)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated in |
||
| canBeMissing: true, | ||
| selector: adminPendingActionSelector, | ||
| }); | ||
|
|
||
| const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST, {canBeMissing: true}); | ||
| const [technicalContactSettings] = useOnyx(`${ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_DOMAIN_MEMBER}${domainAccountID}`, { | ||
| canBeMissing: false, | ||
| selector: technicalContactSettingsSelector, | ||
| }); | ||
| const [domainErrors] = useOnyx(`${ONYXKEYS.COLLECTION.DOMAIN_ERRORS}${domainAccountID}`, {canBeMissing: true}); | ||
| const [domainPendingActions] = useOnyx(`${ONYXKEYS.COLLECTION.DOMAIN_PENDING_ACTIONS}${domainAccountID}`, {canBeMissing: true}); | ||
|
|
||
| const currentUserAccountID = getCurrentUserAccountID(); | ||
| const isAdmin = adminAccountIDs?.includes(currentUserAccountID); | ||
|
|
@@ -69,6 +77,7 @@ function DomainAdminsPage({route}: DomainAdminsPageProps) { | |
| for (const accountID of adminAccountIDs ?? []) { | ||
| const details = personalDetails?.[accountID]; | ||
| const isPrimaryContact = technicalContactSettings?.technicalContactEmail === details?.login; | ||
| const isPendingActionDelete = domainPendingAction?.[accountID]?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE; | ||
| data.push({ | ||
| keyForList: String(accountID), | ||
| accountID, | ||
|
|
@@ -85,7 +94,9 @@ function DomainAdminsPage({route}: DomainAdminsPageProps) { | |
| ], | ||
| rightElement: isPrimaryContact && <Badge text={translate('domain.admins.primaryContact')} />, | ||
| errors: getLatestError(domainErrors?.adminErrors?.[accountID]?.errors), | ||
| pendingAction: domainPendingActions?.admin?.[accountID]?.pendingAction, | ||
| pendingAction: domainPendingAction?.[accountID]?.pendingAction, | ||
| isInteractive: !isPendingActionDelete, | ||
| isDisabled: isPendingActionDelete, | ||
| }); | ||
| } | ||
|
|
||
|
|
@@ -183,7 +194,7 @@ function DomainAdminsPage({route}: DomainAdminsPageProps) { | |
| showScrollIndicator={false} | ||
| addBottomSafeAreaPadding | ||
| customListHeader={getCustomListHeader()} | ||
| onDismissError={(item: AdminOption) => clearAddAdminError(domainAccountID, item.accountID)} | ||
| onDismissError={(item: AdminOption) => clearAdminError(domainAccountID, item.accountID)} | ||
|
mountiny marked this conversation as resolved.
|
||
| /> | ||
| </ScreenWrapper> | ||
| </DomainNotFoundPageWrapper> | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
Strange that for FR we have double quotes
Let's fix it!
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.
There's difference.
Before:
’After:
'Is it intentional or just generated by translator bot?
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.
That was auto genenerated but the bot. There are both patterns present in the file (other langs too). But
’is more common infr.tsso changed it to match