From 877989e0cc24c3965e7d4601c5a2de685417b84c Mon Sep 17 00:00:00 2001 From: Talisson Costa Date: Mon, 9 Nov 2026 16:42:21 -0300 Subject: [PATCH 1/4] refactor(Change requests): Load and action change requests through RTK Query behind change_requests_rtk The change request page reads one data hook. With change_requests_rtk on, it loads the change request and its feature through RTK Query, and approve, publish, delete and assignee changes are mutations that refresh the counts on success. With it off, the Flux store path is unchanged. Contributes to #8774 Co-Authored-By: Claude Opus 5.5 --- .../hooks/useChangeRequestDetail/index.ts | 15 ++ .../hooks/useChangeRequestDetail/types.ts | 30 ++++ .../useFluxChangeRequestDetail.ts | 66 +++++++++ .../useRtkChangeRequestDetail.ts | 77 ++++++++++ frontend/common/services/useChangeRequest.ts | 135 ++++++++++++++++++ frontend/common/stores/feature-list-store.ts | 30 +++- frontend/common/types/requests.ts | 9 ++ frontend/common/types/responses.ts | 6 + .../pages/ChangeRequestDetailPage.tsx | 53 ++----- 9 files changed, 374 insertions(+), 47 deletions(-) create mode 100644 frontend/common/hooks/useChangeRequestDetail/index.ts create mode 100644 frontend/common/hooks/useChangeRequestDetail/types.ts create mode 100644 frontend/common/hooks/useChangeRequestDetail/useFluxChangeRequestDetail.ts create mode 100644 frontend/common/hooks/useChangeRequestDetail/useRtkChangeRequestDetail.ts diff --git a/frontend/common/hooks/useChangeRequestDetail/index.ts b/frontend/common/hooks/useChangeRequestDetail/index.ts new file mode 100644 index 000000000000..ce1a4445b8cd --- /dev/null +++ b/frontend/common/hooks/useChangeRequestDetail/index.ts @@ -0,0 +1,15 @@ +import Utils from 'common/utils/utils' +import { useFluxChangeRequestDetail } from './useFluxChangeRequestDetail' +import { useRtkChangeRequestDetail } from './useRtkChangeRequestDetail' +import type { ChangeRequestDetail, ChangeRequestDetailParams } from './types' + +export type * from './types' + +export const useChangeRequestDetail = ( + params: Omit, +): ChangeRequestDetail => { + const rtkEnabled = Utils.getFlagsmithHasFeature('change_requests_rtk') + const flux = useFluxChangeRequestDetail({ ...params, enabled: !rtkEnabled }) + const rtk = useRtkChangeRequestDetail({ ...params, enabled: rtkEnabled }) + return rtkEnabled ? rtk : flux +} diff --git a/frontend/common/hooks/useChangeRequestDetail/types.ts b/frontend/common/hooks/useChangeRequestDetail/types.ts new file mode 100644 index 000000000000..2eee99332251 --- /dev/null +++ b/frontend/common/hooks/useChangeRequestDetail/types.ts @@ -0,0 +1,30 @@ +import { + ChangeRequest, + FeatureState, + ProjectFlag, +} from 'common/types/responses' + +export type ChangeRequestAssignees = Pick< + ChangeRequest, + 'approvals' | 'group_assignments' +> + +export type ChangeRequestDetailParams = { + environmentId: string + id: string + projectId: string + enabled: boolean +} + +export type ChangeRequestDetail = { + changeRequest?: ChangeRequest + environmentFlag?: FeatureState + projectFlag?: ProjectFlag + error?: unknown + isUpdating: boolean + notFound: boolean + approve: () => void + commit: (onDone: () => void) => void + remove: (onDone: () => void) => void + updateAssignees: (assignees: ChangeRequestAssignees) => void +} diff --git a/frontend/common/hooks/useChangeRequestDetail/useFluxChangeRequestDetail.ts b/frontend/common/hooks/useChangeRequestDetail/useFluxChangeRequestDetail.ts new file mode 100644 index 000000000000..15b699efa660 --- /dev/null +++ b/frontend/common/hooks/useChangeRequestDetail/useFluxChangeRequestDetail.ts @@ -0,0 +1,66 @@ +import { useEffect, useState } from 'react' +import AppActions from 'common/dispatcher/app-actions' +import ChangeRequestStore from 'common/stores/change-requests-store' +import { + ChangeRequest, + FeatureState, + ProjectFlag, +} from 'common/types/responses' +import type { ChangeRequestDetail, ChangeRequestDetailParams } from './types' + +type FluxFlags = Record< + string, + { environmentFlag: FeatureState; projectFlag: ProjectFlag } | undefined +> + +export const useFluxChangeRequestDetail = ({ + enabled, + environmentId, + id, + projectId, +}: ChangeRequestDetailParams): ChangeRequestDetail => { + const [, setUpdate] = useState(0) + + useEffect(() => { + if (!enabled) return + AppActions.getChangeRequest(id, projectId, environmentId) + }, [enabled, id, projectId, environmentId]) + + useEffect(() => { + if (!enabled) return + const forceUpdate = () => setUpdate(Date.now()) + ChangeRequestStore.on('change', forceUpdate) + return () => { + ChangeRequestStore.off('change', forceUpdate) + } + }, [enabled]) + + const models: Record = + ChangeRequestStore.model + const flags: FluxFlags = ChangeRequestStore.flags + const changeRequest = models[id] + const error = ChangeRequestStore.error + + return { + approve: () => AppActions.actionChangeRequest(id, 'approve'), + changeRequest, + commit: (onDone) => AppActions.actionChangeRequest(id, 'commit', onDone), + environmentFlag: flags[id]?.environmentFlag, + error, + isUpdating: ChangeRequestStore.isLoading, + notFound: !!error && !changeRequest, + projectFlag: flags[id]?.projectFlag, + remove: (onDone) => AppActions.deleteChangeRequest(id, onDone), + updateAssignees: ({ approvals, group_assignments }) => { + if (ChangeRequestStore.isLoading || !changeRequest) return + AppActions.updateChangeRequest({ + approvals, + description: changeRequest.description, + feature_states: changeRequest.feature_states, + group_assignments, + id: changeRequest.id, + title: changeRequest.title, + }) + }, + } +} diff --git a/frontend/common/hooks/useChangeRequestDetail/useRtkChangeRequestDetail.ts b/frontend/common/hooks/useChangeRequestDetail/useRtkChangeRequestDetail.ts new file mode 100644 index 000000000000..45e9cf9d7877 --- /dev/null +++ b/frontend/common/hooks/useChangeRequestDetail/useRtkChangeRequestDetail.ts @@ -0,0 +1,77 @@ +import type { FetchBaseQueryError } from '@reduxjs/toolkit/query' +import type { SerializedError } from '@reduxjs/toolkit' +import { + useActionChangeRequestMutation, + useDeleteChangeRequestMutation, + useGetChangeRequestQuery, + useUpdateChangeRequestAssigneesMutation, +} from 'common/services/useChangeRequest' +import type { ChangeRequestDetail, ChangeRequestDetailParams } from './types' + +type RequestError = FetchBaseQueryError | SerializedError | undefined + +const hasDetail = (data: unknown): data is { detail: string } => + typeof data === 'object' && + data !== null && + 'detail' in data && + typeof data.detail === 'string' + +// Match the Flux store, which shows the API's detail message on its own +const toErrorMessage = (error: RequestError) => { + if (error && 'data' in error && hasDetail(error.data)) { + return error.data.detail + } + return error +} + +export const useRtkChangeRequestDetail = ({ + enabled, + environmentId, + id, + projectId, +}: ChangeRequestDetailParams): ChangeRequestDetail => { + const { + data, + error: loadError, + isFetching, + } = useGetChangeRequestQuery( + { environmentId, id, projectId }, + { skip: !enabled }, + ) + const [action, actionState] = useActionChangeRequestMutation() + const [updateAssignees, updateState] = + useUpdateChangeRequestAssigneesMutation() + const [deleteChangeRequest, deleteState] = useDeleteChangeRequestMutation() + + const isUpdating = + isFetching || + actionState.isLoading || + updateState.isLoading || + deleteState.isLoading + const error = + actionState.error || updateState.error || deleteState.error || loadError + + return { + approve: () => { + action({ actionType: 'approve', id }) + }, + changeRequest: data?.changeRequest, + commit: async (onDone) => { + const res = await action({ actionType: 'commit', id }) + if (!('error' in res)) onDone() + }, + environmentFlag: data?.environmentFlag, + error: toErrorMessage(error), + isUpdating, + notFound: !!loadError && !data, + projectFlag: data?.projectFlag, + remove: async (onDone) => { + const res = await deleteChangeRequest({ id: Number(id) }) + if (!('error' in res)) onDone() + }, + updateAssignees: (assignees) => { + if (isUpdating || !data) return + updateAssignees({ ...assignees, id: data.changeRequest.id }) + }, + } +} diff --git a/frontend/common/services/useChangeRequest.ts b/frontend/common/services/useChangeRequest.ts index 472965928441..93714d32767c 100644 --- a/frontend/common/services/useChangeRequest.ts +++ b/frontend/common/services/useChangeRequest.ts @@ -2,6 +2,8 @@ import { ChangeRequest, ChangeSet, FeatureState, + PagedResponse, + ProjectFlag, Res, } from 'common/types/responses' import { Req } from 'common/types/requests' @@ -10,11 +12,41 @@ import Utils from 'common/utils/utils' import sortBy from 'lodash/sortBy' import moment from 'moment' import transformCorePaging from 'common/transformCorePaging' +import { sortMultivariateOptions } from 'common/utils/multivariate' +import { + addFeatureSegmentsToFeatureStates, + featureStateService, +} from './useFeatureState' export const changeRequestService = service .enhanceEndpoints({ addTagTypes: ['ChangeRequest'] }) .injectEndpoints({ endpoints: (builder) => ({ + actionChangeRequest: builder.mutation< + Res['actionChangeRequest'], + Req['actionChangeRequest'] + >({ + invalidatesTags: (_, error, { id }) => + error + ? [] + : [ + { id, type: 'ChangeRequest' }, + { id: 'LIST', type: 'ChangeRequest' }, + ], + onQueryStarted: async (_, { dispatch, queryFulfilled }) => { + try { + await queryFulfilled + dispatch(featureStateService.util.invalidateTags(['FeatureState'])) + } catch { + // The mutation's own error state reports the failure + } + }, + query: ({ actionType, id }) => ({ + body: {}, + method: 'POST', + url: `features/workflows/change-requests/${id}/${actionType}/`, + }), + }), createEnvironmentChangeRequest: builder.mutation< Res['createEnvironmentChangeRequest'], Req['createEnvironmentChangeRequest'] @@ -57,6 +89,58 @@ export const changeRequestService = service url: `features/workflows/change-requests/${id}/`, }), }), + getChangeRequest: builder.query< + Res['changeRequestDetail'], + Req['getChangeRequest'] + >({ + providesTags: (_, _2, { id }) => [{ id, type: 'ChangeRequest' }], + queryFn: async ( + { environmentId, id, projectId }, + _api, + _extraOptions, + baseQuery, + ) => { + const res = await baseQuery({ + url: `features/workflows/change-requests/${id}/`, + }) + if (res.error) return { error: res.error } + const raw = res.data as ChangeRequest + const changeRequest: ChangeRequest = { + ...raw, + feature_states: await Promise.all( + raw.feature_states.map(addFeatureSegmentsToFeatureStates), + ), + } + const feature = + changeRequest.feature_states[0]?.feature || + changeRequest.change_sets?.[0]?.feature + const [environmentFlags, projectFlagRes] = await Promise.all([ + baseQuery({ + url: `environments/${environmentId}/featurestates/?feature=${feature}`, + }), + baseQuery({ url: `projects/${projectId}/features/${feature}/` }), + ]) + // The feature may have been deleted since, which the page explains + if (environmentFlags.error || projectFlagRes.error) { + return { data: { changeRequest } } + } + const projectFlag = projectFlagRes.data as ProjectFlag + return { + data: { + changeRequest, + environmentFlag: ( + environmentFlags.data as PagedResponse + ).results[0], + projectFlag: { + ...projectFlag, + multivariate_options: sortMultivariateOptions( + projectFlag.multivariate_options, + ), + }, + }, + } + }, + }), getChangeRequests: builder.query< Res['changeRequests'], Req['getChangeRequests'] @@ -80,6 +164,43 @@ export const changeRequestService = service url: `features/workflows/change-requests/${changeRequest.id}/`, }), }), + updateChangeRequestAssignees: builder.mutation< + Res['updateChangeRequest'], + Req['updateChangeRequestAssignees'] + >({ + invalidatesTags: (_, error, { id }) => + error + ? [] + : [ + { id: `${id}`, type: 'ChangeRequest' }, + { id: 'LIST', type: 'ChangeRequest' }, + ], + // Re-read first so the PUT carries the stored change, not the page's copy + queryFn: async ( + { approvals, group_assignments, id }, + _api, + _extraOptions, + baseQuery, + ) => { + const url = `features/workflows/change-requests/${id}/` + const res = await baseQuery({ url }) + if (res.error) return { error: res.error } + const current = res.data as ChangeRequest + const put = await baseQuery({ + body: { + ...current, + approvals, + environment_feature_versions: + current.environment_feature_versions?.map((v) => v.uuid), + group_assignments, + }, + method: 'PUT', + url, + }) + if (put.error) return { error: put.error } + return { data: put.data as Res['updateChangeRequest'] } + }, + }), // END OF ENDPOINTS }), }) @@ -106,12 +227,26 @@ export async function updateChangeRequest( changeRequestService.endpoints.updateChangeRequest.initiate(data, options), ) } +export async function actionChangeRequest( + store: any, + data: Req['actionChangeRequest'], + options?: Parameters< + typeof changeRequestService.endpoints.actionChangeRequest.initiate + >[1], +) { + return store.dispatch( + changeRequestService.endpoints.actionChangeRequest.initiate(data, options), + ) +} // END OF FUNCTION_EXPORTS export const { + useActionChangeRequestMutation, useCreateEnvironmentChangeRequestMutation, useDeleteChangeRequestMutation, + useGetChangeRequestQuery, useGetChangeRequestsQuery, + useUpdateChangeRequestAssigneesMutation, useUpdateChangeRequestMutation, // END OF EXPORTS } = changeRequestService diff --git a/frontend/common/stores/feature-list-store.ts b/frontend/common/stores/feature-list-store.ts index 56ae9b431d5f..4d7e3e3a85a1 100644 --- a/frontend/common/stores/feature-list-store.ts +++ b/frontend/common/stores/feature-list-store.ts @@ -37,6 +37,7 @@ import { getVersionFeatureState } from 'common/services/useVersionFeatureState' import { getFeatureStates } from 'common/services/useFeatureState' import { getSegments } from 'common/services/useSegment' import { + actionChangeRequest, changeRequestService, updateChangeRequest, } from 'common/services/useChangeRequest' @@ -535,6 +536,9 @@ const controller = { commit: boolean, ) => { store.saving() + const changeRequestsRtk = Utils.getFlagsmithHasFeature( + 'change_requests_rtk', + ) try { API.trackEvent(Constants.events.EDIT_FEATURE) const env: Environment = ProjectStore.getEnvironment(environmentId) as any @@ -705,8 +709,18 @@ const controller = { : updatedChangeRequest, ) : Promise.resolve() - ).then(() => { + ).then(async () => { if (commit) { + if (changeRequestsRtk) { + const res = await actionChangeRequest(getStore(), { + actionType: 'commit', + id: `${updatedChangeRequest.id}`, + }) + if (!('error' in res)) { + AppActions.refreshFeatures(projectId, environmentId) + } + return + } AppActions.actionChangeRequest( updatedChangeRequest.id, 'commit', @@ -718,15 +732,17 @@ const controller = { getStore().dispatch( changeRequestService.util.invalidateTags(['ChangeRequest']), ) - AppActions.getChangeRequest( - updatedChangeRequest.id, - projectId, - environmentId, - ) + if (!changeRequestsRtk) { + AppActions.getChangeRequest( + updatedChangeRequest.id, + projectId, + environmentId, + ) + } } }) prom.then(() => { - if (featureStateId) { + if (featureStateId && !changeRequestsRtk) { AppActions.getChangeRequest( changeRequestData.id, projectId, diff --git a/frontend/common/types/requests.ts b/frontend/common/types/requests.ts index a99864c3f2e8..5cc482f41127 100644 --- a/frontend/common/types/requests.ts +++ b/frontend/common/types/requests.ts @@ -39,6 +39,8 @@ import { } from './responses' import { UtmsType } from './utms' +export type ChangeRequestAction = 'approve' | 'commit' + export type UpdateProjectBody = { name: string hide_disabled_flags?: boolean @@ -701,6 +703,13 @@ export type Req = { committed?: boolean }> updateChangeRequest: ChangeRequest + getChangeRequest: { id: string; environmentId: string; projectId: string } + actionChangeRequest: { id: string; actionType: ChangeRequestAction } + updateChangeRequestAssignees: { + id: number + approvals: ChangeRequest['approvals'] + group_assignments: ChangeRequest['group_assignments'] + } getGroupSummaries: { orgId: number } diff --git a/frontend/common/types/responses.ts b/frontend/common/types/responses.ts index f8dee3a87451..2262cea98e27 100644 --- a/frontend/common/types/responses.ts +++ b/frontend/common/types/responses.ts @@ -1686,6 +1686,12 @@ export type Res = { projectChangeRequests: PagedResponse projectChangeRequest: ProjectChangeRequest actionChangeRequest: {} + changeRequestDetail: { + changeRequest: ChangeRequest + // Missing when the feature has since been deleted + environmentFlag?: FeatureState + projectFlag?: ProjectFlag + } identityTrait: { id: string } identityTraits: IdentityTrait[] conversionEvents: PagedResponse diff --git a/frontend/web/components/pages/ChangeRequestDetailPage.tsx b/frontend/web/components/pages/ChangeRequestDetailPage.tsx index 781e2f59f40d..42cb764ecc1a 100644 --- a/frontend/web/components/pages/ChangeRequestDetailPage.tsx +++ b/frontend/web/components/pages/ChangeRequestDetailPage.tsx @@ -1,7 +1,6 @@ import { FC, ReactNode, useEffect, useMemo, useState } from 'react' import Icon from 'components/icons/Icon' import OrganisationStore from 'common/stores/organisation-store' -import ChangeRequestStore from 'common/stores/change-requests-store' import FeatureListStore from 'common/stores/feature-list-store' import { useGetMyGroupsQuery } from 'common/services/useMyGroup' import CreateFeatureModal from 'components/modals/create-feature' @@ -44,6 +43,7 @@ import { useHistory } from 'react-router-dom' import { openPublishChangeRequestConfirm } from 'components/PublishChangeRequestModal' import { getChangeRequestLiveDate } from 'common/utils/getChangeRequestLiveDate' import { EnvironmentPermission } from 'common/types/permissions.types' +import { useChangeRequestDetail } from 'common/hooks/useChangeRequestDetail' type ChangeRequestPageType = { match: { @@ -60,21 +60,8 @@ const ChangeRequestDetailPage: FC = ({ match }) => { const { environmentId, id, projectId } = match.params const [_, setUpdate] = useState(Date.now()) const [updateChangeRequest] = useUpdateChangeRequestMutation() - const error = ChangeRequestStore.error - const changeRequest = ( - ChangeRequestStore.model as Record | undefined - )?.[id] - const flags = ( - ChangeRequestStore.flags as Record< - string, - { - environmentFlag: FeatureState - projectFlag: ProjectFlag - } - > - )[id] - const environmentFlag = flags?.environmentFlag - const projectFlag = flags && flags.projectFlag + const detail = useChangeRequestDetail({ environmentId, id, projectId }) + const { changeRequest, environmentFlag, error, projectFlag } = detail const approvePermission = useHasPermission({ id: environmentId, level: 'environment', @@ -93,54 +80,40 @@ const ChangeRequestDetailPage: FC = ({ match }) => { permission: EnvironmentPermission.ADMIN, }) - useEffect(() => { - AppActions.getChangeRequest(id, projectId, environmentId) - }, [id, projectId, environmentId]) - useEffect(() => { const forceUpdate = () => { setUpdate(Date.now()) } - ChangeRequestStore.on('change', forceUpdate) FeatureListStore.on('change', forceUpdate) OrganisationStore.on('change', forceUpdate) return () => { - ChangeRequestStore.off('change', forceUpdate) FeatureListStore.off('change', forceUpdate) OrganisationStore.off('change', forceUpdate) } }, []) const addOwner = (id: number, isUser = true) => { - if (ChangeRequestStore.isLoading || !changeRequest) return - AppActions.updateChangeRequest({ + if (!changeRequest) return + detail.updateAssignees({ approvals: isUser ? changeRequest.approvals.concat([{ user: id }]) : changeRequest.approvals, - description: changeRequest.description, - feature_states: changeRequest.feature_states, group_assignments: isUser ? changeRequest.group_assignments : changeRequest.group_assignments.concat([{ group: id }]), - id: changeRequest.id, - title: changeRequest.title, }) } const removeOwner = (id: number, isUser = true) => { - if (ChangeRequestStore.isLoading || !changeRequest) return - AppActions.updateChangeRequest({ + if (!changeRequest) return + detail.updateAssignees({ approvals: isUser ? changeRequest.approvals.filter((v) => v.user !== id) : changeRequest.approvals, - description: changeRequest.description, - feature_states: changeRequest.feature_states, group_assignments: isUser ? changeRequest.group_assignments : changeRequest.group_assignments.filter((v) => v.group !== id), - id: changeRequest.id, - title: changeRequest.title, }) } @@ -154,7 +127,7 @@ const ChangeRequestDetailPage: FC = ({ match }) => { ), destructive: true, onYes: () => { - AppActions.deleteChangeRequest(id, () => { + detail.remove(() => { history.replace( `/project/${projectId}/environment/${environmentId}/change-requests`, ) @@ -232,7 +205,7 @@ const ChangeRequestDetailPage: FC = ({ match }) => { } const approveChangeRequest = () => { - AppActions.actionChangeRequest(id, 'approve') + detail.approve() } const publishChangeRequest = () => { @@ -258,7 +231,7 @@ const ChangeRequestDetailPage: FC = ({ match }) => { isScheduled, onYes: (ignore_conflicts) => { const commitChangeRequest = () => { - AppActions.actionChangeRequest(id, 'commit', () => { + detail.commit(() => { AppActions.refreshFeatures(projectId, environmentId) }) } @@ -281,7 +254,7 @@ const ChangeRequestDetailPage: FC = ({ match }) => { }) } - if (error && !changeRequest) { + if (detail.notFound) { return (
= ({ match }) => { return (
= ({ match }) => { scheduledDate={getChangeRequestLiveDate(changeRequest)} deleteChangeRequest={deleteChangeRequest} editChangeRequest={ - !changeRequest?.committed_at + !changeRequest?.committed_at && projectFlag && environmentFlag ? () => editChangeRequest(projectFlag, environmentFlag) : undefined } From 6973cb9ddace5d0aa7a63893a5a258a6509d733a Mon Sep 17 00:00:00 2001 From: Talisson Costa Date: Mon, 9 Nov 2026 16:55:11 -0300 Subject: [PATCH 2/4] fix(Change requests): Report failed feature lookups and skip focus refetches on the RTK path A failed lookup now shows the error instead of "feature since deleted", which is kept for a 404. The page no longer refetches and fades on window focus, like the Flux store. Assignee updates take the route id like the other change request calls. Co-Authored-By: Claude Opus 5.5 --- .../useRtkChangeRequestDetail.ts | 8 +++++--- frontend/common/services/useChangeRequest.ts | 6 ++++-- frontend/common/types/requests.ts | 2 +- .../web/components/pages/ChangeRequestDetailPage.tsx | 11 +++++++++++ 4 files changed, 21 insertions(+), 6 deletions(-) diff --git a/frontend/common/hooks/useChangeRequestDetail/useRtkChangeRequestDetail.ts b/frontend/common/hooks/useChangeRequestDetail/useRtkChangeRequestDetail.ts index 45e9cf9d7877..80fd82b48c4c 100644 --- a/frontend/common/hooks/useChangeRequestDetail/useRtkChangeRequestDetail.ts +++ b/frontend/common/hooks/useChangeRequestDetail/useRtkChangeRequestDetail.ts @@ -36,7 +36,8 @@ export const useRtkChangeRequestDetail = ({ isFetching, } = useGetChangeRequestQuery( { environmentId, id, projectId }, - { skip: !enabled }, + // Like the Flux store, refetch after an action, not on window focus + { refetchOnFocus: false, skip: !enabled }, ) const [action, actionState] = useActionChangeRequestMutation() const [updateAssignees, updateState] = @@ -63,7 +64,8 @@ export const useRtkChangeRequestDetail = ({ environmentFlag: data?.environmentFlag, error: toErrorMessage(error), isUpdating, - notFound: !!loadError && !data, + notFound: + !data && !!loadError && 'status' in loadError && loadError.status === 404, projectFlag: data?.projectFlag, remove: async (onDone) => { const res = await deleteChangeRequest({ id: Number(id) }) @@ -71,7 +73,7 @@ export const useRtkChangeRequestDetail = ({ }, updateAssignees: (assignees) => { if (isUpdating || !data) return - updateAssignees({ ...assignees, id: data.changeRequest.id }) + updateAssignees({ ...assignees, id }) }, } } diff --git a/frontend/common/services/useChangeRequest.ts b/frontend/common/services/useChangeRequest.ts index 93714d32767c..15941852dfcc 100644 --- a/frontend/common/services/useChangeRequest.ts +++ b/frontend/common/services/useChangeRequest.ts @@ -120,10 +120,12 @@ export const changeRequestService = service }), baseQuery({ url: `projects/${projectId}/features/${feature}/` }), ]) - // The feature may have been deleted since, which the page explains - if (environmentFlags.error || projectFlagRes.error) { + // A 404 means the feature was deleted since, which the page explains + if (projectFlagRes.error?.status === 404) { return { data: { changeRequest } } } + if (projectFlagRes.error) return { error: projectFlagRes.error } + if (environmentFlags.error) return { error: environmentFlags.error } const projectFlag = projectFlagRes.data as ProjectFlag return { data: { diff --git a/frontend/common/types/requests.ts b/frontend/common/types/requests.ts index 5cc482f41127..fd213742531a 100644 --- a/frontend/common/types/requests.ts +++ b/frontend/common/types/requests.ts @@ -706,7 +706,7 @@ export type Req = { getChangeRequest: { id: string; environmentId: string; projectId: string } actionChangeRequest: { id: string; actionType: ChangeRequestAction } updateChangeRequestAssignees: { - id: number + id: string approvals: ChangeRequest['approvals'] group_assignments: ChangeRequest['group_assignments'] } diff --git a/frontend/web/components/pages/ChangeRequestDetailPage.tsx b/frontend/web/components/pages/ChangeRequestDetailPage.tsx index 42cb764ecc1a..ec10cac0a3fe 100644 --- a/frontend/web/components/pages/ChangeRequestDetailPage.tsx +++ b/frontend/web/components/pages/ChangeRequestDetailPage.tsx @@ -266,6 +266,17 @@ const ChangeRequestDetailPage: FC = ({ match }) => {
) } + if (!changeRequest && error) { + return ( +
+ +
+ ) + } if (!changeRequest || OrganisationStore.isLoading) { return (
Date: Mon, 9 Nov 2026 17:02:31 -0300 Subject: [PATCH 3/4] refactor(Change requests): Drop the flag and remove the Flux change request store The change request page and publishing from the feature modal now always use RTK Query, so change-requests-store.js and its actions go. Co-Authored-By: Claude Opus 5.5 --- .../common/dispatcher/action-constants.js | 4 - frontend/common/dispatcher/app-actions.js | 29 ---- ...estDetail.ts => useChangeRequestDetail.ts} | 35 +++- .../hooks/useChangeRequestDetail/index.ts | 15 -- .../hooks/useChangeRequestDetail/types.ts | 30 ---- .../useFluxChangeRequestDetail.ts | 66 -------- .../common/stores/change-requests-store.js | 158 ------------------ frontend/common/stores/feature-list-store.ts | 44 +---- frontend/web/components/pages/SegmentPage.tsx | 2 - 9 files changed, 38 insertions(+), 345 deletions(-) rename frontend/common/hooks/{useChangeRequestDetail/useRtkChangeRequestDetail.ts => useChangeRequestDetail.ts} (76%) delete mode 100644 frontend/common/hooks/useChangeRequestDetail/index.ts delete mode 100644 frontend/common/hooks/useChangeRequestDetail/types.ts delete mode 100644 frontend/common/hooks/useChangeRequestDetail/useFluxChangeRequestDetail.ts delete mode 100644 frontend/common/stores/change-requests-store.js diff --git a/frontend/common/dispatcher/action-constants.js b/frontend/common/dispatcher/action-constants.js index 6b44608b7d8c..44118b4bf115 100644 --- a/frontend/common/dispatcher/action-constants.js +++ b/frontend/common/dispatcher/action-constants.js @@ -1,14 +1,12 @@ import BaseActions from './base/_action-constants' const Actions = Object.assign({}, BaseActions, { 'ACCEPT_INVITE': 'ACCEPT_INVITE', - 'ACTION_CHANGE_REQUEST': 'ACTION_CHANGE_REQUEST', 'CHANGE_USER_FLAG': 'CHANGE_USER_FLAG', 'CONFIRM_TWO_FACTOR': 'CONFIRM_TWO_FACTOR', 'CREATE_ENV': 'CREATE_ENV', 'CREATE_FLAG': 'CREATE_FLAG', 'CREATE_ORGANISATION': 'CREATE_ORGANISATION', 'CREATE_PROJECT': 'CREATE_PROJECT', - 'DELETE_CHANGE_REQUEST': 'DELETE_CHANGE_REQUEST', 'DELETE_ENVIRONMENT': 'DELETE_ENVIRONMENT', 'DELETE_ORGANISATION': 'DELETE_ORGANISATION', 'DELETE_PROJECT': 'DELETE_PROJECT', @@ -24,7 +22,6 @@ const Actions = Object.assign({}, BaseActions, { 'EDIT_PROJECT': 'EDIT_PROJECT', 'EDIT_USER_FLAG': 'EDIT_USER_FLAG', 'ENABLE_TWO_FACTOR': 'ENABLE_TWO_FACTOR', - 'GET_CHANGE_REQUEST': 'GET_CHANGE_REQUEST', 'GET_ENVIRONMENT': 'GET_ENVIRONMENT', 'GET_IDENTITY': 'GET_IDENTITY', 'GET_ORGANISATION': 'GET_ORGANISATION', @@ -37,7 +34,6 @@ const Actions = Object.assign({}, BaseActions, { 'SELECT_ORGANISATION': 'SELECT_ORGANISATION', 'TOGGLE_USER_FLAG': 'TOGGLE_USER_FLAG', 'TWO_FACTOR_LOGIN': 'TWO_FACTOR_LOGIN', - 'UPDATE_CHANGE_REQUEST': 'UPDATE_CHANGE_REQUEST', 'UPDATE_SUBSCRIPTION': 'UPDATE_SUBSCRIPTION', 'UPDATE_USER_ROLE': 'UPDATE_USER_ROLE', }) diff --git a/frontend/common/dispatcher/app-actions.js b/frontend/common/dispatcher/app-actions.js index e2a4ea8c1d34..9e0c4bf6a0ca 100644 --- a/frontend/common/dispatcher/app-actions.js +++ b/frontend/common/dispatcher/app-actions.js @@ -8,14 +8,6 @@ const AppActions = Object.assign({}, BaseAppActions, { id, }) }, - actionChangeRequest(id, action, cb) { - Dispatcher.handleViewAction({ - action, - actionType: Actions.ACTION_CHANGE_REQUEST, - cb, - id, - }) - }, changeUserFlag(identity) { Dispatcher.handleViewAction({ actionType: Actions.CHANGE_USER_FLAG, @@ -59,13 +51,6 @@ const AppActions = Object.assign({}, BaseAppActions, { }) }, - deleteChangeRequest(id, cb) { - Dispatcher.handleViewAction({ - actionType: Actions.DELETE_CHANGE_REQUEST, - cb, - id, - }) - }, deleteEnv(env) { Dispatcher.handleViewAction({ actionType: Actions.DELETE_ENVIRONMENT, @@ -187,14 +172,6 @@ const AppActions = Object.assign({}, BaseAppActions, { actionType: Actions.ENABLE_TWO_FACTOR, }) }, - getChangeRequest(id, projectId, environmentId) { - Dispatcher.handleViewAction({ - actionType: Actions.GET_CHANGE_REQUEST, - environmentId, - id, - projectId, - }) - }, getIdentity(envId, id) { Dispatcher.handleViewAction({ actionType: Actions.GET_IDENTITY, @@ -274,12 +251,6 @@ const AppActions = Object.assign({}, BaseAppActions, { pin, }) }, - updateChangeRequest(changeRequest) { - Dispatcher.handleViewAction({ - actionType: Actions.UPDATE_CHANGE_REQUEST, - changeRequest, - }) - }, updateSubscription(hostedPageId) { Dispatcher.handleViewAction({ actionType: Actions.UPDATE_SUBSCRIPTION, diff --git a/frontend/common/hooks/useChangeRequestDetail/useRtkChangeRequestDetail.ts b/frontend/common/hooks/useChangeRequestDetail.ts similarity index 76% rename from frontend/common/hooks/useChangeRequestDetail/useRtkChangeRequestDetail.ts rename to frontend/common/hooks/useChangeRequestDetail.ts index 80fd82b48c4c..1d8da0d0004c 100644 --- a/frontend/common/hooks/useChangeRequestDetail/useRtkChangeRequestDetail.ts +++ b/frontend/common/hooks/useChangeRequestDetail.ts @@ -1,12 +1,40 @@ import type { FetchBaseQueryError } from '@reduxjs/toolkit/query' import type { SerializedError } from '@reduxjs/toolkit' +import { + ChangeRequest, + FeatureState, + ProjectFlag, +} from 'common/types/responses' import { useActionChangeRequestMutation, useDeleteChangeRequestMutation, useGetChangeRequestQuery, useUpdateChangeRequestAssigneesMutation, } from 'common/services/useChangeRequest' -import type { ChangeRequestDetail, ChangeRequestDetailParams } from './types' + +export type ChangeRequestAssignees = Pick< + ChangeRequest, + 'approvals' | 'group_assignments' +> + +type ChangeRequestDetailParams = { + environmentId: string + id: string + projectId: string +} + +export type ChangeRequestDetail = { + changeRequest?: ChangeRequest + environmentFlag?: FeatureState + projectFlag?: ProjectFlag + error?: unknown + isUpdating: boolean + notFound: boolean + approve: () => void + commit: (onDone: () => void) => void + remove: (onDone: () => void) => void + updateAssignees: (assignees: ChangeRequestAssignees) => void +} type RequestError = FetchBaseQueryError | SerializedError | undefined @@ -24,8 +52,7 @@ const toErrorMessage = (error: RequestError) => { return error } -export const useRtkChangeRequestDetail = ({ - enabled, +export const useChangeRequestDetail = ({ environmentId, id, projectId, @@ -37,7 +64,7 @@ export const useRtkChangeRequestDetail = ({ } = useGetChangeRequestQuery( { environmentId, id, projectId }, // Like the Flux store, refetch after an action, not on window focus - { refetchOnFocus: false, skip: !enabled }, + { refetchOnFocus: false }, ) const [action, actionState] = useActionChangeRequestMutation() const [updateAssignees, updateState] = diff --git a/frontend/common/hooks/useChangeRequestDetail/index.ts b/frontend/common/hooks/useChangeRequestDetail/index.ts deleted file mode 100644 index ce1a4445b8cd..000000000000 --- a/frontend/common/hooks/useChangeRequestDetail/index.ts +++ /dev/null @@ -1,15 +0,0 @@ -import Utils from 'common/utils/utils' -import { useFluxChangeRequestDetail } from './useFluxChangeRequestDetail' -import { useRtkChangeRequestDetail } from './useRtkChangeRequestDetail' -import type { ChangeRequestDetail, ChangeRequestDetailParams } from './types' - -export type * from './types' - -export const useChangeRequestDetail = ( - params: Omit, -): ChangeRequestDetail => { - const rtkEnabled = Utils.getFlagsmithHasFeature('change_requests_rtk') - const flux = useFluxChangeRequestDetail({ ...params, enabled: !rtkEnabled }) - const rtk = useRtkChangeRequestDetail({ ...params, enabled: rtkEnabled }) - return rtkEnabled ? rtk : flux -} diff --git a/frontend/common/hooks/useChangeRequestDetail/types.ts b/frontend/common/hooks/useChangeRequestDetail/types.ts deleted file mode 100644 index 2eee99332251..000000000000 --- a/frontend/common/hooks/useChangeRequestDetail/types.ts +++ /dev/null @@ -1,30 +0,0 @@ -import { - ChangeRequest, - FeatureState, - ProjectFlag, -} from 'common/types/responses' - -export type ChangeRequestAssignees = Pick< - ChangeRequest, - 'approvals' | 'group_assignments' -> - -export type ChangeRequestDetailParams = { - environmentId: string - id: string - projectId: string - enabled: boolean -} - -export type ChangeRequestDetail = { - changeRequest?: ChangeRequest - environmentFlag?: FeatureState - projectFlag?: ProjectFlag - error?: unknown - isUpdating: boolean - notFound: boolean - approve: () => void - commit: (onDone: () => void) => void - remove: (onDone: () => void) => void - updateAssignees: (assignees: ChangeRequestAssignees) => void -} diff --git a/frontend/common/hooks/useChangeRequestDetail/useFluxChangeRequestDetail.ts b/frontend/common/hooks/useChangeRequestDetail/useFluxChangeRequestDetail.ts deleted file mode 100644 index 15b699efa660..000000000000 --- a/frontend/common/hooks/useChangeRequestDetail/useFluxChangeRequestDetail.ts +++ /dev/null @@ -1,66 +0,0 @@ -import { useEffect, useState } from 'react' -import AppActions from 'common/dispatcher/app-actions' -import ChangeRequestStore from 'common/stores/change-requests-store' -import { - ChangeRequest, - FeatureState, - ProjectFlag, -} from 'common/types/responses' -import type { ChangeRequestDetail, ChangeRequestDetailParams } from './types' - -type FluxFlags = Record< - string, - { environmentFlag: FeatureState; projectFlag: ProjectFlag } | undefined -> - -export const useFluxChangeRequestDetail = ({ - enabled, - environmentId, - id, - projectId, -}: ChangeRequestDetailParams): ChangeRequestDetail => { - const [, setUpdate] = useState(0) - - useEffect(() => { - if (!enabled) return - AppActions.getChangeRequest(id, projectId, environmentId) - }, [enabled, id, projectId, environmentId]) - - useEffect(() => { - if (!enabled) return - const forceUpdate = () => setUpdate(Date.now()) - ChangeRequestStore.on('change', forceUpdate) - return () => { - ChangeRequestStore.off('change', forceUpdate) - } - }, [enabled]) - - const models: Record = - ChangeRequestStore.model - const flags: FluxFlags = ChangeRequestStore.flags - const changeRequest = models[id] - const error = ChangeRequestStore.error - - return { - approve: () => AppActions.actionChangeRequest(id, 'approve'), - changeRequest, - commit: (onDone) => AppActions.actionChangeRequest(id, 'commit', onDone), - environmentFlag: flags[id]?.environmentFlag, - error, - isUpdating: ChangeRequestStore.isLoading, - notFound: !!error && !changeRequest, - projectFlag: flags[id]?.projectFlag, - remove: (onDone) => AppActions.deleteChangeRequest(id, onDone), - updateAssignees: ({ approvals, group_assignments }) => { - if (ChangeRequestStore.isLoading || !changeRequest) return - AppActions.updateChangeRequest({ - approvals, - description: changeRequest.description, - feature_states: changeRequest.feature_states, - group_assignments, - id: changeRequest.id, - title: changeRequest.title, - }) - }, - } -} diff --git a/frontend/common/stores/change-requests-store.js b/frontend/common/stores/change-requests-store.js deleted file mode 100644 index f34e6a436d19..000000000000 --- a/frontend/common/stores/change-requests-store.js +++ /dev/null @@ -1,158 +0,0 @@ -import Dispatcher from 'common/dispatcher/dispatcher' -import { sortMultivariateOptions } from 'common/utils/multivariate' -import BaseStore from './base/_store' -import data from 'common/data/base/_data' -import { addFeatureSegmentsToFeatureStates } from 'common/services/useFeatureState' -import { getStore } from 'common/store' -import { changeRequestService } from 'common/services/useChangeRequest' -import { featureStateService } from 'common/services/useFeatureState' -const transformChangeRequest = async (changeRequest) => { - const feature_states = await Promise.all( - changeRequest.feature_states.map(addFeatureSegmentsToFeatureStates), - ) - - return { - ...changeRequest, - feature_states, - } -} -const controller = { - actionChangeRequest: (id, action, cb) => { - store.loading() - data - .post( - `${Project.api}features/workflows/change-requests/${id}/${action}/`, - {}, - ) - .then(() => { - data - .get(`${Project.api}features/workflows/change-requests/${id}/`) - .then(async (res) => { - store.model[id] = await transformChangeRequest(res) - cb && cb() - store.loaded() - getStore().dispatch( - changeRequestService.util.invalidateTags(['ChangeRequest']), - ) - getStore().dispatch( - featureStateService.util.invalidateTags(['FeatureState']), - ) - }) - }) - .catch((e) => API.ajaxHandler(store, e)) - }, - deleteChangeRequest: (id, cb) => { - store.loading() - data - .delete(`${Project.api}features/workflows/change-requests/${id}/`) - .then(() => { - store.loaded() - getStore().dispatch( - changeRequestService.util.invalidateTags(['ChangeRequest']), - ) - cb() - }) - .catch((e) => API.ajaxHandler(store, e)) - }, - getChangeRequest: (id, projectId, environmentId) => { - store.loading() - data - .get(`${Project.api}features/workflows/change-requests/${id}/`) - .then(async (apiResponse) => { - const res = await transformChangeRequest(apiResponse) - const feature = - res.feature_states[0]?.feature || res.change_sets[0]?.feature - return Promise.all([ - data.get( - `${Project.api}environments/${environmentId}/featurestates/?feature=${feature}`, - ), - data.get(`${Project.api}projects/${projectId}/features/${feature}/`), - ]) - .then(([environmentFlag, projectFlag]) => { - store.flags[id] = { - environmentFlag: environmentFlag.results[0], - // Every other read of a feature sorts these, so without it the - // change request lists the variations in a different order. - projectFlag: { - ...projectFlag, - multivariate_options: - projectFlag.multivariate_options && - sortMultivariateOptions(projectFlag.multivariate_options), - }, - } - }) - .finally(() => { - store.model[id] = res - store.loaded() - }) - }) - .catch((e) => API.ajaxHandler(store, e)) - }, - updateChangeRequest: (changeRequest) => { - store.loading() - data - .get( - `${Project.api}features/workflows/change-requests/${changeRequest.id}/`, - ) - .then((res) => { - data - .put( - `${Project.api}features/workflows/change-requests/${changeRequest.id}/`, - { - ...res, - approvals: changeRequest.approvals, - description: changeRequest.description, - environment_feature_versions: - changeRequest?.environment_feature_versions?.map((v) => v.uuid), - group_assignments: changeRequest.group_assignments, - title: changeRequest.title, - }, - ) - .then(async () => { - const res = await data.get( - `${Project.api}features/workflows/change-requests/${changeRequest.id}/`, - ) - store.model[changeRequest.id] = await transformChangeRequest(res) - getStore().dispatch( - changeRequestService.util.invalidateTags(['ChangeRequest']), - ) - store.loaded() - }) - .catch((e) => API.ajaxHandler(store, e)) - }) - }, -} - -const store = Object.assign({}, BaseStore, { - committed: {}, - flags: {}, - id: 'change-request-store', - model: {}, - scheduled: {}, -}) - -store.dispatcherIndex = Dispatcher.register(store, (payload) => { - const action = payload.action // this is our action from handleViewAction - switch (action.actionType) { - case Actions.GET_CHANGE_REQUEST: - controller.getChangeRequest( - action.id, - parseInt(action.projectId), - action.environmentId, - ) - break - case Actions.UPDATE_CHANGE_REQUEST: - controller.updateChangeRequest(action.changeRequest) - break - case Actions.DELETE_CHANGE_REQUEST: - controller.deleteChangeRequest(action.id, action.cb) - break - case Actions.ACTION_CHANGE_REQUEST: - controller.actionChangeRequest(action.id, action.action, action.cb) - break - default: - break - } -}) -controller.store = store -export default controller.store diff --git a/frontend/common/stores/feature-list-store.ts b/frontend/common/stores/feature-list-store.ts index 4d7e3e3a85a1..8d75720f1316 100644 --- a/frontend/common/stores/feature-list-store.ts +++ b/frontend/common/stores/feature-list-store.ts @@ -536,9 +536,6 @@ const controller = { commit: boolean, ) => { store.saving() - const changeRequestsRtk = Utils.getFlagsmithHasFeature( - 'change_requests_rtk', - ) try { API.trackEvent(Constants.events.EDIT_FEATURE) const env: Environment = ProjectStore.getEnvironment(environmentId) as any @@ -670,7 +667,6 @@ const controller = { : `${Project.api}environments/${environmentId}/create-change-request/` return data[reqType](url, req).then( (updatedChangeRequest: ChangeRequest) => { - let prom = Promise.resolve() const shouldUpdateMv = multivariate_options && updatedChangeRequest.feature_states?.[0] if (shouldUpdateMv) { @@ -697,7 +693,7 @@ const controller = { ) } - prom = ( + void ( shouldUpdateMv || (commit && ignore_conflicts) ? updateChangeRequest( getStore(), @@ -711,43 +707,17 @@ const controller = { : Promise.resolve() ).then(async () => { if (commit) { - if (changeRequestsRtk) { - const res = await actionChangeRequest(getStore(), { - actionType: 'commit', - id: `${updatedChangeRequest.id}`, - }) - if (!('error' in res)) { - AppActions.refreshFeatures(projectId, environmentId) - } - return + const res = await actionChangeRequest(getStore(), { + actionType: 'commit', + id: `${updatedChangeRequest.id}`, + }) + if (!('error' in res)) { + AppActions.refreshFeatures(projectId, environmentId) } - AppActions.actionChangeRequest( - updatedChangeRequest.id, - 'commit', - () => { - AppActions.refreshFeatures(projectId, environmentId) - }, - ) } else { getStore().dispatch( changeRequestService.util.invalidateTags(['ChangeRequest']), ) - if (!changeRequestsRtk) { - AppActions.getChangeRequest( - updatedChangeRequest.id, - projectId, - environmentId, - ) - } - } - }) - prom.then(() => { - if (featureStateId && !changeRequestsRtk) { - AppActions.getChangeRequest( - changeRequestData.id, - projectId, - environmentId, - ) } }) return updatedChangeRequest diff --git a/frontend/web/components/pages/SegmentPage.tsx b/frontend/web/components/pages/SegmentPage.tsx index 6f0dc80e166a..0adb06cf573c 100644 --- a/frontend/web/components/pages/SegmentPage.tsx +++ b/frontend/web/components/pages/SegmentPage.tsx @@ -5,7 +5,6 @@ import { useHistory, useRouteMatch } from 'react-router-dom' import CreateSegment from 'components/modals/CreateSegment' import ProjectStore from 'common/stores/project-store' import { Environment } from 'common/types/responses' -import ChangeRequestStore from 'common/stores/change-requests-store' import Breadcrumb from 'components/Breadcrumb' import { useHasPermission } from 'common/providers/Permission' import Icon from 'components/icons/Icon' @@ -41,7 +40,6 @@ const SegmentPage: FC = ({}) => { } return (
Date: Mon, 9 Nov 2026 17:18:00 -0300 Subject: [PATCH 4/4] fix(Change requests): Clear an earlier action's error when the next one starts Each mutation keeps its last error, so a failed approve stayed on screen after a later successful change. The not-found and error message rules move to requestError.ts, shared by the hook and the query, with unit tests. Co-Authored-By: Claude Opus 5.5 --- .../common/hooks/useChangeRequestDetail.ts | 34 +++++------ frontend/common/services/useChangeRequest.ts | 3 +- .../utils/__tests__/requestError.test.ts | 56 +++++++++++++++++++ frontend/common/utils/requestError.ts | 21 +++++++ 4 files changed, 92 insertions(+), 22 deletions(-) create mode 100644 frontend/common/utils/__tests__/requestError.test.ts create mode 100644 frontend/common/utils/requestError.ts diff --git a/frontend/common/hooks/useChangeRequestDetail.ts b/frontend/common/hooks/useChangeRequestDetail.ts index 1d8da0d0004c..dab773f00fd2 100644 --- a/frontend/common/hooks/useChangeRequestDetail.ts +++ b/frontend/common/hooks/useChangeRequestDetail.ts @@ -1,5 +1,3 @@ -import type { FetchBaseQueryError } from '@reduxjs/toolkit/query' -import type { SerializedError } from '@reduxjs/toolkit' import { ChangeRequest, FeatureState, @@ -11,6 +9,7 @@ import { useGetChangeRequestQuery, useUpdateChangeRequestAssigneesMutation, } from 'common/services/useChangeRequest' +import { isNotFoundError, toErrorMessage } from 'common/utils/requestError' export type ChangeRequestAssignees = Pick< ChangeRequest, @@ -36,22 +35,6 @@ export type ChangeRequestDetail = { updateAssignees: (assignees: ChangeRequestAssignees) => void } -type RequestError = FetchBaseQueryError | SerializedError | undefined - -const hasDetail = (data: unknown): data is { detail: string } => - typeof data === 'object' && - data !== null && - 'detail' in data && - typeof data.detail === 'string' - -// Match the Flux store, which shows the API's detail message on its own -const toErrorMessage = (error: RequestError) => { - if (error && 'data' in error && hasDetail(error.data)) { - return error.data.detail - } - return error -} - export const useChangeRequestDetail = ({ environmentId, id, @@ -63,7 +46,7 @@ export const useChangeRequestDetail = ({ isFetching, } = useGetChangeRequestQuery( { environmentId, id, projectId }, - // Like the Flux store, refetch after an action, not on window focus + // Refetch after an action, not on window focus { refetchOnFocus: false }, ) const [action, actionState] = useActionChangeRequestMutation() @@ -78,28 +61,37 @@ export const useChangeRequestDetail = ({ deleteState.isLoading const error = actionState.error || updateState.error || deleteState.error || loadError + // Each mutation keeps its last error, so clear them when a new action starts + const clearErrors = () => { + actionState.reset() + updateState.reset() + deleteState.reset() + } return { approve: () => { + clearErrors() action({ actionType: 'approve', id }) }, changeRequest: data?.changeRequest, commit: async (onDone) => { + clearErrors() const res = await action({ actionType: 'commit', id }) if (!('error' in res)) onDone() }, environmentFlag: data?.environmentFlag, error: toErrorMessage(error), isUpdating, - notFound: - !data && !!loadError && 'status' in loadError && loadError.status === 404, + notFound: !data && isNotFoundError(loadError), projectFlag: data?.projectFlag, remove: async (onDone) => { + clearErrors() const res = await deleteChangeRequest({ id: Number(id) }) if (!('error' in res)) onDone() }, updateAssignees: (assignees) => { if (isUpdating || !data) return + clearErrors() updateAssignees({ ...assignees, id }) }, } diff --git a/frontend/common/services/useChangeRequest.ts b/frontend/common/services/useChangeRequest.ts index 15941852dfcc..de82b5478e47 100644 --- a/frontend/common/services/useChangeRequest.ts +++ b/frontend/common/services/useChangeRequest.ts @@ -13,6 +13,7 @@ import sortBy from 'lodash/sortBy' import moment from 'moment' import transformCorePaging from 'common/transformCorePaging' import { sortMultivariateOptions } from 'common/utils/multivariate' +import { isNotFoundError } from 'common/utils/requestError' import { addFeatureSegmentsToFeatureStates, featureStateService, @@ -121,7 +122,7 @@ export const changeRequestService = service baseQuery({ url: `projects/${projectId}/features/${feature}/` }), ]) // A 404 means the feature was deleted since, which the page explains - if (projectFlagRes.error?.status === 404) { + if (isNotFoundError(projectFlagRes.error)) { return { data: { changeRequest } } } if (projectFlagRes.error) return { error: projectFlagRes.error } diff --git a/frontend/common/utils/__tests__/requestError.test.ts b/frontend/common/utils/__tests__/requestError.test.ts new file mode 100644 index 000000000000..035101000172 --- /dev/null +++ b/frontend/common/utils/__tests__/requestError.test.ts @@ -0,0 +1,56 @@ +import { isNotFoundError, toErrorMessage } from 'common/utils/requestError' + +describe('isNotFoundError', () => { + it('is true for a 404', () => { + expect(isNotFoundError({ data: {}, status: 404 })).toBe(true) + }) + + it.each([ + ['no error', undefined], + ['a server error', { data: {}, status: 500 }], + ['a permission error', { data: {}, status: 403 }], + ['a network error', { error: 'Failed to fetch', status: 'FETCH_ERROR' }], + ['a serialized error', { message: 'Boom', name: 'Error' }], + ] as const)('is false for %s', (_, error) => { + expect(isNotFoundError(error)).toBe(false) + }) +}) + +describe('toErrorMessage', () => { + it("returns the API's detail message", () => { + // Given + const error = { data: { detail: 'Already committed.' }, status: 400 } + + // When + const message = toErrorMessage(error) + + // Then + expect(message).toBe('Already committed.') + }) + + it('returns field errors as they are', () => { + // Given + const error = { data: { title: ['This field is required.'] }, status: 400 } + + // When + const message = toErrorMessage(error) + + // Then + expect(message).toBe(error) + }) + + it('returns errors without a body as they are', () => { + // Given + const error = { message: 'Boom', name: 'Error' } + + // When + const message = toErrorMessage(error) + + // Then + expect(message).toBe(error) + }) + + it('returns nothing when there is no error', () => { + expect(toErrorMessage(undefined)).toBeUndefined() + }) +}) diff --git a/frontend/common/utils/requestError.ts b/frontend/common/utils/requestError.ts new file mode 100644 index 000000000000..20181007c429 --- /dev/null +++ b/frontend/common/utils/requestError.ts @@ -0,0 +1,21 @@ +import type { FetchBaseQueryError } from '@reduxjs/toolkit/query' +import type { SerializedError } from '@reduxjs/toolkit' + +export type RequestError = FetchBaseQueryError | SerializedError | undefined + +export const isNotFoundError = (error: RequestError) => + !!error && 'status' in error && error.status === 404 + +const hasDetail = (data: unknown): data is { detail: string } => + typeof data === 'object' && + data !== null && + 'detail' in data && + typeof data.detail === 'string' + +// Show the API's detail message on its own, not the whole error object +export const toErrorMessage = (error: RequestError) => { + if (error && 'data' in error && hasDetail(error.data)) { + return error.data.detail + } + return error +}