Address comments: checkUrlValid to return boolean, pass in invalid UR…

…L as undefined Signed-off-by: Abby Hu <abigailhu2000@gmail.com>
opensearch-project · Aug 28, 2021 · c56d3ff · c56d3ff
1 parent 3cd7bb6
commit c56d3ff
Show file tree

Hide file tree

Showing 19 changed files with 138 additions and 83 deletions.
diff --git a/...c/chrome/ui/header/branding/__snapshots__/opensearch_dashboards_custom_logo.test.tsx.snap b/...c/chrome/ui/header/branding/__snapshots__/opensearch_dashboards_custom_logo.test.tsx.snap
diff --git a/src/core/public/chrome/ui/header/branding/opensearch_dashboards_custom_logo.test.tsx b/src/core/public/chrome/ui/header/branding/opensearch_dashboards_custom_logo.test.tsx
@@ -17,7 +17,7 @@ describe('Custom Logo', () => {
     expect(component).toMatchSnapshot();
   });
   it('Take in a invalid URL string', () => {
-    const branding = { logoUrl: '' };
+    const branding = { logoUrl: undefined };
     const component = mountWithIntl(<CustomLogo {...branding} />);
     expect(component).toMatchSnapshot();
   });

diff --git a/src/core/public/chrome/ui/header/branding/opensearch_dashboards_custom_logo.tsx b/src/core/public/chrome/ui/header/branding/opensearch_dashboards_custom_logo.tsx
@@ -32,19 +32,20 @@
 
 import React from 'react';
 import '../header_logo.scss';
+import { OpenSearchDashboardsLogoDarkMode } from './opensearch_dashboards_logo_darkmode';
 
 export interface CustomLogoType {
-  logoUrl: string;
+  logoUrl: string | undefined;
 }
 
 export const CustomLogo = ({ ...branding }: CustomLogoType) => {
-  const defaultUrl =
-    'https://opensearch.org/assets/brand/SVG/Logo/opensearch_dashboards_logo_darkmode.svg';
-  return (
+  return branding.logoUrl === undefined ? (
+    OpenSearchDashboardsLogoDarkMode()
+  ) : (
     <img
       data-test-subj="customLogo"
-      data-test-image-url={branding.logoUrl === '' ? defaultUrl : branding.logoUrl}
-      src={branding.logoUrl === '' ? defaultUrl : branding.logoUrl}
+      data-test-image-url={branding.logoUrl}
+      src={branding.logoUrl}
       alt="logo"
       loading="lazy"
       className="logoImage"

diff --git a/src/core/public/chrome/ui/header/header.tsx b/src/core/public/chrome/ui/header/header.tsx
@@ -87,7 +87,7 @@ export interface HeaderProps {
   isLocked$: Observable<boolean>;
   loadingCount$: ReturnType<HttpStart['getLoadingCount$']>;
   onIsLockedUpdate: OnIsLockedUpdate;
-  branding: { logoUrl: string };
+  branding: { logoUrl: string | undefined };
 }
 
 export function Header({

diff --git a/src/core/public/chrome/ui/header/header_logo.tsx b/src/core/public/chrome/ui/header/header_logo.tsx
@@ -104,7 +104,7 @@ interface Props {
   navLinks$: Observable<ChromeNavLink[]>;
   forceNavigation$: Observable<boolean>;
   navigateToApp: (appId: string) => void;
-  logoUrl: string;
+  logoUrl: string | undefined;
 }
 
 export function HeaderLogo({ href, navigateToApp, logoUrl, ...observables }: Props) {

diff --git a/src/core/public/index.ts b/src/core/public/index.ts
@@ -237,8 +237,8 @@ export interface CoreSetup<TPluginsStart extends object = object, TStart = unkno
   injectedMetadata: {
     getInjectedVar: (name: string, defaultValue?: any) => unknown;
     getBranding: () => {
-      logoUrl: string;
-      smallLogoUrl: string;
+      logoUrl: string | undefined;
+      smallLogoUrl: string | undefined;
       title: string;
     };
   };
@@ -297,8 +297,8 @@ export interface CoreStart {
   injectedMetadata: {
     getInjectedVar: (name: string, defaultValue?: any) => unknown;
     getBranding: () => {
-      logoUrl: string;
-      smallLogoUrl: string;
+      logoUrl: string | undefined;
+      smallLogoUrl: string | undefined;
       title: string;
     };
   };

diff --git a/src/core/public/injected_metadata/injected_metadata_service.ts b/src/core/public/injected_metadata/injected_metadata_service.ts
@@ -77,8 +77,8 @@ export interface InjectedMetadataParams {
       };
     };
     branding: {
-      logoUrl: string;
-      smallLogoUrl: string;
+      logoUrl: string | undefined;
+      smallLogoUrl: string | undefined;
       title: string;
     };
   };
@@ -186,8 +186,8 @@ export interface InjectedMetadataSetup {
     [key: string]: unknown;
   };
   getBranding: () => {
-    logoUrl: string;
-    smallLogoUrl: string;
+    logoUrl: string | undefined;
+    smallLogoUrl: string | undefined;
     title: string;
   };
 }

diff --git a/src/core/server/rendering/__snapshots__/rendering_service.test.ts.snap b/src/core/server/rendering/__snapshots__/rendering_service.test.ts.snap
diff --git a/src/core/server/rendering/rendering_service.test.ts b/src/core/server/rendering/rendering_service.test.ts
@@ -136,32 +136,28 @@ describe('RenderingService', () => {
       });
     });
   });
-  describe('checkUrlvalid()', () => {
+  describe('checkUrlValid()', () => {
     it('SVG URL is valid', async () => {
       const result = await service.checkUrlValid(
         'https://opensearch.org/assets/brand/SVG/Mark/opensearch_mark_default.svg',
-        'error'
-      );
-      expect(result).toEqual(
-        'https://opensearch.org/assets/brand/SVG/Mark/opensearch_mark_default.svg'
+        'config'
       );
+      expect(result).toEqual(true);
     });
     it('PNG URL is valid', async () => {
       const result = await service.checkUrlValid(
         'https://opensearch.org/assets/brand/PNG/Mark/opensearch_mark_default.png',
-        'error'
-      );
-      expect(result).toEqual(
-        'https://opensearch.org/assets/brand/PNG/Mark/opensearch_mark_default.png'
+        'config'
       );
+      expect(result).toEqual(true);
     });
     it('URL does not contain svg, or png', async () => {
       const result = await service.checkUrlValid('https://validUrl', 'error');
-      expect(result).toEqual('');
+      expect(result).toEqual(false);
     });
     it('URL is invalid', async () => {
       const result = await service.checkUrlValid('http://notfound.svg', 'error');
-      expect(result).toEqual('');
+      expect(result).toEqual(false);
     });
   });
 });
diff --git a/src/core/server/rendering/rendering_service.tsx b/src/core/server/rendering/rendering_service.tsx
@@ -63,13 +63,13 @@ export class RenderingService {
       .pipe(first())
       .toPromise();
 
-    const validLogoUrl = await this.checkUrlValid(
+    const isLogoUrlValid = await this.checkUrlValid(
       opensearchDashboardsConfig.branding.logoUrl,
-      'Config logoUrl is not found or invalid, default OpenSearch logo will be rendered on the main page'
+      'logoUrl'
     );
-    const validSmallLogoUrl = await this.checkUrlValid(
+    const isSmallLogoUrlValid = await this.checkUrlValid(
       opensearchDashboardsConfig.branding.smallLogoUrl,
-      'Config smallLogoUrl is not found or invalid, default OpenSearch logo will be rendered on the welcome page'
+      'smallLogoUrl'
     );
 
     return {
@@ -121,8 +121,12 @@ export class RenderingService {
               uiSettings: settings,
             },
             branding: {
-              logoUrl: validLogoUrl,
-              smallLogoUrl: validSmallLogoUrl,
+              logoUrl:
+                isLogoUrlValid === true ? opensearchDashboardsConfig.branding.logoUrl : undefined,
+              smallLogoUrl:
+                isSmallLogoUrlValid === true
+                  ? opensearchDashboardsConfig.branding.smallLogoUrl
+                  : undefined,
               title: opensearchDashboardsConfig.branding.title,
             },
           },
@@ -141,18 +145,18 @@ export class RenderingService {
     return ((await browserConfig?.pipe(take(1)).toPromise()) ?? {}) as Record<string, any>;
   }
 
-  public checkUrlValid = async (url: string, errorMessage: string): Promise<string> => {
+  public checkUrlValid = async (url: string, configName?: string): Promise<boolean> => {
     if (url.match(/\.(png|svg)$/) === null) {
-      this.logger.get('branding').error(errorMessage);
-      return '';
+      this.logger.get('branding').warn(configName + ' config is not found or invalid.');
+      return false;
     }
     return await Axios.get(url, { adapter: AxiosHttpAdapter })
       .then(() => {
-        return url;
+        return true;
       })
       .catch(() => {
-        this.logger.get('branding').error(errorMessage);
-        return '';
+        this.logger.get('branding').warn(configName + ' config is not found or invalid');
+        return false;
       });
   };
 }
diff --git a/src/core/server/rendering/types.ts b/src/core/server/rendering/types.ts
@@ -75,8 +75,8 @@ export interface RenderingMetadata {
       };
     };
     branding: {
-      logoUrl: string;
-      smallLogoUrl: string;
+      logoUrl: string | undefined;
+      smallLogoUrl: string | undefined;
       title: string;
     };
   };

diff --git a/src/legacy/server/config/schema.js b/src/legacy/server/config/schema.js
@@ -234,12 +234,8 @@ export default () =>
       // TODO Also allow units here like in opensearch config once this is moved to the new platform
       autocompleteTimeout: Joi.number().integer().min(1).default(1000),
       branding: Joi.object({
-        logoUrl: Joi.string().default(
-          'https://opensearch.org/assets/brand/SVG/Logo/opensearch_dashboards_logo_darkmode.svg'
-        ),
-        smallLogoUrl: Joi.string().default(
-          'https://opensearch.org/assets/brand/SVG/Logo/opensearch_dashboards_logo_darkmode.svg'
-        ),
+        logoUrl: Joi.string().default('/'),
+        smallLogoUrl: Joi.string().default('/'),
         title: Joi.string().default('OpenSearch Dashboards'),
       }),
     }).default(),

diff --git a/src/plugins/home/public/application/components/__snapshots__/welcome.test.tsx.snap b/src/plugins/home/public/application/components/__snapshots__/welcome.test.tsx.snap
diff --git a/src/plugins/home/public/application/components/_welcome.scss b/src/plugins/home/public/application/components/_welcome.scss
@@ -19,7 +19,7 @@
   padding: 10px 10px 10px 10px;
 }
 
-.homWelcome__customLogo{
+.homWelcome__customLogo {
   height: 100%;
   max-width: 100%;
 }

diff --git a/src/plugins/home/public/application/components/home.js b/src/plugins/home/public/application/components/home.js
@@ -208,6 +208,7 @@ export class Home extends Component {
 
   render() {
     const { isLoading, isWelcomeEnabled, isNewOpenSearchDashboardsInstance } = this.state;
+
     if (isWelcomeEnabled) {
       if (isLoading) {
         return this.renderLoading();
@@ -216,7 +217,6 @@ export class Home extends Component {
         return this.renderWelcome();
       }
     }
-
     return this.renderNormal();
   }
 }

diff --git a/src/plugins/home/public/application/components/welcome.test.tsx b/src/plugins/home/public/application/components/welcome.test.tsx
@@ -51,7 +51,6 @@ test('should render a Welcome screen with the telemetry disclaimer', () => {
 */
 
 const branding = {
-  logoUrl: '/',
   smallLogoUrl: '/',
   title: 'OpenSearch Dashboards',
 };
@@ -93,8 +92,7 @@ test('fires opt-in seen when mounted', () => {
 
 test('should render a Welcome screen with the default OpenSearch Dashboards branding', () => {
   const defaultBranding = {
-    logoUrl: '/',
-    smallLogoUrl: '',
+    smallLogoUrl: undefined,
     title: 'OpenSearch Dashboards',
   };
   const component = shallow(
@@ -105,7 +103,6 @@ test('should render a Welcome screen with the default OpenSearch Dashboards bran
 
 test('should render a Welcome screen with customized branding', () => {
   const customBranding = {
-    logoUrl: '/',
     smallLogoUrl: '/custom',
     title: 'custom title',
   };

diff --git a/src/plugins/home/public/application/components/welcome.tsx b/src/plugins/home/public/application/components/welcome.tsx
@@ -59,8 +59,7 @@ interface Props {
   onSkip: () => void;
   telemetry?: TelemetryPluginStart;
   branding: {
-    logoUrl: string;
-    smallLogoUrl: string;
+    smallLogoUrl: string | undefined;
     title: string;
   };
 }
@@ -153,7 +152,7 @@ export class Welcome extends React.Component<Props> {
           <header className="homWelcome__header">
             <div className="homWelcome__content eui-textCenter">
               <EuiSpacer size="xl" />
-              {branding.smallLogoUrl === '' ? (
+              {branding.smallLogoUrl === undefined ? (
                 <span className="homWelcome__logo">
                   <EuiIcon type={OpenSearchMarkCentered} size="original" />
                 </span>