FIX: custom logo failure leads to repetitive api calls for that image url (#22100)

* fix the custom logo failure leads to repetative api calls for that image url

* remove unwanted wait here

openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/CustomThemeConfig.spec.ts

@@ -10,7 +10,7 @@
  *  See the License for the specific language governing permissions and
  *  limitations under the License.
  */
-import test, { expect } from '@playwright/test';
+import test, { expect, Request } from '@playwright/test';
 import { GlobalSettingOptions } from '../../constant/settings';
 import { redirectToHomePage } from '../../utils/common';
 import { settingClick } from '../../utils/sidebar';
@@ -107,4 +107,43 @@ test.describe('Custom Theme Config Page', () => {
       'rgb(21, 112, 239)'
     );
   });
+
+  test('Should call customMonogramUrlPath only once after save if the monogram is not valid', async ({
+    page,
+  }) => {
+    let monogramUrlCallCount = 0;
+    const monogramRequests: Request[] = [];
+
+    // Track all network requests to the monogram URL
+    page.on('request', (request) => {
+      if (request.url().includes('custom-monogram.png')) {
+        monogramRequests.push(request);
+        monogramUrlCallCount++;
+      }
+    });
+
+    // Fill the monogram URL field
+    await page
+      .locator('[data-testid="customMonogramUrlPath"]')
+      .fill(config.monogram);
+
+    // Fill other required fields to make form valid
+    await page.locator('[data-testid="customLogoUrlPath"]').fill(config.logo);
+
+    // Reset counter before save action
+    monogramUrlCallCount = 0;
+    monogramRequests.length = 0;
+
+    // Click save button and wait for API response
+    const saveResponse = page.waitForResponse('/api/v1/system/settings');
+    await page.locator('[data-testid="save-btn"]').click();
+    await saveResponse;
+
+    // Wait a bit more to catch any additional requests
+    await page.waitForTimeout(2000);
+
+    // Assert monogram URL was called at most once after save
+    expect(monogramUrlCallCount).toBeLessThanOrEqual(1);
+    expect(monogramRequests.length).toBeLessThanOrEqual(1);
+  });
 });

openmetadata-ui/src/main/resources/ui/src/components/common/BrandImage/BrandImage.test.tsx

@@ -10,21 +10,38 @@
  *  See the License for the specific language governing permissions and
  *  limitations under the License.
  */
-import { render, screen } from '@testing-library/react';
-import BrandImage from './BrandImage';
+import { fireEvent, render, screen } from '@testing-library/react';
 
-jest.mock('../../../hooks/useApplicationStore', () => ({
-  useApplicationStore: jest.fn().mockImplementation(() => ({
-    applicationConfig: {
-      customLogoConfig: {
-        customLogoUrlPath: 'https://custom-logo.png',
-        customMonogramUrlPath: 'https://custom-monogram.png',
-      },
-    },
-  })),
+jest.mock('../../../hooks/useApplicationStore');
+jest.mock('../../../utils/BrandData/BrandClassBase', () => ({
+  getMonogram: jest.fn().mockReturnValue({
+    src: '/default-monogram.svg',
+  }),
+  getLogo: jest.fn().mockReturnValue({
+    src: '/default-logo.svg',
+  }),
 }));
 
+import { useApplicationStore } from '../../../hooks/useApplicationStore';
+import BrandImage from './BrandImage';
+
+const mockUseApplicationStore = useApplicationStore as jest.MockedFunction<
+  typeof useApplicationStore
+>;
+
 describe('Test Brand Logo', () => {
+  beforeEach(() => {
+    jest.clearAllMocks();
+    mockUseApplicationStore.mockReturnValue({
+      applicationConfig: {
+        customLogoConfig: {
+          customLogoUrlPath: 'https://custom-logo.png',
+          customMonogramUrlPath: 'https://custom-monogram.png',
+        },
+      },
+    });
+  });
+
   it('Should render the brand logo with default props value', () => {
     render(<BrandImage height="auto" width={152} />);
 
@@ -34,6 +51,7 @@ describe('Test Brand Logo', () => {
     expect(image).toHaveAttribute('alt', 'OpenMetadata Logo');
     expect(image).toHaveAttribute('height', 'auto');
     expect(image).toHaveAttribute('width', '152');
+    expect(image).toHaveAttribute('id', 'brand-image');
   });
 
   it('Should render the brand logo with passed props value', () => {
@@ -59,18 +77,18 @@ describe('Test Brand Logo', () => {
   it('Should render the brand logo based on custom logo config', () => {
     render(
       <BrandImage
-        alt="brand-monogram"
+        alt="brand-logo"
         className="m-auto"
-        dataTestId="brand-monogram"
+        dataTestId="brand-logo"
         height="auto"
         width={152}
       />
     );
 
-    const image = screen.getByTestId('brand-monogram');
+    const image = screen.getByTestId('brand-logo');
 
     expect(image).toBeInTheDocument();
-    expect(image).toHaveAttribute('alt', 'brand-monogram');
+    expect(image).toHaveAttribute('alt', 'brand-logo');
     expect(image).toHaveAttribute('height', 'auto');
     expect(image).toHaveAttribute('width', '152');
     expect(image).toHaveAttribute('src', 'https://custom-logo.png');
@@ -96,4 +114,247 @@ describe('Test Brand Logo', () => {
     expect(image).toHaveAttribute('width', '30');
     expect(image).toHaveAttribute('src', 'https://custom-monogram.png');
   });
+
+  it('Should use default logo when no custom logo config is provided', () => {
+    mockUseApplicationStore.mockReturnValue({
+      applicationConfig: {
+        customLogoConfig: {
+          customLogoUrlPath: '',
+          customMonogramUrlPath: '',
+        },
+      },
+    });
+
+    render(
+      <BrandImage
+        alt="default-logo"
+        dataTestId="default-logo"
+        height={30}
+        width={30}
+      />
+    );
+
+    const image = screen.getByTestId('default-logo');
+
+    expect(image).toBeInTheDocument();
+    expect(image).toHaveAttribute('src', '/default-logo.svg');
+  });
+
+  it('Should use default monogram when no custom monogram config is provided', () => {
+    mockUseApplicationStore.mockReturnValue({
+      applicationConfig: {
+        customLogoConfig: {
+          customLogoUrlPath: '',
+          customMonogramUrlPath: '',
+        },
+      },
+    });
+
+    render(
+      <BrandImage
+        isMonoGram
+        alt="default-monogram"
+        dataTestId="default-monogram"
+        height={30}
+        width={30}
+      />
+    );
+
+    const image = screen.getByTestId('default-monogram');
+
+    expect(image).toBeInTheDocument();
+    expect(image).toHaveAttribute('src', '/default-monogram.svg');
+  });
+
+  it('Should handle missing customLogoConfig gracefully', () => {
+    mockUseApplicationStore.mockReturnValue({
+      applicationConfig: {},
+    });
+
+    render(
+      <BrandImage
+        alt="fallback-logo"
+        dataTestId="fallback-logo"
+        height={30}
+        width={30}
+      />
+    );
+
+    const image = screen.getByTestId('fallback-logo');
+
+    expect(image).toBeInTheDocument();
+    expect(image).toHaveAttribute('src', '/default-logo.svg');
+  });
+
+  it('Should handle missing applicationConfig gracefully', () => {
+    mockUseApplicationStore.mockReturnValue({});
+
+    render(
+      <BrandImage
+        alt="no-config-logo"
+        dataTestId="no-config-logo"
+        height={30}
+        width={30}
+      />
+    );
+
+    const image = screen.getByTestId('no-config-logo');
+
+    expect(image).toBeInTheDocument();
+    expect(image).toHaveAttribute('src', '/default-logo.svg');
+  });
+
+  it('Should use provided src prop when given', () => {
+    const customSrc = 'https://example.com/custom-image.png';
+
+    render(
+      <BrandImage
+        alt="custom-src-logo"
+        dataTestId="custom-src-logo"
+        height={30}
+        src={customSrc}
+        width={30}
+      />
+    );
+
+    const image = screen.getByTestId('custom-src-logo');
+
+    expect(image).toBeInTheDocument();
+    expect(image).toHaveAttribute('src', customSrc);
+  });
+
+  it('Should handle onError and fallback to default logo when image fails to load', () => {
+    render(
+      <BrandImage
+        alt="error-logo"
+        dataTestId="error-logo"
+        height={30}
+        width={30}
+      />
+    );
+
+    const image = screen.getByTestId('error-logo');
+
+    expect(image).toBeInTheDocument();
+    expect(image).toHaveAttribute('src', 'https://custom-logo.png');
+
+    // Simulate image load error
+    fireEvent.error(image);
+
+    expect(image).toHaveAttribute('src', '/default-logo.svg');
+  });
+
+  it('Should handle onError and fallback to default monogram when monogram fails to load', () => {
+    render(
+      <BrandImage
+        isMonoGram
+        alt="error-monogram"
+        dataTestId="error-monogram"
+        height={30}
+        width={30}
+      />
+    );
+
+    const image = screen.getByTestId('error-monogram');
+
+    expect(image).toBeInTheDocument();
+    expect(image).toHaveAttribute('src', 'https://custom-monogram.png');
+
+    // Simulate image load error
+    fireEvent.error(image);
+
+    expect(image).toHaveAttribute('src', '/default-monogram.svg');
+  });
+
+  it('Should handle onError with custom src and fallback to default logo', () => {
+    const customSrc = 'https://example.com/broken-image.png';
+
+    render(
+      <BrandImage
+        alt="broken-custom-logo"
+        dataTestId="broken-custom-logo"
+        height={30}
+        src={customSrc}
+        width={30}
+      />
+    );
+
+    const image = screen.getByTestId('broken-custom-logo');
+
+    expect(image).toBeInTheDocument();
+    expect(image).toHaveAttribute('src', customSrc);
+
+    // Simulate image load error
+    fireEvent.error(image);
+
+    expect(image).toHaveAttribute('src', '/default-logo.svg');
+  });
+
+  it('Should handle onError with custom src and monogram fallback', () => {
+    const customSrc = 'https://example.com/broken-monogram.png';
+
+    render(
+      <BrandImage
+        isMonoGram
+        alt="broken-custom-monogram"
+        dataTestId="broken-custom-monogram"
+        height={30}
+        src={customSrc}
+        width={30}
+      />
+    );
+
+    const image = screen.getByTestId('broken-custom-monogram');
+
+    expect(image).toBeInTheDocument();
+    expect(image).toHaveAttribute('src', customSrc);
+
+    // Simulate image load error
+    fireEvent.error(image);
+
+    expect(image).toHaveAttribute('src', '/default-monogram.svg');
+  });
+
+  it('Should render with all props including optional ones', () => {
+    render(
+      <BrandImage
+        alt="comprehensive test logo"
+        className="test-class custom-brand"
+        dataTestId="comprehensive-test"
+        height="50px"
+        isMonoGram={false}
+        src="https://example.com/test-logo.png"
+        width="100%"
+      />
+    );
+
+    const image = screen.getByTestId('comprehensive-test');
+
+    expect(image).toBeInTheDocument();
+    expect(image).toHaveAttribute('alt', 'comprehensive test logo');
+    expect(image).toHaveAttribute('width', '100%');
+    expect(image).toHaveAttribute('height', '50px');
+    expect(image).toHaveAttribute('src', 'https://example.com/test-logo.png');
+    expect(image).toHaveAttribute('id', 'brand-image');
+    expect(image).toHaveClass('test-class', 'custom-brand');
+  });
+
+  it('Should prioritize src prop over custom logo configuration', () => {
+    const prioritySrc = 'https://priority.com/logo.png';
+
+    render(
+      <BrandImage
+        dataTestId="priority-test"
+        height={30}
+        src={prioritySrc}
+        width={30}
+      />
+    );
+
+    const image = screen.getByTestId('priority-test');
+
+    expect(image).toBeInTheDocument();
+    expect(image).toHaveAttribute('src', prioritySrc);
+    expect(image).not.toHaveAttribute('src', 'https://custom-logo.png');
+  });
 });

openmetadata-ui/src/main/resources/ui/src/components/common/BrandImage/BrandImage.tsx

@@ -33,20 +33,21 @@ const BrandImage: FC<BrandImageProps> = ({
   src,
   isMonoGram = false,
 }) => {
-  const { MonoGram, Logo } = useMemo(
-    () => ({
-      MonoGram: brandClassBase.getMonogram().src,
-      Logo: brandClassBase.getLogo().src,
-    }),
-    []
-  );
   const { applicationConfig } = useApplicationStore();
-  const { customLogoUrlPath = '', customMonogramUrlPath = '' } =
-    applicationConfig?.customLogoConfig ?? {};
 
-  const logoSource = isMonoGram
-    ? customMonogramUrlPath || MonoGram
-    : customLogoUrlPath || Logo;
+  const { defaultLogo, logoSource } = useMemo(() => {
+    const { customLogoUrlPath = '', customMonogramUrlPath = '' } =
+      applicationConfig?.customLogoConfig ?? {};
+    const monoGram = brandClassBase.getMonogram().src;
+    const logo = brandClassBase.getLogo().src;
+
+    const defaultLogo = isMonoGram ? monoGram : logo;
+    const logoSource = isMonoGram
+      ? customMonogramUrlPath || monoGram
+      : customLogoUrlPath || logo;
+
+    return { defaultLogo, logoSource };
+  }, [isMonoGram, applicationConfig?.customLogoConfig]);
 
   return (
     <img
@@ -58,7 +59,7 @@ const BrandImage: FC<BrandImageProps> = ({
       src={src ?? logoSource}
       width={width}
       onError={(e) => {
-        e.currentTarget.src = logoSource;
+        e.currentTarget.src = defaultLogo;
       }}
     />
   );