MDL-57139 messages/notifications: ensure promise best practices
authorDan Poltawski <dan@moodle.com>
Tue, 10 Jan 2017 10:10:56 +0000 (10:10 +0000)
committerDan Poltawski <dan@moodle.com>
Thu, 1 Jun 2017 09:27:58 +0000 (10:27 +0100)
Previously there were a few issues with the code

* We were capturing a promise without then then .then() that came after
  it, so our promise wouldn't always be completely resolved by the time
  then next piece of code was operating on it

* We weren't catching all errors with .catch()

message/amd/src/message_area_messages.js
message/amd/src/message_area_search.js
message/output/popup/amd/src/message_popover_controller.js
message/output/popup/amd/src/notification_area_control_area.js
message/output/popup/amd/src/notification_popover_controller.js

index 1015e13..4c5711b 100644 (file)
@@ -455,9 +455,8 @@ define(['jquery', 'core/ajax', 'core/templates', 'core/notification', 'core/cust
                     }
                 });
             });
-
             if (requests.length > 0) {
-                Ajax.call(requests)[requests.length - 1].then(function() {
+                $.when(Ajax.call(requests)).then(function() {
                     // Store the last message on the page, and the last message being deleted.
                     var updatemessage = null;
                     var messages = this.messageArea.find(SELECTORS.MESSAGE);
@@ -491,7 +490,7 @@ define(['jquery', 'core/ajax', 'core/templates', 'core/notification', 'core/cust
 
                     // Trigger event letting other modules know messages were deleted.
                     this.messageArea.trigger(Events.MESSAGESDELETED, [this._getUserId(), updatemessage]);
-                }.bind(this)Notification.exception);
+                }.bind(this)).catch(Notification.exception);
             } else {
                 // Trigger event letting other modules know messages were deleted.
                 this.messageArea.trigger(Events.MESSAGESDELETED, this._getUserId());
@@ -526,49 +525,47 @@ define(['jquery', 'core/ajax', 'core/templates', 'core/notification', 'core/cust
          * @private
          */
         Messages.prototype._deleteAllMessages = function() {
-            // Create the confirmation modal if we haven't already.
-            if (!this._confirmationModal) {
-                Str.get_strings([
-                    {key: 'confirm'},
-                    {key: 'deleteallconfirm', component: 'message'}
-                ]).done(function(s) {
-                    ModalFactory.create({
-                        title: s[0],
-                        type: ModalFactory.types.CONFIRM,
-                        body: s[1]
-                    }, this.messageArea.find(SELECTORS.DELETEALLMESSAGES))
-                        .done(function(modal) {
-                            this._confirmationModal = modal;
-
-                            // Only delete the conversation if the user agreed in the confirmation modal.
-                            modal.getRoot().on(ModalEvents.yes, function() {
-                                var otherUserId = this._getUserId();
-                                var request = {
-                                    methodname: 'core_message_delete_conversation',
-                                    args: {
-                                        userid: this.messageArea.getCurrentUserId(),
-                                        otheruserid: otherUserId
-                                    }
-                                };
-
-                                // Delete the conversation.
-                                Ajax.call([request])[0].then(function() {
-                                    // Clear the message area.
-                                    this.messageArea.find(SELECTORS.MESSAGESAREA).empty();
-                                    // Let the app know a conversation was deleted.
-                                    this.messageArea.trigger(Events.CONVERSATIONDELETED, otherUserId);
-                                    this._hideDeleteAction();
-                                }.bind(this), Notification.exception);
-                            }.bind(this));
-
-                            // Display the confirmation.
-                            modal.show();
-                        }.bind(this));
-                }.bind(this));
-            } else {
-                // Otherwise just show the existing modal.
+            if (this._confirmationModal) {
+                // Just show the existing modal.
                 this._confirmationModal.show();
+                return;
             }
+
+            Str.get_strings([
+                {key: 'confirm'},
+                {key: 'deleteallconfirm', component: 'message'}
+            ]).then(function(s) {
+                return ModalFactory.create({
+                    title: s[0],
+                    type: ModalFactory.types.CONFIRM,
+                    body: s[1]
+                }, this.messageArea.find(SELECTORS.DELETEALLMESSAGES));
+            }.bind(this)).then(function(modal) {
+                this._confirmationModal = modal;
+                // Only delete the conversation if the user agreed in the confirmation modal.
+                modal.getRoot().on(ModalEvents.yes, function() {
+                    var otherUserId = this._getUserId();
+                    var request = {
+                        methodname: 'core_message_delete_conversation',
+                        args: {
+                            userid: this.messageArea.getCurrentUserId(),
+                            otheruserid: otherUserId
+                        }
+                    };
+
+                    // Delete the conversation.
+                    Ajax.call([request])[0].then(function() {
+                        // Clear the message area.
+                        this.messageArea.find(SELECTORS.MESSAGESAREA).empty();
+                        // Let the app know a conversation was deleted.
+                        this.messageArea.trigger(Events.CONVERSATIONDELETED, otherUserId);
+                        this._hideDeleteAction();
+                    }.bind(this)).catch(Notification.exception);
+                }.bind(this));
+
+                // Display the confirmation.
+                modal.show();
+            }.bind(this)).catch(Notification.exception);
         };
 
         /**
index 584f4f8..46960d7 100644 (file)
@@ -385,7 +385,7 @@ define(['jquery', 'core/ajax', 'core/templates', 'core/notification', 'core/str'
         this.messageArea.find(SELECTORS.SEARCHFILTER).html(text);
         Str.get_string('removecoursefilter', 'message', text).then(function(languagestring) {
             this.messageArea.find(SELECTORS.SEARCHFILTERAREA).attr('aria-label', languagestring);
-        }.bind(this));
+        }.bind(this)).catch(Notification.exception);
     };
 
     /**
index 8787b28..5699698 100644 (file)
@@ -151,7 +151,7 @@ define(['jquery', 'core/ajax', 'core/templates', 'core/str',
             this.unreadCount = count;
             this.renderUnreadCount();
             this.updateButtonAriaLabel();
-        }.bind(this));
+        }.bind(this)).catch(Notification.exception);
     };
 
     /**
@@ -165,38 +165,27 @@ define(['jquery', 'core/ajax', 'core/templates', 'core/str',
      */
     MessagePopoverController.prototype.renderMessages = function(messages, container) {
         var promises = [];
-        var allhtml = [];
-        var alljs = [];
-
-        if (messages.length) {
-            $.each(messages, function(index, message) {
-                message.contexturl = URL.relativeUrl('/message/index.php', {
-                    user: this.userId,
-                    id: message.userid,
-                });
-
-                message.profileurl = URL.relativeUrl('/user/profile.php', {
-                    id: message.userid,
-                });
-
-                var promise = Templates.render('message_popup/message_content_item', message);
-                promises.push(promise);
-
-                promise.then(function(html, js) {
-                    allhtml[index] = html;
-                    alljs[index] = js;
-                });
-            }.bind(this));
-        }
 
-        return $.when.apply($.when, promises).then(function() {
-            if (messages.length) {
-                $.each(messages, function(index) {
-                    container.append(allhtml[index]);
-                    Templates.runTemplateJS(alljs[index]);
-                });
-            }
-        });
+        $.each(messages, function(index, message) {
+            message.contexturl = URL.relativeUrl('/message/index.php', {
+                user: this.userId,
+                id: message.userid,
+            });
+
+            message.profileurl = URL.relativeUrl('/user/profile.php', {
+                id: message.userid,
+            });
+
+            var promise = Templates.render('message_popup/message_content_item', message)
+            .then(function(html, js) {
+                container.append(html);
+                Templates.runTemplateJS(js);
+                return;
+            });
+            promises.push(promise);
+        }.bind(this));
+
+        return $.when.apply($, promises);
     };
 
     /**
index ee19a0e..5feea45 100644 (file)
@@ -307,39 +307,27 @@ define(['jquery', 'core/templates', 'core/notification', 'core/custom_interactio
      */
     ControlArea.prototype.renderNotifications = function(notifications) {
         var promises = [];
-        var allhtml = [];
-        var alljs = [];
         var container = this.getContent();
 
-        if (notifications.length) {
-            $.each(notifications, function(index, notification) {
-                // Need to remove the contexturl so the item isn't rendered
-                // as a link.
-                var contextUrl = notification.contexturl;
-                delete notification.contexturl;
-
-                var promise = Templates.render(TEMPLATES.NOTIFICATION, notification);
-
-                promises.push(promise);
-                promise.then(function(html, js) {
-                    allhtml[index] = html;
-                    alljs[index] = js;
-                    // Restore it for the cache.
-                    notification.contexturl = contextUrl;
-                    this.setCacheNotification(notification);
-                }.bind(this))
-                .fail(DebugNotification.exception);
+        $.each(notifications, function(index, notification) {
+            // Need to remove the contexturl so the item isn't rendered
+            // as a link.
+            var contextUrl = notification.contexturl;
+            delete notification.contexturl;
+
+            var promise = Templates.render(TEMPLATES.NOTIFICATION, notification)
+            .then(function(html, js) {
+                container.append(html);
+                Templates.runTemplateJS(js);
+                // Restore it for the cache.
+                notification.contexturl = contextUrl;
+                this.setCacheNotification(notification);
+                return;
             }.bind(this));
-        }
+            promises.push(promise);
+        }.bind(this));
 
-        return $.when.apply($.when, promises).then(function() {
-            if (notifications.length) {
-                $.each(notifications, function(index) {
-                    container.append(allhtml[index]);
-                    Templates.runTemplateJS(alljs[index]);
-                });
-            }
-        });
+        return $.when.apply($, promises);
     };
 
     /**
index 4036b70..2d5a2c4 100644 (file)
@@ -200,7 +200,7 @@ define(['jquery', 'core/ajax', 'core/templates', 'core/str', 'core/url',
             this.unreadCount = count;
             this.renderUnreadCount();
             this.updateButtonAriaLabel();
-        }.bind(this));
+        }.bind(this)).catch(DebugNotification.exception);
     };
 
     /**
@@ -226,39 +226,27 @@ define(['jquery', 'core/ajax', 'core/templates', 'core/str', 'core/url',
      */
     NotificationPopoverController.prototype.renderNotifications = function(notifications, container) {
         var promises = [];
-        var allhtml = [];
-        var alljs = [];
-
-        if (notifications.length) {
-            $.each(notifications, function(index, notification) {
-                // Determine what the offset was when loading this notification.
-                var offset = this.getOffset() - this.limit;
-                // Update the view more url to contain the offset to allow the notifications
-                // page to load to the correct position in the list of notifications.
-                notification.viewmoreurl = URL.relativeUrl('/message/output/popup/notifications.php', {
-                    notificationid: notification.id,
-                    offset: offset,
-                });
-
-                var promise = Templates.render('message_popup/notification_content_item', notification);
-                promises.push(promise);
-
-                promise.then(function(html, js) {
-                    allhtml[index] = html;
-                    alljs[index] = js;
-                })
-                .fail(DebugNotification.exception);
-            }.bind(this));
-        }
 
-        return $.when.apply($.when, promises).then(function() {
-            if (notifications.length) {
-                $.each(notifications, function(index) {
-                    container.append(allhtml[index]);
-                    Templates.runTemplateJS(alljs[index]);
-                });
-            }
-        });
+        $.each(notifications, function(index, notification) {
+            // Determine what the offset was when loading this notification.
+            var offset = this.getOffset() - this.limit;
+            // Update the view more url to contain the offset to allow the notifications
+            // page to load to the correct position in the list of notifications.
+            notification.viewmoreurl = URL.relativeUrl('/message/output/popup/notifications.php', {
+                notificationid: notification.id,
+                offset: offset,
+            });
+
+            var promise = Templates.render('message_popup/notification_content_item', notification)
+            .then(function(html, js) {
+                container.append(html);
+                Templates.runTemplateJS(js);
+                return;
+            });
+            promises.push(promise);
+        }.bind(this));
+
+        return $.when.apply($, promises);
     };
 
     /**