-
Notifications
You must be signed in to change notification settings - Fork 45
feat(sdk): guard the delete organization flow #1881
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ import { | |
| } from '@raystack/apsara'; | ||
| import { useFrontier } from '../../../contexts/FrontierContext'; | ||
| import { useTerminology } from '../../../hooks/useTerminology'; | ||
| import { useTokens } from '../../../hooks/useTokens'; | ||
| import { handleConnectError } from '~/utils/error'; | ||
|
|
||
| const deleteOrgSchema = yup | ||
|
|
@@ -44,6 +45,7 @@ export const DeleteOrganizationDialog = ({ | |
| const orgLabel = t.organization({ case: 'capital' }); | ||
| const orgLabelLower = t.organization({ case: 'lower' }); | ||
| const [isAcknowledged, setIsAcknowledged] = useState(false); | ||
| const { tokenBalance } = useTokens(); | ||
|
|
||
| const { mutateAsync: deleteOrganization } = useMutation( | ||
| FrontierServiceQueries.deleteOrganization | ||
|
|
@@ -83,6 +85,7 @@ export const DeleteOrganizationDialog = ({ | |
| } catch (error) { | ||
| handleConnectError(error, { | ||
| PermissionDenied: () => toastManager.add({ title: "You don't have permission to perform this action", type: 'error' }), | ||
| FailedPrecondition: (err) => toastManager.add({ title: `Cannot delete this ${orgLabelLower} yet`, description: err.rawMessage, type: 'error' }), | ||
|
Member
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. This handler uses err.rawMessage for the description, while the NotFound and Default handlers below use err.message. rawMessage is empty when the server sends no message, so the toast can show a blank description. Use err.message for consistency. |
||
| NotFound: (err) => toastManager.add({ title: 'Not found', description: err.message, type: 'error' }), | ||
| Default: (err) => toastManager.add({ title: 'Something went wrong', description: err.message, type: 'error' }), | ||
| }); | ||
|
|
@@ -102,6 +105,13 @@ export const DeleteOrganizationDialog = ({ | |
| This action can not be undone. This will permanently | ||
| delete all the projects and resources in {organization?.title}. | ||
| </Text> | ||
| {tokenBalance > 0 ? ( | ||
| <Text size="small" variant="danger"> | ||
| You have {tokenBalance.toString()} tokens remaining. Deleting | ||
| the {orgLabelLower} forfeits them. Contact support to get the | ||
| amount transferred to your bank account. | ||
|
Member
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. This tells the user to contact support to transfer the amount to their bank for any positive balance. But the backend in #1880 separates purchased tokens from complimentary ones, and only the purchased share is transferable. Its own email even says "These were complimentary tokens, so there is no amount to transfer." Here a user with free or promotional credits is told they can get a bank transfer, which will produce wrong support requests. Match the backend wording, or only promise a transfer for the purchased share. |
||
| </Text> | ||
| ) : null} | ||
|
Comment on lines
+108
to
+114
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. 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift Do not treat an unavailable token balance as zero.
Expose the balance query’s loading and error state. Keep the destructive action unavailable, or require an explicit unresolved-balance confirmation, until a successful balance is known. |
||
| <Field | ||
| label={`Please type name of the ${orgLabel} to confirm.`} | ||
| error={ | ||
|
|
@@ -146,7 +156,7 @@ export const DeleteOrganizationDialog = ({ | |
| variant="solid" | ||
| color="danger" | ||
| type="submit" | ||
| disabled={!deleteTitle || !isAcknowledged} | ||
| disabled={!deleteTitle || !isAcknowledged || isSubmitting} | ||
| data-test-id="frontier-sdk-delete-organization-btn" | ||
| loading={isSubmitting} | ||
| loaderText="Deleting..." | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,10 @@ import { | |
| import { useQueryClient } from '@tanstack/react-query'; | ||
| import { | ||
| FrontierServiceQueries, | ||
| UpdateOrganizationRequestSchema | ||
| UpdateOrganizationRequestSchema, | ||
| RQLRequestSchema, | ||
| RQLFilterSchema, | ||
| RQLSortSchema | ||
| } from '@raystack/proton/frontier'; | ||
| import { | ||
| Button, | ||
|
|
@@ -28,6 +31,9 @@ import { | |
| import { useFrontier } from '../../contexts/FrontierContext'; | ||
| import { usePermissions } from '../../hooks/usePermissions'; | ||
| import { useTerminology } from '../../hooks/useTerminology'; | ||
| import { useOrganizationInvoices } from '../../hooks/useOrganizationInvoices'; | ||
| import { INVOICE_STATES } from '../../utils/constants'; | ||
| import { DEFAULT_PAGE_SIZE } from '../../utils/connect-pagination'; | ||
| import { PERMISSIONS, shouldShowComponent } from '../../../utils'; | ||
| import { AuthTooltipMessage } from '../../utils'; | ||
| import { ViewContainer } from '../../components/view-container'; | ||
|
|
@@ -47,6 +53,26 @@ const generalSchema = yup | |
|
|
||
| type FormData = yup.InferType<typeof generalSchema>; | ||
|
|
||
| // Open invoices with a non-zero amount. The server refuses the delete while | ||
| // any exist, so the delete button greys out and explains why. | ||
| const OPEN_INVOICES_QUERY = create(RQLRequestSchema, { | ||
|
Member
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. This OPEN_INVOICES_QUERY is a verbatim copy of the one in billing/components/payment-issue.tsx, so the two can drift apart if one changes. It also fetches up to the default page size and sorts by created_at just to run .some() for a boolean. limit 1 with no sort answers "any open invoice?" with a smaller query. Consider sharing one constant and trimming it for this existence check. |
||
| filters: [ | ||
| create(RQLFilterSchema, { | ||
| name: 'state', | ||
| operator: 'eq', | ||
| value: { case: 'stringValue', value: INVOICE_STATES.OPEN } | ||
| }), | ||
| create(RQLFilterSchema, { | ||
| name: 'amount', | ||
| operator: 'gt', | ||
| value: { case: 'numberValue', value: 0 } | ||
| }) | ||
| ], | ||
| sort: [create(RQLSortSchema, { name: 'created_at', order: 'desc' })], | ||
| offset: 0, | ||
| limit: DEFAULT_PAGE_SIZE | ||
| }); | ||
|
|
||
| export interface GeneralViewProps { | ||
| onDeleteSuccess?: () => void; | ||
| urlPrefix?: string; | ||
|
|
@@ -97,6 +123,14 @@ export function GeneralView({ onDeleteSuccess, urlPrefix }: GeneralViewProps = { | |
|
|
||
| const isLoading = !organization?.id || isActiveOrganizationLoading || isPermissionsFetching; | ||
|
|
||
| const { invoices } = useOrganizationInvoices({ | ||
| query: OPEN_INVOICES_QUERY, | ||
| enabled: canDeleteWorkspace && !!organization?.id | ||
| }); | ||
| const hasUnpaidInvoices = invoices.some( | ||
|
Member
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. Two issues here. (1) While the invoices query is loading, and when it errors (the error is not read), invoices is [] so hasUnpaidInvoices is false and the delete button is enabled, which is the opposite of the intended grey-out. A user with open invoices can reach the server before the guard applies. (2) .some(inv => inv.state === OPEN) re-checks only state and drops the amount > 0 condition the query applied, so a zero-amount or fully-credited open invoice would still flag as unpaid. If you trust the server filter, invoices.length > 0 is enough; if not, re-check the amount too. |
||
| inv => inv.state === INVOICE_STATES.OPEN | ||
| ); | ||
|
Comment on lines
+126
to
+132
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. 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift Fail closed when the invoice query is not verified.
Use the query status in the delete gate. Keep the button disabled while the check is loading or failed. Show an explicit loading or verification-error tooltip. Based on learnings: count-dependent actions should remain unavailable when the query fails because the UI cannot show a reliable count. Also applies to: 317-325, 332-337 Source: Learnings |
||
|
|
||
| // Update organization form | ||
| const { mutateAsync: updateOrganization } = useMutation( | ||
| FrontierServiceQueries.updateOrganization, | ||
|
|
@@ -280,22 +314,27 @@ export function GeneralView({ onDeleteSuccess, urlPrefix }: GeneralViewProps = { | |
| </Text> | ||
| <Tooltip> | ||
| <Tooltip.Trigger | ||
| disabled={canDeleteWorkspace} | ||
| disabled={canDeleteWorkspace && !hasUnpaidInvoices} | ||
| render={<span className={styles.fitContent} />} | ||
| > | ||
| <Button | ||
| variant="solid" | ||
| color="danger" | ||
| onClick={() => setShowDeleteDialog(true)} | ||
| disabled={!canDeleteWorkspace} | ||
| disabled={!canDeleteWorkspace || hasUnpaidInvoices} | ||
| data-test-id="frontier-sdk-delete-organization-btn" | ||
| > | ||
| Delete {orgLabelLower} | ||
| </Button> | ||
| </Tooltip.Trigger> | ||
| {!canDeleteWorkspace && ( | ||
| {!canDeleteWorkspace ? ( | ||
| <Tooltip.Content>{AuthTooltipMessage}</Tooltip.Content> | ||
| )} | ||
| ) : hasUnpaidInvoices ? ( | ||
| <Tooltip.Content> | ||
| There are unpaid invoices. Pay them from the billing page | ||
| before deleting the {orgLabelLower}. | ||
| </Tooltip.Content> | ||
| ) : null} | ||
| </Tooltip> | ||
| </> | ||
| )} | ||
|
|
||
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.
The parent renders this dialog whenever canDeleteWorkspace is true, not only when it is open, so useTokens() runs getBillingBalance on every General settings load for delete-capable users, not just when they open the delete dialog. For an org whose billing account has no balance support, that query errors and pops an unrelated toast, and it adds a request to every settings visit. Gate the balance query on the dialog being open.