MDL-63303 message: fix bugs in message drawer part 3

* Fix bug for newly created conversations being added to overview page
* Fixed bug with use enter to send preference not updating correctly
* Fix unit tests
* Fix race condition loading new messages while conversation resetting
* Fix ordering and rendering of messages with same timestamp
* Fix missing privacy info in get_conversation web service
* Fix linting errors
* Improve bulk message rendering
This commit is contained in:
Ryan Wyllie
2018-11-15 14:43:21 +08:00
parent a111ab44af
commit 663ccd58ed
18 changed files with 102 additions and 61 deletions
+1 -1
View File
@@ -1 +1 @@
define(["jquery","core/custom_interaction_events","core/pubsub","core_message/message_drawer_view_contact","core_message/message_drawer_view_contacts","core_message/message_drawer_view_conversation","core_message/message_drawer_view_group_info","core_message/message_drawer_view_overview","core_message/message_drawer_view_search","core_message/message_drawer_view_settings","core_message/message_drawer_router","core_message/message_drawer_routes","core_message/message_drawer_events"],function(a,b,c,d,e,f,g,h,i,j,k,l,m){var n={VIEW_CONTACT:'[data-region="view-contact"]',VIEW_CONTACTS:'[data-region="view-contacts"]',VIEW_CONVERSATION:'[data-region="view-conversation"]',VIEW_GROUP_INFO:'[data-region="view-group-info"]',VIEW_OVERVIEW:'[data-region="view-overview"]',VIEW_SEARCH:'[data-region="view-search"]',VIEW_SETTINGS:'[data-region="view-settings"]',ROUTES:"[data-route]",ROUTES_BACK:"[data-route-back]",HEADER_CONTAINER:'[data-region="header-container"]',BODY_CONTAINER:'[data-region="body-container"]',FOOTER_CONTAINER:'[data-region="footer-container"]'},o=function(a,b){var c=a.children(),d=c.filter(n.HEADER_CONTAINER).find(b),e=c.filter(n.BODY_CONTAINER).find(b),f=c.filter(n.FOOTER_CONTAINER).find(b),g=[d,e,f].filter(function(a){return a.length});return g},p=[[l.VIEW_CONTACT,n.VIEW_CONTACT,d.show,d.description],[l.VIEW_CONTACTS,n.VIEW_CONTACTS,e.show,e.description],[l.VIEW_CONVERSATION,n.VIEW_CONVERSATION,f.show,f.description],[l.VIEW_GROUP_INFO,n.VIEW_GROUP_INFO,g.show,g.description],[l.VIEW_OVERVIEW,n.VIEW_OVERVIEW,h.show,h.description],[l.VIEW_SEARCH,n.VIEW_SEARCH,i.show,i.description],[l.VIEW_SETTINGS,n.VIEW_SETTINGS,j.show,j.description]],q=function(a){p.forEach(function(b){k.add(b[0],o(a,b[1]),b[2],b[3])})},r=function(a){a.attr("data-shown")||(k.go(l.VIEW_OVERVIEW),a.attr("data-shown",!0)),a.removeClass("hidden"),a.attr("aria-expanded",!0),a.attr("aria-hidden",!1)},s=function(a){a.addClass("hidden"),a.attr("aria-expanded",!1),a.attr("aria-hidden",!0)},t=function(a){return!a.hasClass("hidden")},u=function(d){b.define(d,[b.events.activate]);var e=/^data-route-param-?(\d*)$/;d.on(b.events.activate,n.ROUTES,function(b,c){for(var d=a(b.target).closest(n.ROUTES),f=d.attr("data-route"),g=[],h=0;h<d[0].attributes.length;h++)g.push(d[0].attributes[h]);var i=g.filter(function(a){var b=a.nodeName,c=e.test(b);return c});i.sort(function(a,b){var c=e.exec(a.nodeName),d=e.exec(b.nodeName),f=c.length>1?c[1]:0,g=d.length>1?d[1]:0;return f<g?-1:g<f?1:0});var j=i.map(function(a){return a.nodeValue}),l=[f].concat(j);k.go.apply(null,l),c.originalEvent.preventDefault()}),d.on(b.events.activate,n.ROUTES_BACK,function(a,b){k.back(),b.originalEvent.preventDefault()}),c.subscribe(m.SHOW,function(){r(d)}),c.subscribe(m.HIDE,function(){s(d)}),c.subscribe(m.TOGGLE_VISIBILITY,function(){t(d)?s(d):r(d)}),c.subscribe(m.SHOW_CONVERSATION,function(a){r(d),k.go(l.VIEW_CONVERSATION,a)}),c.subscribe(m.CREATE_CONVERSATION_WITH_USER,function(a){r(d),k.go(l.VIEW_CONVERSATION,null,"create",a)}),c.subscribe(m.SHOW_SETTINGS,function(){r(d),k.go(l.VIEW_SETTINGS)})},v=function(b){b=a(b),q(b),u(b)};return{init:v}});
define(["jquery","core/custom_interaction_events","core/pubsub","core_message/message_drawer_view_contact","core_message/message_drawer_view_contacts","core_message/message_drawer_view_conversation","core_message/message_drawer_view_group_info","core_message/message_drawer_view_overview","core_message/message_drawer_view_search","core_message/message_drawer_view_settings","core_message/message_drawer_router","core_message/message_drawer_routes","core_message/message_drawer_events"],function(a,b,c,d,e,f,g,h,i,j,k,l,m){var n={VIEW_CONTACT:'[data-region="view-contact"]',VIEW_CONTACTS:'[data-region="view-contacts"]',VIEW_CONVERSATION:'[data-region="view-conversation"]',VIEW_GROUP_INFO:'[data-region="view-group-info"]',VIEW_OVERVIEW:'[data-region="view-overview"]',VIEW_SEARCH:'[data-region="view-search"]',VIEW_SETTINGS:'[data-region="view-settings"]',ROUTES:"[data-route]",ROUTES_BACK:"[data-route-back]",HEADER_CONTAINER:'[data-region="header-container"]',BODY_CONTAINER:'[data-region="body-container"]',FOOTER_CONTAINER:'[data-region="footer-container"]'},o=function(a,b){var c=a.children(),d=c.filter(n.HEADER_CONTAINER).find(b),e=c.filter(n.BODY_CONTAINER).find(b),f=c.filter(n.FOOTER_CONTAINER).find(b),g=[d,e,f].filter(function(a){return a.length});return g},p=[[l.VIEW_CONTACT,n.VIEW_CONTACT,d.show,d.description],[l.VIEW_CONTACTS,n.VIEW_CONTACTS,e.show,e.description],[l.VIEW_CONVERSATION,n.VIEW_CONVERSATION,f.show,f.description],[l.VIEW_GROUP_INFO,n.VIEW_GROUP_INFO,g.show,g.description],[l.VIEW_OVERVIEW,n.VIEW_OVERVIEW,h.show,h.description],[l.VIEW_SEARCH,n.VIEW_SEARCH,i.show,i.description],[l.VIEW_SETTINGS,n.VIEW_SETTINGS,j.show,j.description]],q=function(a){p.forEach(function(b){k.add(b[0],o(a,b[1]),b[2],b[3])})},r=function(a){a.attr("data-shown")||(k.go(l.VIEW_OVERVIEW),a.attr("data-shown",!0)),a.removeClass("hidden"),a.attr("aria-expanded",!0),a.attr("aria-hidden",!1)},s=function(a){a.addClass("hidden"),a.attr("aria-expanded",!1),a.attr("aria-hidden",!0)},t=function(a){return!a.hasClass("hidden")},u=function(d){b.define(d,[b.events.activate]);var e=/^data-route-param-?(\d*)$/;d.on(b.events.activate,n.ROUTES,function(b,c){for(var d=a(b.target).closest(n.ROUTES),f=d.attr("data-route"),g=[],h=0;h<d[0].attributes.length;h++)g.push(d[0].attributes[h]);var i=g.filter(function(a){var b=a.nodeName,c=e.test(b);return c});i.sort(function(a,b){var c=e.exec(a.nodeName),d=e.exec(b.nodeName),f=c.length>1?c[1]:0,g=d.length>1?d[1]:0;return f<g?-1:g<f?1:0});var j=i.map(function(a){return a.nodeValue}),l=[f].concat(j);k.go.apply(null,l),c.originalEvent.preventDefault()}),d.on(b.events.activate,n.ROUTES_BACK,function(a,b){k.back(),b.originalEvent.preventDefault()}),c.subscribe(m.SHOW,function(){r(d)}),c.subscribe(m.HIDE,function(){s(d)}),c.subscribe(m.TOGGLE_VISIBILITY,function(){t(d)?s(d):r(d)}),c.subscribe(m.SHOW_CONVERSATION,function(a){r(d),k.go(l.VIEW_CONVERSATION,a)}),c.subscribe(m.CREATE_CONVERSATION_WITH_USER,function(a){r(d),k.go(l.VIEW_CONVERSATION,null,"create",a)}),c.subscribe(m.SHOW_SETTINGS,function(){r(d),k.go(l.VIEW_SETTINGS)}),c.subscribe(m.PREFERENCES_UPDATED,function(a){var b=a.filter(function(a){return"message_entertosend"==a.type}),c=b.length?b[0]:null;if(c){var e=d.find(n.FOOTER_CONTAINER).find(n.VIEW_CONVERSATION);e.attr("data-enter-to-send",c.value)}})},v=function(b){b=a(b),q(b),u(b)};return{init:v}});
File diff suppressed because one or more lines are too long
File diff suppressed because one or more lines are too long
File diff suppressed because one or more lines are too long
File diff suppressed because one or more lines are too long
File diff suppressed because one or more lines are too long
+12
View File
@@ -229,6 +229,18 @@ function(
show(root);
Router.go(Routes.VIEW_SETTINGS);
});
PubSub.subscribe(Events.PREFERENCES_UPDATED, function(preferences) {
var filteredPreferences = preferences.filter(function(preference) {
return preference.type == 'message_entertosend';
});
var enterToSendPreference = filteredPreferences.length ? filteredPreferences[0] : null;
if (enterToSendPreference) {
var viewConversationFooter = root.find(SELECTORS.FOOTER_CONTAINER).find(SELECTORS.VIEW_CONVERSATION);
viewConversationFooter.attr('data-enter-to-send', enterToSendPreference.value);
}
});
};
/**
+2
View File
@@ -33,6 +33,8 @@ function(
/**
* Trigger an event to create a new conversation in the message drawer.
*
* @param {Number} userId The user id to start a conversation.
*/
var createConversationWithUser = function(userId) {
PubSub.publish(MessageDrawerEvents.CREATE_CONVERSATION_WITH_USER, userId);
+2
View File
@@ -184,6 +184,8 @@ function(
// Update the aria label for the back button.
element.find(SELECTORS.ROUTES_BACK).attr('aria-label', label);
});
return;
});
})
.catch(function() {
@@ -97,6 +97,8 @@ function(
var loadedAllMessages = false;
var messagesOffset = 0;
var newMessagesPollTimer = null;
// If the UI is currently resetting.
var isResetting = true;
// This is the render function which will be generated when this module is
// first called. See generateRenderFunction for details.
var render = null;
@@ -470,7 +472,9 @@ function(
if (!limit) {
return result;
} else if (result.messages.length > limit) {
result.messages = result.messages.slice(1);
// Ignore the last result which was just to test if there are more
// to load.
result.messages = result.messages.slice(0, -1);
} else {
setLoadedAllMessages(true);
}
@@ -509,7 +513,7 @@ function(
var messages = viewState.messages;
var mostRecentMessage = messages.length ? messages[messages.length - 1] : null;
if (mostRecentMessage) {
if (mostRecentMessage && !isResetting) {
// There may be multiple messages with the same time created value since
// the accuracy is only down to the second. The server will include these
// messages in the result (since it does a >= comparison on time from) so
@@ -547,6 +551,8 @@ function(
}
});
}
return $.Deferred().resolve().promise();
};
};
@@ -1236,7 +1242,8 @@ function(
messagesContainer.on(CustomEvents.events.scrollTop, function(e, data) {
var hasMembers = Object.keys(viewState.members).length > 1;
if (!isLoadingMoreMessages && !hasLoadedAllMessages() && hasMembers) {
if (!isResetting && !isLoadingMoreMessages && !hasLoadedAllMessages() && hasMembers) {
isLoadingMoreMessages = true;
var newState = StateManager.setLoadingMessages(viewState, true);
render(newState)
.then(function() {
@@ -1276,31 +1283,18 @@ function(
footer.on(CustomEvents.events.enter, SELECTORS.MESSAGE_TEXT_AREA, function(e, data) {
var enterToSend = footer.attr('data-enter-to-send');
if (enterToSend == true) {
if (enterToSend && enterToSend != 'false' && enterToSend != '0') {
handleSendMessage(e, data);
}
});
PubSub.subscribe(MessageDrawerEvents.ROUTE_CHANGED, function(newRouteData) {
if (newMessagesPollTimer) {
if (newRouteData.route == MessageDrawerRoutes.VIEW_CONVERSATION) {
newMessagesPollTimer.restart();
} else {
if (newRouteData.route != MessageDrawerRoutes.VIEW_CONVERSATION) {
newMessagesPollTimer.stop();
}
}
});
PubSub.subscribe(MessageDrawerEvents.PREFERENCES_UPDATED, function(preferences) {
var filteredPreferences = preferences.filter(function(preference) {
return preference.type == 'message_entertosend';
});
var enterToSendPreference = filteredPreferences.length ? filteredPreferences[0] : null;
if (enterToSendPreference) {
footer.attr('data-enter-to-send', enterToSendPreference.value);
}
});
};
/**
@@ -1529,6 +1523,7 @@ function(
if (isNewConversation) {
// Reset all of the states back to the beginning if we're loading a new
// conversation.
isResetting = true;
var renderPromise = null;
var loggedInUserProfile = getLoggedInUserProfile(body);
if (conversation) {
@@ -1541,12 +1536,22 @@ function(
return renderPromise
.then(function() {
isResetting = false;
// Focus the first element that can receieve it in the header.
header.find(Constants.SELECTORS.CAN_RECEIVE_FOCUS).first().focus();
return;
})
.catch(Notification.exception);
} else if (viewState.type == CONVERSATION_TYPES.PRIVATE && action) {
.catch(function(error) {
isResetting = false;
Notification.exception(error);
});
}
// We're not loading a new conversation so we should reset the poll timer to try to load
// new messages.
resetMessagePollTimer(conversationId);
if (viewState.type == CONVERSATION_TYPES.PRIVATE && action) {
// There are special actions that the user can perform in a private (aka 1-to-1)
// conversation.
var currentOtherUserId = getOtherUserId();
@@ -209,7 +209,11 @@ function(
messagesDiff.missingFromA.forEach(function(message) {
var before = findPositionInArray(dayCurrent.messages, function(candidate) {
return message.timeCreated < candidate.timeCreated;
if (message.timeCreated == candidate.timeCreated) {
return message.id < candidate.id;
} else {
return message.timeCreated < candidate.timeCreated;
}
});
add.push({
@@ -637,33 +637,28 @@ function(
* @return {Promise} Messages rendering promises.
*/
var renderAddMessages = function(header, body, footer, messages, datesCache) {
var messagesRenderPromises = messages.map(function(data) {
var formattedMessages = formatMessagesForTemplate([data.value], datesCache);
return Templates.render(TEMPLATES.MESSAGE, formattedMessages[0]);
var messagesData = messages.map(function(data) {
return data.value;
});
var formattedMessages = formatMessagesForTemplate(messagesData, datesCache);
return $.when.apply($, messagesRenderPromises).then(function() {
// Wait until all of the rendering is done for each of the messages
// to ensure they are added to the page in the correct order.
messages.forEach(function(data, index) {
messagesRenderPromises[index]
.then(function(html) {
if (data.before) {
var element = getMessageElement(body, data.before.id);
return $(html).insertBefore(element);
} else {
var dayContainer = getDayElement(body, data.day.timestamp);
var dayMessagesContainer = dayContainer.find(SELECTORS.DAY_MESSAGES_CONTAINER);
return dayMessagesContainer.append(html);
}
})
.catch(function() {
// Silently ignore failed renders.
});
return Templates.render(TEMPLATES.MESSAGES, {messages: formattedMessages})
.then(function(html) {
var messageList = $(html);
messages.forEach(function(data) {
var messageHtml = messageList.find('[data-message-id="' + data.value.id + '"]');
if (data.before) {
var element = getMessageElement(body, data.before.id);
return messageHtml.insertBefore(element);
} else {
var dayContainer = getDayElement(body, data.day.timestamp);
var dayMessagesContainer = dayContainer.find(SELECTORS.DAY_MESSAGES_CONTAINER);
return dayMessagesContainer.append(messageHtml);
}
});
return;
});
return;
});
};
/**
@@ -129,6 +129,10 @@ define(['jquery'], function($) {
return -1;
} else if (a.timeCreated > b.timeCreated) {
return 1;
} else if (a.id < b.id) {
return -1;
} else if (a.id > b.id) {
return 1;
} else {
return 0;
}
@@ -130,7 +130,7 @@ function(
formattedConversation.userid = otherUser.id;
formattedConversation.showonlinestatus = otherUser.showonlinestatus;
formattedConversation.isonline = otherUser.isosnline;
formattedConversation.isonline = otherUser.isonline;
formattedConversation.isblocked = otherUser.isblocked;
}
@@ -354,6 +354,9 @@ function(
imageurl: conversation.imageUrl,
};
// Cache the conversation.
loadedConversationsById[conversation.id] = conversation;
return Templates.render(TEMPLATES.CONVERSATIONS_LIST, {conversations: [formattedConversation]})
.then(function(html) {
var contentContainer = LazyLoadList.getContentContainer(root);
+8
View File
@@ -446,6 +446,10 @@ define(['jquery', 'core/ajax', 'core/notification'], function($, Ajax, Notificat
/**
* Send messages to a conversation.
*
* @param {int} conversationId The conversation id
* @param {string[]} messages List of messages to send
* @return {object} jQuery promise
*/
var sendMessagesToConversation = function(conversationId, messages) {
var formattedMessages = messages.map(function(message) {
@@ -466,6 +470,10 @@ define(['jquery', 'core/ajax', 'core/notification'], function($, Ajax, Notificat
/**
* Send a message to a conversation.
*
* @param {int} conversationId The conversation id
* @param {string} text The message text
* @return {object} jQuery promise
*/
var sendMessageToConversation = function(conversationId, text) {
return sendMessagesToConversation(conversationId, [text])
+5 -2
View File
@@ -855,6 +855,7 @@ class api {
$userid,
$conversationid,
$includecontactrequests,
$includeprivacyinfo,
$memberoffset,
$memberlimit
);
@@ -2897,18 +2898,20 @@ class api {
* @param int $userid The user we are returning the conversation members for, used by helper::get_member_info.
* @param int $conversationid The id of the conversation
* @param bool $includecontactrequests Do we want to include contact requests with this data?
* @param bool $includeprivacyinfo Do we want to include privacy requests with this data?
* @param int $limitfrom
* @param int $limitnum
* @return array
*/
public static function get_conversation_members(int $userid, int $conversationid, bool $includecontactrequests = false,
int $limitfrom = 0, int $limitnum = 0) : array {
bool $includeprivacyinfo = false, int $limitfrom = 0,
int $limitnum = 0) : array {
global $DB;
if ($members = $DB->get_records('message_conversation_members', ['conversationid' => $conversationid],
'timecreated ASC, id ASC', 'userid', $limitfrom, $limitnum)) {
$userids = array_keys($members);
$members = helper::get_member_info($userid, $userids, $includecontactrequests);
$members = helper::get_member_info($userid, $userids, $includecontactrequests, $includeprivacyinfo);
return $members;
}
+6 -3
View File
@@ -764,6 +764,8 @@ class core_message_external extends external_api {
'conversationid' => new external_value(PARAM_INT, 'The id of the conversation'),
'includecontactrequests' => new external_value(PARAM_BOOL, 'Do we want to include contact requests?',
VALUE_DEFAULT, false),
'includeprivacyinfo' => new external_value(PARAM_BOOL, 'Do we want to include privacy info?',
VALUE_DEFAULT, false),
'limitfrom' => new external_value(PARAM_INT, 'Limit from', VALUE_DEFAULT, 0),
'limitnum' => new external_value(PARAM_INT, 'Limit number', VALUE_DEFAULT, 0)
]
@@ -776,12 +778,13 @@ class core_message_external extends external_api {
* @param int $userid The user we are returning the conversation members for, used by helper::get_member_info.
* @param int $conversationid The id of the conversation
* @param bool $includecontactrequests Do we want to include contact requests with this data?
* @param bool $includeprivacyinfo Do we want to include privacy info?
* @param int $limitfrom
* @param int $limitnum
* @return array
*/
public static function get_conversation_members(int $userid, int $conversationid, bool $includecontactrequests = false,
int $limitfrom = 0, int $limitnum = 0) {
bool $includeprivacyinfo = false, int $limitfrom = 0, int $limitnum = 0) {
global $CFG, $USER;
// Check if messaging is enabled.
@@ -797,6 +800,7 @@ class core_message_external extends external_api {
'userid' => $userid,
'conversationid' => $conversationid,
'includecontactrequests' => $includecontactrequests,
'includeprivacyinfo' => $includeprivacyinfo,
'limitfrom' => $limitfrom,
'limitnum' => $limitnum
];
@@ -812,9 +816,8 @@ class core_message_external extends external_api {
throw new moodle_exception('You are not a member of this conversation.');
}
return \core_message\api::get_conversation_members($params['userid'], $params['conversationid'], $params['includecontactrequests'],
$params['limitfrom'], $params['limitnum']);
$params['includeprivacyinfo'], $params['limitfrom'], $params['limitnum']);
}
/**
+4 -4
View File
@@ -5509,12 +5509,12 @@ class core_message_externallib_testcase extends externallib_advanced_testcase {
$this->assertCount(2, $writtenmessages);
$this->assertObjectHasAttribute('id', $writtenmessages[0]);
$this->assertEquals($user1->id, $writtenmessages[0]->useridfrom);
$this->assertEquals($messages[0]['text'], $writtenmessages[0]->text);
$this->assertEquals('<p>a message from user 1</p>', $writtenmessages[0]->text);
$this->assertNotEmpty($writtenmessages[0]->timecreated);
$this->assertObjectHasAttribute('id', $writtenmessages[1]);
$this->assertEquals($user1->id, $writtenmessages[1]->useridfrom);
$this->assertEquals($messages[1]['text'], $writtenmessages[1]->text);
$this->assertEquals('<p>another message from user 1</p>', $writtenmessages[1]->text);
$this->assertNotEmpty($writtenmessages[1]->timecreated);
}
@@ -5561,12 +5561,12 @@ class core_message_externallib_testcase extends externallib_advanced_testcase {
$this->assertCount(2, $writtenmessages);
$this->assertObjectHasAttribute('id', $writtenmessages[0]);
$this->assertEquals($user1->id, $writtenmessages[0]->useridfrom);
$this->assertEquals($messages[0]['text'], $writtenmessages[0]->text);
$this->assertEquals('<p>a message from user 1 to group conv</p>', $writtenmessages[0]->text);
$this->assertNotEmpty($writtenmessages[0]->timecreated);
$this->assertObjectHasAttribute('id', $writtenmessages[1]);
$this->assertEquals($user1->id, $writtenmessages[1]->useridfrom);
$this->assertEquals($messages[1]['text'], $writtenmessages[1]->text);
$this->assertEquals('<p>another message from user 1 to group conv</p>', $writtenmessages[1]->text);
$this->assertNotEmpty($writtenmessages[1]->timecreated);
}