diff --git a/app/assets/javascripts/discourse/components/group-manage-save-button.js b/app/assets/javascripts/discourse/components/group-manage-save-button.js index 9adcf9ea82f..31217a190fc 100644 --- a/app/assets/javascripts/discourse/components/group-manage-save-button.js +++ b/app/assets/javascripts/discourse/components/group-manage-save-button.js @@ -1,6 +1,7 @@ import discourseComputed from "discourse-common/utils/decorators"; import Component from "@ember/component"; import { popupAjaxError } from "discourse/lib/ajax-error"; +import { popupAutomaticMembershipAlert } from "discourse/controllers/groups-new"; export default Component.extend({ saving: null, @@ -14,8 +15,14 @@ export default Component.extend({ actions: { save() { this.set("saving", true); + const group = this.model; - return this.model + popupAutomaticMembershipAlert( + group.id, + group.automatic_membership_email_domains + ); + + return group .save() .then(() => { this.set("saved", true); diff --git a/app/assets/javascripts/discourse/controllers/groups-new.js b/app/assets/javascripts/discourse/controllers/groups-new.js index 05013cb7da3..25cefd43007 100644 --- a/app/assets/javascripts/discourse/controllers/groups-new.js +++ b/app/assets/javascripts/discourse/controllers/groups-new.js @@ -1,18 +1,54 @@ import { action } from "@ember/object"; import Controller from "@ember/controller"; +import { ajax } from "discourse/lib/ajax"; import { popupAjaxError } from "discourse/lib/ajax-error"; +export function popupAutomaticMembershipAlert(group_id, email_domains) { + if (!email_domains) { + return; + } + + const data = {}; + data.automatic_membership_email_domains = email_domains; + + if (group_id) { + data.id = group_id; + } + + ajax(`/admin/groups/automatic_membership_count.json`, { + type: "PUT", + data + }).then(result => { + const count = result.user_count; + + if (count > 0) { + bootbox.alert( + I18n.t( + "admin.groups.manage.membership.automatic_membership_user_count", + { count } + ) + ); + } + }); +} + export default Controller.extend({ saving: null, @action save() { this.set("saving", true); + const group = this.model; - this.model + popupAutomaticMembershipAlert( + group.id, + group.automatic_membership_email_domains + ); + + group .create() .then(() => { - this.transitionToRoute("group.members", this.model.name); + this.transitionToRoute("group.members", group.name); }) .catch(popupAjaxError) .finally(() => this.set("saving", false)); diff --git a/app/assets/javascripts/discourse/models/group.js b/app/assets/javascripts/discourse/models/group.js index 43e1b525bc1..ea4d03d0158 100644 --- a/app/assets/javascripts/discourse/models/group.js +++ b/app/assets/javascripts/discourse/models/group.js @@ -184,7 +184,6 @@ const Group = RestModel.extend({ visibility_level: this.visibility_level, members_visibility_level: this.members_visibility_level, automatic_membership_email_domains: this.emailDomains, - automatic_membership_retroactive: !!this.automatic_membership_retroactive, title: this.title, primary_group: !!this.primary_group, grant_trust_level: this.grant_trust_level, diff --git a/app/assets/javascripts/discourse/templates/components/groups-form-membership-fields.hbs b/app/assets/javascripts/discourse/templates/components/groups-form-membership-fields.hbs index b92c6787162..2b4fdfa9280 100644 --- a/app/assets/javascripts/discourse/templates/components/groups-form-membership-fields.hbs +++ b/app/assets/javascripts/discourse/templates/components/groups-form-membership-fields.hbs @@ -17,14 +17,6 @@ onChange=(action "onChangeEmailDomainsSetting") options=(hash allowAny=true) }} - - {{plugin-outlet name="groups-form-membership-below-automatic" diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index 05831871f1a..a8413ebefcd 100644 --- a/app/controllers/admin/groups_controller.rb +++ b/app/controllers/admin/groups_controller.rb @@ -121,6 +121,28 @@ class Admin::GroupsController < Admin::AdminController render json: success_json end + def automatic_membership_count + domains = Group.get_valid_email_domains(params.require(:automatic_membership_email_domains)) + group_id = params[:id] + user_count = 0 + + if domains.present? + if group_id.present? + group = Group.find_by(id: group_id) + raise Discourse::NotFound unless group + + return can_not_modify_automatic if group.automatic + + existing_domains = group.automatic_membership_email_domains.split("|") + domains -= existing_domains + end + + user_count = Group.automatic_membership_users(domains.join("|")).count + end + + render json: { user_count: user_count } + end + protected def can_not_modify_automatic @@ -137,7 +159,6 @@ class Admin::GroupsController < Admin::AdminController :visibility_level, :members_visibility_level, :automatic_membership_email_domains, - :automatic_membership_retroactive, :title, :primary_group, :grant_trust_level, diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 0df1f490120..c9a623d819c 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -537,7 +537,6 @@ class GroupsController < ApplicationController :name, :grant_trust_level, :automatic_membership_email_domains, - :automatic_membership_retroactive, :publish_read_state ]) diff --git a/app/jobs/regular/automatic_group_membership.rb b/app/jobs/regular/automatic_group_membership.rb index e10bc7a0136..bf9f6b213d3 100644 --- a/app/jobs/regular/automatic_group_membership.rb +++ b/app/jobs/regular/automatic_group_membership.rb @@ -11,16 +11,10 @@ module Jobs group = Group.find_by(id: group_id) raise Discourse::InvalidParameters.new(:group_id) if group.nil? - return unless group.automatic_membership_retroactive + domains = group.automatic_membership_email_domains + return if domains.blank? - domains = group.automatic_membership_email_domains.gsub('.', '\.') - - User.joins(:user_emails) - .where("user_emails.email ~* '@(#{domains})$'") - .where("users.id NOT IN (SELECT user_id FROM group_users WHERE group_users.group_id = ?)", group_id) - .activated - .where(staged: false) - .find_each do |user| + Group.automatic_membership_users(domains).find_each do |user| next unless user.email_confirmed? group.add(user, automatic: true) GroupActionLogger.new(Discourse.system_user, group).log_add_user_to_group(user) diff --git a/app/models/group.rb b/app/models/group.rb index 0f8f3d82d40..3e9db5180ce 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -770,12 +770,10 @@ class Group < ActiveRecord::Base def automatic_membership_email_domains_format_validator return if self.automatic_membership_email_domains.blank? - domains = self.automatic_membership_email_domains.split("|") - domains.each do |domain| - domain.sub!(/^https?:\/\//, '') - domain.sub!(/\/.*$/, '') + domains = Group.get_valid_email_domains(self.automatic_membership_email_domains) do |domain| self.errors.add :base, (I18n.t('groups.errors.invalid_domain', domain: domain)) unless domain =~ /\A[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,24}(:[0-9]{1,5})?(\/.*)?\Z/i end + self.automatic_membership_email_domains = domains.join("|") end @@ -791,7 +789,7 @@ class Group < ActiveRecord::Base end def automatic_group_membership - if self.automatic_membership_retroactive + if self.automatic_membership_email_domains.present? Jobs.enqueue(:automatic_group_membership, group_id: self.id) end end @@ -842,6 +840,32 @@ class Group < ActiveRecord::Base end end + def self.automatic_membership_users(domains, group_id = nil) + pattern = "@(#{domains.gsub('.', '\.')})$" + + users = User.joins(:user_emails).where("user_emails.email ~* ?", pattern).activated.where(staged: false) + users = users.where("users.id NOT IN (SELECT user_id FROM group_users WHERE group_users.group_id = ?)", group_id) if group_id.present? + + users + end + + def self.get_valid_email_domains(value) + valid_domains = [] + + value.split("|").each do |domain| + domain.sub!(/^https?:\/\//, '') + domain.sub!(/\/.*$/, '') + + if domain =~ /\A[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,24}(:[0-9]{1,5})?(\/.*)?\Z/i + valid_domains << domain + else + yield domain + end + end + + valid_domains + end + private def validate_grant_trust_level @@ -887,7 +911,6 @@ end # automatic :boolean default(FALSE), not null # user_count :integer default(0), not null # automatic_membership_email_domains :text -# automatic_membership_retroactive :boolean default(FALSE) # primary_group :boolean default(FALSE), not null # title :string # grant_trust_level :integer diff --git a/app/serializers/basic_group_serializer.rb b/app/serializers/basic_group_serializer.rb index df250920d8a..1b68420dd3c 100644 --- a/app/serializers/basic_group_serializer.rb +++ b/app/serializers/basic_group_serializer.rb @@ -10,7 +10,6 @@ class BasicGroupSerializer < ApplicationSerializer :messageable_level, :visibility_level, :automatic_membership_email_domains, - :automatic_membership_retroactive, :primary_group, :title, :grant_trust_level, @@ -56,10 +55,6 @@ class BasicGroupSerializer < ApplicationSerializer scope.is_admin? end - def include_automatic_membership_retroactive? - scope.is_admin? - end - def include_has_messages? staff? || scope.can_see_group_messages?(object) end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 4ccd3d397ac..94a3b84af2b 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -3460,7 +3460,7 @@ en: effects: Effects trust_levels_none: "None" automatic_membership_email_domains: "Users who register with an email domain that exactly matches one in this list will be automatically added to this group:" - automatic_membership_retroactive: "Apply the same email domain rule to add existing registered users" + automatic_membership_user_count: "%{count} users have the new email domains and will be added to the group." primary_group: "Automatically set as primary group" name_placeholder: "Group name, no spaces, same as username rule" primary: "Primary Group" diff --git a/config/routes.rb b/config/routes.rb index c98b6b3edba..e280af6dd1c 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -94,6 +94,7 @@ Discourse::Application.routes.draw do get 'bulk' get 'bulk-complete' => 'groups#bulk' put 'bulk' => 'groups#bulk_perform' + put "automatic_membership_count" => "groups#automatic_membership_count" end member do put "owners" => "groups#add_owners" diff --git a/db/post_migrate/20200415140830_drop_automatic_membership_retroactive_from_group.rb b/db/post_migrate/20200415140830_drop_automatic_membership_retroactive_from_group.rb new file mode 100644 index 00000000000..4e7b779d6bf --- /dev/null +++ b/db/post_migrate/20200415140830_drop_automatic_membership_retroactive_from_group.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class DropAutomaticMembershipRetroactiveFromGroup < ActiveRecord::Migration[6.0] + DROPPED_COLUMNS ||= { + groups: %i{ + automatic_membership_retroactive + } + } + + def up + DROPPED_COLUMNS.each do |table, columns| + Migration::ColumnDropper.execute_drop(table, columns) + end + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/spec/jobs/automatic_group_membership_spec.rb b/spec/jobs/automatic_group_membership_spec.rb index afac2b7f3fd..8a6611e52c9 100644 --- a/spec/jobs/automatic_group_membership_spec.rb +++ b/spec/jobs/automatic_group_membership_spec.rb @@ -20,7 +20,7 @@ describe Jobs::AutomaticGroupMembership do user6 = Fabricate(:user, email: "sso2@wat.com") user6.create_single_sign_on_record(external_id: 456, external_email: "sso2@wat.com", last_payload: "") - group = Fabricate(:group, automatic_membership_email_domains: "wat.com", automatic_membership_retroactive: true) + group = Fabricate(:group, automatic_membership_email_domains: "wat.com") begin automatic = nil diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 96e7f277576..0cb8362c804 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -942,27 +942,25 @@ describe Group do end describe '#automatic_group_membership' do - describe 'for a automatic_membership_retroactive group' do - let(:group) { Fabricate(:group, automatic_membership_retroactive: true) } + let(:group) { Fabricate(:group, automatic_membership_email_domains: "example.com") } - it "should be triggered on create and update" do - expect { group } - .to change { Jobs::AutomaticGroupMembership.jobs.size }.by(1) + it "should be triggered on create and update" do + expect { group } + .to change { Jobs::AutomaticGroupMembership.jobs.size }.by(1) - job = Jobs::AutomaticGroupMembership.jobs.last + job = Jobs::AutomaticGroupMembership.jobs.last - expect(job["args"].first["group_id"]).to eq(group.id) + expect(job["args"].first["group_id"]).to eq(group.id) - Jobs::AutomaticGroupMembership.jobs.clear + Jobs::AutomaticGroupMembership.jobs.clear - expect do - group.update!(name: 'asdiaksjdias') - end.to change { Jobs::AutomaticGroupMembership.jobs.size }.by(1) + expect do + group.update!(name: 'asdiaksjdias') + end.to change { Jobs::AutomaticGroupMembership.jobs.size }.by(1) - job = Jobs::AutomaticGroupMembership.jobs.last + job = Jobs::AutomaticGroupMembership.jobs.last - expect(job["args"].first["group_id"]).to eq(group.id) - end + expect(job["args"].first["group_id"]).to eq(group.id) end end diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index 0f1027edf3d..5edf1bcb6fc 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/spec/requests/groups_controller_spec.rb @@ -657,8 +657,7 @@ describe GroupsController do mentionable_level: 2, messageable_level: 2, default_notification_level: 0, - grant_trust_level: 0, - automatic_membership_retroactive: false + grant_trust_level: 0 ) expect do @@ -668,7 +667,6 @@ describe GroupsController do messageable_level: 1, visibility_level: 1, automatic_membership_email_domains: 'test.org', - automatic_membership_retroactive: true, title: 'haha', primary_group: true, grant_trust_level: 1, @@ -707,7 +705,6 @@ describe GroupsController do expect(group.messageable_level).to eq(1) expect(group.default_notification_level).to eq(1) expect(group.automatic_membership_email_domains).to eq(nil) - expect(group.automatic_membership_retroactive).to eq(false) expect(group.title).to eq('haha') expect(group.primary_group).to eq(false) expect(group.incoming_email).to eq(nil) @@ -737,7 +734,6 @@ describe GroupsController do group.update!( visibility_level: 2, members_visibility_level: 2, - automatic_membership_retroactive: false, grant_trust_level: 0 ) @@ -748,7 +744,6 @@ describe GroupsController do incoming_email: 'test@mail.org', primary_group: true, automatic_membership_email_domains: 'test.org', - automatic_membership_retroactive: true, grant_trust_level: 2, visibility_level: 1, members_visibility_level: 3 @@ -765,7 +760,6 @@ describe GroupsController do expect(group.visibility_level).to eq(1) expect(group.members_visibility_level).to eq(3) expect(group.automatic_membership_email_domains).to eq('test.org') - expect(group.automatic_membership_retroactive).to eq(true) expect(group.grant_trust_level).to eq(2) expect(Jobs::AutomaticGroupMembership.jobs.first["args"].first["group_id"]) diff --git a/spec/serializers/basic_group_serializer_spec.rb b/spec/serializers/basic_group_serializer_spec.rb index b905f5d4a12..7231e5d29ae 100644 --- a/spec/serializers/basic_group_serializer_spec.rb +++ b/spec/serializers/basic_group_serializer_spec.rb @@ -41,19 +41,17 @@ describe BasicGroupSerializer do end describe '#automatic_membership_email_domains' do - fab!(:group) { Fabricate(:group, automatic_membership_email_domains: 'ilovediscourse.com', automatic_membership_retroactive: true) } + fab!(:group) { Fabricate(:group, automatic_membership_email_domains: 'ilovediscourse.com') } let(:admin_guardian) { Guardian.new(Fabricate(:admin)) } it 'should include email domains for admin' do subject = described_class.new(group, scope: admin_guardian, root: false, owner_group_ids: [group.id]) expect(subject.as_json[:automatic_membership_email_domains]).to eq('ilovediscourse.com') - expect(subject.as_json[:automatic_membership_retroactive]).to eq(true) end it 'should not include email domains for other users' do subject = described_class.new(group, scope: guardian, root: false, owner_group_ids: [group.id]) expect(subject.as_json[:automatic_membership_email_domains]).to eq(nil) - expect(subject.as_json[:automatic_membership_retroactive]).to eq(nil) end end diff --git a/test/javascripts/acceptance/admin-user-index-test.js b/test/javascripts/acceptance/admin-user-index-test.js index 6ea2fd87f0a..4c57cbbf979 100644 --- a/test/javascripts/acceptance/admin-user-index-test.js +++ b/test/javascripts/acceptance/admin-user-index-test.js @@ -15,7 +15,6 @@ acceptance("Admin - User Index", { alias_level: 99, visible: true, automatic_membership_email_domains: "", - automatic_membership_retroactive: false, primary_group: false, title: null, grant_trust_level: null, @@ -38,7 +37,9 @@ acceptance("Admin - User Index", { QUnit.test("can edit username", async assert => { pretender.put("/users/sam/preferences/username", () => [ 200, - { "Content-Type": "application/json" }, + { + "Content-Type": "application/json" + }, { id: 2, username: "new-sam" } ]); diff --git a/test/javascripts/acceptance/group-manage-logs-test.js b/test/javascripts/acceptance/group-manage-logs-test.js index 1a3d6ada4af..f0587b3b2ee 100644 --- a/test/javascripts/acceptance/group-manage-logs-test.js +++ b/test/javascripts/acceptance/group-manage-logs-test.js @@ -13,7 +13,6 @@ acceptance("Group logs", { alias_level: 0, visible: true, automatic_membership_email_domains: "", - automatic_membership_retroactive: false, primary_group: true, title: "Team Snorlax", grant_trust_level: null, diff --git a/test/javascripts/acceptance/group-manage-membership-test.js b/test/javascripts/acceptance/group-manage-membership-test.js index e43678a7c8b..c7837a3ec5d 100644 --- a/test/javascripts/acceptance/group-manage-membership-test.js +++ b/test/javascripts/acceptance/group-manage-membership-test.js @@ -13,11 +13,6 @@ QUnit.test("As an admin", async assert => { "it should display automatic membership label" ); - assert.ok( - find(".groups-form-automatic-membership-retroactive").length === 1, - "it should display automatic membership retroactive checkbox" - ); - assert.ok( find(".groups-form-primary-group").length === 1, "it should display set as primary group checkbox" diff --git a/test/javascripts/acceptance/group-requests-test.js b/test/javascripts/acceptance/group-requests-test.js index 68ccf7f867c..9c4e9a8a72a 100644 --- a/test/javascripts/acceptance/group-requests-test.js +++ b/test/javascripts/acceptance/group-requests-test.js @@ -17,7 +17,6 @@ acceptance("Group Requests", { messageable_level: 0, visibility_level: 0, automatic_membership_email_domains: "", - automatic_membership_retroactive: false, primary_group: false, title: "Macdonald", grant_trust_level: null, diff --git a/test/javascripts/acceptance/search-full-test.js b/test/javascripts/acceptance/search-full-test.js index e961e8015a2..d869af65a32 100644 --- a/test/javascripts/acceptance/search-full-test.js +++ b/test/javascripts/acceptance/search-full-test.js @@ -30,7 +30,6 @@ acceptance("Search - Full Page", { alias_level: 0, visible: true, automatic_membership_email_domains: null, - automatic_membership_retroactive: false, primary_group: false, title: null, grant_trust_level: null, diff --git a/test/javascripts/fixtures/group-fixtures.js b/test/javascripts/fixtures/group-fixtures.js index 89842f3245a..b509e79cd40 100644 --- a/test/javascripts/fixtures/group-fixtures.js +++ b/test/javascripts/fixtures/group-fixtures.js @@ -9,7 +9,6 @@ export default { messageable_level: 99, visibility_level: 0, automatic_membership_email_domains: null, - automatic_membership_retroactive: false, primary_group: false, title: null, grant_trust_level: null, diff --git a/test/javascripts/fixtures/groups-fixtures.js b/test/javascripts/fixtures/groups-fixtures.js index 46d6511dd22..b6c020ee936 100644 --- a/test/javascripts/fixtures/groups-fixtures.js +++ b/test/javascripts/fixtures/groups-fixtures.js @@ -9,7 +9,6 @@ export default { alias_level: 0, visible: true, automatic_membership_email_domains: "", - automatic_membership_retroactive: false, primary_group: false, title: null, grant_trust_level: null, @@ -31,7 +30,6 @@ export default { alias_level: 99, visible: true, automatic_membership_email_domains: "", - automatic_membership_retroactive: false, primary_group: false, title: null, grant_trust_level: null, @@ -61,7 +59,6 @@ export default { alias_level: 0, visible: true, automatic_membership_email_domains: "", - automatic_membership_retroactive: false, primary_group: false, title: null, grant_trust_level: null, diff --git a/test/javascripts/fixtures/topic.js b/test/javascripts/fixtures/topic.js index dedf9814b83..b520532febb 100644 --- a/test/javascripts/fixtures/topic.js +++ b/test/javascripts/fixtures/topic.js @@ -4060,7 +4060,6 @@ export default { alias_level: 99, visible: true, automatic_membership_email_domains: "", - automatic_membership_retroactive: false, primary_group: false, title: null, grant_trust_level: null, diff --git a/test/javascripts/fixtures/user_fixtures.js b/test/javascripts/fixtures/user_fixtures.js index b22180109da..2bac0bfe343 100644 --- a/test/javascripts/fixtures/user_fixtures.js +++ b/test/javascripts/fixtures/user_fixtures.js @@ -204,7 +204,6 @@ export default { alias_level: 0, visible: true, automatic_membership_email_domains: null, - automatic_membership_retroactive: false, primary_group: false, title: null }, @@ -216,7 +215,6 @@ export default { alias_level: 0, visible: true, automatic_membership_email_domains: null, - automatic_membership_retroactive: false, primary_group: false, title: null } @@ -2250,7 +2248,11 @@ export default { { extras: null, description: "Most Posts", user_id: 5460 }, { extras: null, description: "Frequent Poster", user_id: 402 }, { extras: null, description: "Frequent Poster", user_id: 5707 }, - { extras: "latest", description: "Most Recent Poster", user_id: 32 } + { + extras: "latest", + description: "Most Recent Poster", + user_id: 32 + } ] }, { @@ -2539,7 +2541,6 @@ export default { messageable_level: 0, visibility_level: 0, automatic_membership_email_domains: null, - automatic_membership_retroactive: false, primary_group: false, title: null, grant_trust_level: null, @@ -2567,7 +2568,6 @@ export default { messageable_level: 0, visibility_level: 0, automatic_membership_email_domains: null, - automatic_membership_retroactive: false, primary_group: false, title: null, grant_trust_level: null, @@ -2595,7 +2595,6 @@ export default { messageable_level: 0, visibility_level: 0, automatic_membership_email_domains: null, - automatic_membership_retroactive: false, primary_group: false, title: null, grant_trust_level: null, @@ -2623,7 +2622,6 @@ export default { messageable_level: 0, visibility_level: 0, automatic_membership_email_domains: null, - automatic_membership_retroactive: false, primary_group: false, title: null, grant_trust_level: null,