Refactor message corrections

- Save date for older message versions and display it in the modal
- Properly handle the correction being received *before* the corrected message

CHANGES.md

@@ -14,6 +14,7 @@
 - New API method [\_converse.api.disco.features.get](https://conversejs.org/docs/html/api/-_converse.api.disco.features.html#.get)
 - New config setting [muc_show_join_leave_status](https://conversejs.org/docs/html/configuration.html#muc-show-join-leave-status)
 - New event: `chatBoxBlurred`.
+- Properly handle message correction being received before the corrected message
 - #1296: `embedded` view mode shows `chatbox-navback` arrow in header
 - #1465: When highlighting a roster contact, they're incorrectly shown as online
 - #1532: Converse reloads on enter pressed in the filter box

sass/_messages.scss

@@ -1,4 +1,9 @@
 #conversejs {
+    .older-msg {
+        time {
+            font-weight: bold;
+        }
+    }
     .message {
         .mention {
             font-weight: bold;

spec/messages.js

@@ -76,8 +76,11 @@
             const corrected_message = view.model.messages.at(0);
             expect(corrected_message.get('msgid')).toBe(first_msg.get('msgid'));
             expect(corrected_message.get('correcting')).toBe(false);
-            expect(corrected_message.get('older_versions').length).toBe(1);
-            expect(corrected_message.get('older_versions')[0]).toBe('But soft, what light through yonder airlock breaks?');
+
+            const older_versions = corrected_message.get('older_versions');
+            const keys = Object.keys(older_versions);
+            expect(keys.length).toBe(1);
+            expect(older_versions[keys[0]]).toBe('But soft, what light through yonder airlock breaks?');
 
             expect(view.el.querySelectorAll('.chat-msg').length).toBe(1);
             expect(u.hasClass('correcting', view.el.querySelector('.chat-msg'))).toBe(false);
@@ -181,8 +184,11 @@
             const corrected_message = view.model.messages.at(0);
             expect(corrected_message.get('msgid')).toBe(first_msg.get('msgid'));
             expect(corrected_message.get('correcting')).toBe(false);
-            expect(corrected_message.get('older_versions').length).toBe(1);
-            expect(corrected_message.get('older_versions')[0]).toBe('But soft, what light through yonder airlock breaks?');
+
+            const older_versions = corrected_message.get('older_versions');
+            const keys = Object.keys(older_versions);
+            expect(keys.length).toBe(1);
+            expect(older_versions[keys[0]]).toBe('But soft, what light through yonder airlock breaks?');
 
             expect(view.el.querySelectorAll('.chat-msg').length).toBe(1);
             await test_utils.waitUntil(() => (u.hasClass('correcting', view.el.querySelector('.chat-msg')) === false), 500);
@@ -1472,8 +1478,8 @@
                 await test_utils.waitUntil(() => u.isVisible(modal.el), 1000);
                 const older_msgs = modal.el.querySelectorAll('.older-msg');
                 expect(older_msgs.length).toBe(2);
-                expect(older_msgs[0].textContent).toBe('But soft, what light through yonder airlock breaks?');
-                expect(older_msgs[1].textContent).toBe('But soft, what light through yonder chimney breaks?');
+                expect(older_msgs[0].childNodes[0].nodeName).toBe('TIME');
+                expect(older_msgs[0].childNodes[1].textContent).toBe(': But soft, what light through yonder airlock breaks?');
                 done();
             }));
 
@@ -2425,8 +2431,10 @@
             await test_utils.waitUntil(() => u.isVisible(modal.el), 1000);
             const older_msgs = modal.el.querySelectorAll('.older-msg');
             expect(older_msgs.length).toBe(2);
-            expect(older_msgs[0].textContent).toBe('But soft, what light through yonder airlock breaks?');
-            expect(older_msgs[1].textContent).toBe('But soft, what light through yonder chimney breaks?');
+            expect(older_msgs[0].childNodes[1].textContent).toBe(': But soft, what light through yonder airlock breaks?');
+            expect(older_msgs[0].childNodes[0].nodeName).toBe('TIME');
+            expect(older_msgs[1].childNodes[0].nodeName).toBe('TIME');
+            expect(older_msgs[1].childNodes[1].textContent).toBe(': But soft, what light through yonder chimney breaks?');
             done();
         }));
 
@@ -2495,8 +2503,11 @@
             const corrected_message = view.model.messages.at(0);
             expect(corrected_message.get('msgid')).toBe(first_msg.get('msgid'));
             expect(corrected_message.get('correcting')).toBe(false);
-            expect(corrected_message.get('older_versions').length).toBe(1);
-            expect(corrected_message.get('older_versions')[0]).toBe('But soft, what light through yonder airlock breaks?');
+
+            const older_versions = corrected_message.get('older_versions');
+            const keys = Object.keys(older_versions);
+            expect(keys.length).toBe(1);
+            expect(older_versions[keys[0]]).toBe('But soft, what light through yonder airlock breaks?');
 
             expect(view.el.querySelectorAll('.chat-msg').length).toBe(1);
             expect(u.hasClass('correcting', view.el.querySelector('.chat-msg'))).toBe(false);

src/converse-message-view.js

@@ -69,7 +69,8 @@ converse.plugins.add('converse-message-view', {
             toHTML () {
                 return tpl_message_versions_modal(Object.assign(
                     this.model.toJSON(), {
-                    '__': __
+                    '__': __,
+                    'dayjs': dayjs
                 }));
             }
         });

src/headless/converse-chatboxes.js

@@ -382,30 +382,52 @@ converse.plugins.add('converse-chatboxes', {
                 }
             },
 
-            handleMessageCorrection (stanza) {
-                const replace = sizzle(`replace[xmlns="${Strophe.NS.MESSAGE_CORRECT}"]`, stanza).pop();
-                if (replace) {
-                    const msgid = replace && replace.getAttribute('id') || stanza.getAttribute('id'),
-                        message = msgid && this.messages.findWhere({msgid});
-
-                    if (!message) {
-                        // XXX: Looks like we received a correction for a
-                        // non-existing message, probably due to MAM.
-                        // Not clear what can be done about this... we'll
-                        // just create it as a separate message for now.
-                        return false;
-                    }
-                    const older_versions = message.get('older_versions') || [];
-                    older_versions.push(message.get('message'));
-                    message.save({
-                        'message': this.getMessageBody(stanza),
-                        'references': this.getReferencesFromStanza(stanza),
-                        'older_versions': older_versions,
-                        'edited': (new Date()).toISOString()
-                    });
-                    return true;
+            /**
+             * If the passed in `message` stanza contains an
+             * [XEP-0308](https://xmpp.org/extensions/xep-0308.html#usecase)
+             * `<replace>` element, return its `id` attribute.
+             * @private
+             * @method _converse.ChatBox#getReplaceId
+             * @param { XMLElement } stanza
+             */
+            getReplaceId (stanza) {
+                const el = sizzle(`replace[xmlns="${Strophe.NS.MESSAGE_CORRECT}"]`, stanza).pop();
+                if (el) {
+                    return el.getAttribute('id');
                 }
-                return false;
+            },
+
+            /**
+             * Determine whether the passed in message attributes represent a
+             * message which corrects a previously received message, or an
+             * older message which has already been corrected.
+             * In both cases, update the corrected message accordingly.
+             * @private
+             * @method _converse.ChatBox#correctMessage
+             * @param { object } attrs - Attributes representing a received
+             *     message, as returned by
+             *     {@link _converse.ChatBox.getMessageAttributesFromStanza}
+             */
+            correctMessage (attrs) {
+                if (!attrs.msgid || !attrs.from) {
+                    return;
+                }
+                const message = this.messages.findWhere({'msgid': attrs.msgid, 'from': attrs.from});
+                if (!message) {
+                    return;
+                }
+                const older_versions = message.get('older_versions') || {};
+                if ((attrs.time < message.get('time')) && message.get('edited')) {
+                    // This is an older message which has been corrected already
+                    older_versions[attrs.time] = attrs['message'];
+                    message.save({'older_versions': older_versions});
+                } else {
+                    // This is a correction of an earlier message we already received
+                    older_versions[message.get('time')] = message.get('message');
+                    attrs = Object.assign(attrs, {'older_versions': older_versions});
+                    message.save(attrs);
+                }
+                return message;
             },
 
             async getDuplicateMessage (stanza) {
@@ -619,8 +641,8 @@ converse.plugins.add('converse-chatboxes', {
                 const attrs = this.getOutgoingMessageAttributes(text, spoiler_hint);
                 let message = this.messages.findWhere('correcting')
                 if (message) {
-                    const older_versions = message.get('older_versions') || [];
-                    older_versions.push(message.get('message'));
+                    const older_versions = message.get('older_versions') || {};
+                    older_versions[message.get('time')] = message.get('message');
                     message.save({
                         'correcting': false,
                         'edited': (new Date()).toISOString(),
@@ -772,15 +794,17 @@ converse.plugins.add('converse-chatboxes', {
              *  message stanza, if it was contained, otherwise it's the message stanza itself.
              */
             getMessageAttributesFromStanza (stanza, original_stanza) {
-                const spoiler = sizzle(`spoiler[xmlns="${Strophe.NS.SPOILER}"]`, original_stanza).pop(),
-                      delay = sizzle(`delay[xmlns="${Strophe.NS.DELAY}"]`, original_stanza).pop(),
-                      text = this.getMessageBody(stanza) || undefined,
-                      chat_state = stanza.getElementsByTagName(_converse.COMPOSING).length && _converse.COMPOSING ||
+                const spoiler = sizzle(`spoiler[xmlns="${Strophe.NS.SPOILER}"]`, original_stanza).pop();
+                const delay = sizzle(`delay[xmlns="${Strophe.NS.DELAY}"]`, original_stanza).pop();
+                const text = this.getMessageBody(stanza) || undefined;
+                const chat_state = stanza.getElementsByTagName(_converse.COMPOSING).length && _converse.COMPOSING ||
                             stanza.getElementsByTagName(_converse.PAUSED).length && _converse.PAUSED ||
                             stanza.getElementsByTagName(_converse.INACTIVE).length && _converse.INACTIVE ||
                             stanza.getElementsByTagName(_converse.ACTIVE).length && _converse.ACTIVE ||
                             stanza.getElementsByTagName(_converse.GONE).length && _converse.GONE;
 
+                const replaced_id = this.getReplaceId(stanza)
+                const msgid = replaced_id || stanza.getAttribute('id') || original_stanza.getAttribute('id');
                 const attrs = Object.assign({
                     'chat_state': chat_state,
                     'is_archived': this.isArchived(original_stanza),
@@ -788,7 +812,7 @@ converse.plugins.add('converse-chatboxes', {
                     'is_spoiler': !_.isNil(spoiler),
                     'is_single_emoji': text ? u.isSingleEmoji(text) : false,
                     'message': text,
-                    'msgid': stanza.getAttribute('id'),
+                    'msgid': msgid,
                     'references': this.getReferencesFromStanza(stanza),
                     'subject': _.propertyOf(stanza.querySelector('subject'))('textContent'),
                     'thread': _.propertyOf(stanza.querySelector('thread'))('textContent'),
@@ -796,7 +820,6 @@ converse.plugins.add('converse-chatboxes', {
                     'type': stanza.getAttribute('type')
                 }, this.getStanzaIDs(original_stanza));
 
-
                 if (attrs.type === 'groupchat') {
                     attrs.from = stanza.getAttribute('from');
                     attrs.nick = Strophe.unescapeNode(Strophe.getResourceFromJid(attrs.from));
@@ -818,8 +841,13 @@ converse.plugins.add('converse-chatboxes', {
                 if (spoiler) {
                     attrs.spoiler_hint = spoiler.textContent.length > 0 ? spoiler.textContent : '';
                 }
+                if (replaced_id) {
+                    attrs['edited'] = (new Date()).toISOString();
+                }
                 // We prefer to use one of the XEP-0359 unique and stable stanza IDs as the Model id, to avoid duplicates.
-                attrs['id'] = attrs['origin_id'] || attrs[`stanza_id ${attrs.from}`] || _converse.connection.getUniqueId();
+                attrs['id'] = attrs['origin_id'] ||
+                    attrs[`stanza_id ${attrs.from}`] ||
+                    _converse.connection.getUniqueId();
                 return attrs;
             },
 
@@ -1021,13 +1049,12 @@ converse.plugins.add('converse-chatboxes', {
                         chatbox.updateMessage(message, original_stanza);
                     }
                     if (!message &&
-                            !chatbox.handleMessageCorrection(stanza) &&
                             !chatbox.handleReceipt (stanza, from_jid, is_carbon, is_me, is_mam) &&
                             !chatbox.handleChatMarker(stanza, from_jid, is_carbon, is_roster_contact, is_mam)) {
 
                         const attrs = await chatbox.getMessageAttributesFromStanza(stanza, original_stanza);
                         if (attrs['chat_state'] || !u.isEmptyMessage(attrs)) {
-                            const msg = chatbox.messages.create(attrs);
+                            const msg = chatbox.correctMessage(attrs) || chatbox.messages.create(attrs);
                             chatbox.incrementUnreadMsgCounter(msg);
                         }
                     }

src/headless/converse-core.js

@@ -9,7 +9,7 @@ import Backbone from "backbone";
 import BrowserStorage from "backbone.browserStorage";
 import Promise from "es6-promise/dist/es6-promise.auto";
 import _ from "./lodash.noconflict";
-import advancedFormat from 'dayjs/plugin/advancedFormat'
+import advancedFormat from 'dayjs/plugin/advancedFormat';
 import dayjs from "dayjs";
 import i18n from "./i18n";
 import pluggable from "pluggable.js/src/pluggable";
@@ -19,7 +19,7 @@ import u from "@converse/headless/utils/core";
 
 Backbone = Backbone.noConflict();
 
-dayjs.extend(advancedFormat)
+dayjs.extend(advancedFormat);
 
 // Strophe globals
 const b64_sha1 = SHA1.b64_sha1;

src/headless/converse-muc.js

@@ -1158,19 +1158,19 @@ converse.plugins.add('converse-muc', {
                     this.updateMessage(message, original_stanza);
                 }
                 if (message ||
-                        this.handleMessageCorrection(stanza) ||
                         this.isReceipt(stanza) ||
                         this.isChatMarker(stanza)) {
                     return _converse.api.trigger('message', {'stanza': original_stanza});
                 }
 
-                const attrs = await this.getMessageAttributesFromStanza(stanza, original_stanza);
+                let attrs = await this.getMessageAttributesFromStanza(stanza, original_stanza);
                 if (attrs.nick &&
                         !this.subjectChangeHandled(attrs) &&
                         !this.ignorableCSN(attrs) &&
                         (attrs['chat_state'] || !u.isEmptyMessage(attrs))) {
 
-                    const msg = this.messages.create(this.addOccupantData(attrs));
+                    attrs = this.addOccupantData(attrs);
+                    const msg = this.correctMessage(attrs) || this.messages.create(attrs);
                     this.incrementUnreadMsgCounter(msg);
                     if (forwarded && msg && msg.get('sender')  === 'me') {
                         msg.save({'received': (new Date()).toISOString()});

src/templates/message_versions_modal.html

@@ -7,7 +7,7 @@
             </div>
             <div class="modal-body">
                 <h4>Older versions</h4>
-                {[o.older_versions.forEach(function (text) { ]} <p class="older-msg">{{{text}}}</p> {[ }); ]}
+                {[Object.keys(o.older_versions).forEach(function (k) { ]} <p class="older-msg"><time>{{{o.dayjs(k).format('MMM D, YYYY, HH:mm:ss')}}}</time>: {{{o.older_versions[k]}}}</p> {[ }); ]}
                 <hr/>
                 <h4>Current version</h4>
                 <p>{{{o.message}}}</p>