[AXON-265] New notification logic to notify for every unseen jira ticket assigned to the user (#268)

Author: marcomura

.gitignore

@@ -19,3 +19,4 @@ coverage
 .generated/
 e2e/.resources/
 e2e/.test-extensions/
+*.orig

CHANGELOG.md

@@ -1,16 +1,15 @@
-## What's new in 3.4.24
-
-### Improvements 
-- In PR details page, made UI Changes to make the comment section and Add Reviewer option cleaner and user friendly
-
 ## What's new in 3.4.23
 
 ### Improvements 
+
 - Improved the pull request details page with idea to make UI more intuitive and close to Butbucket UI.
+- In PR details page, made UI Changes to make the comment section and Add Reviewer option cleaner and user friendly
+- Improved the notification for new Jira issues, which now includes issues created long time ago but just assigned to you.
 
 ## What's new in 3.4.22
 
-### Improvements 
+### Improvements
+
 - Changed text in the onboarding flow to "Sign in to ____." Used to be "What version of {product} do you use?"
 
 ## What's new in 3.4.21

src/views/jira/treeViews/jiraAssignedWorkItemsViewProvider.test.ts

@@ -1,4 +1,4 @@
-import { forceCastTo } from '../../../../testsutil';
+import { expansionCastTo, forceCastTo } from '../../../../testsutil';
 import { AssignedWorkItemsViewProvider } from './jiraAssignedWorkItemsViewProvider';
 import { Container } from '../../../container';
 import { JQLManager } from '../../../jira/jqlManager';
@@ -8,6 +8,9 @@ import { JQLEntry } from '../../../config/model';
 import { MinimalIssue } from '@atlassianlabs/jira-pi-common-models';
 import { DetailedSiteInfo } from '../../../atlclients/authInfo';
 import * as vscode from 'vscode';
+import { PromiseRacer } from 'src/util/promises';
+import { JiraNotifier } from './jiraNotifier';
+import { RefreshTimer } from 'src/views/RefreshTimer';
 
 const mockedJqlEntry = forceCastTo<JQLEntry>({
     id: 'jqlId',
@@ -52,6 +55,7 @@ const mockedIssue3 = forceCastTo<MinimalIssue<DetailedSiteInfo>>({
     children: [],
 });
 
+jest.mock('./jiraNotifier');
 jest.mock('../searchJiraHelper');
 jest.mock('../../../container', () => ({
     Container: {
@@ -78,7 +82,9 @@ jest.mock('../../../container', () => ({
     },
 }));
 
-class PromiseRacerMockClass {
+type ExtractPublic<T> = { [P in keyof T]: T[P] };
+
+class PromiseRacerMockClass implements ExtractPublic<PromiseRacer<any>> {
     public static LastInstance: PromiseRacerMockClass | undefined = undefined;
     private count: number;
     private mockedData: any[] = [];
@@ -108,6 +114,44 @@ jest.mock('../../../util/promises', () => ({
     PromiseRacer: jest.fn().mockImplementation((promises) => new PromiseRacerMockClass(promises)),
 }));
 
+class JiraNotifierMockClass implements ExtractPublic<JiraNotifier> {
+    public static LastInstance: JiraNotifierMockClass | undefined = undefined;
+    constructor() {
+        JiraNotifierMockClass.LastInstance = this;
+    }
+    public ignoreAssignedIssues(issues: MinimalIssue<DetailedSiteInfo>[]): void {}
+    public notifyForNewAssignedIssues(issues: MinimalIssue<DetailedSiteInfo>[]): void {}
+}
+
+jest.mock('./jiraNotifier', () => ({
+    JiraNotifier: jest.fn().mockImplementation(() => new JiraNotifierMockClass()),
+}));
+
+class RefreshTimerMockClass implements ExtractPublic<RefreshTimer> {
+    public static LastInstance: RefreshTimerMockClass | undefined = undefined;
+    constructor(
+        _enabledConfigPath: string | undefined,
+        _intervalConfigPath: string,
+        public refreshFunc: () => void,
+    ) {
+        RefreshTimerMockClass.LastInstance = this;
+    }
+    dispose(): void {}
+    isEnabled(): boolean {
+        return false;
+    }
+    setActive(active: boolean): void {}
+}
+
+jest.mock('../../RefreshTimer', () => ({
+    RefreshTimer: jest
+        .fn()
+        .mockImplementation(
+            (enabledConfigPath, intervalConfigPath, refreshFunc) =>
+                new RefreshTimerMockClass(enabledConfigPath, intervalConfigPath, refreshFunc),
+        ),
+}));
+
 const mockedTreeView = {
     onDidChangeVisibility: () => {},
 };
@@ -118,13 +162,44 @@ describe('AssignedWorkItemsViewProvider', () => {
     beforeEach(() => {
         jest.spyOn(vscode.window, 'createTreeView').mockReturnValue(mockedTreeView as any);
         provider = undefined;
+
+        PromiseRacerMockClass.LastInstance = undefined;
+        JiraNotifierMockClass.LastInstance = undefined;
+        RefreshTimerMockClass.LastInstance = undefined;
     });
 
     afterEach(() => {
         provider?.dispose();
         jest.restoreAllMocks();
     });
 
+    describe('initialization', () => {
+        it('onDidSitesAvailableChange is registered during construction', async () => {
+            let onDidSitesAvailableChangeCallback = undefined;
+            jest.spyOn(Container.siteManager, 'onDidSitesAvailableChange').mockImplementation(
+                (func: any, parent: any): any => {
+                    onDidSitesAvailableChangeCallback = (...args: any[]) => func.apply(parent, args);
+                },
+            );
+
+            provider = new AssignedWorkItemsViewProvider();
+
+            expect(onDidSitesAvailableChangeCallback).toBeDefined();
+        });
+
+        it('RefreshTimer is registered during construction and triggers refresh', async () => {
+            let dataChanged = false;
+
+            provider = new AssignedWorkItemsViewProvider();
+            provider.onDidChangeTreeData(() => (dataChanged = true), undefined);
+
+            expect(RefreshTimerMockClass.LastInstance).toBeDefined();
+
+            RefreshTimerMockClass.LastInstance?.refreshFunc();
+            expect(dataChanged).toBeTruthy();
+        });
+    });
+
     describe('getAllDefaultJQLEntries', () => {
         it('should initialize with configure Jira message if no JQL entries', async () => {
             jest.spyOn(Container.jqlManager, 'getAllDefaultJQLEntries').mockReturnValue([]);
@@ -139,7 +214,7 @@ describe('AssignedWorkItemsViewProvider', () => {
         });
 
         it('should initialize with JQL promises if JQL entries exist, returns empty', async () => {
-            const jqlEntries = [forceCastTo<JQLEntry>({ siteId: 'site1', query: 'query1' })];
+            const jqlEntries = [expansionCastTo<JQLEntry>({ siteId: 'site1', query: 'query1' })];
             jest.spyOn(Container.jqlManager, 'getAllDefaultJQLEntries').mockReturnValue(jqlEntries);
             provider = new AssignedWorkItemsViewProvider();
 
@@ -153,7 +228,7 @@ describe('AssignedWorkItemsViewProvider', () => {
         });
 
         it('should initialize with JQL promises if JQL entries exist, returns issues', async () => {
-            const jqlEntries = [forceCastTo<JQLEntry>({ siteId: 'site1', query: 'query1' })];
+            const jqlEntries = [expansionCastTo<JQLEntry>({ siteId: 'site1', query: 'query1' })];
             jest.spyOn(Container.jqlManager, 'getAllDefaultJQLEntries').mockReturnValue(jqlEntries);
             provider = new AssignedWorkItemsViewProvider();
 
@@ -180,18 +255,43 @@ describe('AssignedWorkItemsViewProvider', () => {
         });
     });
 
-    describe('onDidSitesAvailableChange', () => {
-        it('onDidSitesAvailableChange is registered during construction', async () => {
-            let onDidSitesAvailableChangeCallback = undefined;
-            jest.spyOn(Container.siteManager, 'onDidSitesAvailableChange').mockImplementation(
-                (func: any, parent: any): any => {
-                    onDidSitesAvailableChangeCallback = (...args: any[]) => func.apply(parent, args);
-                },
-            );
+    describe('JiraNotifier', () => {
+        it("doesn't notify when the provider is fetching for the first time", async () => {
+            const jqlEntries = [expansionCastTo<JQLEntry>({ siteId: 'site1', query: 'query1' })];
+
+            jest.spyOn(Container.jqlManager, 'getAllDefaultJQLEntries').mockReturnValue(jqlEntries);
+            jest.spyOn(vscode.window, 'showInformationMessage');
 
             provider = new AssignedWorkItemsViewProvider();
 
-            expect(onDidSitesAvailableChangeCallback).toBeDefined();
+            jest.spyOn(JiraNotifierMockClass.LastInstance!, 'ignoreAssignedIssues');
+            jest.spyOn(JiraNotifierMockClass.LastInstance!, 'notifyForNewAssignedIssues');
+
+            PromiseRacerMockClass.LastInstance?.mockData([mockedIssue1, mockedIssue2, mockedIssue3]);
+            await provider.getChildren();
+
+            expect(JiraNotifierMockClass.LastInstance!.ignoreAssignedIssues).toHaveBeenCalled();
+            expect(JiraNotifierMockClass.LastInstance!.notifyForNewAssignedIssues).not.toHaveBeenCalled();
+        });
+
+        it('it notifies for newly fetched items', async () => {
+            const jqlEntries = [expansionCastTo<JQLEntry>({ siteId: 'site1', query: 'query1' })];
+
+            jest.spyOn(Container.jqlManager, 'getAllDefaultJQLEntries').mockReturnValue(jqlEntries);
+            jest.spyOn(vscode.window, 'showInformationMessage');
+
+            provider = new AssignedWorkItemsViewProvider();
+
+            PromiseRacerMockClass.LastInstance?.mockData([mockedIssue1, mockedIssue2, mockedIssue3]);
+            await provider.getChildren();
+
+            jest.spyOn(JiraNotifierMockClass.LastInstance!, 'ignoreAssignedIssues');
+            jest.spyOn(JiraNotifierMockClass.LastInstance!, 'notifyForNewAssignedIssues');
+
+            await provider.getChildren();
+
+            expect(JiraNotifierMockClass.LastInstance!.ignoreAssignedIssues).not.toHaveBeenCalled();
+            expect(JiraNotifierMockClass.LastInstance!.notifyForNewAssignedIssues).toHaveBeenCalled();
         });
     });
 });

src/views/jira/treeViews/jiraAssignedWorkItemsViewProvider.ts

@@ -17,9 +17,9 @@ import { JiraIssueNode, TreeViewIssue, executeJqlQuery, loginToJiraMessageNode }
 import { configuration } from '../../../config/configuration';
 import { CommandContext, setCommandContext } from '../../../commandContext';
 import { SitesAvailableUpdateEvent } from '../../../siteManager';
-import { NewIssueMonitor } from '../../../jira/newIssueMonitor';
 import { RefreshTimer } from '../../RefreshTimer';
 import { viewScreenEvent } from '../../../analytics';
+import { JiraNotifier } from './jiraNotifier';
 
 const AssignedWorkItemsViewProviderId = 'atlascode.views.jira.assignedWorkItemsTreeView';
 
@@ -29,13 +29,15 @@ export class AssignedWorkItemsViewProvider extends Disposable implements TreeDat
     private _onDidChangeTreeData = new EventEmitter<TreeItem | undefined | void>();
     readonly onDidChangeTreeData = this._onDidChangeTreeData.event;
 
-    private _disposable: Disposable;
-    private _initPromises: PromiseRacer<TreeViewIssue[]> | undefined;
-    private _initChildren: TreeItem[] = [];
-    private _newIssueMonitor: NewIssueMonitor;
+    private readonly _disposable: Disposable;
+    private readonly _initPromises: PromiseRacer<TreeViewIssue[]> | undefined;
+    private readonly _initChildren: TreeItem[] = [];
+    private readonly _jiraNotifier = new JiraNotifier();
+
+    private _skipNotificationForNextFetch = false;
 
     constructor() {
-        super(() => this.dispose);
+        super(() => this.dispose());
 
         setCommandContext(CommandContext.JiraExplorer, false);
         setCommandContext(CommandContext.AssignedIssueExplorer, Container.config.jira.explorer.enabled);
@@ -50,8 +52,6 @@ export class AssignedWorkItemsViewProvider extends Disposable implements TreeDat
             treeView,
         );
 
-        this._newIssueMonitor = new NewIssueMonitor(() => Container.jqlManager.getAllDefaultJQLEntries());
-
         const jqlEntries = Container.jqlManager.getAllDefaultJQLEntries();
         if (jqlEntries.length) {
             this._initPromises = new PromiseRacer(jqlEntries.map(executeJqlQuery));
@@ -72,24 +72,28 @@ export class AssignedWorkItemsViewProvider extends Disposable implements TreeDat
     private onConfigurationChanged(e: ConfigurationChangeEvent): void {
         if (configuration.changed(e, 'jira.explorer.enabled')) {
             setCommandContext(CommandContext.AssignedIssueExplorer, Container.config.jira.explorer.enabled);
-            this.refresh();
+            this.refreshWithoutNotifications();
         } else if (configuration.changed(e, 'jira.explorer')) {
-            this.refresh();
+            this.refreshWithoutNotifications();
         }
     }
 
     private onSitesDidChange(e: SitesAvailableUpdateEvent): void {
         if (e.product.key === ProductJira.key) {
-            this.refresh();
+            this.refreshWithoutNotifications();
         }
     }
 
     public dispose(): void {
         this._disposable.dispose();
     }
 
+    private refreshWithoutNotifications(): void {
+        this._skipNotificationForNextFetch = true;
+        this.refresh();
+    }
+
     private refresh(): void {
-        this._newIssueMonitor.checkForNewIssues();
         this._onDidChangeTreeData.fire();
     }
 
@@ -120,6 +124,8 @@ export class AssignedWorkItemsViewProvider extends Disposable implements TreeDat
                 }
 
                 SearchJiraHelper.appendIssues(issues, AssignedWorkItemsViewProviderId);
+                this._jiraNotifier.ignoreAssignedIssues(issues);
+
                 this._initChildren.push(...this.buildTreeItemsFromIssues(issues));
                 break;
             }
@@ -139,7 +145,16 @@ export class AssignedWorkItemsViewProvider extends Disposable implements TreeDat
             }
 
             const allIssues = (await Promise.all(jqlEntries.map(executeJqlQuery))).flat();
+
+            if (this._skipNotificationForNextFetch) {
+                this._skipNotificationForNextFetch = false;
+                this._jiraNotifier.ignoreAssignedIssues(allIssues);
+            } else {
+                this._jiraNotifier.notifyForNewAssignedIssues(allIssues);
+            }
+
             SearchJiraHelper.setIssues(allIssues, AssignedWorkItemsViewProviderId);
+
             return this.buildTreeItemsFromIssues(allIssues);
         }
     }