From ac6c1acbed7817f2769810abf8d8f47080799151 Mon Sep 17 00:00:00 2001
From: Guo Xiang Tan <tgx_world@hotmail.com>
Date: Fri, 2 Jun 2017 23:15:57 +0900
Subject: [PATCH] FIX: Groups that do not have any owners should not allow
membership requests.
---
app/models/group.rb | 7 +++++
config/locales/server.en.yml | 1 +
...735_fix_group_allow_membership_requests.rb | 13 ++++++++
.../admin/groups_controller_spec.rb | 30 +++++++++++++------
spec/models/group_spec.rb | 20 +++++++++++++
5 files changed, 62 insertions(+), 9 deletions(-)
create mode 100644 db/migrate/20170602132735_fix_group_allow_membership_requests.rb
diff --git a/app/models/group.rb b/app/models/group.rb
index dc4de2d1338..ecfab9ec29b 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -36,6 +36,7 @@ class Group < ActiveRecord::Base
validates_uniqueness_of :name, case_sensitive: false
validate :automatic_membership_email_domains_format_validator
validate :incoming_email_validator
+ validate :can_allow_membership_requests
validates :flair_url, url: true, if: Proc.new { |g| g.flair_url && g.flair_url[0,3] != 'fa-' }
AUTO_GROUPS = {
@@ -511,6 +512,12 @@ SQL
private
+ def can_allow_membership_requests
+ if self.allow_membership_requests && !self.group_users.where(owner: true).exists?
+ self.errors.add(:base, I18n.t('groups.errors.cant_allow_membership_requests'))
+ end
+ end
+
def enqueue_update_mentions_job
Jobs.enqueue(:update_group_mentions,
previous_name: self.name_was,
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index a1998eeb084..b46d1483cef 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -264,6 +264,7 @@ en:
invalid_incoming_email: "'%{email}' is not a valid email address."
email_already_used_in_group: "'%{email}' is already used by the group '%{group_name}'."
email_already_used_in_category: "'%{email}' is already used by the category '%{category_name}'."
+ cant_allow_membership_requests: "You cannot allow membership requests for a group without any owners."
default_names:
everyone: "everyone"
admins: "admins"
diff --git a/db/migrate/20170602132735_fix_group_allow_membership_requests.rb b/db/migrate/20170602132735_fix_group_allow_membership_requests.rb
new file mode 100644
index 00000000000..1f3a5ff803f
--- /dev/null
+++ b/db/migrate/20170602132735_fix_group_allow_membership_requests.rb
@@ -0,0 +1,13 @@
+class FixGroupAllowMembershipRequests < ActiveRecord::Migration
+ def up
+ execute <<~SQL
+ UPDATE groups g
+ SET allow_membership_requests = 'f'
+ WHERE NOT EXISTS (SELECT 1 FROM group_users gu WHERE gu.owner = 't' AND gu.group_id = g.id LIMIT 1)
+ SQL
+ end
+
+ def down
+ raise ActiveRecord::IrreversibleMigration
+ end
+end
diff --git a/spec/controllers/admin/groups_controller_spec.rb b/spec/controllers/admin/groups_controller_spec.rb
index bb7ebbf71a3..d24aaaa5e2b 100644
--- a/spec/controllers/admin/groups_controller_spec.rb
+++ b/spec/controllers/admin/groups_controller_spec.rb
@@ -1,6 +1,8 @@
require 'rails_helper'
describe Admin::GroupsController do
+ let(:user) { Fabricate(:user) }
+ let(:group) { Fabricate(:group) }
before do
@admin = log_in(:admin)
@@ -77,7 +79,7 @@ describe Admin::GroupsController do
end
end
- context ".create" do
+ context "#create" do
it "strip spaces on the group name" do
xhr :post, :create, { group: { name: " bob " } }
@@ -92,23 +94,33 @@ describe Admin::GroupsController do
end
- context ".update" do
+ context "#update" do
+ it 'should update a group' do
+ group.add_owner(user)
+
+ expect do
+ xhr :put, :update, { id: group.id, group: {
+ visible: "false",
+ allow_membership_requests: "true"
+ } }
+ end.to change { GroupHistory.count }.by(2)
+
+ expect(response).to be_success
+
+ group.reload
+ expect(group.visible).to eq(false)
+ expect(group.allow_membership_requests).to eq(true)
+ end
it "ignore name change on automatic group" do
expect do
- xhr :put, :update, { id: 1, group: {
- name: "WAT",
- visible: "true",
- allow_membership_requests: "true"
- } }
+ xhr :put, :update, { id: 1, group: { name: "WAT" } }
end.to change { GroupHistory.count }.by(1)
expect(response).to be_success
group = Group.find(1)
expect(group.name).not_to eq("WAT")
- expect(group.visible).to eq(true)
- expect(group.allow_membership_requests).to eq(true)
end
it "doesn't launch the 'automatic group membership' job when it's not retroactive" do
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 1455f098157..b066566c113 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -77,6 +77,26 @@ describe Group do
group.incoming_email = "foo@bar.org"
expect(group.valid?).to eq(true)
end
+
+ context 'when a group has no owners' do
+ it 'should not allow membership requests' do
+ group.allow_membership_requests = true
+
+ expect(group.valid?).to eq(false)
+
+ expect(group.errors.full_messages).to include(I18n.t(
+ "groups.errors.cant_allow_membership_requests"
+ ))
+
+ group.allow_membership_requests = false
+ group.save!
+
+ group.add_owner(user)
+ group.allow_membership_requests = true
+
+ expect(group.valid?).to eq(true)
+ end
+ end
end
def real_admins