Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Commit

Permalink
bugfix: fix an issue where the Notifier would incorrectly fire for no…
Browse files Browse the repository at this point in the history 
…n-timeline events (#9664)

* bugfix: fix an issue where the Notifier would incorrectly fire for non-timeline events

This was caused by listening for ClientEvent.Event, not RoomEvent.Timeline.
Fixes element-hq/element-web#17263

* tsc strict checks maybe

* More types?

* Update src/Notifier.ts

Co-authored-by: Michael Telatynski <7t3chguy@gmail.com>

* Update src/Notifier.ts

* fix LL test; review comments

* More tests

* More tsc strict checks..

* More strict ts..

* More ts

Co-authored-by: Michael Telatynski <7t3chguy@gmail.com>
  • Loading branch information
@kegsay @t3chguy
kegsay and t3chguy committed Dec 1, 2022
1 parent ccfb455 commit 1f703b8
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 20 deletions.
20 changes: 17 additions & 3 deletions src/Notifier.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
PermissionChanged as PermissionChangedEvent,
} from "@matrix-org/analytics-events/types/typescript/PermissionChanged";
import { ISyncStateData, SyncState } from "matrix-js-sdk/src/sync";
import { IRoomTimelineData } from "matrix-js-sdk/src/matrix";

import { MatrixClientPeg } from './MatrixClientPeg';
import { PosthogAnalytics } from "./PosthogAnalytics";
Expand Down Expand Up @@ -217,7 +218,7 @@ export const Notifier = {
this.boundOnRoomReceipt = this.boundOnRoomReceipt || this.onRoomReceipt.bind(this);
this.boundOnEventDecrypted = this.boundOnEventDecrypted || this.onEventDecrypted.bind(this);

MatrixClientPeg.get().on(ClientEvent.Event, this.boundOnEvent);
MatrixClientPeg.get().on(RoomEvent.Timeline, this.boundOnEvent);
MatrixClientPeg.get().on(RoomEvent.Receipt, this.boundOnRoomReceipt);
MatrixClientPeg.get().on(MatrixEventEvent.Decrypted, this.boundOnEventDecrypted);
MatrixClientPeg.get().on(ClientEvent.Sync, this.boundOnSyncStateChange);
Expand All @@ -227,7 +228,7 @@ export const Notifier = {

stop: function(this: typeof Notifier) {
if (MatrixClientPeg.get()) {
MatrixClientPeg.get().removeListener(ClientEvent.Event, this.boundOnEvent);
MatrixClientPeg.get().removeListener(RoomEvent.Timeline, this.boundOnEvent);
MatrixClientPeg.get().removeListener(RoomEvent.Receipt, this.boundOnRoomReceipt);
MatrixClientPeg.get().removeListener(MatrixEventEvent.Decrypted, this.boundOnEventDecrypted);
MatrixClientPeg.get().removeListener(ClientEvent.Sync, this.boundOnSyncStateChange);
Expand Down Expand Up @@ -368,7 +369,15 @@ export const Notifier = {
}
},

onEvent: function(this: typeof Notifier, ev: MatrixEvent) {
onEvent: function(
this: typeof Notifier,
ev: MatrixEvent,
room: Room | undefined,
toStartOfTimeline: boolean | undefined,
removed: boolean,
data: IRoomTimelineData,
) {
if (!data.liveEvent) return; // only notify for new things, not old.
if (!this.isSyncing) return; // don't alert for any messages initially
if (ev.getSender() === MatrixClientPeg.get().getUserId()) return;

Expand Down Expand Up @@ -428,6 +437,11 @@ export const Notifier = {
}
}
const room = MatrixClientPeg.get().getRoom(roomId);
if (!room) {
// e.g we are in the process of joining a room.
// Seen in the cypress lazy-loading test.
return;
}

const actions = MatrixClientPeg.get().getPushActionsForEvent(ev);

Expand Down
66 changes: 49 additions & 17 deletions test/Notifier-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ limitations under the License.

import { mocked, MockedObject } from "jest-mock";
import { ClientEvent, MatrixClient } from "matrix-js-sdk/src/client";
import { Room } from "matrix-js-sdk/src/models/room";
import { MatrixEvent } from "matrix-js-sdk/src/models/event";
import { Room, RoomEvent } from "matrix-js-sdk/src/models/room";
import { IContent, MatrixEvent } from "matrix-js-sdk/src/models/event";
import { SyncState } from "matrix-js-sdk/src/sync";
import { waitFor } from "@testing-library/react";

Expand Down Expand Up @@ -60,12 +60,19 @@ describe("Notifier", () => {
let mockClient: MockedObject<MatrixClient>;
let testRoom: Room;
let accountDataEventKey: string;
let accountDataStore = {};
let accountDataStore: Record<string, MatrixEvent | undefined> = {};

let mockSettings: Record<string, boolean> = {};

const userId = "@bob:example.org";

const emitLiveEvent = (event: MatrixEvent): void => {
mockClient!.emit(RoomEvent.Timeline, event, testRoom, false, false, {
liveEvent: true,
timeline: testRoom.getLiveTimeline(),
});
};

beforeEach(() => {
accountDataStore = {};
mockClient = getMockClientWithEventEmitter({
Expand Down Expand Up @@ -150,7 +157,7 @@ describe("Notifier", () => {
});

it('does not create notifications before syncing has started', () => {
mockClient!.emit(ClientEvent.Event, event);
emitLiveEvent(event);

expect(MockPlatform.displayNotification).not.toHaveBeenCalled();
expect(MockPlatform.loudNotification).not.toHaveBeenCalled();
Expand All @@ -160,7 +167,30 @@ describe("Notifier", () => {
const ownEvent = new MatrixEvent({ sender: userId });

mockClient!.emit(ClientEvent.Sync, SyncState.Syncing, null);
mockClient!.emit(ClientEvent.Event, ownEvent);
emitLiveEvent(ownEvent);

expect(MockPlatform.displayNotification).not.toHaveBeenCalled();
expect(MockPlatform.loudNotification).not.toHaveBeenCalled();
});

it('does not create notifications for non-live events (scrollback)', () => {
mockClient!.emit(ClientEvent.Sync, SyncState.Syncing, null);
mockClient!.emit(RoomEvent.Timeline, event, testRoom, false, false, {
liveEvent: false,
timeline: testRoom.getLiveTimeline(),
});

expect(MockPlatform.displayNotification).not.toHaveBeenCalled();
expect(MockPlatform.loudNotification).not.toHaveBeenCalled();
});

it('does not create notifications for rooms which cannot be obtained via client.getRoom', () => {
mockClient!.emit(ClientEvent.Sync, SyncState.Syncing, null);
mockClient.getRoom.mockReturnValue(null);
mockClient!.emit(RoomEvent.Timeline, event, testRoom, false, false, {
liveEvent: true,
timeline: testRoom.getLiveTimeline(),
});

expect(MockPlatform.displayNotification).not.toHaveBeenCalled();
expect(MockPlatform.loudNotification).not.toHaveBeenCalled();
Expand All @@ -175,15 +205,15 @@ describe("Notifier", () => {
});

mockClient!.emit(ClientEvent.Sync, SyncState.Syncing, null);
mockClient!.emit(ClientEvent.Event, event);
emitLiveEvent(event);

expect(MockPlatform.displayNotification).not.toHaveBeenCalled();
expect(MockPlatform.loudNotification).not.toHaveBeenCalled();
});

it('creates desktop notification when enabled', () => {
mockClient!.emit(ClientEvent.Sync, SyncState.Syncing, null);
mockClient!.emit(ClientEvent.Event, event);
emitLiveEvent(event);

expect(MockPlatform.displayNotification).toHaveBeenCalledWith(
testRoom.name,
Expand All @@ -196,7 +226,7 @@ describe("Notifier", () => {

it('creates a loud notification when enabled', () => {
mockClient!.emit(ClientEvent.Sync, SyncState.Syncing, null);
mockClient!.emit(ClientEvent.Event, event);
emitLiveEvent(event);

expect(MockPlatform.loudNotification).toHaveBeenCalledWith(
event, testRoom,
Expand All @@ -212,7 +242,7 @@ describe("Notifier", () => {
});

mockClient!.emit(ClientEvent.Sync, SyncState.Syncing, null);
mockClient!.emit(ClientEvent.Event, event);
emitLiveEvent(event);

// desktop notification created
expect(MockPlatform.displayNotification).toHaveBeenCalled();
Expand All @@ -222,12 +252,13 @@ describe("Notifier", () => {
});

describe("_displayPopupNotification", () => {
it.each([
const testCases: {event: IContent | undefined, count: number}[] = [
{ event: { is_silenced: true }, count: 0 },
{ event: { is_silenced: false }, count: 1 },
{ event: undefined, count: 1 },
])("does not dispatch when notifications are silenced", ({ event, count }) => {
mockClient.setAccountData(accountDataEventKey, event);
];
it.each(testCases)("does not dispatch when notifications are silenced", ({ event, count }) => {
mockClient.setAccountData(accountDataEventKey, event!);
Notifier._displayPopupNotification(testEvent, testRoom);
expect(MockPlatform.displayNotification).toHaveBeenCalledTimes(count);
});
Expand All @@ -243,16 +274,17 @@ describe("Notifier", () => {
});

describe("_playAudioNotification", () => {
it.each([
const testCases: {event: IContent | undefined, count: number}[] = [
{ event: { is_silenced: true }, count: 0 },
{ event: { is_silenced: false }, count: 1 },
{ event: undefined, count: 1 },
])("does not dispatch when notifications are silenced", ({ event, count }) => {
];
it.each(testCases)("does not dispatch when notifications are silenced", ({ event, count }) => {
// It's not ideal to only look at whether this function has been called
// but avoids starting to look into DOM stuff
Notifier.getSoundForRoom = jest.fn();

mockClient.setAccountData(accountDataEventKey, event);
mockClient.setAccountData(accountDataEventKey, event!);
Notifier._playAudioNotification(testEvent, testRoom);
expect(Notifier.getSoundForRoom).toHaveBeenCalledTimes(count);
});
Expand All @@ -267,7 +299,7 @@ describe("Notifier", () => {
notify: true,
tweaks: {},
});

Notifier.start();
Notifier.onSyncStateChange(SyncState.Syncing);
});

Expand All @@ -283,7 +315,7 @@ describe("Notifier", () => {
content: {},
event: true,
});
Notifier.onEvent(callEvent);
emitLiveEvent(callEvent);
return callEvent;
};

Expand Down

0 comments on commit 1f703b8

Please sign in to comment.