UX: Use topic list for displaying group messages on group page.

https://meta.discourse.org/t/group-inbox-on-a-groups-page-mockup/71319

app/assets/javascripts/discourse/components/group-navigation.js.es6

@@ -1,15 +1,3 @@
-import computed from 'ember-addons/ember-computed-decorators';
-
 export default Ember.Component.extend({
   tagName: '',
-
-  @computed('group')
-  availableTabs(group) {
-    return this.get('tabs').filter(t => {
-      if (t.admin) {
-        return this.currentUser ? this.currentUser.canManageGroup(group) : false;
-      }
-      return true;
-    });
-  }
 });

app/assets/javascripts/discourse/controllers/group-activity.js.es6

@@ -1,14 +1,4 @@
-import computed from 'ember-addons/ember-computed-decorators';
-
 export default Ember.Controller.extend({
   application: Ember.inject.controller(),
   queryParams: ['category_id'],
-
-  @computed('model.is_group_user')
-  showGroupMessages(isGroupUser) {
-    if (!this.siteSettings.enable_personal_messages) {
-      return false;
-    }
-    return isGroupUser || (this.currentUser && this.currentUser.admin);
-  }
 });

app/assets/javascripts/discourse/controllers/group.js.es6

@@ -1,4 +1,4 @@
-import { default as computed, observes } from 'ember-addons/ember-computed-decorators';
+import { default as computed } from 'ember-addons/ember-computed-decorators';
 
 const Tab = Ember.Object.extend({
   init() {
@@ -14,16 +14,49 @@ export default Ember.Controller.extend({
   counts: null,
   showing: 'members',
 
-  tabs: [
+  @computed('showMessages', 'model.user_count')
-    Tab.create({ name: 'members', route: 'group.index', icon: 'users' }),
+  tabs(showMessages, userCount) {
-    Tab.create({ name: 'activity' }),
+    const membersTab = Tab.create({
+      name: 'members',
+      route: 'group.index',
+      icon: 'users'
+    });
+
+    membersTab.set('count', userCount);
+
+    const defaultTabs = [
+      membersTab,
+      Tab.create({ name: 'activity' })
+    ];
+
+    if (showMessages) {
+      defaultTabs.push(Tab.create({
+        name: 'messages.inbox', i18nKey: 'messages'
+      }));
+    }
+
+    if (this.currentUser && this.currentUser.canManageGroup(this.model)) {
+      defaultTabs.push(...[
         Tab.create({
-      name: 'edit', i18nKey: 'edit.title', icon: 'pencil', admin: true
+          name: 'edit', i18nKey: 'edit.title', icon: 'pencil'
         }),
         Tab.create({
-      name: 'logs', i18nKey: 'logs.title', icon: 'list-alt', admin: true
+          name: 'logs', i18nKey: 'logs.title', icon: 'list-alt'
         })
-  ],
+      ]);
+    }
+
+    return defaultTabs;
+  },
+
+  @computed('model.is_group_user')
+  showMessages(isGroupUser) {
+    if (!this.siteSettings.enable_personal_messages) {
+      return false;
+    }
+
+    return isGroupUser || (this.currentUser && this.currentUser.admin);
+  },
 
   @computed('model.is_group_owner', 'model.automatic')
   canEditGroup(isGroupOwner, automatic) {
@@ -50,11 +83,6 @@ export default Ember.Controller.extend({
     return this.currentUser && messageable;
   },
 
-  @observes('model.user_count')
-  _setMembersTabCount() {
-    this.get('tabs')[0].set('count', this.get('model.user_count'));
-  },
-
   actions: {
     messageGroup() {
       this.send('createNewMessageViaParams', this.get('model.name'));

app/assets/javascripts/discourse/routes/app-route-map.js.es6

@@ -56,11 +56,15 @@ export default function() {
       this.route('posts');
       this.route('topics');
       this.route('mentions');
-      this.route('messages');
     });
 
     this.route('logs');
     this.route('edit');
+
+    this.route('messages', function() {
+      this.route('inbox');
+      this.route('archive');
+    });
   });
 
   // User routes

app/assets/javascripts/discourse/routes/build-group-messages-route.js.es6

@@ -0,0 +1,35 @@
+import UserTopicListRoute from "discourse/routes/user-topic-list";
+
+export default (type) => {
+  return UserTopicListRoute.extend({
+    titleToken() {
+      return I18n.t(`user.messages.${type}`);
+    },
+
+    model() {
+      const groupName = this.modelFor('group').get('name');
+      const username = this.currentUser.get("username_lower");
+      let filter = `topics/private-messages-group/${username}/${groupName}`;
+      if (this._isArchive()) filter = `${filter}/archive`;
+      return this.store.findFiltered("topicList", { filter });
+    },
+
+    setupController() {
+      this._super.apply(this, arguments);
+
+      const groupName = this.modelFor('group').get('name');
+      let channel = `/private-messages/group/${groupName}`;
+      if (this._isArchive()) channel = `${channel}/archive`;
+      this.controllerFor("user-topics-list").subscribe(channel);
+
+      this.controllerFor("user-topics-list").setProperties({
+        hideCategory: true,
+        showPosters: true
+      });
+    },
+
+    _isArchive() {
+      return type === 'archive';
+    },
+  });
+};

app/assets/javascripts/discourse/routes/group-activity-messages.js.es6

@@ -1,3 +0,0 @@
-import { buildGroupPage } from 'discourse/routes/group-activity-posts';
-
-export default buildGroupPage('messages');

app/assets/javascripts/discourse/routes/group-edit.js.es6

@@ -1,4 +1,4 @@
-export default Ember.Route.extend({
+export default Discourse.Route.extend({
   titleToken() {
     return I18n.t('groups.edit.title');
   },

app/assets/javascripts/discourse/routes/group-logs.js.es6

@@ -1,6 +1,6 @@
-export default Ember.Route.extend({
+export default Discourse.Route.extend({
   titleToken() {
-    return I18n.t('groups.logs');
+    return I18n.t('groups.logs.title');
   },
 
   model() {

app/assets/javascripts/discourse/routes/group-messages-archive.js.es6

@@ -0,0 +1,3 @@
+import buildGroupMessagesRoute from 'discourse/routes/build-group-messages-route';
+
+export default buildGroupMessagesRoute('archive');

app/assets/javascripts/discourse/routes/group-messages-inbox.js.es6

@@ -0,0 +1,3 @@
+import buildGroupMessagesRoute from 'discourse/routes/build-group-messages-route';
+
+export default buildGroupMessagesRoute('inbox');

app/assets/javascripts/discourse/routes/group-messages.js.es6

@@ -0,0 +1,5 @@
+export default Discourse.Route.extend({
+  titleToken() {
+    return I18n.t('groups.messages');
+  },
+});

app/assets/javascripts/discourse/templates/components/group-navigation.hbs

@@ -1,5 +1,5 @@
 {{#mobile-nav class='group-nav' desktopClass="nav nav-pills" currentPath=currentPath}}
-  {{#each availableTabs as |tab|}}
+  {{#each tabs as |tab|}}
     <li>
       {{#link-to tab.route group title=tab.message class=tab.name}}
         {{#if tab.icon}}{{d-icon tab.icon}}{{/if}}

app/assets/javascripts/discourse/templates/group/activity.hbs

@@ -5,9 +5,6 @@
     {{#if siteSettings.enable_mentions}}
       {{group-activity-filter filter="mentions" categoryId=category_id}}
     {{/if}}
-    {{#if showGroupMessages}}
-      {{group-activity-filter filter="messages"}}
-    {{/if}}
     {{plugin-outlet name="below-group-activity-nav" tagName='' connectorTagName='li'}}
   {{/mobile-nav}}
 

app/assets/javascripts/discourse/templates/group/messages.hbs

@@ -0,0 +1,22 @@
+<div class="group-table">
+  <div class="wrapper">
+    {{#d-section class="group-messages-navigation" pageClass="user-messages"}}
+      {{#mobile-nav class='messages-nav' desktopClass='nav-stacked action-list' currentPath=currentPath}}
+        <li>
+          {{#link-to 'group.messages.inbox' model.name}}
+            {{i18n 'user.messages.inbox'}}
+          {{/link-to}}
+        </li>
+        <li>
+          {{#link-to 'group.messages.archive' model.name}}
+            {{i18n 'user.messages.archive'}}
+          {{/link-to}}
+        </li>
+      {{/mobile-nav}}
+    {{/d-section}}
+
+    <section class='messages'>
+      {{outlet}}
+    </section>
+  </div>
+</div>

app/assets/stylesheets/desktop/user.scss

@@ -31,7 +31,7 @@
   margin-top: 18px;
 }
 
-.user-table {
+.user-table, .group-table {
   width: 100%;
   display: table;
   table-layout: fixed;

app/controllers/groups_controller.rb

@@ -5,7 +5,6 @@ class GroupsController < ApplicationController
     :mentionable,
     :messageable,
     :update,
-    :messages,
     :histories,
     :request_membership,
     :search
@@ -124,19 +123,6 @@ class GroupsController < ApplicationController
     render 'posts/latest', formats: [:rss]
   end
 
-  def messages
-    group = find_group(:group_id)
-    posts = if guardian.can_see_group_messages?(group)
-      group.messages_for(
-        guardian,
-        params.permit(:before_post_id, :category_id)
-      ).where(post_number: 1).limit(20).to_a
-    else
-      []
-    end
-    render_serialized posts, GroupPostSerializer
-  end
-
   def members
     group = find_group(:group_id)
 

app/models/topic_tracking_state.rb

@@ -257,7 +257,9 @@ SQL
 
     if post && allowed_user_ids.include?(post.user_id)
       channels["/private-messages/sent"] = [post.user_id]
-    elsif archive_user_id
+    end
+
+    if archive_user_id
       user_ids = [archive_user_id]
 
       [
@@ -267,19 +269,21 @@ SQL
       ].each do |channel|
         channels[channel] = user_ids
       end
-    else
-      topic.allowed_groups.each do |group|
-        group_channels = []
-        group_channels << "/private-messages/group/#{group.name.downcase}"
-        group_channels << "#{group_channels.first}/archive" if group_archive
-        group_channels.each { |channel| channels[channel] = group.users.pluck(:id) }
-      end
     end
 
     if channels.except("/private-messages/sent").blank?
       channels["/private-messages/inbox"] = allowed_user_ids
     end
 
+    topic.allowed_groups.each do |group|
+      group_user_ids = group.users.pluck(:id)
+      next if group_user_ids.blank?
+      group_channels = []
+      group_channels << "/private-messages/group/#{group.name.downcase}"
+      group_channels << "#{group_channels.first}/archive" if group_archive
+      group_channels.each { |channel| channels[channel] = group_user_ids }
+    end
+
     message = {
       topic_id: topic.id
     }

config/routes.rb

@@ -456,7 +456,6 @@ Discourse::Application.routes.draw do
     get 'members'
     get 'posts'
     get 'mentions'
-    get 'messages'
     get 'counts'
     get 'mentionable'
     get 'messageable'
@@ -467,8 +466,16 @@ Discourse::Application.routes.draw do
     end
 
     member do
-      get 'activity' => "groups#show"
+      %w{
-      get 'activity/:filter' => "groups#show"
+        activity
+        activity/:filter
+        messages
+        messages/inbox
+        messages/archive
+      }.each do |path|
+        get path => 'groups#show'
+      end
+
       put "members" => "groups#add_members"
       delete "members" => "groups#remove_member"
       post "request_membership" => "groups#request_membership"

spec/models/topic_tracking_state_spec.rb

@@ -104,11 +104,16 @@ describe TopicTrackingState do
       it "should publish the right message" do
         messages = MessageBus.track_publish do
           TopicTrackingState.publish_private_message(
-            private_message_topic,
+            private_message_topic
           )
         end
 
-        expect(messages.count).to eq(2)
+        expect(messages.count).to eq(3)
+
+        message = messages.first
+        expect(message.channel).to eq('/private-messages/inbox')
+        expect(message.data["topic_id"]).to eq(private_message_topic.id)
+        expect(message.user_ids).to eq(private_message_topic.allowed_users.map(&:id))
 
         [group1, group2].each do |group|
           message = messages.find do |message|
@@ -129,7 +134,12 @@ describe TopicTrackingState do
             )
           end
 
-          expect(messages.count).to eq(4)
+          expect(messages.count).to eq(5)
+
+          message = messages.first
+          expect(message.channel).to eq('/private-messages/inbox')
+          expect(message.data["topic_id"]).to eq(private_message_topic.id)
+          expect(message.user_ids).to eq(private_message_topic.allowed_users.map(&:id))
 
           [group1, group2].each do |group|
             group_channel = "/private-messages/group/#{group.name}"
@@ -160,6 +170,12 @@ describe TopicTrackingState do
         )
       end
 
+      let!(:group) do
+        group = Fabricate(:group, users: [Fabricate(:user)])
+        private_message_topic.allowed_groups << group
+        group
+      end
+
       it 'should publish the right message' do
         messages = MessageBus.track_publish do
           TopicTrackingState.publish_private_message(
@@ -168,11 +184,12 @@ describe TopicTrackingState do
           )
         end
 
-        expect(messages.count).to eq(2)
+        expect(messages.count).to eq(3)
 
         [
           ['/private-messages/inbox', private_message_topic.allowed_users.map(&:id)],
-          ['/private-messages/sent', [user.id]]
+          ['/private-messages/sent', [user.id]],
+          ["/private-messages/group/#{group.name}", [group.users.first.id]]
         ].each do |channel, user_ids|
           message = messages.find do |message|
             message.channel == channel

spec/requests/list_controller_spec.rb

@@ -2,6 +2,7 @@ require 'rails_helper'
 
 RSpec.describe ListController do
   let(:topic) { Fabricate(:topic) }
+  let(:group) { Fabricate(:group) }
 
   describe '#index' do
     it "doesn't throw an error with a negative page" do
@@ -109,9 +110,33 @@ RSpec.describe ListController do
     end
   end
 
-  describe '#group_topics' do
+  describe '#private_messages_group' do
-    let(:group) { Fabricate(:group) }
+    let(:user) do
+      user = Fabricate(:user)
+      group.add(user)
+      sign_in(user)
+      user
+    end
 
+    let!(:topic) do
+      Fabricate(:private_message_topic,
+        allowed_groups: [group],
+      )
+    end
+
+    let(:private_post) { Fabricate(:post, topic: topic) }
+
+    it 'should return the right response' do
+      get "/topics/private-messages-group/#{user.username}/#{group.name}.json"
+
+      expect(response.status).to eq(200)
+
+      expect(JSON.parse(response.body)["topic_list"]["topics"].first["id"])
+        .to eq(topic.id)
+    end
+  end
+
+  describe '#group_topics' do
     %i{user user2}.each do |user|
       let(user) do
         user = Fabricate(:user)

test/javascripts/acceptance/groups-test.js.es6

@@ -1,6 +1,29 @@
 import { acceptance, logIn } from "helpers/qunit-helpers";
 
-acceptance("Groups");
+const response = object => {
+  return [
+    200,
+    { "Content-Type": "application/json" },
+    object
+  ];
+};
+
+acceptance("Groups", {
+  beforeEach() {
+    server.get('/groups/snorlax.json', () => { // eslint-disable-line no-undef
+      return response({"basic_group":{"id":41,"automatic":false,"name":"snorlax","user_count":1,"alias_level":0,"visible":true,"automatic_membership_email_domains":"","automatic_membership_retroactive":false,"primary_group":true,"title":"Team Snorlax","grant_trust_level":null,"incoming_email":null,"has_messages":false,"flair_url":"","flair_bg_color":"","flair_color":"","bio_raw":"","bio_cooked":null,"public":true,"is_group_user":true,"is_group_owner":true}});
+    });
+
+    // Workaround while awaiting https://github.com/tildeio/route-recognizer/issues/53
+    server.get('/groups/snorlax/logs.json', request => { // eslint-disable-line no-undef
+      if (request.queryParams["filters[action]"]) {
+        return response({"logs":[{"action":"change_group_setting","subject":"title","prev_value":null,"new_value":"Team Snorlax","created_at":"2016-12-12T08:27:46.408Z","acting_user":{"id":1,"username":"tgx","avatar_template":"/images/avatar.png"},"target_user":null}],"all_loaded":true});
+      } else {
+        return response({"logs":[{"action":"change_group_setting","subject":"title","prev_value":null,"new_value":"Team Snorlax","created_at":"2016-12-12T08:27:46.408Z","acting_user":{"id":1,"username":"tgx","avatar_template":"/images/avatar.png"},"target_user":null},{"action":"add_user_to_group","subject":null,"prev_value":null,"new_value":null,"created_at":"2016-12-12T08:27:27.725Z","acting_user":{"id":1,"username":"tgx","avatar_template":"/images/avatar.png"},"target_user":{"id":1,"username":"tgx","avatar_template":"/images/avatar.png"}}],"all_loaded":true});
+      }
+    });
+  }
+});
 
 QUnit.test("Browsing Groups", assert => {
   visit("/groups");
@@ -49,6 +72,12 @@ QUnit.test("Anonymous Viewing Group", assert => {
     assert.ok(count('.avatar-flair .fa-adjust') === 1, "it displays the group's avatar flair");
     assert.ok(count('.group-members tr') > 0, "it lists group members");
     assert.ok(count('.group-message-button') === 0, 'it does not show group message button');
+
+    assert.equal(
+      count(".nav-pills li a[title='Messages']"),
+      0,
+      'it deos not show group messages navigation link'
+    );
   });
 
   click(".nav-pills li a[title='Activity']");
@@ -71,11 +100,6 @@ QUnit.test("Anonymous Viewing Group", assert => {
   });
 
   andThen(() => {
-    assert.equal(
-      find(".group-activity li a[href='/groups/discourse/activity/messages']").length,
-      0,
-      'it should not show messages tab if user is not a group user or admin'
-    );
     assert.ok(find(".nav-pills li a[title='Edit Group']").length === 0, 'it should not show messages tab if user is not admin');
     assert.ok(find(".nav-pills li a[title='Logs']").length === 0, 'it should not show Logs tab if user is not admin');
     assert.ok(count('.group-post') > 0, "it lists stream items");
@@ -124,6 +148,48 @@ QUnit.test("User Viewing Group", assert => {
   });
 });
 
+QUnit.test("Admin viewing group messages when there are no messages", assert => {
+  server.get('/topics/private-messages-group/eviltrout/discourse.json', () => { // eslint-disable-line no-undef
+    return response({ topic_list: { topics: [] } });
+  });
+
+  logIn();
+  Discourse.reset();
+
+  visit("/groups/discourse");
+
+  click(".nav-pills li a[title='Messages']");
+
+  andThen(() => {
+    assert.equal(
+      find(".alert").text().trim(),
+      I18n.t('choose_topic.none_found'),
+      'it should display the right alert'
+    );
+  });
+});
+
+QUnit.test("Admin viewing group messages", assert => {
+  server.get('/topics/private-messages-group/eviltrout/discourse.json', () => { // eslint-disable-line no-undef
+    return response({"users":[{"id":2, "username":"bruce1", "avatar_template":"/letter_avatar_proxy/v2/letter/b/9de053/{size}.png"}, {"id":3, "username":"CodingHorror", "avatar_template":"/letter_avatar_proxy/v2/letter/c/e8c25b/{size}.png"}], "primary_groups":[], "topic_list":{"can_create_topic":true, "draft":null, "draft_key":"new_topic", "draft_sequence":0, "per_page":30, "topics":[{"id":12199, "title":"This is a private message 1", "fancy_title":"This is a private message 1", "slug":"this-is-a-private-message-1", "posts_count":0, "reply_count":0, "highest_post_number":0, "image_url":null, "created_at":"2018-03-16T03:38:45.583Z", "last_posted_at":null, "bumped":true, "bumped_at":"2018-03-16T03:38:45.583Z", "unseen":false, "pinned":false, "unpinned":null, "visible":true, "closed":false, "archived":false, "bookmarked":null, "liked":null, "views":0, "like_count":0, "has_summary":false, "archetype":"private_message", "last_poster_username":"bruce1", "category_id":null, "pinned_globally":false, "featured_link":null, "posters":[{"extras":"latest single", "description":"Original Poster, Most Recent Poster", "user_id":2, "primary_group_id":null}], "participants":[{"extras":"latest", "description":null, "user_id":2, "primary_group_id":null}, {"extras":null, "description":null, "user_id":3, "primary_group_id":null}]}]}});
+  });
+
+  logIn();
+  Discourse.reset();
+
+  visit("/groups/discourse");
+
+  click(".nav-pills li a[title='Messages']");
+
+  andThen(() => {
+    assert.equal(
+      find(".topic-list-item .link-top-line").text().trim(),
+      "This is a private message 1",
+      'it should display the list of group topics'
+    );
+  });
+});
+
 QUnit.test("Admin Viewing Group", assert => {
   logIn();
   Discourse.reset();
@@ -136,14 +202,4 @@ QUnit.test("Admin Viewing Group", assert => {
     assert.equal(count('.group-message-button'), 1, 'it displays show group message button');
     assert.equal(find('.group-info-name').text(), 'Awesome Team', 'it should display the group name');
   });
-
-  click(".nav-pills li a[title='Activity']");
-
-  andThen(() => {
-    assert.equal(
-      find(".group-activity li a[href='/groups/discourse/activity/messages']").length,
-      1,
-      'it should show messages tab if user is admin'
-    );
-  });
 });