From ed6efff643cfe4b3a8e08bcad08305f5e6d1d9d6 Mon Sep 17 00:00:00 2001 From: Youssef Khedher Date: Thu, 28 Oct 2021 12:32:29 +0100 Subject: [PATCH] Fix/variants: Fix delete one variant + remove switch when add first variant (#466) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ivar Conradi Østhus Co-authored-by: Fredrik Oseberg --- .../feature-toggle/feature.spec.js | 2 +- frontend/src/component/common/util.js | 3 + .../feature/FeatureView/FeatureView.jsx | 12 ++ .../AddFeatureVariant/AddFeatureVariant.tsx | 132 ++++++++++++------ .../FeatureVariantsList.tsx | 36 +++-- .../RedirectFeatureView.tsx | 4 +- .../feature/RedirectFeatureView/index.ts | 3 + .../__snapshots__/routes-test.jsx.snap | 9 ++ frontend/src/component/menu/routes.js | 9 ++ .../src/hooks/api/actions/useApi/useApi.ts | 6 +- frontend/src/store/api-helper.js | 17 ++- 11 files changed, 175 insertions(+), 58 deletions(-) diff --git a/frontend/cypress/integration/feature-toggle/feature.spec.js b/frontend/cypress/integration/feature-toggle/feature.spec.js index f9a1488bd3..6b9168e2fa 100644 --- a/frontend/cypress/integration/feature-toggle/feature.spec.js +++ b/frontend/cypress/integration/feature-toggle/feature.spec.js @@ -276,8 +276,8 @@ describe('feature toggle', () => { cy.wait('@variantcreation'); }); it('Can set weight to fixed value for one of the variants', () => { - const variantName = 'my-new-variant'; cy.wait(500); + cy.visit(`/projects/default/features2/${featureToggleName}/variants`); cy.get('[data-test=VARIANT_EDIT_BUTTON]').first().click(); cy.get('[data-test=VARIANT_NAME_INPUT]') diff --git a/frontend/src/component/common/util.js b/frontend/src/component/common/util.js index 01bca94b6f..4f3ec6af1d 100644 --- a/frontend/src/component/common/util.js +++ b/frontend/src/component/common/util.js @@ -50,6 +50,9 @@ export const trim = value => { }; export function updateWeight(variants, totalWeight) { + if (variants.length === 0){ + return []; + } const variantMetadata = variants.reduce( ({ remainingPercentage, variableVariantCount }, variant) => { if (variant.weight && variant.weightType === weightTypes.FIX) { diff --git a/frontend/src/component/feature/FeatureView/FeatureView.jsx b/frontend/src/component/feature/FeatureView/FeatureView.jsx index dca3fc6f9a..49ea3a6ced 100644 --- a/frontend/src/component/feature/FeatureView/FeatureView.jsx +++ b/frontend/src/component/feature/FeatureView/FeatureView.jsx @@ -40,6 +40,8 @@ import { projectFilterGenerator } from '../../../utils/project-filter-generator' import { getToggleCopyPath } from '../../../utils/route-path-helpers'; import useFeatureApi from '../../../hooks/api/actions/useFeatureApi/useFeatureApi'; import useToast from '../../../hooks/useToast'; +import useUiConfig from '../../../hooks/api/getters/useUiConfig/useUiConfig'; +import { getTogglePath } from '../../../utils/route-path-helpers'; const FeatureView = ({ activeTab, @@ -68,6 +70,16 @@ const FeatureView = ({ const { changeFeatureProject } = useFeatureApi(); const { toast, setToastData } = useToast(); const archive = !Boolean(isFeatureView); + const { uiConfig } = useUiConfig(); + + useEffect(() => { + if(uiConfig.flags.E && featureToggle && featureToggle.project) { + const newTogglePAth = getTogglePath(featureToggle.project, featureToggleName, true); + history.push(newTogglePAth); + } + + }, [featureToggleName, uiConfig, featureToggle, history]) + useEffect(() => { scrollToTop(); diff --git a/frontend/src/component/feature/FeatureView2/FeatureVariants/FeatureVariantsList/AddFeatureVariant/AddFeatureVariant.tsx b/frontend/src/component/feature/FeatureView2/FeatureVariants/FeatureVariantsList/AddFeatureVariant/AddFeatureVariant.tsx index 45aa1221bf..4d9a3b7153 100644 --- a/frontend/src/component/feature/FeatureView2/FeatureVariants/FeatureVariantsList/AddFeatureVariant/AddFeatureVariant.tsx +++ b/frontend/src/component/feature/FeatureView2/FeatureVariants/FeatureVariantsList/AddFeatureVariant/AddFeatureVariant.tsx @@ -1,4 +1,4 @@ -import React, { useEffect, useState } from 'react'; +import { useEffect, useState } from 'react'; import PropTypes from 'prop-types'; import { FormControl, @@ -21,6 +21,11 @@ import Dialogue from '../../../../../common/Dialogue'; import { trim, modalStyles } from '../../../../../common/util'; import PermissionSwitch from '../../../../../common/PermissionSwitch/PermissionSwitch'; import { UPDATE_FEATURE } from '../../../../../providers/AccessProvider/permissions'; +import useFeature from '../../../../../../hooks/api/getters/useFeature/useFeature'; +import { useParams } from 'react-router-dom'; +import { IFeatureViewParams } from '../../../../../../interfaces/params'; +import { IFeatureVariant } from '../../../../../../interfaces/featureToggle'; +import cloneDeep from 'lodash.clonedeep'; const payloadOptions = [ { key: 'string', label: 'string' }, @@ -34,8 +39,8 @@ const AddVariant = ({ showDialog, closeDialog, save, - validateName, editVariant, + validateName, title, editing, }) => { @@ -44,6 +49,9 @@ const AddVariant = ({ const [overrides, setOverrides] = useState([]); const [error, setError] = useState({}); const commonStyles = useCommonStyles(); + const { projectId, featureId } = useParams(); + const { feature } = useFeature(projectId, featureId); + const [variants, setVariants] = useState([]); const clear = () => { if (editVariant) { @@ -55,6 +63,8 @@ const AddVariant = ({ }); if (editVariant.payload) { setPayload(editVariant.payload); + } else { + setPayload(EMPTY_PAYLOAD); } if (editVariant.overrides) { setOverrides(editVariant.overrides); @@ -69,6 +79,16 @@ const AddVariant = ({ setError({}); }; + const setClonedVariants = clonedVariants => + setVariants(cloneDeep(clonedVariants)); + + useEffect(() => { + if (feature) { + setClonedVariants(feature.variants); + } + /* eslint-disable-next-line react-hooks/exhaustive-deps */ + }, [feature.variants]); + useEffect(() => { clear(); // eslint-disable-next-line react-hooks/exhaustive-deps @@ -119,10 +139,15 @@ const AddVariant = ({ clear(); closeDialog(); } catch (error) { - if (error.message.includes('duplicate value')) { + if (error?.body?.details[0]?.message?.includes('duplicate value')) { setError({ name: 'A variant with that name already exists.' }); + } else if ( + error?.body?.details[0]?.message?.includes('must be a number') + ) { + setError({ weight: 'Weight must be a number' }); } else { - const msg = error.message || 'Could not add variant'; + const msg = + error?.body?.details[0]?.message || 'Could not add variant'; setError({ general: msg }); } } @@ -215,46 +240,66 @@ const AddVariant = ({ />
- - - % - - ), - }} - style={{ marginRight: '0.8rem' }} - value={data.weight || ''} - error={Boolean(error.weight)} - type="number" - disabled={!isFixWeight} - onChange={setVariantValue} - /> - - - - 0} + show={ + + + + } + label="Custom percentage" /> - } - label="Custom percentage" - /> - - + + + } + /> + + + + % + + ), + }} + style={{ marginRight: '0.8rem' }} + value={data.weight} + error={Boolean(error.weight)} + helperText={error.weight} + type="number" + disabled={!isFixWeight} + onChange={e => { + setVariantValue(e); + }} + /> + + } + />

Payload @@ -339,6 +384,7 @@ AddVariant.propTypes = { closeDialog: PropTypes.func.isRequired, save: PropTypes.func.isRequired, validateName: PropTypes.func.isRequired, + validateWeight: PropTypes.func.isRequired, editVariant: PropTypes.object, title: PropTypes.string, uiConfig: PropTypes.object, diff --git a/frontend/src/component/feature/FeatureView2/FeatureVariants/FeatureVariantsList/FeatureVariantsList.tsx b/frontend/src/component/feature/FeatureView2/FeatureVariants/FeatureVariantsList/FeatureVariantsList.tsx index aeb57f2e0d..ec8ea38dea 100644 --- a/frontend/src/component/feature/FeatureView2/FeatureVariants/FeatureVariantsList/FeatureVariantsList.tsx +++ b/frontend/src/component/feature/FeatureView2/FeatureVariants/FeatureVariantsList/FeatureVariantsList.tsx @@ -162,7 +162,6 @@ const FeatureOverviewVariants = () => { }; const removeVariant = async (name: string) => { - console.log(`Removing variant ${name}`); let updatedVariants = variants.filter(v => v.name !== name); try { await updateVariants( @@ -204,17 +203,21 @@ const FeatureOverviewVariants = () => { successText: string ) => { const newVariants = updateWeight(variants, 1000); - const patch = createPatch(newVariants); if (patch.length === 0) return; - await patchFeatureToggle(projectId, featureId, patch); - refetch(); - setToastData({ - show: true, - type: 'success', - text: successText, - }); + await patchFeatureToggle(projectId, featureId, patch) + .then(() => { + refetch(); + setToastData({ + show: true, + type: 'success', + text: successText, + }); + }) + .catch(e => { + throw e; + }); }; const validateName = (name: string) => { @@ -222,6 +225,14 @@ const FeatureOverviewVariants = () => { return { name: 'Name is required' }; } }; + + const validateWeight = (weight: number) => { + const weightValue = parseInt(weight); + if (weightValue > 100 || weightValue < 0) { + return { weight: 'weight must be between 0 and 100' }; + } + }; + const delDialogueMarkup = useDeleteVariantMarkup({ show: delDialog.show, onClick: e => { @@ -280,7 +291,11 @@ const FeatureOverviewVariants = () => { { setEditing(false); - setEditVariant({}); + if (variants.length === 0) { + setEditVariant({ weight: 1000 }); + } else { + setEditVariant({ weightType: 'variable' }); + } setShowAddVariant(true); }} className={styles.addVariantButton} @@ -305,6 +320,7 @@ const FeatureOverviewVariants = () => { }} editing={editing} validateName={validateName} + validateWeight={validateWeight} editVariant={editVariant} title={editing ? 'Edit variant' : 'Add variant'} /> diff --git a/frontend/src/component/feature/RedirectFeatureView/RedirectFeatureView.tsx b/frontend/src/component/feature/RedirectFeatureView/RedirectFeatureView.tsx index 4f0adb4db8..3d97d1ae4a 100644 --- a/frontend/src/component/feature/RedirectFeatureView/RedirectFeatureView.tsx +++ b/frontend/src/component/feature/RedirectFeatureView/RedirectFeatureView.tsx @@ -6,11 +6,13 @@ interface IRedirectFeatureViewProps { featureToggle: any; features: any; fetchFeatureToggles: () => void; + newPath: boolean; } const RedirectFeatureView = ({ featureToggle, fetchFeatureToggles, + newPath = false, }: IRedirectFeatureViewProps) => { useEffect(() => { if (!featureToggle) { @@ -22,7 +24,7 @@ const RedirectFeatureView = ({ if (!featureToggle) return null; return ( ); }; diff --git a/frontend/src/component/feature/RedirectFeatureView/index.ts b/frontend/src/component/feature/RedirectFeatureView/index.ts index 8b5389521f..d9e2b5b595 100644 --- a/frontend/src/component/feature/RedirectFeatureView/index.ts +++ b/frontend/src/component/feature/RedirectFeatureView/index.ts @@ -4,8 +4,11 @@ import { fetchFeatureToggles } from '../../../store/feature-toggle/actions'; import RedirectFeatureView from './RedirectFeatureView'; +import { E } from '../../common/flags'; + export default connect( (state, props) => ({ + newPath: !!state.uiConfig.toJS().flags[E], featureToggle: state.features .toJS() .find(toggle => toggle.name === props.featureToggleName), diff --git a/frontend/src/component/menu/__tests__/__snapshots__/routes-test.jsx.snap b/frontend/src/component/menu/__tests__/__snapshots__/routes-test.jsx.snap index 5872d21257..ad801ba46d 100644 --- a/frontend/src/component/menu/__tests__/__snapshots__/routes-test.jsx.snap +++ b/frontend/src/component/menu/__tests__/__snapshots__/routes-test.jsx.snap @@ -75,6 +75,15 @@ Array [ "title": "Create", "type": "protected", }, + Object { + "component": [Function], + "layout": "main", + "menu": Object {}, + "parent": "/features", + "path": "/projects/:projectId/features/:name", + "title": ":name", + "type": "protected", + }, Object { "component": [Function], "flag": "P", diff --git a/frontend/src/component/menu/routes.js b/frontend/src/component/menu/routes.js index 48c9714224..d5c562a03e 100644 --- a/frontend/src/component/menu/routes.js +++ b/frontend/src/component/menu/routes.js @@ -117,6 +117,15 @@ export const routes = [ layout: 'main', menu: {}, }, + { + path: '/projects/:projectId/features/:name', + parent: '/features', + title: ':name', + component: RedirectFeatureViewPage, + type: 'protected', + layout: 'main', + menu: {}, + }, { path: '/projects/:id/:activeTab', parent: '/projects', diff --git a/frontend/src/hooks/api/actions/useApi/useApi.ts b/frontend/src/hooks/api/actions/useApi/useApi.ts index 06ce23277e..a81e39e0a2 100644 --- a/frontend/src/hooks/api/actions/useApi/useApi.ts +++ b/frontend/src/hooks/api/actions/useApi/useApi.ts @@ -8,6 +8,7 @@ import { } from '../../../../constants/statusCodes'; import { AuthenticationError, + BadRequestError, ForbiddenError, headers, NotFoundError, @@ -107,7 +108,8 @@ const useAPI = ({ } if (propagateErrors) { - throw new Error(); + const response = await res.json(); + throw new BadRequestError(res.status, response); } } @@ -160,7 +162,7 @@ const useAPI = ({ if (res.status > 399) { const response = await res.json(); - + if (response?.details?.length > 0) { const error = response.details[0]; if (propagateErrors) { diff --git a/frontend/src/store/api-helper.js b/frontend/src/store/api-helper.js index 89d4a17603..e528135295 100644 --- a/frontend/src/store/api-helper.js +++ b/frontend/src/store/api-helper.js @@ -28,13 +28,28 @@ export class AuthenticationError extends Error { export class ForbiddenError extends Error { constructor(statusCode, body = {}) { - super(body.details?.length > 0 ? body.details[0].message : 'You cannot perform this action'); + super( + body.details?.length > 0 + ? body.details[0].message + : 'You cannot perform this action' + ); this.name = 'ForbiddenError'; this.statusCode = statusCode; this.body = body; } } +export class BadRequestError extends Error { + constructor(statusCode, body = {}) { + super( + body.details?.length > 0 ? body.details[0].message : 'Bad request' + ); + this.name = 'BadRequestError'; + this.statusCode = statusCode; + this.body = body; + } +} + export class NotFoundError extends Error { constructor(statusCode) { super(