FIX: Amend broken group automatic member dialog (#31854)

When creating or editing a group, we are meant to show a dialog telling the admin how many members will be automatically added.

app/assets/javascripts/discourse/app/components/group-manage-save-button.gjs

@@ -5,13 +5,13 @@ import { service } from "@ember/service";
 import { or } from "truth-helpers";
 import DButton from "discourse/components/d-button";
 import GroupDefaultNotificationsModal from "discourse/components/modal/group-default-notifications";
-import { popupAutomaticMembershipAlert } from "discourse/controllers/groups-new";
 import { popupAjaxError } from "discourse/lib/ajax-error";
 import discourseComputed from "discourse/lib/decorators";
 import { i18n } from "discourse-i18n";
 
 export default class GroupManageSaveButton extends Component {
   @service modal;
+  @service groupAutomaticMembersDialog;
 
   saving = null;
   disabled = false;
@@ -51,7 +51,7 @@ export default class GroupManageSaveButton extends Component {
   }
 
   @action
-  save() {
+  async save() {
     if (this.beforeSave) {
       this.beforeSave();
     }
@@ -59,11 +59,16 @@ export default class GroupManageSaveButton extends Component {
     this.set("saving", true);
     const group = this.model;
 
-    popupAutomaticMembershipAlert(
+    const accepted = await this.groupAutomaticMembersDialog.showConfirm(
       group.id,
       group.automatic_membership_email_domains
     );
 
+    if (!accepted) {
+      this.set("saving", false);
+      return;
+    }
+
     const opts = {};
     if (this.updateExistingUsers !== null) {
       opts.update_existing_users = this.updateExistingUsers;

app/assets/javascripts/discourse/app/controllers/groups-new.js

@@ -1,42 +1,12 @@
 import Controller from "@ember/controller";
 import { action } from "@ember/object";
 import { service } from "@ember/service";
-import { ajax } from "discourse/lib/ajax";
 import { popupAjaxError } from "discourse/lib/ajax-error";
 import discourseComputed from "discourse/lib/decorators";
-import { i18n } from "discourse-i18n";
-
-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) {
-      this.dialog.alert(
-        i18n("admin.groups.manage.membership.automatic_membership_user_count", {
-          count,
-        })
-      );
-    }
-  });
-}
 
 export default class GroupsNewController extends Controller {
-  @service dialog;
   @service router;
+  @service groupAutomaticMembersDialog;
 
   saving = null;
 
@@ -51,15 +21,20 @@ export default class GroupsNewController extends Controller {
   }
 
   @action
-  save() {
+  async save() {
     this.set("saving", true);
     const group = this.model;
 
-    popupAutomaticMembershipAlert(
+    const accepted = await this.groupAutomaticMembersDialog.showConfirm(
       group.id,
       group.automatic_membership_email_domains
     );
 
+    if (!accepted) {
+      this.set("saving", false);
+      return;
+    }
+
     group
       .create()
       .then(() => {

app/assets/javascripts/discourse/app/lib/constants.js

@@ -71,6 +71,8 @@ export const AUTO_GROUPS = {
 
 export const GROUP_SMTP_SSL_MODES = { none: 0, ssl_tls: 1, starttls: 2 };
 
+export const MAX_AUTO_MEMBERSHIP_DOMAINS_LOOKUP = 10;
+
 export const MAX_NOTIFICATIONS_LIMIT_PARAMS = 60;
 
 export const TOPIC_VISIBILITY_REASONS = {

app/assets/javascripts/discourse/app/services/group-automatic-members-dialog.js

@@ -0,0 +1,69 @@
+import Service, { service } from "@ember/service";
+import { ajax } from "discourse/lib/ajax";
+import { popupAjaxError } from "discourse/lib/ajax-error";
+import { MAX_AUTO_MEMBERSHIP_DOMAINS_LOOKUP } from "discourse/lib/constants";
+import { i18n } from "discourse-i18n";
+
+export default class GroupAutomaticMembersDialog extends Service {
+  @service dialog;
+
+  async showConfirm(group_id, email_domains) {
+    const domainCount = email_domains?.split("|")?.length ?? 0;
+
+    if (domainCount === 0) {
+      return Promise.resolve(true);
+    }
+
+    // On the back-end we compare every single user's e-mail to each e-mail
+    // domain by regular expression. At some point this is a but much work
+    // just to display this dialog, so go with a generic message instead.
+    if (domainCount > MAX_AUTO_MEMBERSHIP_DOMAINS_LOOKUP) {
+      return new Promise((resolve) => {
+        this.dialog.confirm({
+          message: i18n(
+            "admin.groups.manage.membership.automatic_membership_user_unknown_count"
+          ),
+          didConfirm: () => resolve(true),
+          didCancel: () => resolve(false),
+        });
+      });
+    }
+
+    const data = { automatic_membership_email_domains: email_domains };
+
+    if (group_id) {
+      data.id = group_id;
+    }
+
+    try {
+      const result = await ajax(
+        `/admin/groups/automatic_membership_count.json`,
+        {
+          type: "PUT",
+          data,
+        }
+      );
+
+      const count = result.user_count;
+
+      if (count > 0) {
+        return new Promise((resolve) => {
+          this.dialog.confirm({
+            message: i18n(
+              "admin.groups.manage.membership.automatic_membership_user_count",
+              {
+                count,
+              }
+            ),
+            didConfirm: () => resolve(true),
+            didCancel: () => resolve(false),
+          });
+        });
+      }
+
+      return Promise.resolve(true);
+    } catch (error) {
+      popupAjaxError(error);
+    }
+  }
+}

app/assets/javascripts/discourse/tests/acceptance/group-manage-email-settings-test.js

@@ -62,6 +62,11 @@ acceptance(
           success: "OK",
         });
       });
+      server.put("/admin/groups/automatic_membership_count.json", () => {
+        return helper.response({
+          user_count: 0,
+        });
+      });
     });
 
     test("enabling SMTP, testing, and saving", async function (assert) {

app/controllers/admin/groups_controller.rb

@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 class Admin::GroupsController < Admin::StaffController
+  MAX_AUTO_MEMBERSHIP_DOMAINS_LOOKUP = 10
+
   def create
     guardian.ensure_can_create_group!
 
@@ -94,6 +96,15 @@ class Admin::GroupsController < Admin::StaffController
         domains -= existing_domains
       end
 
+      if domains.size > MAX_AUTO_MEMBERSHIP_DOMAINS_LOOKUP
+        raise Discourse::InvalidParameters.new(
+                I18n.t(
+                  "groups.errors.counting_too_many_email_domains",
+                  count: MAX_AUTO_MEMBERSHIP_DOMAINS_LOOKUP,
+                ),
+              )
+      end
+
       user_count = Group.automatic_membership_users(domains.join("|")).count
     end
 

config/locales/client.en.yml

@@ -5652,6 +5652,7 @@ en:
             automatic_membership_user_count:
               one: "%{count} user has the new email domains and will be added to the group."
               other: "%{count} users have the new email domains and will be added to the group."
+            automatic_membership_user_unknown_count: "Users with one of the new email domains will be added to the group."
             automatic_membership_associated_groups: "Users who are members of a group on a service listed here will be automatically added to this group when they log in with the service."
             primary_group: "Automatically set as primary group"
           alert:

config/locales/server.en.yml

@@ -639,6 +639,7 @@ en:
         one: "Maximum %{count} user can be added at once"
         other: "Maximum %{count} users can be added at once"
       usernames_or_emails_required: "Usernames or emails must be present"
+      counting_too_many_email_domains: "Maximum %{count} email domains can be counted at once"
       no_invites_with_discourse_connect: "You can invite only registered users when DiscourseConnect is enabled"
       no_invites_without_local_logins: "You can invite only registered users when local logins are disabled"
     default_names:

lib/tasks/javascript.rake

@@ -156,6 +156,8 @@ task "javascript:update_constants" => :environment do
 
     export const GROUP_SMTP_SSL_MODES = #{Group.smtp_ssl_modes.to_json};
 
+    export const MAX_AUTO_MEMBERSHIP_DOMAINS_LOOKUP = #{Admin::GroupsController::MAX_AUTO_MEMBERSHIP_DOMAINS_LOOKUP};
+
     export const MAX_NOTIFICATIONS_LIMIT_PARAMS = #{NotificationsController::INDEX_LIMIT};
 
     export const TOPIC_VISIBILITY_REASONS = #{Topic.visibility_reasons.to_json};

spec/requests/admin/groups_controller_spec.rb

@@ -448,6 +448,18 @@ RSpec.describe Admin::GroupsController do
         expect(response.parsed_body["user_count"]).to eq(2)
       end
 
+      it "responds with a 400 for a long list of domains" do
+        put "/admin/groups/automatic_membership_count.json",
+            params: {
+              automatic_membership_email_domains: 1.upto(11).map { |n| "domain#{n}.com" }.join("|"),
+              id: group.id,
+            }
+        expect(response.status).to eq(400)
+        expect(response.parsed_body["errors"]).to contain_exactly(
+          "You supplied invalid parameters to the request: Maximum 10 email domains can be counted at once",
+        )
+      end
+
       it "doesn't respond with 500 if domain is invalid" do
         group = Fabricate(:group)
 

spec/system/groups/group_spec.rb

@@ -2,11 +2,67 @@
 
 describe "Group", type: :system do
   let(:group_page) { PageObjects::Pages::Group.new }
+  let(:group_index_page) { PageObjects::Pages::GroupIndex.new }
+  let(:group_form_page) { PageObjects::Pages::GroupForm.new }
+  let(:dialog) { PageObjects::Components::Dialog.new }
   fab!(:admin)
   fab!(:group)
 
   before { sign_in(admin) }
 
+  describe "create a group" do
+    context "when there are no existing users matching the auto e-mail domains" do
+      it "creates a new group" do
+        group_index_page.visit
+        group_index_page.click_new_group
+
+        group_form_page.fill_in("name", with: "illuminati")
+        group_form_page.fill_in("full_name", with: "The Illuminati")
+
+        expect(group_form_page).to have_css(".tip.good")
+
+        group_form_page.add_automatic_email_domain("illumi.net")
+        group_form_page.click_save
+
+        expect(page).to have_current_path("/g/illuminati")
+      end
+    end
+
+    context "when there are existing users matching the auto e-mail domains" do
+      before { Fabricate(:user, email: "ted@illumi.net") }
+
+      it "notifies about automatic members and creates a new group" do
+        group_index_page.visit
+        group_index_page.click_new_group
+
+        group_form_page.fill_in("name", with: "illuminati")
+        group_form_page.fill_in("full_name", with: "The Illuminati")
+
+        expect(group_form_page).to have_css(".tip.good")
+
+        group_form_page.add_automatic_email_domain("illumi.net")
+
+        group_form_page.click_save
+        expect(dialog).to be_open
+        expect(dialog).to have_content(
+          I18n.t(
+            "admin_js.admin.groups.manage.membership.automatic_membership_user_count",
+            count: 1,
+          ),
+        )
+
+        dialog.click_no
+        expect(page).to have_current_path("/g/custom/new")
+
+        group_form_page.click_save
+        expect(dialog).to be_open
+
+        dialog.click_yes
+        expect(page).to have_current_path("/g/illuminati")
+      end
+    end
+  end
+
   describe "delete a group" do
     it "redirects to groups index page" do
       group_page.visit(group)

spec/system/page_objects/pages/group_form.rb

@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+module PageObjects
+  module Pages
+    class GroupForm < PageObjects::Pages::Base
+      def add_automatic_email_domain(domain)
+        select_kit =
+          PageObjects::Components::SelectKit.new(".group-form-automatic-membership-automatic")
+        select_kit.expand
+        select_kit.search(domain)
+        select_kit.select_row_by_value(domain)
+        self
+      end
+
+      def click_save
+        page.find(".group-form-save").click
+        self
+      end
+
+      private
+
+      def automatic_email_domain_multi_select
+        page.find(".group-form-automatic-membership-automatic")
+      end
+    end
+  end
+end

spec/system/page_objects/pages/group_index.rb

@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+module PageObjects
+  module Pages
+    class GroupIndex < PageObjects::Pages::Base
+      def visit
+        page.visit("/groups")
+        self
+      end
+
+      def click_new_group
+        page.find(".groups-header-new").click
+        self
+      end
+    end
+  end
+end