Extract message.set_read_flag().

This code adds 'read' to message.flags and sets message.unread
to false.

It's not clear that the boolean message.unread is used in any
meaningful way, but we set it to false to avoid confusion.  The
bankruptcy code was not doing this before.

Another quirk that existed before was that you could get two
'read' flags in a message when you declared bankruptcy.  It's
also plausible that this could happen if you marked a message
as read via two different ways.  It probably did not cause
user-facing bugs, but it would be confusing for troubleshooting.

Fixes #5032.

frontend_tests/node_tests/unread.js

@@ -424,6 +424,18 @@ stream_data.get_stream_id = function () {
 }());
 
 (function test_message_unread() {
+    var message = {flags: ['starred'], unread: true};
+    assert(unread.message_unread(message));
+
+    unread.set_read_flag(message);
+    assert(!unread.message_unread(message));
+    assert(!message.unread);
+
+    // idempotency
+    unread.set_read_flag(message);
+    assert(!unread.message_unread(message));
+    assert.deepEqual(message.flags, ['starred', 'read']);
+
     // Test some code that might be overly defensive, for line coverage sake.
     assert(!unread.message_unread(undefined));
     assert(unread.message_unread({flags: []}));

static/js/unread.js

@@ -389,6 +389,25 @@ exports.num_unread_for_person = function (user_ids_string) {
     return exports.unread_pm_counter.num_unread(user_ids_string);
 };
 
+exports.set_read_flag = function (message) {
+    /*
+        Our data structures allow us to know if a message_id is unread/read,
+        but we also need to set message.unread for our rendering code.
+
+        We also have code that uses message.flags, so we maintain that data
+        as well. The server sends us flags (e.g. ['read', 'starred']), so
+        our code on the "edges" needs that representation.
+
+        It is kind of painful to have three different representations, but
+        we fortunately only set read/unread in a few places in our code.
+    */
+    message.flags = message.flags || [];
+    if (!_.contains(message.flags, 'read')) {
+        message.flags.push('read');
+    }
+    message.unread = false;
+};
+
 return exports;
 }());
 if (typeof module !== 'undefined') {

static/js/unread_ops.js

@@ -4,8 +4,7 @@ var exports = {};
 
 exports.mark_all_as_read = function mark_all_as_read(cont) {
     _.each(message_list.all.all_messages(), function (msg) {
-        msg.flags = msg.flags || [];
+        unread.set_read_flag(msg);
-        msg.flags.push('read');
     });
     unread.declare_bankruptcy();
     unread_ui.update_unread_counts();
@@ -24,9 +23,7 @@ function process_newly_read_message(message, options) {
     // This code gets called when a message becomes newly read, whether
     // due to local things like advancing the pointer, or due to us
     // getting notified by the server that a message has been read.
-    message.flags = message.flags || [];
+    unread.set_read_flag(message);
-    message.flags.push('read');
-    message.unread = false;
 
     home_msg_list.show_message_as_read(message, options);
     message_list.all.show_message_as_read(message, options);