From e1d1673f8d38c8ac03bd2b46db45b0ec77fe9f8f Mon Sep 17 00:00:00 2001 From: Martin Giger Date: Sat, 10 Apr 2021 12:01:54 +0300 Subject: [PATCH] Bug 1699123 - Only leave conversation if join fails. r=clokep Differential Revision: https://phabricator.services.mozilla.com/D111436 --HG-- extra : amend_source : 258ec053a37b5074f07661994c04411b3fe5fdae --- chat/protocols/matrix/matrix.jsm | 55 ++++++++++------- .../test/test_genericMatrixConversation.js | 60 +++++++++++++++++++ .../matrix/test/test_roomTypeChange.js | 4 +- 3 files changed, 95 insertions(+), 24 deletions(-) diff --git a/chat/protocols/matrix/matrix.jsm b/chat/protocols/matrix/matrix.jsm index d2291e494b..a77ea53e05 100644 --- a/chat/protocols/matrix/matrix.jsm +++ b/chat/protocols/matrix/matrix.jsm @@ -201,8 +201,16 @@ var GenericMatrixConversation = { * Leave the room if we close the conversation. */ close() { - this._close?.(); this._account._client.leave(this._roomId); + this.forget(); + }, + + /** + * Forget about this conversation instance. This closes the conversation in + * the UI, but doesn't update the user's membership in the room. + */ + forget() { + this._close?.(); this._account.roomList.delete(this._roomId); GenericConversationPrototype.close.call(this); }, @@ -654,11 +662,8 @@ MatrixAccount.prototype = { // We want to remove all the conversations. We are not using conv.close // function call because we don't want user to leave all the matrix rooms. // User just want to remove the account so we need to remove the listed - // conversations. GenericConversationPrototype.close is used at various - // places in the file. It's because of the same reason, we want to remove - // the conversation only, don't want user to leave the room. conv.close - // function call will make user leave the room and close the conversation. - GenericConversationPrototype.close.call(conv); + // conversations. + conv.forget(); } delete this.roomList; // We want to clear data stored for syncing in indexedDB so when @@ -971,9 +976,7 @@ MatrixAccount.prototype = { // a DM room by SDK a direct conversation and all other rooms as a group // conversations. if (member.membership === "leave" && member.userId == this.userId) { - this.roomList.delete(member.roomId); - conv._close?.(); - GenericConversationPrototype.close.call(conv); + conv.forget(); } else if ( member.membership === "join" || member.membership === "leave" @@ -1129,9 +1132,7 @@ MatrixAccount.prototype = { // Ensure existing conversations are up to date for (const [roomId, conv] of this.roomList.entries()) { if (!joinedRooms.includes(roomId)) { - this.roomList.delete(roomId); - conv._close?.(); - GenericConversationPrototype.close.call(conv); + conv.forget(); } else { const updatedConv = this.checkRoomForUpdate(conv); updatedConv.catchup().catch(error => updatedConv.ERROR(error)); @@ -1243,7 +1244,7 @@ MatrixAccount.prototype = { * @returns {MatrixDirectConversation} */ convertToDM(groupConv) { - GenericConversationPrototype.close.call(groupConv); + groupConv.forget(); let conv = new MatrixDirectConversation(this, groupConv._roomId); groupConv.replaceRoom(conv); this.roomList.set(groupConv._roomId, conv); @@ -1261,8 +1262,7 @@ MatrixAccount.prototype = { * @return {MatrixConversation} */ convertToGroup(directConv) { - directConv._close(); - GenericConversationPrototype.close.call(directConv); + directConv.forget(); let conv = new MatrixConversation(this, directConv._roomId, this.userId); directConv.replaceRoom(conv); this.roomList.set(directConv._roomId, conv); @@ -1313,17 +1313,21 @@ MatrixAccount.prototype = { conv.joining = true; this._client .joinRoom(roomId) + .catch(error => { + conv.joining = false; + conv.close(); + throw error; + }) .then(room => { conv.initRoom(room); conv.joining = false; }) .catch(error => { this.ERROR(error); - conv.joining = false; - // TODO we should probably only close (send a leave) when the actual - // join failed, not when initRoom errors, at least for rooms we get - // during initial sync. - conv.close(); + if (conv.joining) { + conv.joining = false; + conv.forget(); + } }); return conv; @@ -1555,6 +1559,11 @@ MatrixAccount.prototype = { conv.joining = true; this._client .joinRoom(DMRoomId || roomID) + .catch(error => { + conv.joining = false; + conv.close(); + throw error; + }) .then(room => { conv.initRoom(room); conv.joining = false; @@ -1563,8 +1572,10 @@ MatrixAccount.prototype = { this.ERROR( "Error creating conversation " + (DMRoomId || roomID) + ": " + error ); - conv.joining = false; - conv.close(); + if (conv.joining) { + conv.joining = false; + conv.forget(); + } }); return conv; diff --git a/chat/protocols/matrix/test/test_genericMatrixConversation.js b/chat/protocols/matrix/test/test_genericMatrixConversation.js index 907048071f..ed0d1dfa00 100644 --- a/chat/protocols/matrix/test/test_genericMatrixConversation.js +++ b/chat/protocols/matrix/test/test_genericMatrixConversation.js @@ -239,3 +239,63 @@ function makeEvent(sender, content = {}, redacted = false) { }, }; } + +add_task(function test_forgetWith_close() { + const roomList = new Map(); + const roomStub = { + _close() { + this.closeCalled = true; + }, + _roomId: "foo", + _account: { + roomList, + }, + // stubs for jsProtoHelper implementations + addObserver() {}, + unInit() {}, + }; + roomList.set(roomStub._roomId, roomStub); + Services.conversations.addConversation(roomStub); + + matrix.GenericMatrixConversation.forget.call(roomStub); + ok(!roomList.has(roomStub._roomId)); + ok(roomStub.closeCalled); +}); + +add_task(function test_forgetWithout_close() { + const roomList = new Map(); + const roomStub = { + _roomId: "foo", + _account: { + roomList, + }, + // stubs for jsProtoHelper implementations + addObserver() {}, + unInit() {}, + }; + roomList.set(roomStub._roomId, roomStub); + Services.conversations.addConversation(roomStub); + + matrix.GenericMatrixConversation.forget.call(roomStub); + ok(!roomList.has(roomStub._roomId)); +}); + +add_task(function test_close() { + const roomStub = { + forget() { + this.forgetCalled = true; + }, + _roomId: "foo", + _account: { + _client: { + leave(roomId) { + roomStub.leftRoom = roomId; + }, + }, + }, + }; + + matrix.GenericMatrixConversation.close.call(roomStub); + equal(roomStub.leftRoom, roomStub._roomId); + ok(roomStub.forgetCalled); +}); diff --git a/chat/protocols/matrix/test/test_roomTypeChange.js b/chat/protocols/matrix/test/test_roomTypeChange.js index 666fade9fa..2f7b184bb6 100644 --- a/chat/protocols/matrix/test/test_roomTypeChange.js +++ b/chat/protocols/matrix/test/test_roomTypeChange.js @@ -65,7 +65,7 @@ add_task(async function test_toDMConversation() { const roomListInstance = acc.roomList.get(roomId); strictEqual(roomListInstance, newRoom); } finally { - newRoom.close(); + newRoom.forget(); } }); @@ -84,6 +84,6 @@ add_task(async function test_toGroupConversation() { const roomListInstance = acc.roomList.get(roomId); strictEqual(roomListInstance, newRoom); } finally { - newRoom.close(); + newRoom.forget(); } });