diff --git a/packages/api-v4/src/object-storage/objectStorageKeys.ts b/packages/api-v4/src/object-storage/objectStorageKeys.ts index 6dd4d417adb..df8f3b0ce48 100644 --- a/packages/api-v4/src/object-storage/objectStorageKeys.ts +++ b/packages/api-v4/src/object-storage/objectStorageKeys.ts @@ -1,4 +1,7 @@ -import { createObjectStorageKeysSchema } from '@linode/validation/lib/objectStorageKeys.schema'; +import { + createObjectStorageKeysSchema, + updateObjectStorageKeysSchema, +} from '@linode/validation/lib/objectStorageKeys.schema'; import { API_ROOT } from '../constants'; import Request, { setData, @@ -51,7 +54,7 @@ export const updateObjectStorageKey = ( Request( setMethod('PUT'), setURL(`${API_ROOT}/object-storage/keys/${encodeURIComponent(id)}`), - setData(data, createObjectStorageKeysSchema) + setData(data, updateObjectStorageKeysSchema) ); /** diff --git a/packages/manager/.changeset/pr-10118-upcoming-features-1706544516803.md b/packages/manager/.changeset/pr-10118-upcoming-features-1706544516803.md new file mode 100644 index 00000000000..8096f0f7f20 --- /dev/null +++ b/packages/manager/.changeset/pr-10118-upcoming-features-1706544516803.md @@ -0,0 +1,5 @@ +--- +"@linode/manager": Upcoming Features +--- + +"Save" button in Edit Access Key drawer disabled unless field values are changed ([#10118](https://github.com/linode/manager/pull/10118)) diff --git a/packages/manager/src/features/Linodes/LinodesCreate/LinodeCreateContainer.tsx b/packages/manager/src/features/Linodes/LinodesCreate/LinodeCreateContainer.tsx index 84863f4f2f7..770e8708e11 100644 --- a/packages/manager/src/features/Linodes/LinodesCreate/LinodeCreateContainer.tsx +++ b/packages/manager/src/features/Linodes/LinodesCreate/LinodeCreateContainer.tsx @@ -698,10 +698,10 @@ class LinodeCreateContainer extends React.PureComponent { selectedTypeID: this.params.typeID, showGDPRCheckbox: Boolean( !this.props.profile.data?.restricted && - isEURegion( - getSelectedRegionGroup(this.props.regionsData, this.params.regionID) - ) && - this.props.agreements?.data?.eu_model + isEURegion( + getSelectedRegionGroup(this.props.regionsData, this.params.regionID) + ) && + this.props.agreements?.data?.eu_model ), signedAgreement: false, }; @@ -833,10 +833,10 @@ class LinodeCreateContainer extends React.PureComponent { const request = createType === 'fromLinode' ? () => - this.props.linodeActions.cloneLinode({ - sourceLinodeId: linodeID!, - ...payload, - }) + this.props.linodeActions.cloneLinode({ + sourceLinodeId: linodeID!, + ...payload, + }) : () => this.props.linodeActions.createLinode(payload); this.setState({ formIsSubmitting: true }); diff --git a/packages/manager/src/features/ObjectStorage/AccessKeyLanding/AccessKeyDrawer.tsx b/packages/manager/src/features/ObjectStorage/AccessKeyLanding/AccessKeyDrawer.tsx index 9a078e7a585..f461ee44804 100644 --- a/packages/manager/src/features/ObjectStorage/AccessKeyLanding/AccessKeyDrawer.tsx +++ b/packages/manager/src/features/ObjectStorage/AccessKeyLanding/AccessKeyDrawer.tsx @@ -6,7 +6,7 @@ import { Scope, } from '@linode/api-v4/lib/object-storage'; import { createObjectStorageKeysSchema } from '@linode/validation/lib/objectStorageKeys.schema'; -import { Formik } from 'formik'; +import { Formik, FormikProps } from 'formik'; import * as React from 'react'; import { ActionsPanel } from 'src/components/ActionsPanel/ActionsPanel'; @@ -33,7 +33,10 @@ export interface AccessKeyDrawerProps { // If the mode is 'editing', we should have an ObjectStorageKey to edit objectStorageKey?: ObjectStorageKey; onClose: () => void; - onSubmit: (values: ObjectStorageKeyRequest, formikProps: any) => void; + onSubmit: ( + values: ObjectStorageKeyRequest, + formikProps: FormikProps + ) => void; open: boolean; } @@ -120,7 +123,10 @@ export const AccessKeyDrawer = (props: AccessKeyDrawerProps) => { label: initialLabelValue, }; - const handleSubmit = (values: ObjectStorageKeyRequest, formikProps: any) => { + const handleSubmit = ( + values: ObjectStorageKeyRequest, + formikProps: FormikProps + ) => { // If the user hasn't toggled the Limited Access button, // don't include any bucket_access information in the payload. diff --git a/packages/manager/src/features/ObjectStorage/AccessKeyLanding/AccessKeyLanding.tsx b/packages/manager/src/features/ObjectStorage/AccessKeyLanding/AccessKeyLanding.tsx index dce6f9c8221..38d48b1d327 100644 --- a/packages/manager/src/features/ObjectStorage/AccessKeyLanding/AccessKeyLanding.tsx +++ b/packages/manager/src/features/ObjectStorage/AccessKeyLanding/AccessKeyLanding.tsx @@ -5,7 +5,7 @@ import { revokeObjectStorageKey, updateObjectStorageKey, } from '@linode/api-v4/lib/object-storage'; -import { FormikBag } from 'formik'; +import { FormikBag, FormikHelpers } from 'formik'; import * as React from 'react'; import { DocumentTitleSegment } from 'src/components/DocumentTitle'; @@ -96,7 +96,11 @@ export const AccessKeyLanding = (props: Props) => { const handleCreateKey = ( values: ObjectStorageKeyRequest, - { setErrors, setStatus, setSubmitting }: FormikProps + { + setErrors, + setStatus, + setSubmitting, + }: FormikHelpers ) => { // Clear out status (used for general errors) setStatus(null); @@ -152,7 +156,11 @@ export const AccessKeyLanding = (props: Props) => { const handleEditKey = ( values: ObjectStorageKeyRequest, - { setErrors, setStatus, setSubmitting }: FormikProps + { + setErrors, + setStatus, + setSubmitting, + }: FormikHelpers ) => { // This shouldn't happen, but just in case. if (!keyToEdit) { @@ -170,7 +178,7 @@ export const AccessKeyLanding = (props: Props) => { setSubmitting(true); - updateObjectStorageKey(keyToEdit.id, { label: values.label }) + updateObjectStorageKey(keyToEdit.id, values) .then((_) => { setSubmitting(false); diff --git a/packages/manager/src/features/ObjectStorage/AccessKeyLanding/AccessKeyTable/AccessKeyActionMenu.tsx b/packages/manager/src/features/ObjectStorage/AccessKeyLanding/AccessKeyTable/AccessKeyActionMenu.tsx index b8f85026d29..ac87472e441 100644 --- a/packages/manager/src/features/ObjectStorage/AccessKeyLanding/AccessKeyTable/AccessKeyActionMenu.tsx +++ b/packages/manager/src/features/ObjectStorage/AccessKeyLanding/AccessKeyTable/AccessKeyActionMenu.tsx @@ -38,7 +38,7 @@ export const AccessKeyActionMenu = ({ onClick: () => { openDrawer('editing', objectStorageKey); }, - title: 'Edit Label', + title: isObjMultiClusterEnabled ? 'Edit' : 'Edit Label', }, { onClick: () => { diff --git a/packages/manager/src/features/ObjectStorage/AccessKeyLanding/OMC_AccessKeyDrawer.tsx b/packages/manager/src/features/ObjectStorage/AccessKeyLanding/OMC_AccessKeyDrawer.tsx index 6d4979acab9..a99f0d036ab 100644 --- a/packages/manager/src/features/ObjectStorage/AccessKeyLanding/OMC_AccessKeyDrawer.tsx +++ b/packages/manager/src/features/ObjectStorage/AccessKeyLanding/OMC_AccessKeyDrawer.tsx @@ -5,9 +5,10 @@ import { ObjectStorageKey, ObjectStorageKeyRequest, Scope, + UpdateObjectStorageKeyRequest, } from '@linode/api-v4/lib/object-storage'; import { createObjectStorageKeysSchema } from '@linode/validation/lib/objectStorageKeys.schema'; -import { useFormik } from 'formik'; +import { useFormik, FormikProps } from 'formik'; import React, { useEffect, useState } from 'react'; import { ActionsPanel } from 'src/components/ActionsPanel/ActionsPanel'; @@ -28,6 +29,7 @@ import { confirmObjectStorage } from '../utilities'; import { AccessKeyRegions } from './AccessKeyRegions/AccessKeyRegions'; import { LimitedAccessControls } from './LimitedAccessControls'; import { MODE } from './types'; +import { generateUpdatePayload, hasLabelOrRegionsChanged } from './utils'; export interface AccessKeyDrawerProps { isRestrictedUser: boolean; @@ -35,7 +37,12 @@ export interface AccessKeyDrawerProps { // If the mode is 'editing', we should have an ObjectStorageKey to edit objectStorageKey?: ObjectStorageKey; onClose: () => void; - onSubmit: (values: ObjectStorageKeyRequest, formikProps: any) => void; + onSubmit: ( + values: ObjectStorageKeyRequest | UpdateObjectStorageKeyRequest, + formikProps: FormikProps< + ObjectStorageKeyRequest | UpdateObjectStorageKeyRequest + > + ) => void; open: boolean; } @@ -116,10 +123,11 @@ export const OMC_AccessKeyDrawer = (props: AccessKeyDrawerProps) => { // and so not included in Formik's types const [limitedAccessChecked, setLimitedAccessChecked] = useState(false); - const title = createMode ? 'Create Access Key' : 'Edit Access Key Label'; + const title = createMode ? 'Create Access Key' : 'Edit Access Key'; const initialLabelValue = !createMode && objectStorageKey ? objectStorageKey.label : ''; + const initialRegions = !createMode && objectStorageKey ? objectStorageKey.regions?.map((region) => region.id) @@ -148,13 +156,25 @@ export const OMC_AccessKeyDrawer = (props: AccessKeyDrawerProps) => { } : { ...values, bucket_access: null }; - onSubmit(payload, formik); + const updatePayload = generateUpdatePayload(values, initialValues); + + if (mode !== 'creating') { + onSubmit(updatePayload, formik); + } else { + onSubmit(payload, formik); + } }, validateOnBlur: true, validateOnChange: false, validationSchema: createObjectStorageKeysSchema, }); + const isSaveDisabled = + isRestrictedUser || + (mode !== 'creating' && + objectStorageKey && + !hasLabelOrRegionsChanged(formik.values, objectStorageKey)); + const beforeSubmit = () => { confirmObjectStorage( accountSettings?.object_storage || 'active', @@ -257,6 +277,7 @@ export const OMC_AccessKeyDrawer = (props: AccessKeyDrawerProps) => { 'bucket_access', getDefaultScopes(bucketsInRegions, regionsLookup) ); + formik.validateField('regions'); }} onChange={(values) => { const bucketsInRegions = buckets?.filter( @@ -287,10 +308,7 @@ export const OMC_AccessKeyDrawer = (props: AccessKeyDrawerProps) => { { + const initialValues: FormState = { + bucket_access: [], + label: 'initialLabel', + regions: ['region1', 'region2'], + }; + + it('should return empty object if no changes', () => { + const updatedValues = { ...initialValues }; + expect(generateUpdatePayload(updatedValues, initialValues)).toEqual({}); + }); + + it('should return updated label if only label changed', () => { + const updatedValues = { ...initialValues, label: 'newLabel' }; + expect(generateUpdatePayload(updatedValues, initialValues)).toEqual({ + label: 'newLabel', + }); + }); + + it('should return updated regions if only regions changed', () => { + const updatedValues = { ...initialValues, regions: ['region3', 'region4'] }; + expect(generateUpdatePayload(updatedValues, initialValues)).toEqual({ + regions: ['region3', 'region4'], + }); + }); + + it('should return updated label and regions if both changed', () => { + const updatedValues = { + bucket_access: [], + label: 'newLabel', + regions: ['region3', 'region4'], + }; + expect(generateUpdatePayload(updatedValues, initialValues)).toEqual({ + label: 'newLabel', + regions: ['region3', 'region4'], + }); + }); +}); + +describe('hasLabelOrRegionsChanged', () => { + const updatedValues: FormState = { + bucket_access: [], + label: 'initialLabel', + regions: ['region3', 'region4'], + }; + const initialValues: ObjectStorageKey = { + access_key: '', + bucket_access: null, + id: 0, + label: updatedValues.label, + limited: false, + regions: [ + { id: 'region3', s3_endpoint: '' }, + { id: 'region4', s3_endpoint: '' }, + ], + + secret_key: '', + }; + + it('returns false when both label and regions are unchanged', () => { + expect(hasLabelOrRegionsChanged(updatedValues, initialValues)).toBe(false); + }); + + it('returns true when only the label has changed', () => { + expect( + hasLabelOrRegionsChanged( + { ...updatedValues, label: 'newLabel' }, + initialValues + ) + ).toBe(true); + }); + + it('returns true when only the regions have changed', () => { + expect( + hasLabelOrRegionsChanged( + { + ...updatedValues, + regions: ['region5'], + }, + initialValues + ) + ).toBe(true); + }); + + it('returns true when both label and regions have changed', () => { + expect( + hasLabelOrRegionsChanged( + { ...updatedValues, label: 'newLabel', regions: ['region5'] }, + initialValues + ) + ).toBe(true); + }); +}); diff --git a/packages/manager/src/features/ObjectStorage/AccessKeyLanding/utils.ts b/packages/manager/src/features/ObjectStorage/AccessKeyLanding/utils.ts new file mode 100644 index 00000000000..8008022a36a --- /dev/null +++ b/packages/manager/src/features/ObjectStorage/AccessKeyLanding/utils.ts @@ -0,0 +1,73 @@ +import { ObjectStorageKey } from '@linode/api-v4/lib/object-storage'; + +import { areArraysEqual } from 'src/utilities/areArraysEqual'; +import { sortByString } from 'src/utilities/sort-by'; + +import { FormState } from './OMC_AccessKeyDrawer'; + +type UpdatePayload = + | { label: FormState['label']; regions: FormState['regions'] } + | { label: FormState['label'] } + | { regions: FormState['regions'] } + | {}; + +const sortRegionOptions = (a: string, b: string) => { + return sortByString(a, b, 'asc'); +}; + +/** + * Generates an update payload for edit access key based on changes in form values. + * + * @param {FormState} updatedValues - The current state of the form. + * @param {FormState} initialValues - The initial state of the form for comparison. + * @returns An object containing the fields that have changed. + */ +export const generateUpdatePayload = ( + updatedValues: FormState, + initialValues: FormState +): UpdatePayload => { + let updatePayload = {}; + + const labelChanged = updatedValues.label !== initialValues.label; + const regionsChanged = !areArraysEqual( + [...updatedValues.regions].sort(sortRegionOptions), + [...initialValues.regions].sort(sortRegionOptions) + ); + + if (labelChanged && regionsChanged) { + updatePayload = { + label: updatedValues.label, + regions: updatedValues.regions, + }; + } else if (labelChanged) { + updatePayload = { label: updatedValues.label }; + } else if (regionsChanged) { + updatePayload = { regions: updatedValues.regions }; + } + + return updatePayload; +}; + +/** + * Determines if there have been any changes in the label or regions + * between the updated form values and the initial values. + * + * @param {FormState} updatedValues - The current state of the form. + * @param {ObjectStorageKey} initialValues - The initial values for comparison. + * @returns {boolean} True if there are changes in label or regions, false otherwise. + */ +export const hasLabelOrRegionsChanged = ( + updatedValues: FormState, + initialValues: ObjectStorageKey +): boolean => { + const regionsChanged = !areArraysEqual( + [...updatedValues.regions].sort(sortRegionOptions), + [...initialValues.regions?.map((region) => region.id)].sort( + sortRegionOptions + ) + ); + + const labelChanged = updatedValues.label !== initialValues.label; + + return labelChanged || regionsChanged; +}; diff --git a/packages/manager/src/mocks/serverHandlers.ts b/packages/manager/src/mocks/serverHandlers.ts index 6d2552aa742..32f14cd2c61 100644 --- a/packages/manager/src/mocks/serverHandlers.ts +++ b/packages/manager/src/mocks/serverHandlers.ts @@ -1006,33 +1006,34 @@ export const handlers = [ ...objectStorageKeyFactory.buildList(1, { regions: [ { id: 'us-east', s3_endpoint: 'us-east.com' }, - { id: 'us-east', s3_endpoint: 'us-east.com' }, - { id: 'us-east', s3_endpoint: 'us-east.com' }, - { id: 'us-east', s3_endpoint: 'us-east.com' }, - { id: 'us-east', s3_endpoint: 'us-east.com' }, - { id: 'us-east', s3_endpoint: 'us-east.com' }, - { id: 'us-east', s3_endpoint: 'us-east.com' }, + { id: 'nl-ams', s3_endpoint: 'nl-ams.com' }, + { id: 'us-southeast', s3_endpoint: 'us-southeast.com' }, + { id: 'in-maa', s3_endpoint: 'in-maa.com' }, + { id: 'us-lax', s3_endpoint: 'us-lax.com' }, + { id: 'us-mia', s3_endpoint: 'us-mia.com' }, + { id: 'it-mil', s3_endpoint: 'it-mil.com' }, ], }), ...objectStorageKeyFactory.buildList(1, { regions: [ { id: 'us-east', s3_endpoint: 'us-east.com' }, - { id: 'us-east', s3_endpoint: 'us-east.com' }, - { id: 'us-east', s3_endpoint: 'us-east.com' }, - { id: 'us-east', s3_endpoint: 'us-east.com' }, - { id: 'us-east', s3_endpoint: 'us-east.com' }, + { id: 'nl-ams', s3_endpoint: 'nl-ams.com' }, + { id: 'us-southeast', s3_endpoint: 'us-southeast.com' }, + { id: 'in-maa', s3_endpoint: 'in-maa.com' }, + { id: 'us-lax', s3_endpoint: 'us-lax.com' }, ], }), ...objectStorageKeyFactory.buildList(1, { regions: [ { id: 'us-east', s3_endpoint: 'us-east.com' }, - { id: 'us-east', s3_endpoint: 'us-east.com' }, + { id: 'nl-ams', s3_endpoint: 'nl-ams.com' }, ], }), ]) ) ); }), + rest.post('*object-storage/keys', (req, res, ctx) => { const { label, regions } = req.body as ObjectStorageKeyRequest; @@ -1050,6 +1051,26 @@ export const handlers = [ ) ); }), + rest.put('*object-storage/keys/:id', (req, res, ctx) => { + const { label, regions } = req.body as ObjectStorageKeyRequest; + + const regionsData = regions?.map((region: string) => ({ + id: region, + s3_endpoint: `${region}.com`, + })); + + return res( + ctx.json( + objectStorageKeyFactory.build({ + label, + regions: regionsData, + }) + ) + ); + }), + rest.delete('*object-storage/keys/:id', (req, res, ctx) => { + return res(ctx.json({})); + }), rest.get('*/domains', (req, res, ctx) => { const domains = domainFactory.buildList(10); return res(ctx.json(makeResourcePage(domains))); diff --git a/packages/validation/src/objectStorageKeys.schema.ts b/packages/validation/src/objectStorageKeys.schema.ts index 9f9406dc3c8..b15dfe2ace8 100644 --- a/packages/validation/src/objectStorageKeys.schema.ts +++ b/packages/validation/src/objectStorageKeys.schema.ts @@ -1,10 +1,24 @@ import { object, string, array } from 'yup'; +const labelErrorMessage = 'Label must be between 3 and 50 characters.'; + export const createObjectStorageKeysSchema = object({ label: string() .required('Label is required.') - .min(3, 'Label must be between 3 and 50 characters.') - .max(50, 'Label must be between 3 and 50 characters.') + .min(3, labelErrorMessage) + .max(50, labelErrorMessage) + .trim(), + regions: array() + .of(string()) + .min(1, 'Regions must include at least one region') + .notRequired(), +}); + +export const updateObjectStorageKeysSchema = object({ + label: string() + .notRequired() + .min(3, labelErrorMessage) + .max(50, labelErrorMessage) .trim(), regions: array() .of(string())