From a35227918f733ea85a75c07c304ea3e390f3dc3d Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Wed, 14 Mar 2018 19:40:28 +0800 Subject: [PATCH] UX: Display group topics in a topic list. --- .../controllers/group-activity-topics.js.es6 | 7 ++ .../routes/group-activity-topics.js.es6 | 12 ++- .../templates/group-activity-topics.hbs | 5 + app/controllers/groups_controller.rb | 9 -- app/controllers/list_controller.rb | 13 +++ config/routes.rb | 2 +- lib/topic_query.rb | 10 ++ spec/components/topic_query_spec.rb | 43 +++++++- spec/requests/list_controller_spec.rb | 79 +++++++++++++- .../javascripts/acceptance/groups-test.js.es6 | 3 +- .../fixtures/group-fixtures.js.es6 | 100 ++++++++++++++++++ .../helpers/create-pretender.js.es6 | 4 +- 12 files changed, 269 insertions(+), 18 deletions(-) create mode 100644 app/assets/javascripts/discourse/controllers/group-activity-topics.js.es6 create mode 100644 app/assets/javascripts/discourse/templates/group-activity-topics.hbs diff --git a/app/assets/javascripts/discourse/controllers/group-activity-topics.js.es6 b/app/assets/javascripts/discourse/controllers/group-activity-topics.js.es6 new file mode 100644 index 00000000000..e538cb9c77f --- /dev/null +++ b/app/assets/javascripts/discourse/controllers/group-activity-topics.js.es6 @@ -0,0 +1,7 @@ +export default Ember.Controller.extend({ + actions: { + loadMore() { + this.get('model').loadMore(); + } + } +}); diff --git a/app/assets/javascripts/discourse/routes/group-activity-topics.js.es6 b/app/assets/javascripts/discourse/routes/group-activity-topics.js.es6 index 16164a51bb3..5310f0cad65 100644 --- a/app/assets/javascripts/discourse/routes/group-activity-topics.js.es6 +++ b/app/assets/javascripts/discourse/routes/group-activity-topics.js.es6 @@ -1,3 +1,11 @@ -import { buildGroupPage } from 'discourse/routes/group-activity-posts'; +export default Discourse.Route.extend({ + titleToken() { + return I18n.t(`groups.topics`); + }, -export default buildGroupPage('topics'); + model() { + return this.store.findFiltered("topicList", { + filter: `topics/groups/${this.modelFor('group').get('name')}` + }); + } +}); diff --git a/app/assets/javascripts/discourse/templates/group-activity-topics.hbs b/app/assets/javascripts/discourse/templates/group-activity-topics.hbs new file mode 100644 index 00000000000..7e87f59d094 --- /dev/null +++ b/app/assets/javascripts/discourse/templates/group-activity-topics.hbs @@ -0,0 +1,5 @@ +{{#load-more class="paginated-topics-list" selector=".paginated-topics-list .topic-list tr" action="loadMore"}} + {{basic-topic-list topicList=model + showPosters=true}} + {{conditional-loading-spinner condition=model.loadingMore}} +{{/load-more}} diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 7ba55917fee..f4881391381 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -101,15 +101,6 @@ class GroupsController < ApplicationController render 'posts/latest', formats: [:rss] end - def topics - group = find_group(:group_id) - posts = group.posts_for( - guardian, - params.permit(:before_post_id, :category_id) - ).where(post_number: 1).limit(20) - render_serialized posts.to_a, GroupPostSerializer - end - def mentions raise Discourse::NotFound unless SiteSetting.enable_mentions? group = find_group(:group_id) diff --git a/app/controllers/list_controller.rb b/app/controllers/list_controller.rb index ba7946b1d32..e267de7bd8a 100644 --- a/app/controllers/list_controller.rb +++ b/app/controllers/list_controller.rb @@ -50,6 +50,7 @@ class ListController < ApplicationController TopTopic.periods.map { |p| :"category_top_#{p}" }, TopTopic.periods.map { |p| :"category_none_top_#{p}" }, TopTopic.periods.map { |p| :"parent_category_category_top_#{p}" }, + :group_topics ].flatten # Create our filters @@ -131,6 +132,18 @@ class ListController < ApplicationController respond_with_list(list) end + def group_topics + group = Group.find_by(name: params[:group_name]) + raise Discourse::NotFound unless group + guardian.ensure_can_see_group!(group) + + list_opts = build_topic_list_options + list = generate_list_for("group_topics", group, list_opts) + list.more_topics_url = url_for(construct_url_with(:next, list_opts)) + list.prev_topics_url = url_for(construct_url_with(:prev, list_opts)) + respond_with_list(list) + end + def self.generate_message_route(action) define_method("#{action}") do list_opts = build_topic_list_options diff --git a/config/routes.rb b/config/routes.rb index e02bd44f2b5..bc091d4a9f5 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -455,7 +455,6 @@ Discourse::Application.routes.draw do get 'members' get 'posts' - get 'topics' get 'mentions' get 'messages' get 'counts' @@ -608,6 +607,7 @@ Discourse::Application.routes.draw do get "private-messages-archive/:username" => "list#private_messages_archive", as: "topics_private_messages_archive" get "private-messages-unread/:username" => "list#private_messages_unread", as: "topics_private_messages_unread" get "private-messages-tags/:username/:tag_id.json" => "list#private_messages_tag", as: "topics_private_messages_tag", constraints: StaffConstraint.new + get "groups/:group_name.json" => "list#group_topics", as: "group_topics" scope "/private-messages-group/:username", group_name: RouteFormat.username do get ":group_name.json" => "list#private_messages_group", as: "topics_private_messages_group" diff --git a/lib/topic_query.rb b/lib/topic_query.rb index ee65210573e..35311ba0a85 100644 --- a/lib/topic_query.rb +++ b/lib/topic_query.rb @@ -220,6 +220,16 @@ class TopicQuery .where('um.user_id IS NULL') end + def list_group_topics(group) + list = default_results.where(" + topics.user_id IN ( + SELECT user_id FROM group_users gu WHERE gu.group_id = #{group.id.to_i} + ) + ") + + create_list(:group_topics, {}, list) + end + def list_private_messages(user) list = private_messages_for(user, :user) diff --git a/spec/components/topic_query_spec.rb b/spec/components/topic_query_spec.rb index 47bf9f4c23c..98f162f77b9 100644 --- a/spec/components/topic_query_spec.rb +++ b/spec/components/topic_query_spec.rb @@ -885,10 +885,49 @@ describe TopicQuery do expect(suggested_topics[1, 3]).to include(closed_topic.id) expect(suggested_topics[1, 3]).to include(archived_topic.id) end - end end - end + describe '#list_group_topics' do + let(:group) { Fabricate(:group) } + + let(:user) do + user = Fabricate(:user) + group.add(user) + user + end + + let(:user2) do + user = Fabricate(:user) + group.add(user) + user + end + + let(:private_category) do + Fabricate(:private_category, group: group) + end + + let!(:private_message_topic) { Fabricate(:private_message_post, user: user).topic } + let!(:topic1) { Fabricate(:topic, user: user) } + let!(:topic2) { Fabricate(:topic, user: user, category: Fabricate(:category)) } + let!(:topic3) { Fabricate(:topic, user: user, category: private_category) } + let!(:topic4) { Fabricate(:topic) } + let!(:topic5) { Fabricate(:topic, user: user, visible: false) } + let!(:topic6) { Fabricate(:topic, user: user2) } + + it 'should return the right list' do + topics = TopicQuery.new.list_group_topics(group).topics + + expect(topics).to contain_exactly(topic1, topic2, topic6) + + topics = TopicQuery.new(user).list_group_topics(group).topics + + expect(topics).to contain_exactly(topic1, topic2, topic3, topic6) + + topics = TopicQuery.new(user2).list_group_topics(group).topics + + expect(topics).to contain_exactly(topic1, topic2, topic3, topic6) + end + end end diff --git a/spec/requests/list_controller_spec.rb b/spec/requests/list_controller_spec.rb index 9456e46fbff..f4862827deb 100644 --- a/spec/requests/list_controller_spec.rb +++ b/spec/requests/list_controller_spec.rb @@ -104,7 +104,84 @@ RSpec.describe ListController do [moderator, admin].each do |user| sign_in(user) get "/topics/private-messages-tags/#{user.username}/#{tag.name}.json" - expect(response).to be_success + expect(response.status).to eq(200) + end + end + end + + describe '#group_topics' do + let(:group) { Fabricate(:group) } + + %i{user user2}.each do |user| + let(user) do + user = Fabricate(:user) + group.add(user) + user + end + end + + let!(:topic) { Fabricate(:topic, user: user) } + let!(:topic2) { Fabricate(:topic, user: user2) } + let!(:another_topic) { Fabricate(:topic) } + + describe 'when an invalid group name is given' do + it 'should return the right response' do + get "/topics/groups/something.json" + + expect(response.status).to eq(404) + end + end + + describe 'for an anon user' do + describe 'public visible group' do + it 'should return the right response' do + get "/topics/groups/#{group.name}.json" + + expect(response.status).to eq(200) + expect(JSON.parse(response.body)["topic_list"]).to be_present + end + end + + describe 'restricted group' do + before { group.update!(visibility_level: Group.visibility_levels[:staff]) } + + it 'should return the right response' do + get "/topics/groups/#{group.name}.json" + + expect(response.status).to eq(403) + end + end + end + + describe 'for a normal user' do + before { sign_in(Fabricate(:user)) } + + describe 'restricted group' do + before { group.update!(visibility_level: Group.visibility_levels[:staff]) } + + it 'should return the right response' do + get "/topics/groups/#{group.name}.json" + + expect(response.status).to eq(403) + end + end + end + + describe 'for a group user' do + before do + sign_in(user) + end + + it 'should be able to view the topics started by group users' do + get "/topics/groups/#{group.name}.json" + + expect(response.status).to eq(200) + + topics = JSON.parse(response.body)["topic_list"]["topics"] + + expect(topics.map { |topic| topic["id"] }).to contain_exactly( + topic.id, topic2.id + ) end end end diff --git a/test/javascripts/acceptance/groups-test.js.es6 b/test/javascripts/acceptance/groups-test.js.es6 index 4e098f31694..12298b6882e 100644 --- a/test/javascripts/acceptance/groups-test.js.es6 +++ b/test/javascripts/acceptance/groups-test.js.es6 @@ -60,7 +60,8 @@ QUnit.test("Anonymous Viewing Group", assert => { click(".group-activity-nav li a[href='/groups/discourse/activity/topics']"); andThen(() => { - assert.ok(count('.group-post') > 0, "it lists stream items"); + assert.ok(find('.topic-list'), "it shows the topic list"); + assert.equal(count('.topic-list-item'), 2, "it lists stream items"); }); click(".group-activity-nav li a[href='/groups/discourse/activity/mentions']"); diff --git a/test/javascripts/fixtures/group-fixtures.js.es6 b/test/javascripts/fixtures/group-fixtures.js.es6 index 07cb5ef9f5f..27a4a6bbf15 100644 --- a/test/javascripts/fixtures/group-fixtures.js.es6 +++ b/test/javascripts/fixtures/group-fixtures.js.es6 @@ -16,6 +16,106 @@ export default { "messageable":true } }, + "/topics/groups/discourse.json":{ + "users":[ + { + "id":2, + "username":"bruce1", + "avatar_template":"/letter_avatar_proxy/v2/letter/b/9de053/{size}.png" + }, + { + "id":1, + "username":"bruce0", + "avatar_template":"/letter_avatar_proxy/v2/letter/b/90ced4/{size}.png" + } + ], + "primary_groups":[], + "topic_list":{ + "can_create_topic":true, + "draft":null, + "draft_key":"new_topic", + "draft_sequence":1, + "per_page":30, + "topics":[ + { + "id":12074, + "title":"This is a test topic 1", + "fancy_title":"This is a test topic 1", + "slug":"this-is-a-test-topic-1", + "posts_count":0, + "reply_count":0, + "highest_post_number":0, + "image_url":null, + "created_at":"2018-03-15T03:12:48.955Z", + "last_posted_at":null, + "bumped":true, + "bumped_at":"2018-03-15T03:12:48.955Z", + "unseen":true, + "pinned":false, + "unpinned":null, + "visible":true, + "closed":false, + "archived":false, + "bookmarked":null, + "liked":null, + "views":0, + "like_count":0, + "has_summary":false, + "archetype":"regular", + "last_poster_username":"bruce1", + "category_id":1, + "pinned_globally":false, + "featured_link":null, + "posters":[ + { + "extras":"latest single", + "description":"Original Poster, Most Recent Poster", + "user_id":2, + "primary_group_id":null + } + ] + }, + { + "id":12073, + "title":"This is a test topic 0", + "fancy_title":"This is a test topic 0", + "slug":"this-is-a-test-topic-0", + "posts_count":0, + "reply_count":0, + "highest_post_number":0, + "image_url":null, + "created_at":"2018-03-15T03:12:48.899Z", + "last_posted_at":null, + "bumped":true, + "bumped_at":"2018-03-15T03:12:48.900Z", + "unseen":true, + "pinned":false, + "unpinned":null, + "visible":true, + "closed":false, + "archived":false, + "bookmarked":null, + "liked":null, + "views":0, + "like_count":0, + "has_summary":false, + "archetype":"regular", + "last_poster_username":"bruce0", + "category_id":1, + "pinned_globally":false, + "featured_link":null, + "posters":[ + { + "extras":"latest single", + "description":"Original Poster, Most Recent Poster", + "user_id":1, + "primary_group_id":null + } + ] + } + ] + } + }, "/groups/discourse/counts.json":{ "counts":{ "posts":17829, diff --git a/test/javascripts/helpers/create-pretender.js.es6 b/test/javascripts/helpers/create-pretender.js.es6 index 28e2607b21b..30d0e2a6ef7 100644 --- a/test/javascripts/helpers/create-pretender.js.es6 +++ b/test/javascripts/helpers/create-pretender.js.es6 @@ -295,8 +295,8 @@ export default function() { return response(200, []); }); - this.get("/groups/discourse/topics.json", () => { - return response(200, fixturesByUrl['/groups/discourse/posts.json']); + this.get("/topics/groups/discourse.json", () => { + return response(200, fixturesByUrl['/topics/groups/discourse.json']); }); this.get("/groups/discourse/mentions.json", () => {