From 7c96990119226eeaaac472d475ff5124c6a64f31 Mon Sep 17 00:00:00 2001 From: Maxim Shishmarev Date: Wed, 6 Nov 2019 13:22:21 +1100 Subject: [PATCH] Correctly handle server errors in multi-deletion --- js/models/conversations.js | 40 ++++++++++++++------------ js/modules/loki_app_dot_net_api.js | 36 +++++++++++++++++++---- js/views/conversation_view.js | 9 ++++-- ts/components/conversation/Message.tsx | 6 ++-- tslint.json | 5 +++- 5 files changed, 63 insertions(+), 33 deletions(-) diff --git a/js/models/conversations.js b/js/models/conversations.js index baa298346..82845b5a0 100644 --- a/js/models/conversations.js +++ b/js/models/conversations.js @@ -2487,30 +2487,32 @@ return false; } - let success; - const shouldBeDeleted = []; - if (messages.length > 1) { - success = await channelAPI.deleteMessages( - messages.map(m => m.getServerId()) - ); - } else { - success = await channelAPI.deleteMessages([messages[0].getServerId()]); - } + const invalidMessages = messages.filter(m => !m.getServerId()); + const pendingMessages = messages.filter(m => m.getServerId()); - messages.forEach(m => { + let deletedServerIds = []; + let ignoredServerIds = []; - // If the message has errors it is likely not saved - // on the server, so we delete it locally unconditionally - const shouldDeleteLocally = success || m.hasErrors() || !m.getServerId(); + if (pendingMessages.length > 0) { + const result = await channelAPI.deleteMessages( + pendingMessages.map(m => m.getServerId()) + ); + deletedServerIds = result.deletedIds; + ignoredServerIds = result.ignoredIds; + } - if (shouldDeleteLocally) { - this.removeMessage(m.id); - } + const toDeleteLocallyServerIds = _.union( + deletedServerIds, + ignoredServerIds + ); + let toDeleteLocally = messages.filter(m => + toDeleteLocallyServerIds.includes(m.getServerId()) + ); + toDeleteLocally = _.union(toDeleteLocally, invalidMessages); - }); + toDeleteLocally.forEach(m => this.removeMessage(m.id)); - /// TODO: not sure what to return here - return shouldDeleteLocally; + return toDeleteLocally; }, removeMessage(messageId) { diff --git a/js/modules/loki_app_dot_net_api.js b/js/modules/loki_app_dot_net_api.js index 6b2f651b1..8398ef65c 100644 --- a/js/modules/loki_app_dot_net_api.js +++ b/js/modules/loki_app_dot_net_api.js @@ -525,18 +525,42 @@ class LokiPublicChannelAPI { this.modStatus ? `loki/v1/moderation/messages` : `loki/v1/messages`, { method: 'DELETE', params: { ids: serverIds } } ); - if (!res.err && res.response) { - log.info(`deleted ${serverIds} on ${this.baseChannelUrl}`); - return true; + if (!res.err) { + const deletedIds = res.response.data + .filter(d => d.is_deleted) + .map(d => d.id); + + if (deletedIds.length > 0) { + log.info(`deleted ${serverIds} on ${this.baseChannelUrl}`); + } + + const failedIds = res.response.data + .filter(d => !d.is_deleted) + .map(d => d.id); + + if (failedIds.length > 0) { + log.warn(`failed to delete ${failedIds} on ${this.baseChannelUrl}`); + } + + // Note: if there is no entry for message, we assume it wasn't found + // on the server, so it is not treated as explicitly failed + const ignoredIds = _.difference( + serverIds, + _.union(failedIds, deletedIds) + ); + + if (ignoredIds.length > 0) { + log.warn(`No response for ${ignoredIds} on ${this.baseChannelUrl}`); + } + + return { deletedIds, ignoredIds }; } - // fire an alert - log.warn(`failed to delete ${serverIds} on ${this.baseChannelUrl}`); if (canThrow) { throw new textsecure.PublicChatError( 'Failed to delete public chat message' ); } - return false; + return { deletedIds: [], ignoredIds: [] }; } // used for sending messages diff --git a/js/views/conversation_view.js b/js/views/conversation_view.js index 1f6426ad5..7689071f0 100644 --- a/js/views/conversation_view.js +++ b/js/views/conversation_view.js @@ -1385,18 +1385,21 @@ })(); const doDelete = async () => { + let toDeleteLocally; + if (this.model.isPublic()) { - const success = await this.model.deletePublicMessages(messages); - if (!success) { + toDeleteLocally = await this.model.deletePublicMessages(messages); + if (toDeleteLocally.length === 0) { // Message failed to delete from server, show error? return; } } else { messages.forEach(m => this.model.messageCollection.remove(m.id)); + toDeleteLocally = messages; } await Promise.all( - messages.map(async m => { + toDeleteLocally.map(async m => { await window.Signal.Data.removeMessage(m.id, { Message: Whisper.Message, }); diff --git a/ts/components/conversation/Message.tsx b/ts/components/conversation/Message.tsx index d4e1f21e0..81263d625 100644 --- a/ts/components/conversation/Message.tsx +++ b/ts/components/conversation/Message.tsx @@ -1076,9 +1076,7 @@ export class Message extends React.PureComponent { )} > {this.renderError(isIncoming)} - {isRss - ? null - : this.renderMenu(!isIncoming, triggerId)} + {isRss ? null : this.renderMenu(!isIncoming, triggerId)}
{ {this.renderSendMessageButton()}
{this.renderError(!isIncoming)} - {(isRss || multiSelectMode) + {isRss || multiSelectMode ? null : this.renderMenu(isIncoming, triggerId)} {multiSelectMode ? null : this.renderContextMenu(triggerId)} diff --git a/tslint.json b/tslint.json index fae7c9a7c..b114819d2 100644 --- a/tslint.json +++ b/tslint.json @@ -145,7 +145,10 @@ "method": "render", "comment": "Usage has been approved by Ryan Tharp on 2019-07-22" } - ] + ], + // Reasonable functions can exceed the default of 100 lines + // due to auto-formatting + "max-func-body-length": [true, 150] }, "rulesDirectory": ["node_modules/tslint-microsoft-contrib"] }