From 8623a02d986446cbb19426d310df73f4d63cb872 Mon Sep 17 00:00:00 2001 From: Challa Venkata Raghava Reddy Date: Wed, 13 Mar 2019 18:17:57 +0530 Subject: [PATCH] streams: Avoid showing more topics option when it's useless. This makes the "more topics" option which appears below the list of known topics in the left sidebar appear only when it's possible there are actually more topics to be displayed. Two specific cases it resolves completely include: * Newly created realms; this widget was a common source of confusion for new organization administrators. * Newly created streams. There are still some corner cases this doesn't handle, e.g. if you just joined a private stream with protected history, but there isn't as easy a fix for those. Essentially rewritten by tabbott to fix code duplication and comment extensively. Fixes #10265. --- frontend_tests/node_tests/stream_data.js | 35 ++++++++++++++++++++++ frontend_tests/node_tests/topic_list.js | 1 + static/js/message_list.js | 1 - static/js/stream_data.js | 38 ++++++++++++++++++++++++ static/js/topic_list.js | 9 +++++- 5 files changed, 82 insertions(+), 2 deletions(-) diff --git a/frontend_tests/node_tests/stream_data.js b/frontend_tests/node_tests/stream_data.js index 35a9506c3f..76860a484f 100644 --- a/frontend_tests/node_tests/stream_data.js +++ b/frontend_tests/node_tests/stream_data.js @@ -7,6 +7,8 @@ set_global('$', function () { }); set_global('blueslip', global.make_zblueslip()); +set_global('document', null); +global.stub_out_jquery(); zrequire('color_data'); zrequire('util'); @@ -16,6 +18,11 @@ zrequire('people'); zrequire('stream_color'); zrequire('stream_data'); zrequire('marked', 'third/marked/lib/marked'); +zrequire('FetchStatus', 'js/fetch_status'); +zrequire('Filter', 'js/filter'); +zrequire('MessageListData', 'js/message_list_data'); +zrequire('MessageListView', 'js/message_list_view'); +zrequire('message_list'); run_test('basics', () => { var denmark = { @@ -808,3 +815,31 @@ run_test('get_invite_stream_data', () => { }); assert.deepEqual(stream_data.get_invite_stream_data(), expected_list); }); + +run_test('all_topics_in_cache', () => { + // Add a new stream with first_message_id set. + var general = { + name: 'general', + stream_id: 21, + first_message_id: null, + }; + var messages = [ + {id: 1, stream_id: 21}, + {id: 2, stream_id: 21}, + {id: 3, stream_id: 21}, + ]; + var sub = stream_data.create_sub_from_server_data('general', general); + + assert.equal(stream_data.all_topics_in_cache(sub), false); + + message_list.all.data.add_messages(messages); + assert.equal(stream_data.all_topics_in_cache(sub), false); + message_list.all.fetch_status.has_found_newest = () => {return true;}; + assert.equal(stream_data.all_topics_in_cache(sub), true); + + sub.first_message_id = 0; + assert.equal(stream_data.all_topics_in_cache(sub), false); + + sub.first_message_id = 2; + assert.equal(stream_data.all_topics_in_cache(sub), true); +}); diff --git a/frontend_tests/node_tests/topic_list.js b/frontend_tests/node_tests/topic_list.js index 602aca9747..fdc9babf69 100644 --- a/frontend_tests/node_tests/topic_list.js +++ b/frontend_tests/node_tests/topic_list.js @@ -6,6 +6,7 @@ set_global('unread', {}); set_global('muting', {}); set_global('stream_popover', {}); set_global('templates', {}); +set_global('message_list', {}); zrequire('hash_util'); zrequire('stream_data'); diff --git a/static/js/message_list.js b/static/js/message_list.js index d00ca0ab59..f1007aa048 100644 --- a/static/js/message_list.js +++ b/static/js/message_list.js @@ -426,7 +426,6 @@ exports.MessageList.prototype = { get_last_message_sent_by_me: function () { return this.data.get_last_message_sent_by_me(); }, - }; exports.all = new exports.MessageList({ diff --git a/static/js/stream_data.js b/static/js/stream_data.js index 83921e5c48..b768985d28 100644 --- a/static/js/stream_data.js +++ b/static/js/stream_data.js @@ -356,6 +356,43 @@ exports.get_announcement_only = function (stream_name) { return sub.is_announcement_only; }; +exports.all_topics_in_cache = function (sub) { + // Checks whether this browser's cache in message_list.all has all + // messages from a given stream, and thus all historical topics + // for it. + + // If the cache isn't initialized, it's a clear false. + if (message_list.all === undefined || message_list.all.empty()) { + return false; + } + + // If the cache doesn't have the latest messages, we can't be sure + // we have all topics. + if (!message_list.all.fetch_status.has_found_newest()) { + return false; + } + + // This happens before message_list.all is initialized; just + // return false since the cache is empty. + var first_cached_message = message_list.all.first(); + if (first_cached_message === undefined) { + return false; + } + + if (sub.first_message_id === null) { + // If the stream has no message history, we have it all + // vacuously. This should be a very rare condition, since + // stream creation sends a message. + return true; + } + + // Now, we can just compare the first cached message to the first + // message ID in the stream; if it's older, we're good, otherwise, + // we might be missing the oldest topics in this stream in our + // cache. + return first_cached_message.id <= sub.first_message_id; +}; + var default_stream_ids = new Dict(); exports.set_realm_default_streams = function (realm_default_streams) { @@ -516,6 +553,7 @@ exports.create_sub_from_server_data = function (stream_name, attrs) { email_notifications: page_params.enable_stream_email_notifications, description: '', rendered_description: '', + first_message_id: attrs.first_message_id, }); exports.set_subscribers(sub, subscriber_user_ids); diff --git a/static/js/topic_list.js b/static/js/topic_list.js index 1b48809ff7..cb0258982c 100644 --- a/static/js/topic_list.js +++ b/static/js/topic_list.js @@ -116,9 +116,16 @@ exports.widget = function (parent_elem, my_stream_id) { ul.append(li); }); + // Now, we decide whether we need to show the "more topics" + // widget. We need it if there are at least 5 topics in the + // frontend's cache, or if we (possibly) don't have all + // historical topics in the browser's cache. var show_more = self.build_more_topics_section(); - ul.append(show_more); + var sub = stream_data.get_sub_by_id(my_stream_id); + if (topic_names.length > max_topics || !stream_data.all_topics_in_cache(sub)) { + ul.append(show_more); + } return ul; };