From 9ee2be4a0d26cc402fc6812860c66a417c2263ab Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Thu, 13 Jul 2017 10:21:16 -0400 Subject: [PATCH] Use client_message_id as key for sent_messages lookups. We now use a client-side message id to track the state of our sent messages. This sets up future commits to start tracking state earlier in the message's life cycle. It also avoids ugly reify logic where we capture an event to update our data structure to key on the server's message id instead of the local id. That eliminates the node test as well. Another node test gets deleted here, just because it's not worth the trouble with upcoming refactorings. --- frontend_tests/node_tests/compose.js | 35 +---------------- static/js/compose.js | 18 ++++++--- static/js/echo.js | 7 +++- static/js/message_util.js | 2 +- static/js/sent_messages.js | 58 ++++++++++++---------------- static/js/server_events.js | 1 + 6 files changed, 46 insertions(+), 75 deletions(-) diff --git a/frontend_tests/node_tests/compose.js b/frontend_tests/node_tests/compose.js index 6e8bc63a6d..286a5869bd 100644 --- a/frontend_tests/node_tests/compose.js +++ b/frontend_tests/node_tests/compose.js @@ -284,16 +284,7 @@ people.add(bob); assert(reify_message_id_checked); }()); -(function test_mark_rendered_content_disparity() { - sent_messages.mark_rendered_content_disparity(13, true); - assert.deepEqual(sent_messages.send_times_data[13].data.rendered_content_disparity, true); -}()); - (function test_report_as_received() { - var msg = { - id: 12, - sent_by_me: true, - }; var set_timeout_called = false; global.patch_builtin('setTimeout', function (func, delay) { assert.equal(delay, 0); @@ -310,14 +301,13 @@ people.add(bob); assert(payload.data.locally_echoed); assert(!payload.data.rendered_content_disparity); }; - sent_messages.report_as_received(msg); + sent_messages.report_as_received(12); assert.equal(typeof(data.received), 'object'); assert.equal(typeof(data.displayed), 'object'); assert(set_timeout_called); delete sent_messages.send_times_data[13]; - msg.id = 13; - sent_messages.report_as_received(msg); + sent_messages.report_as_received(13); data = sent_messages.get_message_state(13).data; assert.equal(typeof(data.received), 'object'); assert.equal(typeof(data.displayed), 'object'); @@ -1243,27 +1233,6 @@ function test_with_mock_socket(test_params) { assert(!$("#preview_message_area").visible()); assert($("#markdown_preview").visible()); }()); - - (function test_message_id_changed_document() { - sent_messages.initialize(); - var handler = $(document).get_on_handler('message_id_changed'); - sent_messages.send_times_data = { - 1031: { - data: 'Test data!', - }, - }; - event.old_id = 1031; - event.new_id = 1045; - - handler(event); - - var send_times_data = { - 1045: { - data: 'Test data!', - }, - }; - assert.deepEqual(sent_messages.send_times_data, send_times_data); - }()); }()); (function test_upload_started() { diff --git a/static/js/compose.js b/static/js/compose.js index c8ac14d48b..17e9a9638d 100644 --- a/static/js/compose.js +++ b/static/js/compose.js @@ -216,11 +216,15 @@ exports.send_message_success = function (local_id, message_id, locally_echoed) { }; exports.transmit_message = function (request, on_success, error) { - request.client_message_id = sent_messages.get_new_client_message_id({ + var client_message_id = sent_messages.get_new_client_message_id({ local_id: request.local_id, }); - sent_messages.clear(request.id); + // The host will attach this to message events, and it will allow + // us to match up the right messages to this request. + request.client_message_id = client_message_id; + + sent_messages.clear(client_message_id); var local_id = request.local_id; var start_time = new Date(); @@ -231,11 +235,13 @@ exports.transmit_message = function (request, on_success, error) { // box and turning off spinners and reifying locally echoed messages. on_success(data); - var message_id = data.id; - // Once everything is reified, get ready to report times to the server. - sent_messages.process_success(message_id, start_time, locally_echoed); - sent_messages.set_timer_for_restarting_event_loop(message_id); + sent_messages.process_success({ + client_message_id: client_message_id, + start: start_time, + locally_echoed: locally_echoed, + }); + sent_messages.set_timer_for_restarting_event_loop(client_message_id); } if (page_params.use_websockets) { diff --git a/static/js/echo.js b/static/js/echo.js index 8e0b508649..4fed65bcbe 100644 --- a/static/js/echo.js +++ b/static/js/echo.js @@ -173,11 +173,14 @@ exports.process_from_server = function process_from_server(messages) { if (client_message.content !== message.content) { client_message.content = message.content; updated = true; - sent_messages.mark_rendered_content_disparity(message.id, true); + sent_messages.mark_rendered_content_disparity({ + client_message_id: message.client_message_id, + changed: true, + }); } msgs_to_rerender.push(client_message); locally_processed_ids.push(client_message.id); - sent_messages.report_as_received(client_message); + sent_messages.report_as_received(message.client_message_id); delete waiting_for_ack[client_message.id]; return false; } diff --git a/static/js/message_util.js b/static/js/message_util.js index a5afb654a0..a8cfd34835 100644 --- a/static/js/message_util.js +++ b/static/js/message_util.js @@ -23,7 +23,7 @@ exports.add_messages = function add_messages(messages, msg_list, opts) { if (msg_list === home_msg_list && opts.messages_are_new) { _.each(messages, function (message) { if (message.local_id === undefined) { - sent_messages.report_as_received(message); + sent_messages.report_as_received(message.client_message_id); } }); } diff --git a/static/js/sent_messages.js b/static/js/sent_messages.js index d6a98bf03e..27ea179d39 100644 --- a/static/js/sent_messages.js +++ b/static/js/sent_messages.js @@ -100,42 +100,42 @@ exports.message_state = function () { return self; }; -exports.get_message_state = function (message_id) { - if (exports.send_times_data[message_id] === undefined) { - exports.send_times_data[message_id] = exports.message_state(); +exports.get_message_state = function (client_message_id) { + if (exports.send_times_data[client_message_id] === undefined) { + exports.send_times_data[client_message_id] = exports.message_state(); } - return exports.send_times_data[message_id]; + return exports.send_times_data[client_message_id]; }; -function mark_end_to_end_receive_time(message_id) { - var state = exports.get_message_state(message_id); +function mark_end_to_end_receive_time(client_message_id) { + var state = exports.get_message_state(client_message_id); state.mark_received(); } -function mark_end_to_end_display_time(message_id) { - var state = exports.get_message_state(message_id); +function mark_end_to_end_display_time(client_message_id) { + var state = exports.get_message_state(client_message_id); state.mark_displayed(); } -exports.mark_rendered_content_disparity = function (message_id, changed) { - var state = exports.get_message_state(message_id); - state.mark_disparity(changed); +exports.mark_rendered_content_disparity = function (opts) { + var state = exports.get_message_state(opts.client_message_id); + state.mark_disparity(opts.changed); }; -exports.report_as_received = function report_as_received(message) { - if (message.sent_by_me) { - mark_end_to_end_receive_time(message.id); +exports.report_as_received = function report_as_received(client_message_id) { + if (client_message_id) { + mark_end_to_end_receive_time(client_message_id); setTimeout(function () { - mark_end_to_end_display_time(message.id); + mark_end_to_end_display_time(client_message_id); }, 0); } }; -exports.process_success = function (message_id, start_time, locally_echoed) { +exports.process_success = function (opts) { var send_finished = new Date(); - var send_time = (send_finished - start_time); + var send_time = (send_finished - opts.start); if (feature_flags.log_send_times) { blueslip.log("send time: " + send_time); } @@ -143,36 +143,28 @@ exports.process_success = function (message_id, start_time, locally_echoed) { exports.send_times_log.push(send_time); } - var state = exports.get_message_state(message_id); + var state = exports.get_message_state(opts.client_message_id); state.process_success({ - start: start_time, + start: opts.start, send_finished: send_finished, - locally_echoed: locally_echoed, + locally_echoed: opts.locally_echoed, }); }; -exports.set_timer_for_restarting_event_loop = function (message_id) { +exports.set_timer_for_restarting_event_loop = function (client_message_id) { setTimeout(function () { - if (!exports.send_times_data[message_id].was_received()) { - blueslip.log("Restarting get_events due to delayed receipt of sent message " + message_id); + if (!exports.send_times_data[client_message_id].was_received()) { + blueslip.log("Restarting get_events due to delayed receipt of sent message " + client_message_id); server_events.restart_get_events(); } }, 5000); }; -exports.clear = function (message_id) { - delete exports.send_times_data[message_id]; +exports.clear = function (client_message_id) { + delete exports.send_times_data[client_message_id]; }; exports.initialize = function () { - $(document).on('message_id_changed', function (event) { - if (exports.send_times_data[event.old_id] !== undefined) { - var value = exports.send_times_data[event.old_id]; - exports.send_times_data[event.new_id] = value; - delete exports.send_times_data[event.old_id]; - } - }); - exports.reset_id_state(); }; diff --git a/static/js/server_events.js b/static/js/server_events.js index 642b7dbad3..3358b315b2 100644 --- a/static/js/server_events.js +++ b/static/js/server_events.js @@ -74,6 +74,7 @@ function get_events_success(events) { msg.local_id = sent_messages.get_local_id({ client_message_id: event.client_message_id, }); + msg.client_message_id = event.client_message_id; messages.push(msg); break;