UX: drop the `automatic_membership_retroactive` column from groups model. (#9430)

2020-04-22 22:07:39 +05:30 · 2020-04-22 22:07:39 +05:30 · df0c386f8a
parent bf0c055a9a
commit df0c386f8a
25 changed files with 144 additions and 82 deletions
--- 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);
--- 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));
--- 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,
--- 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)
    }}
-
-    <label>
-      {{input type="checkbox"
-          checked=model.automatic_membership_retroactive
-          class="groups-form-automatic-membership-retroactive"}}
-
-      {{i18n "admin.groups.manage.membership.automatic_membership_retroactive"}}
-    </label>
  </div>

  {{plugin-outlet name="groups-form-membership-below-automatic"
--- 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,
--- 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
          ])

--- 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)
--- 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
--- 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
--- 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"
--- 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"
--- a/db/post_migrate/20200415140830_drop_automatic_membership_retroactive_from_group.rb
+++ 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
--- 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
--- 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

--- 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"])
--- 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
--- 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" }
  ]);

--- 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,
--- 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"
--- 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,
--- 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,
--- 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,
--- 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,
--- 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,
--- 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,