diff --git a/frontend/common/services/useMultivariateOption.ts b/frontend/common/services/useMultivariateOption.ts new file mode 100644 index 000000000000..03989c8dfae8 --- /dev/null +++ b/frontend/common/services/useMultivariateOption.ts @@ -0,0 +1,132 @@ +import { MultivariateOption, ProjectFlag, Res } from 'common/types/responses' +import { Req } from 'common/types/requests' +import { service } from 'common/service' + +export const multivariateOptionService = service.injectEndpoints({ + endpoints: (builder) => ({ + createMultivariateOption: builder.mutation< + Res['multivariateOption'], + Req['createMultivariateOption'] + >({ + query: (query) => ({ + body: query.body, + method: 'POST', + url: `projects/${query.project_id}/features/${query.feature_id}/mv-options/`, + }), + }), + saveMultivariateOptions: builder.mutation< + Res['saveMultivariateOptions'], + Req['saveMultivariateOptions'] + >({ + // No invalidatesTags: every save chain already ends with a broad + // invalidateTags(['ProjectFlag', 'FeatureList']) once the downstream + // feature-state save completes — invalidating here too would refetch + // every subscribed query twice per save. + queryFn: async (args, _, _2, baseQuery) => { + const featureUrl = `projects/${args.project_id}/features/${args.feature_id}/` + // Diff against the server's current options rather than any client + // cache, so stale state can never turn an update into a duplicate + // create. + const flagRes = await baseQuery({ method: 'GET', url: featureUrl }) + if (flagRes.error) { + return { error: flagRes.error } + } + const serverOptions = + (flagRes.data as ProjectFlag)?.multivariate_options || [] + const errors: Record = {} + // Results are written back by input index — downstream feature + // state saves map weights to option ids positionally. Requests run + // sequentially so newly created options get ascending ids in input + // order, which is the order the UI displays. + const ordered: MultivariateOption[] = [] + for (let i = 0; i < args.multivariate_options.length; i++) { + const v = args.multivariate_options[i] + let original + if (v.id) { + original = serverOptions.find((m) => m.id === v.id) + } else if (v.key) { + original = serverOptions.find((m) => !!m.key && m.key === v.key) + } + const body = { + ...v, + default_percentage_allocation: 0, + feature: args.feature_id, + } + const res = await baseQuery( + original + ? { + body, + method: 'PUT', + url: `${featureUrl}mv-options/${original.id}/`, + } + : { + body, + method: 'POST', + url: `${featureUrl}mv-options/`, + }, + ) + if (res.error) { + errors[i] = (res.error as { data?: any })?.data ?? null + } else { + ordered[i] = res.data as MultivariateOption + } + } + if (Object.keys(errors).length) { + return { data: { errors, multivariate_options: ordered } } + } + const deleted = serverOptions.filter( + (m) => !ordered.find((o) => o?.id === m.id), + ) + const deleteResults = await Promise.all( + deleted.map((m) => + baseQuery({ + method: 'DELETE', + url: `${featureUrl}mv-options/${m.id}/`, + }), + ), + ) + const failedDelete = deleteResults.find((r) => r.error) + if (failedDelete) { + return { error: failedDelete.error } + } + return { data: { errors: null, multivariate_options: ordered } } + }, + }), + // END OF ENDPOINTS + }), +}) + +export async function createMultivariateOption( + store: any, + data: Req['createMultivariateOption'], + options?: Parameters< + typeof multivariateOptionService.endpoints.createMultivariateOption.initiate + >[1], +) { + return store.dispatch( + multivariateOptionService.endpoints.createMultivariateOption.initiate( + data, + options, + ), + ) +} +export async function saveMultivariateOptions( + store: any, + data: Req['saveMultivariateOptions'], + options?: Parameters< + typeof multivariateOptionService.endpoints.saveMultivariateOptions.initiate + >[1], +) { + return store.dispatch( + multivariateOptionService.endpoints.saveMultivariateOptions.initiate( + data, + options, + ), + ) +} + +export const { + useCreateMultivariateOptionMutation, + useSaveMultivariateOptionsMutation, + // END OF EXPORTS +} = multivariateOptionService diff --git a/frontend/common/stores/feature-list-store.ts b/frontend/common/stores/feature-list-store.ts index 2dfeff5f8e8f..56ae9b431d5f 100644 --- a/frontend/common/stores/feature-list-store.ts +++ b/frontend/common/stores/feature-list-store.ts @@ -45,6 +45,10 @@ import { FEATURES_PAGE_SIZE } from 'common/services/useProjectFlag' import Dispatcher from 'common/dispatcher/dispatcher' import BaseStore from './base/_store' import data from 'common/data/base/_data' +import { + createMultivariateOption, + saveMultivariateOptions, +} from 'common/services/useMultivariateOption' import { createSegmentOverride } from 'common/services/useSegmentOverride' import { getStore } from 'common/store' let createdFirstFeature = false @@ -122,13 +126,17 @@ const controller = { // Sequential so options get ascending ids in input order, which is // the order the UI displays. for (const v of flag.multivariate_options || []) { - await data.post( - `${Project.api}projects/${projectId}/features/${res.data.id}/mv-options/`, - { + const mvRes = await createMultivariateOption(getStore(), { + body: { ...v, feature: res.data.id, }, - ) + feature_id: res.data.id, + project_id: projectId, + }) + if (mvRes.error) { + throw mvRes.error + } } return data.get( `${Project.api}projects/${projectId}/features/${res.data.id}/`, @@ -242,90 +250,45 @@ const controller = { }) return } + store.error = null const originalFlag = store.model && store.model.features ? store.model.features.find((v) => v.id === flag.id) : flag - store.error = null - Promise.all( - (flag.multivariate_options || []).map((v, i) => { - let originalMV = null - if (originalFlag?.multivariate_options) { - if (v.id) { - originalMV = originalFlag.multivariate_options.find( - (m: MultivariateOption) => m.id === v.id, - ) - } else if (v.key) { - originalMV = originalFlag.multivariate_options.find( - (m: MultivariateOption) => !!m.key && m.key === v.key, - ) - } - } - const url = `${Project.api}projects/${projectId}/features/${flag.id}/mv-options/` - const mvData = { - ...v, - default_percentage_allocation: 0, - feature: flag.id, - } - return ( - originalMV - ? data.put(`${url}${originalMV.id}/`, mvData) - : data.post(url, mvData) - ) - .then((res) => { - // It's important to preserve the original order of multivariate_options, so that editing feature states can use the updated ID - flag.multivariate_options[i] = res - return { - ...v, - id: res.id, - } - }) - .catch((e) => Promise.reject({ mvIndex: i, source: e })) - }), - ) - .then(() => { - const deletedMv = (originalFlag?.multivariate_options || []).filter( - (v) => !flag.multivariate_options.find((x) => v.id === x.id), - ) - return Promise.all( - deletedMv.map((v) => - data.delete( - `${Project.api}projects/${projectId}/features/${flag.id}/mv-options/${v.id}/`, - ), - ), - ) - }) - .then(() => { - if (onComplete) { - onComplete(flag) - } - }) - .catch((e) => { - if (typeof e?.mvIndex !== 'number') { - API.ajaxHandler(store, e) - return - } - // Attribute the failure to the option that caused it so the UI - // can surface it on the right variation. - const surface = (body: any) => { - store.error = { multivariate_options: { [e.mvIndex]: body } } as any - store.goneABitWest() - } - if (typeof e.source?.text === 'function') { - e.source - .text() - .then((text: string) => { - let body = text - try { - body = JSON.parse(text) - } catch {} - surface(body) - }) - .catch(() => surface(null)) - } else { - surface(e.source ?? null) - } - }) + // Standard flags carry no multivariate data — skip the round-trip. + if ( + !flag.multivariate_options?.length && + !originalFlag?.multivariate_options?.length + ) { + if (onComplete) { + onComplete(flag) + } + return + } + saveMultivariateOptions(getStore(), { + feature_id: flag.id, + multivariate_options: flag.multivariate_options || [], + project_id: projectId, + }).then((res) => { + if (res.error) { + API.ajaxHandler(store, res.error) + return + } + if (res.data.errors) { + store.error = { multivariate_options: res.data.errors } as any + store.goneABitWest() + return + } + // It's important to preserve the original order of multivariate_options, so that editing feature states can use the updated ID + res.data.multivariate_options.forEach( + (v: MultivariateOption, i: number) => { + flag.multivariate_options[i] = v + }, + ) + if (onComplete) { + onComplete(flag) + } + }) }, editFeatureState: async ( projectId, diff --git a/frontend/common/types/requests.ts b/frontend/common/types/requests.ts index eb67f66c6095..5fc1d3e3b228 100644 --- a/frontend/common/types/requests.ts +++ b/frontend/common/types/requests.ts @@ -1039,5 +1039,15 @@ export type Req = { body: Req['createMetric']['body'] } deleteMetric: { environmentId: string; metricId: number } + createMultivariateOption: { + project_id: string | number + feature_id: number + body: Partial & { feature: number } + } + saveMultivariateOptions: { + project_id: string | number + feature_id: number + multivariate_options: Partial[] + } // END OF TYPES } diff --git a/frontend/common/types/responses.ts b/frontend/common/types/responses.ts index 487eb2fd1431..5cc4e39ccaf2 100644 --- a/frontend/common/types/responses.ts +++ b/frontend/common/types/responses.ts @@ -1413,5 +1413,12 @@ export type Res = { experiment: Experiment metric: Metric metrics: PagedResponse + multivariateOption: MultivariateOption + saveMultivariateOptions: { + multivariate_options: MultivariateOption[] + // Per-option API errors keyed by the input option's index; null when all + // requests succeeded. + errors: Record | null + } // END OF TYPES } diff --git a/frontend/e2e/tests/mv-options-tests.pw.ts b/frontend/e2e/tests/mv-options-tests.pw.ts new file mode 100644 index 000000000000..6cd540120f9f --- /dev/null +++ b/frontend/e2e/tests/mv-options-tests.pw.ts @@ -0,0 +1,110 @@ +import { test, expect } from '../test-setup'; +import { byId, log, createHelpers, LONG_TIMEOUT } from '../helpers'; +import { E2E_USER, PASSWORD, E2E_TEST_PROJECT } from '../config'; +import type { Page } from '@playwright/test'; + +// Regression tests for the multivariate option save flow: variants must +// never duplicate on repeated saves and deletes must apply. The v2 +// feature versioning coverage lives in versioning-tests.pw.ts, which +// owns the (irreversible) versioned environment setup. + +const openFeature = async (page: Page, name: string) => { + const featureRow = page.locator('[data-test^="feature-item-"]').filter({ + has: page.locator(`span:text-is("${name}")`), + }).first(); + await featureRow.waitFor({ state: 'visible', timeout: LONG_TIMEOUT }); + await featureRow.dispatchEvent('click'); + await page.locator(byId('update-feature-btn')).first().waitFor({ state: 'visible', timeout: LONG_TIMEOUT }); +}; + +const variantCards = (page: Page) => page.locator('#create-feature-modal .variant-card'); + +test.describe('Multivariate Options', () => { + test('Repeated saves keep the variant set stable @oss', async ({ page }) => { + const { + closeModal, + createRemoteConfig, + editRemoteConfig, + editVariantLabel, + gotoFeatures, + gotoProject, + login, + waitForElementNotExist, + } = createHelpers(page); + + log('Login'); + await login(E2E_USER, PASSWORD); + await gotoProject(E2E_TEST_PROJECT); + + log('Create multivariate flag'); + await createRemoteConfig({ name: 'mv_repeat_save', value: 'ctrl_value', mvs: [ + { value: 'va', weight: 0 }, + { value: 'vb', weight: 0 }, + ]}); + + log('First save: label-only edit'); + await editVariantLabel('mv_repeat_save', 0, 'first_variant'); + + log('Second save: value and weights, without structural changes'); + await editRemoteConfig('mv_repeat_save', 'ctrl_value2', false, [ + { value: 'va', weight: 30 }, + { value: 'vb', weight: 20 }, + ]); + + log('Variant set is unchanged after both saves'); + await gotoFeatures(); + await openFeature(page, 'mv_repeat_save'); + await expect(variantCards(page)).toHaveCount(2); + await expect(page.locator(byId('featureVariationKey0'))).toHaveText('first_variant'); + await closeModal(); + await waitForElementNotExist('#create-feature-modal'); + }); + + test('Variants can be added and removed in a single save @oss', async ({ page }) => { + const { + click, + closeModal, + createRemoteConfig, + gotoFeatures, + gotoProject, + login, + setText, + waitForElementNotExist, + waitForToast, + } = createHelpers(page); + + log('Login'); + await login(E2E_USER, PASSWORD); + await gotoProject(E2E_TEST_PROJECT); + + log('Create multivariate flag'); + await createRemoteConfig({ name: 'mv_add_remove', value: 'root_val', mvs: [ + { value: 'keep', weight: 0 }, + { value: 'drop', weight: 0 }, + ]}); + + log('Remove one variant and add another in the same edit'); + await openFeature(page, 'mv_add_remove'); + await page.locator('#create-feature-modal #delete-multivariate').nth(1).click(); + await click('#confirm-btn-yes'); + await expect(variantCards(page)).toHaveCount(1); + await click(byId('add-variation')); + await page.waitForTimeout(200); + await setText(byId('featureVariationValue1'), 'added'); + await page.waitForTimeout(500); + await click(byId('update-feature-btn')); + await waitForToast(); + await closeModal(); + await waitForElementNotExist('#create-feature-modal'); + + log('Saved set is exactly the kept and added variants'); + await gotoFeatures(); + await openFeature(page, 'mv_add_remove'); + await expect(variantCards(page)).toHaveCount(2); + await expect(page.locator(byId('featureVariationWeightkeep'))).toBeVisible(); + await expect(page.locator(byId('featureVariationWeightadded'))).toBeVisible(); + await expect(page.locator(byId('featureVariationWeightdrop'))).toHaveCount(0); + await closeModal(); + await waitForElementNotExist('#create-feature-modal'); + }); +}); diff --git a/frontend/e2e/tests/versioning-tests.pw.ts b/frontend/e2e/tests/versioning-tests.pw.ts index dbf2248b894e..f512c5c5553d 100644 --- a/frontend/e2e/tests/versioning-tests.pw.ts +++ b/frontend/e2e/tests/versioning-tests.pw.ts @@ -12,16 +12,22 @@ test('Versioning tests - Create, edit, and compare feature versions @oss', async const { assertNumberOfVersions, click, + closeModal, compareVersion, createFeature, createOrganisationAndProject, createRemoteConfig, editRemoteConfig, + gotoFeature, + gotoFeatures, login, + setText, toggleFeature, tryItExpect, + waitForElementNotExist, waitForElementVisible, waitForFeatureSwitch, + waitForToast, waitForToastsToClear, } = createHelpers(page) const flagsmith = await getFlagsmith() @@ -79,6 +85,38 @@ test('Versioning tests - Create, edit, and compare feature versions @oss', async await compareVersion('b', 0, null, true, true, 'small', 'small') await compareVersion('c', 0, null, false, true, null, null) + // =================================================================================== + // Test: Multivariate option edits in a versioned environment + // A label edit plus a structural change (new variant) in a single save must + // survive a reopen — the versioned save consumes option ids positionally + // from the multivariate save, so this pins that contract under v2. + // =================================================================================== + log('Edit variant label and add a variant in one save (versioned env)') + await gotoFeatures() + await gotoFeature('b') + await click(byId('featureVariationKeyEdit0')) + await setText(byId('featureVariationKeyInput0'), 'primary') + await click(byId('featureVariationKeySave0')) + await expect(page.locator(byId('featureVariationKey0'))).toHaveText('primary') + await click(byId('add-variation')) + await page.waitForTimeout(200) + await setText(byId('featureVariationValue2'), 'huge') + await page.waitForTimeout(500) + await click(byId('update-feature-btn')) + await waitForToast() + await closeModal() + await waitForElementNotExist('#create-feature-modal') + + log('Label and new variant survived the versioned save') + await gotoFeatures() + await gotoFeature('b') + await expect(page.locator('#create-feature-modal .variant-card')).toHaveCount(3) + await expect(page.locator(byId('featureVariationKey0'))).toHaveText('primary') + await expect(page.locator(byId('featureVariationWeightbig'))).toHaveValue('100') + await expect(page.locator(byId('featureVariationWeighthuge'))).toBeVisible() + await closeModal() + await waitForElementNotExist('#create-feature-modal') + // =================================================================================== // Test: Row toggle in versioned environment // This tests that toggling a feature via the row switch works when Feature Versioning diff --git a/frontend/web/components/base/forms/InputGroup.js b/frontend/web/components/base/forms/InputGroup.js index 5d53cd1f703e..564a30f5db6c 100644 --- a/frontend/web/components/base/forms/InputGroup.js +++ b/frontend/web/components/base/forms/InputGroup.js @@ -29,7 +29,10 @@ const InputGroup = class extends Component { {this.props.tooltip ? ( +