From 877d997fe13c5885e4414e79cdeaaaa363d43e2b Mon Sep 17 00:00:00 2001 From: Dan Poltawski Date: Tue, 10 Jan 2017 10:10:56 +0000 Subject: [PATCH] MDL-57139 messages/notifications: ensure promise best practices 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 | 85 +++++++++---------- message/amd/src/message_area_search.js | 2 +- .../amd/src/message_popover_controller.js | 47 ++++------ .../amd/src/notification_area_control_area.js | 44 ++++------ .../src/notification_popover_controller.js | 50 +++++------ 5 files changed, 95 insertions(+), 133 deletions(-) diff --git a/message/amd/src/message_area_messages.js b/message/amd/src/message_area_messages.js index 1015e137404..4c5711bafc8 100644 --- a/message/amd/src/message_area_messages.js +++ b/message/amd/src/message_area_messages.js @@ -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); }; /** diff --git a/message/amd/src/message_area_search.js b/message/amd/src/message_area_search.js index 584f4f8c488..46960d7a7d8 100644 --- a/message/amd/src/message_area_search.js +++ b/message/amd/src/message_area_search.js @@ -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); }; /** diff --git a/message/output/popup/amd/src/message_popover_controller.js b/message/output/popup/amd/src/message_popover_controller.js index 8787b28e40b..56996981222 100644 --- a/message/output/popup/amd/src/message_popover_controller.js +++ b/message/output/popup/amd/src/message_popover_controller.js @@ -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, - }); + $.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, - }); + message.profileurl = URL.relativeUrl('/user/profile.php', { + id: message.userid, + }); - var promise = Templates.render('message_popup/message_content_item', message); - promises.push(promise); + 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)); - 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]); - }); - } - }); + return $.when.apply($, promises); }; /** diff --git a/message/output/popup/amd/src/notification_area_control_area.js b/message/output/popup/amd/src/notification_area_control_area.js index ee19a0e16aa..5feea45cf11 100644 --- a/message/output/popup/amd/src/notification_area_control_area.js +++ b/message/output/popup/amd/src/notification_area_control_area.js @@ -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; + $.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); + 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); }; /** diff --git a/message/output/popup/amd/src/notification_popover_controller.js b/message/output/popup/amd/src/notification_popover_controller.js index 4036b70eda0..2d5a2c4d369 100644 --- a/message/output/popup/amd/src/notification_popover_controller.js +++ b/message/output/popup/amd/src/notification_popover_controller.js @@ -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, - }); + $.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); + 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)); - 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]); - }); - } - }); + return $.when.apply($, promises); }; /**