From 0a130f9be0922534bde94e79567ff5424c84ade7 Mon Sep 17 00:00:00 2001 From: Maxim Hayes Date: Tue, 12 Nov 2024 16:56:27 -0500 Subject: [PATCH] feat(notifications): add misc rule, log, type, and test changes - Add types to some RuleContext fields - Add some informative log statements for debugging - Add a test for getRuleContext() - Fix getRuleContext to properly capture authTypes - Remove a few unneeded lines of code --- packages/core/src/login/webview/vue/types.ts | 1 - packages/core/src/notifications/panelNode.ts | 13 +++- packages/core/src/notifications/rules.ts | 48 +++++++++--- packages/core/src/notifications/types.ts | 6 +- .../core/src/test/notifications/rules.test.ts | 75 +++++++++++++++++-- 5 files changed, 117 insertions(+), 26 deletions(-) diff --git a/packages/core/src/login/webview/vue/types.ts b/packages/core/src/login/webview/vue/types.ts index b9936e8d3a1..1b92af8ab72 100644 --- a/packages/core/src/login/webview/vue/types.ts +++ b/packages/core/src/login/webview/vue/types.ts @@ -81,5 +81,4 @@ export type AuthFormId = | 'identityCenterCodeWhisperer' | 'identityCenterCodeCatalyst' | 'identityCenterExplorer' - | 'aggregateExplorer' | 'unknown' diff --git a/packages/core/src/notifications/panelNode.ts b/packages/core/src/notifications/panelNode.ts index 66a29930bcb..1a38c34640d 100644 --- a/packages/core/src/notifications/panelNode.ts +++ b/packages/core/src/notifications/panelNode.ts @@ -6,7 +6,7 @@ import * as vscode from 'vscode' import { ResourceTreeDataProvider, TreeNode } from '../shared/treeview/resourceTreeDataProvider' import { Command, Commands } from '../shared/vscode/commands2' -import { getIcon } from '../shared/icons' +import { Icon, getIcon } from '../shared/icons' import { contextKey, setContext } from '../shared/vscode/setContext' import { NotificationType, ToolkitNotification, getNotificationTelemetryId } from './types' import { ToolkitError } from '../shared/errors' @@ -78,10 +78,19 @@ export class NotificationsNode implements TreeNode { public getChildren() { const buildNode = (n: ToolkitNotification, type: NotificationType) => { + let icon: Icon & { color?: vscode.ThemeColor } + if (type === 'startUp') { + icon = getIcon('vscode-question') as Icon + } else { + icon = getIcon('vscode-alert') as Icon + icon.color = new vscode.ThemeColor('errorForeground') + } + return this.openNotificationCmd.build(n).asTreeNode({ label: n.uiRenderInstructions.content['en-US'].title, - iconPath: type === 'startUp' ? getIcon('vscode-question') : getIcon('vscode-alert'), + iconPath: icon, contextValue: type === 'startUp' ? this.startUpNodeContext : this.emergencyNodeContext, + tooltip: 'Click to open', }) } diff --git a/packages/core/src/notifications/rules.ts b/packages/core/src/notifications/rules.ts index ab661c80930..6c48074ea65 100644 --- a/packages/core/src/notifications/rules.ts +++ b/packages/core/src/notifications/rules.ts @@ -8,6 +8,8 @@ import * as semver from 'semver' import globals from '../shared/extensionGlobals' import { ConditionalClause, RuleContext, DisplayIf, CriteriaCondition, ToolkitNotification, AuthState } from './types' import { getComputeEnvType, getOperatingSystem } from '../shared/telemetry/util' +import { AuthFormId } from '../login/webview/vue/types' +import { getLogger } from '../shared/logger/logger' /** * Evaluates if a given version fits into the parameters specified by a notification, e.g: @@ -96,17 +98,16 @@ export class RuleEngine { } private evaluateRule(criteria: CriteriaCondition) { - const expected = criteria.values + const expected = criteria.values.map((v) => v.toLowerCase()) const expectedSet = new Set(expected) - const isExpected = (i: string) => expectedSet.has(i) - const hasAnyOfExpected = (i: string[]) => i.some((v) => expectedSet.has(v)) + const hasAnyOfExpected = (i: string[]) => i.map((v) => v.toLowerCase()).some((v) => expectedSet.has(v)) const isSuperSetOfExpected = (i: string[]) => { - const s = new Set(i) + const s = new Set(i.map((v) => v.toLowerCase())) return expected.every((v) => s.has(v)) } const isEqualSetToExpected = (i: string[]) => { - const s = new Set(i) + const s = new Set(i.map((v) => v.toLowerCase())) return expected.every((v) => s.has(v)) && i.every((v) => expectedSet.has(v)) } @@ -116,10 +117,9 @@ export class RuleEngine { // So... YAGNI switch (criteria.type) { case 'OS': - // todo: allow lowercase? - return isExpected(this.context.os) + return hasAnyOfExpected([this.context.os]) case 'ComputeEnv': - return isExpected(this.context.computeEnv) + return hasAnyOfExpected([this.context.computeEnv]) case 'AuthType': return hasAnyOfExpected(this.context.authTypes) case 'AuthRegion': @@ -139,16 +139,40 @@ export class RuleEngine { } export async function getRuleContext(context: vscode.ExtensionContext, authState: AuthState): Promise { - return { + const authTypes = + authState.authEnabledConnections === '' + ? [] + : // TODO: There is a large disconnect in the codebase with how auth "types" are stored, displayed, sent to telemetry, etc. + // For now we have this "hack" (more of an inefficiency) until auth code can be properly refactored. + (authState.authEnabledConnections.split(',') as AuthFormId[]).map( + (id): RuleContext['authTypes'][number] => { + if (id.includes('builderId')) { + return 'builderId' + } else if (id.includes('identityCenter')) { + return 'identityCenter' + } else if (id.includes('credentials')) { + return 'credentials' + } + return 'unknown' + } + ) + const ruleContext = { ideVersion: vscode.version, extensionVersion: context.extension.packageJSON.version, os: getOperatingSystem(), computeEnv: await getComputeEnvType(), - authTypes: authState.authEnabledConnections.split(','), - authRegions: authState.awsRegion ? [authState.awsRegion] : [], - authStates: [authState.authStatus], + authTypes: [...new Set(authTypes)], authScopes: authState.authScopes ? authState.authScopes?.split(',') : [], installedExtensions: vscode.extensions.all.map((e) => e.id), activeExtensions: vscode.extensions.all.filter((e) => e.isActive).map((e) => e.id), + + // Toolkit (and eventually Q?) may have multiple connections with different regions and states. + // However, this granularity does not seem useful at this time- only the active connection is considered. + authRegions: authState.awsRegion ? [authState.awsRegion] : [], + authStates: [authState.authStatus], } + const { activeExtensions, installedExtensions, ...loggableRuleContext } = ruleContext + getLogger('notifications').debug('getRuleContext() determined rule context: %O', loggableRuleContext) + + return ruleContext } diff --git a/packages/core/src/notifications/types.ts b/packages/core/src/notifications/types.ts index 12e6876e43d..596a5f2b6bd 100644 --- a/packages/core/src/notifications/types.ts +++ b/packages/core/src/notifications/types.ts @@ -6,7 +6,7 @@ import * as vscode from 'vscode' import { EnvType, OperatingSystem } from '../shared/telemetry/util' import { TypeConstructor } from '../shared/utilities/typeConstructors' -import { AuthUserState } from '../shared/telemetry/telemetry.gen' +import { AuthUserState, AuthStatus } from '../shared/telemetry/telemetry.gen' /** Types of information that we can use to determine whether to show a notification or not. */ export type Criteria = @@ -128,9 +128,9 @@ export interface RuleContext { readonly extensionVersion: string readonly os: OperatingSystem readonly computeEnv: EnvType - readonly authTypes: string[] + readonly authTypes: ('credentials' | 'builderId' | 'identityCenter' | 'unknown')[] readonly authRegions: string[] - readonly authStates: string[] + readonly authStates: AuthStatus[] readonly authScopes: string[] readonly installedExtensions: string[] readonly activeExtensions: string[] diff --git a/packages/core/src/test/notifications/rules.test.ts b/packages/core/src/test/notifications/rules.test.ts index 68053a9cd39..bc6232368fa 100644 --- a/packages/core/src/test/notifications/rules.test.ts +++ b/packages/core/src/test/notifications/rules.test.ts @@ -3,10 +3,16 @@ * SPDX-License-Identifier: Apache-2.0 */ +import * as vscode from 'vscode' import assert from 'assert' -import { RuleEngine } from '../../notifications/rules' +import { RuleEngine, getRuleContext } from '../../notifications/rules' import { DisplayIf, ToolkitNotification, RuleContext } from '../../notifications/types' -import { globals } from '../../shared' +import globals from '../../shared/extensionGlobals' +import { Connection, scopesCodeCatalyst } from '../../auth/connection' +import { getOperatingSystem } from '../../shared/telemetry/util' +import { getAuthFormIdsFromConnection } from '../../auth/utils' +import { builderIdStartUrl } from '../../auth/sso/model' +import { amazonQScopes } from '../../codewhisperer' describe('Notifications Rule Engine', function () { const context: RuleContext = { @@ -365,7 +371,7 @@ describe('Notifications Rule Engine', function () { assert.equal( ruleEngine.shouldDisplayNotification( buildNotification({ - additionalCriteria: [{ type: 'InstalledExtensions', values: ['ext1', 'ext2', 'unkownExtension'] }], + additionalCriteria: [{ type: 'InstalledExtensions', values: ['ext1', 'ext2', 'unknownExtension'] }], }) ), false @@ -394,7 +400,7 @@ describe('Notifications Rule Engine', function () { ) }) - it('should display notification for combined criteria', function () { + it('should display notification for combined case-insensitive criteria', function () { assert.equal( ruleEngine.shouldDisplayNotification( buildNotification({ @@ -418,11 +424,11 @@ describe('Notifications Rule Engine', function () { values: ['1.19.0', '1.20.0'], }, additionalCriteria: [ - { type: 'OS', values: ['LINUX', 'MAC'] }, + { type: 'OS', values: ['LINUX', 'mac'] }, { type: 'ComputeEnv', values: ['local', 'ec2'] }, - { type: 'AuthType', values: ['builderId', 'iamIdentityCenter'] }, - { type: 'AuthRegion', values: ['us-east-1', 'us-west-2'] }, - { type: 'AuthState', values: ['connected'] }, + { type: 'AuthType', values: ['builderid', 'iamIdentityCenter'] }, + { type: 'AuthRegion', values: ['US-EAST-1', 'us-west-2'] }, + { type: 'AuthState', values: ['Connected'] }, { type: 'AuthScopes', values: ['codewhisperer:completions', 'codewhisperer:analysis'] }, { type: 'InstalledExtensions', values: ['ext1', 'ext2'] }, { type: 'ActiveExtensions', values: ['ext1', 'ext2'] }, @@ -473,3 +479,56 @@ describe('Notifications Rule Engine', function () { ) }) }) + +describe('Notifications getRuleContext()', function () { + it('should return the correct rule context', async function () { + const context = globals.context + context.extension.packageJSON.version = '0.0.1' + const authEnabledConnections = ( + [ + { + type: 'iam', + }, + { + type: 'sso', + scopes: amazonQScopes, + startUrl: builderIdStartUrl, + }, + { + type: 'sso', + scopes: scopesCodeCatalyst, + startUrl: builderIdStartUrl, + }, + { + type: 'sso', + scopes: amazonQScopes, + startUrl: 'something', + }, + { + type: 'unknown', + }, + ] as Connection[] + ) + .map((c) => getAuthFormIdsFromConnection(c)) + .join(',') + + const ruleContext = await getRuleContext(context, { + authEnabledConnections, + authScopes: amazonQScopes.join(','), + authStatus: 'connected', + awsRegion: 'us-east-1', + }) + assert.deepStrictEqual(ruleContext, { + ideVersion: vscode.version, + extensionVersion: '0.0.1', + os: getOperatingSystem(), + computeEnv: 'test', + authTypes: ['credentials', 'builderId', 'identityCenter', 'unknown'], + authRegions: ['us-east-1'], + authStates: ['connected'], + authScopes: amazonQScopes, + installedExtensions: vscode.extensions.all.map((e) => e.id), + activeExtensions: vscode.extensions.all.filter((e) => e.isActive).map((e) => e.id), + }) + }) +})