fix(plugin-environment-variables): show v2 submit errors (#10181)

* fix(plugin-environment-variables): show v2 submit errors

* fix(plugin-environment-variables): exclude secrets from value filters

packages/plugins/@nocobase/plugin-environment-variables/src/client-v2/__tests__/EnvironmentPage.test.tsx

@@ -0,0 +1,114 @@
+/**
+ * This file is part of the NocoBase (R) project.
+ * Copyright (c) 2020-2024 NocoBase Co., Ltd.
+ * Authors: NocoBase Team.
+ *
+ * This project is dual-licensed under AGPL-3.0 and NocoBase Commercial License.
+ * For more information, please refer to: https://www.nocobase.com/agreement.
+ */
+
+import { fireEvent, render, screen, waitFor } from '@testing-library/react';
+import { App } from 'antd';
+import React from 'react';
+import { beforeEach, describe, expect, it, vi } from 'vitest';
+import { BulkImportForm, VariableForm } from '../pages/EnvironmentPage';
+
+const state = vi.hoisted(() => ({
+  close: vi.fn(),
+  notificationError: vi.fn(),
+  request: vi.fn(),
+  toErrMessages: vi.fn(),
+}));
+
+vi.mock('@nocobase/flow-engine', async (importOriginal) => {
+  const actual = await importOriginal<typeof import('@nocobase/flow-engine')>();
+  const view = {
+    close: state.close,
+    Footer: ({ children }: { children: React.ReactNode }) => <div>{children}</div>,
+  };
+  const context = {
+    api: {
+      request: state.request,
+      toErrMessages: state.toErrMessages,
+    },
+  };
+
+  return {
+    ...actual,
+    useFlowContext: () => context,
+    useFlowEngine: () => ({ context: { t: (key: string) => key } }),
+    useFlowView: () => view,
+  };
+});
+
+describe('plugin-environment-variables client-v2 forms', () => {
+  beforeEach(() => {
+    state.close.mockReset();
+    state.notificationError.mockReset();
+    state.request.mockReset();
+    state.toErrMessages.mockReset();
+    vi.spyOn(App, 'useApp').mockReturnValue({
+      notification: { error: state.notificationError },
+    } as ReturnType<typeof App.useApp>);
+  });
+
+  it('shows the API error and keeps the drawer open when creating a duplicate variable', async () => {
+    const duplicateError = new Error('Request failed with status code 400');
+    state.request.mockRejectedValue(duplicateError);
+    state.toErrMessages.mockReturnValue([{ message: 'Name already exists' }]);
+    const onSubmitted = vi.fn();
+
+    render(<VariableForm mode="create" onSubmitted={onSubmitted} title="Add variable" />);
+
+    fireEvent.change(screen.getByLabelText('Name :'), { target: { value: 'DUPLICATE_NAME' } });
+    fireEvent.change(screen.getByLabelText('Value :'), { target: { value: 'value' } });
+    fireEvent.click(screen.getByRole('button', { name: 'Submit' }));
+
+    await waitFor(() => {
+      expect(state.notificationError).toHaveBeenCalledWith({ message: 'Name already exists' });
+    });
+    expect(state.toErrMessages).toHaveBeenCalledWith(duplicateError);
+    expect(onSubmitted).not.toHaveBeenCalled();
+    expect(state.close).not.toHaveBeenCalled();
+  });
+
+  it('refreshes the list and closes the drawer after creating a variable successfully', async () => {
+    state.request.mockResolvedValue({ data: { data: {} } });
+    const onSubmitted = vi.fn();
+
+    render(<VariableForm mode="create" onSubmitted={onSubmitted} title="Add variable" />);
+
+    fireEvent.change(screen.getByLabelText('Name :'), { target: { value: 'NEW_NAME' } });
+    fireEvent.change(screen.getByLabelText('Value :'), { target: { value: 'value' } });
+    fireEvent.click(screen.getByRole('button', { name: 'Submit' }));
+
+    await waitFor(() => {
+      expect(state.request).toHaveBeenCalledWith({
+        url: 'environmentVariables:create',
+        method: 'post',
+        data: { name: 'NEW_NAME', type: 'default', value: 'value' },
+      });
+    });
+    expect(onSubmitted).toHaveBeenCalledTimes(1);
+    expect(state.close).toHaveBeenCalledTimes(1);
+    expect(state.notificationError).not.toHaveBeenCalled();
+  });
+
+  it('shows the API error and preserves bulk import input when a duplicate variable is included', async () => {
+    state.request.mockRejectedValue(new Error('Request failed with status code 400'));
+    state.toErrMessages.mockReturnValue([{ message: 'Name already exists' }]);
+    const onSubmitted = vi.fn();
+
+    render(<BulkImportForm onSubmitted={onSubmitted} />);
+
+    fireEvent.change(screen.getByLabelText('Plain text :'), { target: { value: 'DUPLICATE_NAME=value' } });
+    fireEvent.click(screen.getByRole('button', { name: 'Submit' }));
+
+    await waitFor(() => {
+      expect(state.notificationError).toHaveBeenCalledWith({ message: 'Name already exists' });
+    });
+    expect(screen.getByLabelText('Plain text :')).toHaveValue('DUPLICATE_NAME=value');
+    expect(onSubmitted).not.toHaveBeenCalled();
+    expect(state.close).not.toHaveBeenCalled();
+  });
+});

packages/plugins/@nocobase/plugin-environment-variables/src/client-v2/pages/EnvironmentPage.tsx

@@ -40,6 +40,20 @@ type FilterGroupValue = {
   items: any[];
 };
 
+function getFirstErrorMessage(errors: unknown): string | undefined {
+  if (!Array.isArray(errors) || !errors.length) {
+    return;
+  }
+
+  const firstError = errors[0];
+  if (typeof firstError === 'string') {
+    return firstError;
+  }
+  if (typeof firstError === 'object' && firstError !== null && 'message' in firstError) {
+    return typeof firstError.message === 'string' ? firstError.message : undefined;
+  }
+}
+
 const drawerTitleClassName = css`
   display: inline-flex;
   align-items: center;
@@ -307,7 +321,7 @@ const EnvironmentPage: React.FC = observer(() => {
   );
 });
 
-function VariableForm(props: {
+export function VariableForm(props: {
   mode: 'create' | 'edit';
   initialValues?: EnvVariable;
   onSubmitted: () => void;
@@ -317,6 +331,7 @@ function VariableForm(props: {
   const t = useT();
   const ctx = useFlowContext();
   const view = useFlowView();
+  const { notification } = App.useApp();
   const [form] = Form.useForm<EnvVariable>();
   const [submitting, setSubmitting] = useState(false);
 
@@ -324,22 +339,29 @@ function VariableForm(props: {
     const values = await form.validateFields();
     setSubmitting(true);
     try {
-      if (mode === 'create') {
-        await ctx.api.request({ url: 'environmentVariables:create', method: 'post', data: values });
-      } else {
-        await ctx.api.request({
-          url: `environmentVariables:update`,
-          method: 'post',
-          params: { filterByTk: initialValues?.name },
-          data: values,
+      try {
+        if (mode === 'create') {
+          await ctx.api.request({ url: 'environmentVariables:create', method: 'post', data: values });
+        } else {
+          await ctx.api.request({
+            url: `environmentVariables:update`,
+            method: 'post',
+            params: { filterByTk: initialValues?.name },
+            data: values,
+          });
+        }
+      } catch (error) {
+        notification.error({
+          message: getFirstErrorMessage(ctx.api.toErrMessages(error)) || t('Operation failed'),
         });
+        return;
       }
       onSubmitted();
       await view.close();
     } finally {
       setSubmitting(false);
     }
-  }, [ctx.api, form, initialValues?.name, mode, onSubmitted, view]);
+  }, [ctx.api, form, initialValues?.name, mode, notification, onSubmitted, t, view]);
 
   return (
     <div>
@@ -410,11 +432,12 @@ function VariableForm(props: {
   );
 }
 
-function BulkImportForm(props: { onSubmitted: () => void }) {
+export function BulkImportForm(props: { onSubmitted: () => void }) {
   const { onSubmitted } = props;
   const t = useT();
   const ctx = useFlowContext();
   const view = useFlowView();
+  const { notification } = App.useApp();
   const [form] = Form.useForm<{ variables?: string; secret?: string }>();
   const [submitting, setSubmitting] = useState(false);
 
@@ -430,13 +453,20 @@ function BulkImportForm(props: { onSubmitted: () => void }) {
     }
     setSubmitting(true);
     try {
-      await ctx.api.request({ url: 'environmentVariables:create', method: 'post', data: items });
+      try {
+        await ctx.api.request({ url: 'environmentVariables:create', method: 'post', data: items });
+      } catch (error) {
+        notification.error({
+          message: getFirstErrorMessage(ctx.api.toErrMessages(error)) || t('Operation failed'),
+        });
+        return;
+      }
       onSubmitted();
       await view.close();
     } finally {
       setSubmitting(false);
     }
-  }, [ctx.api, form, onSubmitted, view]);
+  }, [ctx.api, form, notification, onSubmitted, t, view]);
 
   return (
     <div>

packages/plugins/@nocobase/plugin-environment-variables/src/server/__tests__/plugin.test.ts

@@ -0,0 +1,51 @@
+/**
+ * This file is part of the NocoBase (R) project.
+ * Copyright (c) 2020-2024 NocoBase Co., Ltd.
+ * Authors: NocoBase Team.
+ *
+ * This project is dual-licensed under AGPL-3.0 and NocoBase Commercial License.
+ * For more information, please refer to: https://www.nocobase.com/agreement.
+ */
+
+import type { Filter } from '@nocobase/database';
+import { describe, expect, it } from 'vitest';
+import { restrictValueFilterToPlainText } from '../plugin';
+
+describe('restrictValueFilterToPlainText', () => {
+  it.each(['$includes', '$notIncludes', '$eq', '$ne'] as const)(
+    'restricts the Value %s operator to plain text variables',
+    (operator) => {
+      const filter = {
+        value: { [operator]: 'test' },
+      } as Filter;
+
+      expect(restrictValueFilterToPlainText(filter)).toEqual({
+        $and: [filter, { type: { $eq: 'default' } }],
+      });
+    },
+  );
+
+  it('restricts nested Value filters to plain text variables', () => {
+    const filter: Filter = {
+      $or: [
+        { name: { $includes: 'API' } },
+        {
+          $and: [{ type: { $eq: 'secret' } }, { value: { $eq: 'test' } }],
+        },
+      ],
+    };
+
+    expect(restrictValueFilterToPlainText(filter)).toEqual({
+      $and: [filter, { type: { $eq: 'default' } }],
+    });
+  });
+
+  it.each([
+    undefined,
+    { name: { $includes: 'API' } } as Filter,
+    { type: { $eq: 'secret' } } as Filter,
+    { $or: [{ name: { $eq: 'API_SECRET' } }, { type: { $eq: 'secret' } }] } as Filter,
+  ])('keeps filters without Value conditions unchanged', (filter) => {
+    expect(restrictValueFilterToPlainText(filter)).toBe(filter);
+  });
+});

packages/plugins/@nocobase/plugin-environment-variables/src/server/plugin.ts

@@ -7,8 +7,37 @@
  * For more information, please refer to: https://www.nocobase.com/agreement.
  */
 
+import type { Filter } from '@nocobase/database';
 import { Plugin } from '@nocobase/server';
 
+const FILTER_LOGIC_OPERATORS = ['$and', '$or'] as const;
+
+function containsValueFilter(filter: unknown): boolean {
+  if (!filter || typeof filter !== 'object' || Array.isArray(filter)) {
+    return false;
+  }
+
+  const condition = filter as Record<string, unknown>;
+  if (Object.prototype.hasOwnProperty.call(condition, 'value')) {
+    return true;
+  }
+
+  return FILTER_LOGIC_OPERATORS.some((operator) => {
+    const items = condition[operator];
+    return Array.isArray(items) && items.some(containsValueFilter);
+  });
+}
+
+export function restrictValueFilterToPlainText(filter?: Filter): Filter | undefined {
+  if (!filter || !containsValueFilter(filter)) {
+    return filter;
+  }
+
+  return {
+    $and: [filter, { type: { $eq: 'default' } }],
+  };
+}
+
 export class PluginEnvironmentVariablesServer extends Plugin {
   updated = false;
 
@@ -158,9 +187,10 @@ export class PluginEnvironmentVariablesServer extends Plugin {
     });
     this.app.resourceManager.registerActionHandler('environmentVariables:list', async (ctx, next) => {
       const repository = this.db.getRepository('environmentVariables');
+      const filter = restrictValueFilterToPlainText(ctx.action.params.filter as Filter | undefined);
       const items = await repository.find({
         sort: 'name',
-        filter: ctx.action.params.filter,
+        filter,
       });
       for (const model of items) {
         if (model.type === 'secret') {