diff --git a/app/assets/javascripts/discourse/controllers/group-manage.js.es6 b/app/assets/javascripts/discourse/controllers/group-manage.js.es6 index 4979911de78..2c8ade4cefb 100644 --- a/app/assets/javascripts/discourse/controllers/group-manage.js.es6 +++ b/app/assets/javascripts/discourse/controllers/group-manage.js.es6 @@ -7,16 +7,13 @@ export default Ember.Controller.extend({ tabs(automatic) { const defaultTabs = [ { route: 'group.manage.profile', title: 'groups.manage.profile.title' }, + { route: 'group.manage.logs', title: 'groups.manage.logs.title' }, ]; if (!automatic) { - defaultTabs.push( + defaultTabs.splice(1, 0, { route: 'group.manage.members', title: 'groups.manage.members.title' } ); - - defaultTabs.push( - { route: 'group.manage.logs', title: 'groups.manage.logs.title' }, - ); } return defaultTabs; diff --git a/app/assets/javascripts/discourse/templates/components/group-form.hbs b/app/assets/javascripts/discourse/templates/components/group-form.hbs index 14107a9328a..5005305df1b 100644 --- a/app/assets/javascripts/discourse/templates/components/group-form.hbs +++ b/app/assets/javascripts/discourse/templates/components/group-form.hbs @@ -1,4 +1,4 @@ -
+ {{#if model.automatic}}
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index ada17605d0c..7281a1c2ba8 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -1,5 +1,4 @@ class GroupsController < ApplicationController - requires_login only: [ :set_notifications, :mentionable, @@ -389,7 +388,7 @@ class GroupsController < ApplicationController def histories group = find_group(:group_id) - guardian.ensure_can_edit!(group) + guardian.ensure_can_edit!(group) unless current_user.admin page_size = 25 offset = (params[:offset] && params[:offset].to_i) || 0 diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index c0d5fe11151..af8987bef3d 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/spec/requests/groups_controller_spec.rb @@ -454,6 +454,18 @@ describe GroupsController do expect(group.incoming_email).to eq("test@mail.org") expect(group.grant_trust_level).to eq(1) end + + it 'should not be allowed to update automatic groups' do + group = Group.find(Group::AUTO_GROUPS[:admins]) + + put "/groups/#{group.id}.json", params: { + group: { + messageable_level: 1 + } + } + + expect(response.status).to eq(403) + end end context "when user is group admin" do @@ -947,7 +959,7 @@ describe GroupsController do end end - describe "group histories" do + describe "#histories" do context 'when user is not signed in' do it 'should raise the right error' do get "/groups/#{group.name}/logs.json" @@ -967,18 +979,20 @@ describe GroupsController do end end - describe 'viewing history' do - context 'public group' do - before do - group.add_owner(user) + describe 'when user is a group owner' do + before do + group.add_owner(user) + sign_in(user) + end + describe 'when viewing a public group' do + before do group.update_attributes!( public_admission: true, public_exit: true ) GroupActionLogger.new(user, group).log_change_group_settings - sign_in(user) end it 'should allow group owner to view history' do @@ -995,40 +1009,56 @@ describe GroupsController do end end - context 'admin' do - let(:admin) { Fabricate(:admin) } + it 'should not be allowed to view history of an automatic group' do + group = Group.find_by(id: Group::AUTO_GROUPS[:admins]) - before do - sign_in(admin) - end + get "/groups/#{group.name}/logs.json" - it 'should be able to view history' do - GroupActionLogger.new(admin, group).log_remove_user_from_group(user) + expect(response.status).to eq(403) + end + end - get "/groups/#{group.name}/logs.json" + context 'when user is an admin' do + let(:admin) { Fabricate(:admin) } - expect(response).to be_success + before do + sign_in(admin) + end - result = JSON.parse(response.body)["logs"].first + it 'should be able to view history' do + GroupActionLogger.new(admin, group).log_remove_user_from_group(user) - expect(result["action"]).to eq(GroupHistory.actions[3].to_s) - end + get "/groups/#{group.name}/logs.json" - it 'should be able to filter through the history' do - GroupActionLogger.new(admin, group).log_add_user_to_group(user) - GroupActionLogger.new(admin, group).log_remove_user_from_group(user) + expect(response).to be_success - get "/groups/#{group.name}/logs.json", params: { - filters: { "action" => "add_user_to_group" } - } + result = JSON.parse(response.body)["logs"].first - expect(response).to be_success + expect(result["action"]).to eq(GroupHistory.actions[3].to_s) + end - logs = JSON.parse(response.body)["logs"] + it 'should be able to view history of automatic groups' do + group = Group.find_by(id: Group::AUTO_GROUPS[:admins]) - expect(logs.count).to eq(1) - expect(logs.first["action"]).to eq(GroupHistory.actions[2].to_s) - end + get "/groups/#{group.name}/logs.json" + + expect(response.status).to eq(200) + end + + it 'should be able to filter through the history' do + GroupActionLogger.new(admin, group).log_add_user_to_group(user) + GroupActionLogger.new(admin, group).log_remove_user_from_group(user) + + get "/groups/#{group.name}/logs.json", params: { + filters: { "action" => "add_user_to_group" } + } + + expect(response).to be_success + + logs = JSON.parse(response.body)["logs"] + + expect(logs.count).to eq(1) + expect(logs.first["action"]).to eq(GroupHistory.actions[2].to_s) end end end diff --git a/test/javascripts/acceptance/group-test.js.es6 b/test/javascripts/acceptance/group-test.js.es6 index 21ed7a9d97c..f4f68f9855c 100644 --- a/test/javascripts/acceptance/group-test.js.es6 +++ b/test/javascripts/acceptance/group-test.js.es6 @@ -56,6 +56,18 @@ QUnit.test("Anonymous Viewing Group", assert => { }); }); +QUnit.test("Anonymous Viewing Automatic Group", assert => { + visit("/groups/moderators"); + + andThen(() => { + assert.equal( + count(".nav-pills li a[title='Manage']"), + 0, + 'it deos not show group messages navigation link' + ); + }); +}); + QUnit.test("User Viewing Group", assert => { logIn(); Discourse.reset(); @@ -128,7 +140,6 @@ QUnit.test("Admin viewing group messages", assert => { Discourse.reset(); visit("/groups/discourse"); - click(".nav-pills li a[title='Messages']"); andThen(() => { @@ -156,3 +167,18 @@ QUnit.test("Admin Viewing Group", assert => { assert.equal(find('.group-info-name').text(), 'Awesome Team', 'it should display the group name'); }); }); + +QUnit.test("Admin Viewing Automatic Group", assert => { + logIn(); + Discourse.reset(); + + visit("/groups/moderators"); + click(".nav-pills li a[title='Manage']"); + + andThen(() => { + assert.equal( + count('.groups-form .control-group'), 5, + 'it should display the right fields' + ); + }); +}); diff --git a/test/javascripts/fixtures/group-fixtures.js.es6 b/test/javascripts/fixtures/group-fixtures.js.es6 index 44e616cb17f..2dacb930aad 100644 --- a/test/javascripts/fixtures/group-fixtures.js.es6 +++ b/test/javascripts/fixtures/group-fixtures.js.es6 @@ -1,4 +1,37 @@ export default { + "/groups/moderators":{ + "group": { + "id": 50, + "automatic": true, + "name": "moderators", + "display_name": "moderators", + "mentionable_level": 0, + "messageable_level": 99, + "visibility_level": 0, + "automatic_membership_email_domains": null, + "automatic_membership_retroactive": false, + "primary_group": false, + "title": null, + "grant_trust_level": null, + "incoming_email": null, + "has_messages": true, + "flair_url": null, + "flair_bg_color": null, + "flair_color": null, + "bio_raw": null, + "bio_cooked": null, + "public_admission": false, + "public_exit": false, + "allow_membership_requests": false, + "full_name": null, + "default_notification_level": 2, + "membership_request_template": null, + "is_group_user": true, + "is_group_owner": true, + "mentionable": false, + "messageable": true + }, + }, "/groups/discourse":{ "group":{ "id":47, diff --git a/test/javascripts/helpers/create-pretender.js.es6 b/test/javascripts/helpers/create-pretender.js.es6 index 541c1743206..678d4a782c5 100644 --- a/test/javascripts/helpers/create-pretender.js.es6 +++ b/test/javascripts/helpers/create-pretender.js.es6 @@ -308,6 +308,10 @@ export default function() { return response(200, fixturesByUrl['/groups/discourse/posts.json']); }); + this.get("/groups/moderators/members.json", () => { + return response(200, fixturesByUrl['/groups/discourse/members.json']); + }); + this.get('/t/:topic_id/posts.json', request => { const postIds = request.queryParams.post_ids; const posts = postIds.map(p => ({id: parseInt(p), post_number: parseInt(p) }));