feat(okms): edit settings drawer #19487

mathieumousnier · 2025-10-02T13:56:03Z

Description

Ticket Reference: #MANAGER-17868

Additional Information

packages/manager/apps/okms/public/translations/secret-manager/Messages_fr_FR.json

ref: #MANAGER-17868 Signed-off-by: Mathieu Mousnier <[email protected]>

tibs245 · 2025-10-07T07:45:52Z

...c/modules/secret-manager/components/form/SecretDeactivateVersionAfterFormField.component.tsx

+          <label htmlFor={field.name} slot="label" className="mb-1"></label>
+          <label slot="label" className="flex items-center gap-2 relative mb-1">
+            {t('deactivate_version_after')}
+            <HelpIconWithTooltip
+              label={t('form_tooltip_deactivate_version_after')}
+            />
+          </label>


Why htmlFor is necessary and not put it in child space ?

It's not strange to have 2 labels collapsed ?

Good catch. The empty label tag is some forgotten code.

pauldkn · 2025-10-07T07:45:06Z

...pps/okms/src/modules/secret-manager/components/form/SecretCasRequiredFormField.component.tsx

+export const casRequiredToFormValue = (
+  casRequired: boolean,
+): CasRequiredFormValue => {
+  return casRequired ? 'active' : 'inactive';
+};
+
+export const formValueToCasRequired = (
+  formValue: CasRequiredFormValue,
+): boolean => {
+  return formValue === 'active';
+};


Maybe to move in a utils file ?

I see you point, but these utility functions are purely related to the component and shouldn't be used if it is not while using the component.
When we are in this configuration I prefer to colocate the code.

tibs245 · 2025-10-07T07:56:35Z

...modules/secret-manager/pages/drawers/editMetadataDrawer/EditMetadataDrawerForm.component.tsx

+  };
+
+  return (
+    <div className="flex flex-col h-full">


It's a section ?

the drawer component already implements header, section and footer tags

tibs245 · 2025-10-07T08:00:47Z

packages/manager/apps/okms/src/modules/secret-manager/utils/queryStrings.ts

+type QueryStringValue = string | number | boolean | undefined | null;
+
+export const buildQueryString = (params: Record<string, QueryStringValue>) => {
+  const queryParams = new URLSearchParams();
+  Object.entries(params).forEach(([key, value]) => {
+    if (value !== undefined && value !== null) {
+      queryParams.set(key, String(value));
+    }
+  });
+  const queryString = queryParams.toString();
+  return queryString ? `?${queryString}` : null;
+};


May be a little unit test on it can be useful 👌

tibs245 · 2025-10-07T08:02:39Z

packages/manager/apps/okms/src/modules/secret-manager/utils/cas.ts

+export const addCurrentVersionToCas = (
+  currentVersion: number,
+  casRequired: boolean,
+  isSettingCasRequired?: boolean,
+) => {
+  if (isSettingCasRequired || casRequired) {
+    return currentVersion;
+  }
+  return undefined;
+};


As it's a very little function. I think a unit test can be quick to do and useful if you want implement refactor on future

tibs245 · 2025-10-07T08:04:05Z

...ges/manager/apps/okms/src/modules/secret-manager/validation/metadata/metadataSchema.spec.tsx

+  describe('deactivateVersionAfter field validation', () => {
+    it('should return error for invalid duration format', () => {
+      const invalidMetadata = {
+        ...MOCK_METADATA_VALID,
+        deactivateVersionAfter: 'invalid-duration',
+      };
+      const result = getSchemaParsingResult(invalidMetadata);
+      expect(result.success).toBe(false);
+      expect(result.error.issues[0].path).toEqual(['deactivateVersionAfter']);
+      expect(result.error.issues[0].message).toBe(
+        labels.secretManager.error_invalid_duration,
+      );
+    });
+
+    it('should return error for duration with invalid units', () => {
+      const invalidMetadata = {
+        ...MOCK_METADATA_VALID,
+        deactivateVersionAfter: '30x',
+      };
+      const result = getSchemaParsingResult(invalidMetadata);
+      expect(result.success).toBe(false);
+      expect(result.error.issues[0].path).toEqual(['deactivateVersionAfter']);
+      expect(result.error.issues[0].message).toBe(
+        labels.secretManager.error_invalid_duration,
+      );
+    });
+
+    it('should return error for duration with no unit', () => {
+      const invalidMetadata = {
+        ...MOCK_METADATA_VALID,
+        deactivateVersionAfter: '30',
+      };
+      const result = getSchemaParsingResult(invalidMetadata);
+      expect(result.success).toBe(false);
+      expect(result.error.issues[0].path).toEqual(['deactivateVersionAfter']);
+      expect(result.error.issues[0].message).toBe(
+        labels.secretManager.error_invalid_duration,
+      );
+    });
+
+    it('should return error for empty string', () => {
+      const invalidMetadata = {
+        ...MOCK_METADATA_VALID,
+        deactivateVersionAfter: '',
+      };
+      const result = getSchemaParsingResult(invalidMetadata);
+      expect(result.success).toBe(false);
+      expect(result.error.issues[0].path).toEqual(['deactivateVersionAfter']);
+      expect(result.error.issues[0].message).toBe(
+        labels.secretManager.error_invalid_duration,
+      );
+    });


May be you can reduce it with a it.each if you want

tibs245 · 2025-10-07T08:04:28Z

packages/manager/apps/okms/src/modules/secret-manager/validation/metadata/metadataSchema.tsx

+import { NAMESPACES } from '@ovh-ux/manager-common-translations';
+
+export const MAX_VERSIONS_MIN_VALUE = 0;
+export const MAX_VERSIONS_MAX_VALUE = 24000;


Suggested change

export const MAX_VERSIONS_MAX_VALUE = 24000;

export const MAX_VERSIONS_MAX_VALUE = 24_000;

tibs245

I think you can see If you want apply some suggestion. I think have 2 labels it's may be not the best for accessibility.

But else all is ok 👌

ref: #MANAGER-17868 Signed-off-by: Mathieu Mousnier <[email protected]>

github-actions bot added has conflicts Has conflicts to resolve before merging translation required container feature New feature and removed has conflicts Has conflicts to resolve before merging labels Oct 2, 2025

mathieumousnier changed the base branch from develop to project/secret-manager-b2 October 2, 2025 13:56

github-actions bot added the has conflicts Has conflicts to resolve before merging label Oct 2, 2025

mathieumousnier force-pushed the feat/MANAGER-17868 branch from 3c3fe6c to 7c73003 Compare October 2, 2025 14:04

github-actions bot removed container has conflicts Has conflicts to resolve before merging labels Oct 2, 2025

mathieumousnier force-pushed the feat/MANAGER-17868 branch from 7c73003 to 821b3a6 Compare October 2, 2025 14:06

mathieumousnier marked this pull request as ready for review October 2, 2025 14:10

mathieumousnier requested a review from a team as a code owner October 2, 2025 14:10

mathieumousnier requested review from rjamet-ovh and removed request for a team October 2, 2025 14:10

mathieumousnier changed the title ~~featokms): edit settings drawer~~ feat(okms): edit settings drawer Oct 2, 2025

rjamet-ovh reviewed Oct 2, 2025

View reviewed changes

packages/manager/apps/okms/public/translations/secret-manager/Messages_fr_FR.json Outdated Show resolved Hide resolved

mathieumousnier added 2 commits October 2, 2025 17:20

feat(okms): edit settings drawer

a283d1c

ref: #MANAGER-17868 Signed-off-by: Mathieu Mousnier <[email protected]>

test(okms): edit settings drawer tests

9e1f2d7

ref: #MANAGER-17868 Signed-off-by: Mathieu Mousnier <[email protected]>

mathieumousnier force-pushed the feat/MANAGER-17868 branch from 821b3a6 to 9e1f2d7 Compare October 2, 2025 15:20

rjamet-ovh approved these changes Oct 3, 2025

View reviewed changes

tibs245 reviewed Oct 7, 2025

View reviewed changes

pauldkn approved these changes Oct 7, 2025

View reviewed changes

tibs245 reviewed Oct 7, 2025

View reviewed changes

tibs245 approved these changes Oct 7, 2025

View reviewed changes

refactor(okms): improvements on edit settings drawer code

c285279

ref: #MANAGER-17868 Signed-off-by: Mathieu Mousnier <[email protected]>

mathieumousnier merged commit 1b03bc0 into project/secret-manager-b2 Oct 7, 2025
15 checks passed

mathieumousnier deleted the feat/MANAGER-17868 branch October 7, 2025 13:41

This was referenced Oct 30, 2025

project/secret-manager-batch-2 #19787

Merged

📦 New release - release/okms-w45 #20109

Merged

	export const MAX_VERSIONS_MAX_VALUE = 24000;
	export const MAX_VERSIONS_MAX_VALUE = 24_000;

feat(okms): edit settings drawer #19487

feat(okms): edit settings drawer #19487

Uh oh!

Conversation

mathieumousnier commented Oct 2, 2025

Description

Additional Information

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tibs245 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants