From c56d3ffa9fe1e0b33e3be91e9abe6bf8a0672b10 Mon Sep 17 00:00:00 2001 From: Abby Hu Date: Sat, 28 Aug 2021 00:40:50 +0000 Subject: [PATCH] Address comments: checkUrlValid to return boolean, pass in invalid URL as undefined Signed-off-by: Abby Hu --- ...earch_dashboards_custom_logo.test.tsx.snap | 80 ++++++++++++++++--- ...opensearch_dashboards_custom_logo.test.tsx | 2 +- .../opensearch_dashboards_custom_logo.tsx | 13 +-- src/core/public/chrome/ui/header/header.tsx | 2 +- .../public/chrome/ui/header/header_logo.tsx | 2 +- src/core/public/index.ts | 8 +- .../injected_metadata_service.ts | 8 +- .../rendering_service.test.ts.snap | 10 --- .../rendering/rendering_service.test.ts | 18 ++--- .../server/rendering/rendering_service.tsx | 28 ++++--- src/core/server/rendering/types.ts | 4 +- src/legacy/server/config/schema.js | 8 +- .../__snapshots__/welcome.test.tsx.snap | 13 +-- .../application/components/_welcome.scss | 2 +- .../public/application/components/home.js | 2 +- .../application/components/welcome.test.tsx | 5 +- .../public/application/components/welcome.tsx | 5 +- .../apps/visualize/_custom_branding.js | 9 ++- test/functional/page_objects/common_page.ts | 2 + 19 files changed, 138 insertions(+), 83 deletions(-) diff --git a/src/core/public/chrome/ui/header/branding/__snapshots__/opensearch_dashboards_custom_logo.test.tsx.snap b/src/core/public/chrome/ui/header/branding/__snapshots__/opensearch_dashboards_custom_logo.test.tsx.snap index 86e03ca908e1..650ef0460fe3 100644 --- a/src/core/public/chrome/ui/header/branding/__snapshots__/opensearch_dashboards_custom_logo.test.tsx.snap +++ b/src/core/public/chrome/ui/header/branding/__snapshots__/opensearch_dashboards_custom_logo.test.tsx.snap @@ -104,16 +104,78 @@ exports[`Custom Logo Take in a invalid URL string 1`] = ` "timeZone": null, } } - logoUrl="" > - logo + + + + + + + + + + + + + + + + + `; 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 index 83b6064d2aa1..44b52b442a5e 100644 --- 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(); 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 index 4d267424455b..01e3a0767e2a 100644 --- 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() + ) : ( logo; loadingCount$: ReturnType; 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 index eb12804e7f6f..cb40928c121b 100644 --- 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; forceNavigation$: Observable; 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 index b000f72df337..57bdf8656a9b 100644 --- a/src/core/public/index.ts +++ b/src/core/public/index.ts @@ -237,8 +237,8 @@ export interface CoreSetup 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 index 2b1b9f1d493e..2cd2d0043ad9 100644 --- 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 index c2e21d635e22..762269e81086 100644 --- a/src/core/server/rendering/__snapshots__/rendering_service.test.ts.snap +++ b/src/core/server/rendering/__snapshots__/rendering_service.test.ts.snap @@ -6,8 +6,6 @@ Object { "basePath": "/mock-server-basepath", "branch": Any, "branding": Object { - "logoUrl": "", - "smallLogoUrl": "", "title": "OpenSearch Dashboards", }, "buildNumber": Any, @@ -54,8 +52,6 @@ Object { "basePath": "/mock-server-basepath", "branch": Any, "branding": Object { - "logoUrl": "", - "smallLogoUrl": "", "title": "OpenSearch Dashboards", }, "buildNumber": Any, @@ -102,8 +98,6 @@ Object { "basePath": "/mock-server-basepath", "branch": Any, "branding": Object { - "logoUrl": "", - "smallLogoUrl": "", "title": "OpenSearch Dashboards", }, "buildNumber": Any, @@ -154,8 +148,6 @@ Object { "basePath": "", "branch": Any, "branding": Object { - "logoUrl": "", - "smallLogoUrl": "", "title": "OpenSearch Dashboards", }, "buildNumber": Any, @@ -202,8 +194,6 @@ Object { "basePath": "/mock-server-basepath", "branch": Any, "branding": Object { - "logoUrl": "", - "smallLogoUrl": "", "title": "OpenSearch Dashboards", }, "buildNumber": Any, diff --git a/src/core/server/rendering/rendering_service.test.ts b/src/core/server/rendering/rendering_service.test.ts index 79331ef6f97a..0dc9fb7f5c2d 100644 --- 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 index b7bb9c384085..034a1533fd04 100644 --- 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; } - public checkUrlValid = async (url: string, errorMessage: string): Promise => { + public checkUrlValid = async (url: string, configName?: string): Promise => { 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 index 9e9c08a37c26..518e77bea960 100644 --- 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 index 9d6352587cbe..846e397d08df 100644 --- 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 index bb331c752bad..ece1066f46fb 100644 --- a/src/plugins/home/public/application/components/__snapshots__/welcome.test.tsx.snap +++ b/src/plugins/home/public/application/components/__snapshots__/welcome.test.tsx.snap @@ -146,14 +146,15 @@ exports[`should render a Welcome screen with the default OpenSearch Dashboards b - - - + { */ 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 index 3747c89b1ca6..1f4dbad9fa82 100644 --- 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 {
- {branding.smallLogoUrl === '' ? ( + {branding.smallLogoUrl === undefined ? ( diff --git a/test/functional/apps/visualize/_custom_branding.js b/test/functional/apps/visualize/_custom_branding.js index bc3583645e77..32aba1317b8b 100644 --- a/test/functional/apps/visualize/_custom_branding.js +++ b/test/functional/apps/visualize/_custom_branding.js @@ -30,7 +30,7 @@ * GitHub history for details. */ import expect from '@osd/expect'; -import { UI_SETTINGS } from '../../../../src/plugins/data/common'; +import { UI_SETTINGS } from './index'; export default function ({ getService, getPageObjects }) { const browser = getService('browser'); @@ -46,12 +46,16 @@ export default function ({ getService, getPageObjects }) { const expectedWelcomeLogo = 'https://opensearch.org/assets/brand/SVG/Logo/opensearch_logo_darkmode.svg'; const expectedWelcomeMessage = 'Welcome to OpenSearch'; + + //unloading any pre-existing settings so the welcome page will appear before(async function () { await opensearchArchiver.unload('logstash_functional'); await opensearchArchiver.unload('long_window_logstash'); await opensearchArchiver.unload('visualize'); await PageObjects.common.navigateToApp('home'); }); + + //loading the settings again for after(async function () { await browser.setWindowSize(1280, 800); await opensearchArchiver.loadIfNeeded('logstash_functional'); @@ -62,6 +66,7 @@ export default function ({ getService, getPageObjects }) { [UI_SETTINGS.FORMAT_BYTES_DEFAULT_PATTERN]: '0,0.[000]b', }); }); + it('with customized logo', async () => { await testSubjects.existOrFail('welcomeCustomLogo'); const actualLabel = await testSubjects.getAttribute( @@ -70,6 +75,7 @@ export default function ({ getService, getPageObjects }) { ); expect(actualLabel.toUpperCase()).to.equal(expectedWelcomeLogo.toUpperCase()); }); + it('with customized title', async () => { await testSubjects.existOrFail('welcomeCustomTitle'); const actualLabel = await testSubjects.getAttribute( @@ -84,6 +90,7 @@ export default function ({ getService, getPageObjects }) { this.tags('includeFirefox'); const expectedUrl = 'https://opensearch.org/assets/brand/SVG/Logo/opensearch_logo_darkmode.svg'; + before(async function () { await PageObjects.common.navigateToApp('home'); }); diff --git a/test/functional/page_objects/common_page.ts b/test/functional/page_objects/common_page.ts index 84adae2509e7..57ff47e81cf7 100644 --- a/test/functional/page_objects/common_page.ts +++ b/test/functional/page_objects/common_page.ts @@ -68,6 +68,8 @@ export function CommonPageProvider({ getService, getPageObjects }: FtrProviderCo // Disable the welcome screen. This is relevant for environments // which don't allow to use the yml setting, e.g. cloud production. // It is done here so it applies to logins but also to a login re-use. + + // Update: Enable the welcome screen for functional tests on the welcome screen. await browser.setLocalStorageItem('home:welcome:show', 'true'); let currentUrl = await browser.getCurrentUrl(); log.debug(`currentUrl = ${currentUrl}\n appUrl = ${appUrl}`);