Better error handler for retractions

Continue the work of saving errors onto the original stanza.

sass/_messages.scss

@@ -222,7 +222,6 @@
 
             .chat-msg__error {
                 color: var(--error-color);
-                font-weight: bold;
             }
 
             .chat-msg__media {

spec/retractions.js

@@ -2,6 +2,7 @@
 
 const { Strophe, $iq } = converse.env;
 const u = converse.env.utils;
+const original_timeout = jasmine.DEFAULT_TIMEOUT_INTERVAL;
 
 
 async function sendAndThenRetractMessage (_converse, view) {
@@ -34,6 +35,9 @@ async function sendAndThenRetractMessage (_converse, view) {
 
 fdescribe("Message Retractions", function () {
 
+    beforeEach(() => (jasmine.DEFAULT_TIMEOUT_INTERVAL = 7000));
+    afterEach(() => (jasmine.DEFAULT_TIMEOUT_INTERVAL = original_timeout));
+
     describe("A groupchat message retraction", function () {
 
         it("is not applied if it's not from the right author",
@@ -180,6 +184,7 @@ fdescribe("Message Retractions", function () {
             _converse.connection._dataRecv(mock.createRequest(received_stanza));
             await u.waitUntil(() => view.model.handleModeration.calls.count() === 2);
 
+            await u.waitUntil(() => view.el.querySelectorAll('.chat-msg').length);
             expect(view.el.querySelectorAll('.chat-msg').length).toBe(1);
             expect(view.model.messages.length).toBe(1);
 
@@ -701,7 +706,7 @@ fdescribe("Message Retractions", function () {
             expect(view.model.messages.at(0).get('editable')).toBeTruthy();
 
             const errmsg = view.el.querySelector('.chat-msg__error');
-            expect(errmsg.textContent.trim()).toBe("Your message was not delivered because you weren't allowed to send it.");
+            expect(errmsg.textContent.trim()).toBe("You're not allowed to retract your message.");
             done();
         }));
 
@@ -721,25 +726,23 @@ fdescribe("Message Retractions", function () {
             occupant.save('role', 'member');
             await u.waitUntil(() => view.el.querySelector('.chat-content__notifications').textContent.includes("romeo is no longer a moderator"))
             await sendAndThenRetractMessage(_converse, view);
-            await u.waitUntil(() => view.el.querySelectorAll('.chat-msg--retracted').length === 1);
-
             expect(view.model.messages.length).toBe(1);
             expect(view.model.messages.last().get('retracted')).toBeTruthy();
+            await u.waitUntil(() => view.el.querySelectorAll('.chat-msg--retracted').length === 1);
             const el = view.el.querySelector('.chat-msg--retracted .chat-msg__message div');
             expect(el.textContent.trim()).toBe('romeo has removed this message');
 
             await u.waitUntil(() => view.el.querySelectorAll('.chat-msg').length === 1);
 
             await u.waitUntil(() => view.el.querySelectorAll('.chat-msg--retracted').length === 0);
-            expect(view.model.messages.length).toBe(3);
+            expect(view.model.messages.length).toBe(1);
             expect(view.model.messages.at(0).get('retracted')).toBeFalsy();
             expect(view.model.messages.at(0).get('is_ephemeral')).toBeFalsy();
             expect(view.model.messages.at(0).get('editable')).toBeTruthy();
 
-            const error_messages = view.el.querySelectorAll('.chat-error');
-            expect(error_messages.length).toBe(2);
-            expect(error_messages[0].textContent.trim()).toBe("Sorry, something went wrong while trying to retract your message.");
-            expect(error_messages[1].textContent.trim()).toBe("Timeout Error: No response from server");
+            const error_messages = view.el.querySelectorAll('.chat-msg__error');
+            expect(error_messages.length).toBe(1);
+            expect(error_messages[0].textContent.trim()).toBe('A timeout happened while while trying to retract your message.');
             done();
         }));
 

src/converse-muc-views.js

@@ -31,7 +31,7 @@ import { View } from 'skeletor.js/src/view.js';
 import { __ } from '@converse/headless/i18n';
 import { api, converse } from "@converse/headless/converse-core";
 import { debounce, head, isString, isUndefined } from "lodash";
-import { html, render } from "lit-html";
+import { render } from "lit-html";
 
 const { Strophe, sizzle, $iq, $pres } = converse.env;
 const u = converse.env.utils;
@@ -790,7 +790,7 @@ converse.plugins.add('converse-muc-views', {
                     if (_converse.show_retraction_warning) {
                         messages[1] = retraction_warning;
                     }
-                    !!(await api.confirm(__('Confirm'), messages)) && this.retractOwnMessage(message);
+                    !!(await api.confirm(__('Confirm'), messages)) && this.model.retractOwnMessage(message);
                 } else if (await message.mayBeModerated()) {
                     if (message.get('sender') === 'me') {
                         let messages = [__('Are you sure you want to retract this message?')];
@@ -819,22 +819,6 @@ converse.plugins.add('converse-muc-views', {
                 }
             },
 
-            /**
-             * Retract one of your messages in this groupchat.
-             * @private
-             * @method _converse.ChatRoomView#retractOwnMessage
-             * @param { _converse.Message } message - The message which we're retracting.
-             */
-            retractOwnMessage(message) {
-                this.model.retractOwnMessage(message)
-                    .catch(e => {
-                        const message = __('Sorry, something went wrong while trying to retract your message.');
-                        this.model.createMessage({message, 'type': 'error'});
-                        !u.isErrorStanza(e) && this.model.createMessage({'message': e.message, 'type': 'error'});
-                        log.error(e);
-                    });
-            },
-
             /**
              * Retract someone else's message in this groupchat.
              * @private

src/headless/converse-chat.js

@@ -396,8 +396,33 @@ converse.plugins.add('converse-chat', {
             },
 
             async handleErrormessageStanza (stanza) {
-                if (await this.shouldShowErrorMessage(stanza)) {
-                    this.createMessage(await st.parseMessage(stanza, _converse));
+                const attrs = await st.parseMessage(stanza, _converse);
+                if (!await this.shouldShowErrorMessage(attrs)) {
+                    return;
+                }
+                const message = this.getMessageReferencedByError(attrs);
+                if (message) {
+                    const new_attrs = {'error': attrs.error };
+                    if (attrs.msgid === message.get('retraction_id')) {
+                        // The error message refers to a retraction
+                        new_attrs.retraction_id = undefined;
+                        if (!attrs.error) {
+                            if (attrs.error_condition === 'forbidden') {
+                                new_attrs.error = __("You're not allowed to retract your message.");
+                            } else {
+                                new_attrs.error = __('Sorry, an error occurred while trying to retract your message.');
+                            }
+                        }
+                    } else if (!attrs.error) {
+                        if (attrs.error_condition === 'forbidden') {
+                            new_attrs.error = __("You're not allowed to send a message.");
+                        } else {
+                            new_attrs.error = __('Sorry, an error occurred while trying to send your message.');
+                        }
+                    }
+                    message.save(new_attrs);
+                } else {
+                    this.createMessage(attrs);
                 }
             },
 
@@ -579,27 +604,29 @@ converse.plugins.add('converse-chat', {
                 return this;
             },
 
+            /**
+             * Given an error `<message>` stanza's attributes, find the saved message model which is
+             * referenced by that error.
+             * @param { Object } attrs
+             */
+            getMessageReferencedByError (attrs) {
+                const id = attrs.msgid;
+                return id && this.messages.models.find(m => [m.get('msgid'), m.get('retraction_id')].includes(id));
+            },
+
             /**
              * @private
              * @method _converse.ChatBox#shouldShowErrorMessage
              * @returns {boolean}
              */
-            shouldShowErrorMessage (stanza) {
-                const id = stanza.getAttribute('id');
-                if (id) {
-                    const msgs = this.messages.where({'msgid': id});
-                    const referenced_msgs = msgs.filter(m => m.get('type') !== 'error');
-                    if (!referenced_msgs.length && stanza.querySelector('body') === null) {
-                        // If the error refers to a message not included in our store,
-                        // and it doesn't have a <body> tag, we assume that this was a
-                        // CSI message (which we don't store).
-                        // See https://github.com/conversejs/converse.js/issues/1317
-                        return;
-                    }
-                    const dupes = msgs.filter(m => m.get('type') === 'error');
-                    if (dupes.length) {
-                        return;
-                    }
+            shouldShowErrorMessage (attrs) {
+                const msg = this.getMessageReferencedByError(attrs);
+                if (!msg && attrs.body === null) {
+                    // If the error refers to a message not included in our store,
+                    // and it doesn't have a <body> tag, we assume that this was a
+                    // CSI message (which we don't store).
+                    // See https://github.com/conversejs/converse.js/issues/1317
+                    return;
                 }
                 // Gets overridden in ChatRoom
                 return true;
@@ -765,6 +792,7 @@ converse.plugins.add('converse-chat', {
                 message.save({
                     'retracted': (new Date()).toISOString(),
                     'retracted_id': message.get('origin_id'),
+                    'retraction_id': message.get('id'),
                     'is_ephemeral': true,
                     'editable': false
                 });

src/headless/converse-muc.js

@@ -619,14 +619,37 @@ converse.plugins.add('converse-muc', {
             },
 
             async handleErrormessageStanza (stanza) {
-                if (await this.shouldShowErrorMessage(stanza)) {
-                    const attrs = await st.parseMUCMessage(stanza, this, _converse);
-                    const message = attrs.msgid && this.messages.findWhere({'msgid': attrs.msgid});
-                    if (message) {
-                        message.save({'error': attrs.error});
-                    } else {
-                        this.createMessage(attrs);
+                const attrs = await st.parseMUCMessage(stanza, this, _converse);
+                if (!await this.shouldShowErrorMessage(attrs)) {
+                    return;
+                }
+                const message = this.getMessageReferencedByError(attrs);
+                if (message) {
+                    const new_attrs = {'error': attrs.error };
+                    if (attrs.msgid === message.get('retraction_id')) {
+                        // The error message refers to a retraction
+                        new_attrs.retraction_id = undefined;
+                        if (!attrs.error) {
+                            if (attrs.error_condition === 'forbidden') {
+                                new_attrs.error = __("You're not allowed to retract your message.");
+                            } else if (attrs.error_condition === 'not-acceptable') {
+                                new_attrs.error = __("Your retraction was not delivered because you're not present in the groupchat.");
+                            } else {
+                                new_attrs.error = __('Sorry, an error occurred while trying to retract your message.');
+                            }
+                        }
+                    } else if (!attrs.error) {
+                        if (attrs.error_condition === 'forbidden') {
+                            new_attrs.error = __("You're not allowed to send a message.");
+                        } else if (attrs.error_condition === 'not-acceptable') {
+                            new_attrs.error = __("Your message was not delivered because you're not present in the groupchat.");
+                        } else {
+                            new_attrs.error = __('Sorry, an error occurred while trying to send your message.');
+                        }
                     }
+                    message.save(new_attrs);
+                } else {
+                    this.createMessage(attrs);
                 }
             },
 
@@ -749,20 +772,38 @@ converse.plugins.add('converse-muc', {
              * @param { _converse.Message } message - The message which we're retracting.
              */
             async retractOwnMessage(message) {
+                const origin_id = message.get('origin_id');
+                if (!origin_id) {
+                    throw new Error("Can't retract message without a XEP-0359 Origin ID");
+                }
                 const editable = message.get('editable');
+                const stanza = $msg({
+                        'id': u.getUniqueId(),
+                        'to': this.get('jid'),
+                        'type': "groupchat"
+                    })
+                    .c('store', {xmlns: Strophe.NS.HINTS}).up()
+                    .c("apply-to", {
+                        'id': origin_id,
+                        'xmlns': Strophe.NS.FASTEN
+                    }).c('retract', {xmlns: Strophe.NS.RETRACT});
+
                 // Optimistic save
-                message.save({
+                message.set({
                     'retracted': (new Date()).toISOString(),
-                    'retracted_id': message.get('origin_id'),
+                    'retracted_id': origin_id,
+                    'retraction_id': stanza.nodeTree.getAttribute('id'),
                     'editable': false
                 });
                 try {
-                    await this.sendRetractionMessage(message)
+                    await this.sendTimedMessage(stanza);
                 } catch (e) {
                     message.save({
                         editable,
+                        'error_type': 'timeout',
+                        'error': __('A timeout happened while while trying to retract your message.'),
                         'retracted': undefined,
-                        'retracted_id': undefined,
+                        'retracted_id': undefined
                     });
                     throw e;
                 }
@@ -799,30 +840,6 @@ converse.plugins.add('converse-muc', {
                 return result;
             },
 
-            /**
-             * Sends a message stanza to retract a message in this groupchat.
-             * @private
-             * @method _converse.ChatRoom#sendRetractionMessage
-             * @param { _converse.Message } message - The message which we're retracting.
-             */
-            sendRetractionMessage (message) {
-                const origin_id = message.get('origin_id');
-                if (!origin_id) {
-                    throw new Error("Can't retract message without a XEP-0359 Origin ID");
-                }
-                const msg = $msg({
-                        'id': u.getUniqueId(),
-                        'to': this.get('jid'),
-                        'type': "groupchat"
-                    })
-                    .c('store', {xmlns: Strophe.NS.HINTS}).up()
-                    .c("apply-to", {
-                        'id': origin_id,
-                        'xmlns': Strophe.NS.FASTEN
-                    }).c('retract', {xmlns: Strophe.NS.RETRACT});
-                return this.sendTimedMessage(msg);
-            },
-
             /**
              * Sends an IQ stanza to the XMPP server to retract a message in this groupchat.
              * @private
@@ -1815,13 +1832,11 @@ converse.plugins.add('converse-muc', {
              * @method _converse.ChatRoom#shouldShowErrorMessage
              * @returns {Promise<boolean>}
              */
-            async shouldShowErrorMessage (stanza) {
-                if (sizzle(`not-acceptable[xmlns="${Strophe.NS.STANZAS}"]`, stanza).length) {
-                    if (await this.rejoinIfNecessary()) {
-                        return false;
-                    }
+            async shouldShowErrorMessage (attrs) {
+                if (attrs['error_condition'] === 'not-acceptable' && await this.rejoinIfNecessary()) {
+                    return false;
                 }
-                return _converse.ChatBox.prototype.shouldShowErrorMessage.call(this, stanza);
+                return _converse.ChatBox.prototype.shouldShowErrorMessage.call(this, attrs);
             },
 
             /**

src/headless/utils/stanza.js

@@ -3,7 +3,6 @@ import dayjs from 'dayjs';
 import sizzle from 'sizzle';
 import u from '@converse/headless/utils/core';
 import log from "../log";
-import { __ } from '@converse/headless/i18n';
 import { api } from "@converse/headless/converse-core";
 
 const Strophe = strophe.default.Strophe;
@@ -243,20 +242,6 @@ function getReferences (stanza) {
     });
 }
 
-/**
- * Returns the human readable error message contained in an message stanza of type 'error'.
- * @private
- * @param { XMLElement } stanza - The message stanza
- */
-function getErrorMessage (stanza) {
-    if (stanza.getAttribute('type') === 'error') {
-        const error = stanza.querySelector('error');
-        return error.querySelector('text')?.textContent ||
-            __('Sorry, an error occurred:') + ' ' + error.innerHTML;
-    }
-}
-
-
 function rejectMessage (stanza, text) {
     // Reject an incoming message by replying with an error message of type "cancel".
     api.send(
@@ -278,20 +263,18 @@ function rejectMessage (stanza, text) {
  * @private
  * @param { XMLElement } stanza - The message stanza
  */
-function getMUCErrorMessage (stanza) {
+function getErrorAttributes (stanza) {
     if (stanza.getAttribute('type') === 'error') {
-        const forbidden = sizzle(`error forbidden[xmlns="${Strophe.NS.STANZAS}"]`, stanza).pop();
-        const text = sizzle(`error text[xmlns="${Strophe.NS.STANZAS}"]`, stanza).pop();
-        if (forbidden) {
-            const msg = __("Your message was not delivered because you weren't allowed to send it.");
-            const server_msg = text ? __('The message from the server is: "%1$s"', text.textContent) : '';
-            return server_msg ? `${msg} ${server_msg}` : msg;
-        } else if (sizzle(`not-acceptable[xmlns="${Strophe.NS.STANZAS}"]`, stanza).length) {
-            return __("Your message was not delivered because you're not present in the groupchat.");
-        } else {
-            return text?.textContent;
+        const error = stanza.querySelector('error');
+        const text = sizzle(`text[xmlns="${Strophe.NS.STANZAS}"]`, error).pop();
+        return {
+            'is_error': true,
+            'error_text': text?.textContent,
+            'error_type': error.getAttribute('type'),
+            'error_condition': error.firstElementChild.nodeName
         }
     }
+    return {};
 }
 
 
@@ -458,6 +441,7 @@ const st = {
          * @property { Boolean } is_carbon - Is this message a XEP-0280 Carbon?
          * @property { Boolean } is_delayed - Was delivery of this message was delayed as per XEP-0203?
          * @property { Boolean } is_encrypted -  Is this message XEP-0384  encrypted?
+         * @property { Boolean } is_error - Whether an error was received for this message
          * @property { Boolean } is_headline - Is this a "headline" message?
          * @property { Boolean } is_markable - Can this message be marked with a XEP-0333 chat marker?
          * @property { Boolean } is_marker - Is this message a XEP-0333 Chat Marker?
@@ -470,7 +454,9 @@ const st = {
          * @property { String } chat_state - The XEP-0085 chat state notification contained in this message
          * @property { String } contact_jid - The JID of the other person or entity
          * @property { String } edit - An ISO8601 string recording the time that the message was edited per XEP-0308
-         * @property { String } error - The error message, in case it's an error stanza
+         * @property { String } error_condition - The defined error condition
+         * @property { String } error_text - The error text received from the server
+         * @property { String } error_type - The type of error received from the server
          * @property { String } from - The sender JID
          * @property { String } fullname - The full name of the sender
          * @property { String } marker - The XEP-0333 Chat Marker value
@@ -503,7 +489,6 @@ const st = {
                 is_server_message,
                 'body': stanza.querySelector('body')?.textContent?.trim(),
                 'chat_state': getChatState(stanza),
-                'error': getErrorMessage(stanza),
                 'from': Strophe.getBareJidFromJid(stanza.getAttribute('from')),
                 'is_archived': st.isArchived(original_stanza),
                 'is_carbon': isCarbon(original_stanza),
@@ -523,6 +508,7 @@ const st = {
                 'to': stanza.getAttribute('to'),
                 'type': stanza.getAttribute('type')
             },
+            getErrorAttributes(stanza),
             getOutOfBandAttributes(stanza),
             getSpoilerAttributes(stanza),
             getCorrectionAttributes(stanza, original_stanza),
@@ -589,6 +575,7 @@ const st = {
          * @property { Boolean } is_carbon - Is this message a XEP-0280 Carbon?
          * @property { Boolean } is_delayed - Was delivery of this message was delayed as per XEP-0203?
          * @property { Boolean } is_encrypted -  Is this message XEP-0384  encrypted?
+         * @property { Boolean } is_error - Whether an error was received for this message
          * @property { Boolean } is_headline - Is this a "headline" message?
          * @property { Boolean } is_markable - Can this message be marked with a XEP-0333 chat marker?
          * @property { Boolean } is_marker - Is this message a XEP-0333 Chat Marker?
@@ -600,7 +587,9 @@ const st = {
          * @property { String } body - The contents of the <body> tag of the message stanza
          * @property { String } chat_state - The XEP-0085 chat state notification contained in this message
          * @property { String } edit - An ISO8601 string recording the time that the message was edited per XEP-0308
-         * @property { String } error - The error message, in case it's an error stanza
+         * @property { String } error_condition - The defined error condition
+         * @property { String } error_text - The error text received from the server
+         * @property { String } error_type - The type of error received from the server
          * @property { String } from - The sender JID
          * @property { String } from_muc - The JID of the MUC from which this message was sent
          * @property { String } fullname - The full name of the sender
@@ -632,7 +621,6 @@ const st = {
                 from,
                 'body': stanza.querySelector('body')?.textContent?.trim(),
                 'chat_state': getChatState(stanza),
-                'error': getMUCErrorMessage(stanza),
                 'from_muc': Strophe.getBareJidFromJid(from),
                 'is_archived': st.isArchived(original_stanza),
                 'is_carbon': isCarbon(original_stanza),
@@ -652,6 +640,7 @@ const st = {
                 'to': stanza.getAttribute('to'),
                 'type': stanza.getAttribute('type'),
             },
+            getErrorAttributes(stanza),
             getOutOfBandAttributes(stanza),
             getSpoilerAttributes(stanza),
             getCorrectionAttributes(stanza, original_stanza),