Skip to content

Commit 2e83fe0

Browse files
committed
refactor(notifications): remove unneeded InstalledExtensions criteria
We have both ActiveExtensions (checks what extensions are installed and active), and InstalledExtensions (just what extensions are installed). This removes InstalledExtensions because most of the intention behind it is captured by ActiveExtensions. When would we only care about whats installed but not active? Removing a criteria decreases complexity and confusion in both code and for those drafting notifications, so this is an easy removal.
1 parent f98622b commit 2e83fe0

File tree

4 files changed

+3
-41
lines changed

4 files changed

+3
-41
lines changed

packages/core/src/notifications/rules.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -134,8 +134,6 @@ export class RuleEngine {
134134
return hasAnyOfExpected(this.context.authStates)
135135
case 'AuthScopes':
136136
return isEqualSetToExpected(this.context.authScopes)
137-
case 'InstalledExtensions':
138-
return isSuperSetOfExpected(this.context.installedExtensions)
139137
case 'ActiveExtensions':
140138
return isSuperSetOfExpected(this.context.activeExtensions)
141139
default:
@@ -169,15 +167,15 @@ export async function getRuleContext(context: vscode.ExtensionContext, authState
169167
computeEnv: await getComputeEnvType(),
170168
authTypes: [...new Set(authTypes)],
171169
authScopes: authState.authScopes ? authState.authScopes?.split(',') : [],
172-
installedExtensions: vscode.extensions.all.map((e) => e.id),
173170
activeExtensions: vscode.extensions.all.filter((e) => e.isActive).map((e) => e.id),
174171

175172
// Toolkit (and eventually Q?) may have multiple connections with different regions and states.
176173
// However, this granularity does not seem useful at this time- only the active connection is considered.
177174
authRegions: authState.awsRegion ? [authState.awsRegion] : [],
178175
authStates: [authState.authStatus],
179176
}
180-
const { activeExtensions, installedExtensions, ...loggableRuleContext } = ruleContext
177+
178+
const { activeExtensions, ...loggableRuleContext } = ruleContext
181179
getLogger('notifications').debug('getRuleContext() determined rule context: %O', loggableRuleContext)
182180

183181
return ruleContext

packages/core/src/notifications/types.ts

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,15 +9,7 @@ import { TypeConstructor } from '../shared/utilities/typeConstructors'
99
import { AuthUserState, AuthStatus } from '../shared/telemetry/telemetry.gen'
1010

1111
/** Types of information that we can use to determine whether to show a notification or not. */
12-
export type Criteria =
13-
| 'OS'
14-
| 'ComputeEnv'
15-
| 'AuthType'
16-
| 'AuthRegion'
17-
| 'AuthState'
18-
| 'AuthScopes'
19-
| 'InstalledExtensions'
20-
| 'ActiveExtensions'
12+
export type Criteria = 'OS' | 'ComputeEnv' | 'AuthType' | 'AuthRegion' | 'AuthState' | 'AuthScopes' | 'ActiveExtensions'
2113

2214
/** Generic condition where the type determines how the values are evaluated. */
2315
export interface CriteriaCondition {
@@ -132,7 +124,6 @@ export interface RuleContext {
132124
readonly authRegions: string[]
133125
readonly authStates: AuthStatus[]
134126
readonly authScopes: string[]
135-
readonly installedExtensions: string[]
136127
readonly activeExtensions: string[]
137128
}
138129

packages/core/src/test/notifications/controller.test.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@ describe('Notifications Controller', function () {
3939
authRegions: ['us-east-1'],
4040
authStates: ['connected'],
4141
authScopes: ['codewhisperer:completions', 'codewhisperer:analysis'],
42-
installedExtensions: ['ext1', 'ext2', 'ext3'],
4342
activeExtensions: ['ext1', 'ext2'],
4443
})
4544

packages/core/src/test/notifications/rules.test.ts

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ describe('Notifications Rule Engine', function () {
2424
authRegions: ['us-east-1'],
2525
authStates: ['connected'],
2626
authScopes: ['codewhisperer:completions', 'codewhisperer:analysis'],
27-
installedExtensions: ['ext1', 'ext2', 'ext3'],
2827
activeExtensions: ['ext1', 'ext2'],
2928
}
3029

@@ -405,28 +404,6 @@ describe('Notifications Rule Engine', function () {
405404
)
406405
})
407406

408-
it('should display notification for InstalledExtensions criteria', function () {
409-
assert.equal(
410-
ruleEngine.shouldDisplayNotification(
411-
buildNotification({
412-
additionalCriteria: [{ type: 'InstalledExtensions', values: ['ext1', 'ext2'] }],
413-
})
414-
),
415-
true
416-
)
417-
})
418-
419-
it('should NOT display notification for invalid InstalledExtensions criteria', function () {
420-
assert.equal(
421-
ruleEngine.shouldDisplayNotification(
422-
buildNotification({
423-
additionalCriteria: [{ type: 'InstalledExtensions', values: ['ext1', 'ext2', 'unknownExtension'] }],
424-
})
425-
),
426-
false
427-
)
428-
})
429-
430407
it('should display notification for ActiveExtensions criteria', function () {
431408
assert.equal(
432409
ruleEngine.shouldDisplayNotification(
@@ -479,7 +456,6 @@ describe('Notifications Rule Engine', function () {
479456
{ type: 'AuthRegion', values: ['us-east-1', 'us-west-2'] },
480457
{ type: 'AuthState', values: ['connected'] },
481458
{ type: 'AuthScopes', values: ['codewhisperer:completions', 'codewhisperer:analysis'] },
482-
{ type: 'InstalledExtensions', values: ['ext1', 'ext2'] },
483459
{ type: 'ActiveExtensions', values: ['ext1', 'ext2'] },
484460
],
485461
})
@@ -517,7 +493,6 @@ describe('Notifications Rule Engine', function () {
517493
{ type: 'AuthRegion', values: ['us-east-1', 'us-west-2'] },
518494
{ type: 'AuthState', values: ['connected'] },
519495
{ type: 'AuthScopes', values: ['codewhisperer:completions', 'codewhisperer:analysis'] },
520-
{ type: 'InstalledExtensions', values: ['ex1', 'ext2'] },
521496
{ type: 'ActiveExtensions', values: ['ext1', 'ext2'] },
522497

523498
{ type: 'ComputeEnv', values: ['ec2'] }, // no 'local'
@@ -576,7 +551,6 @@ describe('Notifications getRuleContext()', function () {
576551
authRegions: ['us-east-1'],
577552
authStates: ['connected'],
578553
authScopes: amazonQScopes,
579-
installedExtensions: vscode.extensions.all.map((e) => e.id),
580554
activeExtensions: vscode.extensions.all.filter((e) => e.isActive).map((e) => e.id),
581555
})
582556
})

0 commit comments

Comments
 (0)