FIX: Add `errors` field if group update confirmation (#16260)

* FIX: Redirect if Discourse-Xhr-Redirect is present `handleRedirect` was passed an wrong argument type (a string) instead of a jqXHR object and missed the fields checked in condition, thus always evaluating to `false`. * FIX: Add `errors` field if group update confirmation An explicit confirmation about the effect of the group update is required if the default notification level changes. Previously, if the confirmation was missing the API endpoint failed silently returning a 200 response code and a `user_count` field. This change ensures that a proper error code is returned (422), a descriptive error message and the additional information in the `user_count` field. This commit also refactors the API endpoint to use the `Discourse-Xhr-Redirect` header to redirect the user if the group is no longer visible.
2022-03-24 14:50:44 +02:00 · 2022-03-24 14:50:44 +02:00 · 03ad88f2c2
parent 771dddb711
commit 03ad88f2c2
5 changed files with 106 additions and 138 deletions
--- a/app/assets/javascripts/discourse/app/components/group-manage-save-button.js
+++ b/app/assets/javascripts/discourse/app/components/group-manage-save-button.js
@ -1,5 +1,4 @@
 import Component from "@ember/component";
-import DiscourseURL from "discourse/lib/url";
 import I18n from "I18n";
 import discourseComputed from "discourse-common/utils/decorators";
 import { popupAjaxError } from "discourse/lib/ajax-error";
@ -37,26 +36,7 @@ export default Component.extend({

      return group
        .save(opts)
-        .then((data) => {
-          if (data.user_count) {
-            const controller = showModal("group-default-notifications", {
-              model: {
-                count: data.user_count,
-              },
-            });
-
-            controller.set("onClose", () => {
-              this.updateExistingUsers = controller.updateExistingUsers;
-              this.send("save");
-            });
-
-            return;
-          }
-
-          if (data.route_to) {
-            DiscourseURL.routeTo(data.route_to);
-          }
-
+        .then(() => {
          this.setProperties({
            saved: true,
            updateExistingUsers: null,
@ -66,7 +46,21 @@ export default Component.extend({
            this.afterSave();
          }
        })
-        .catch(popupAjaxError)
+        .catch((error) => {
+          const json = error.jqXHR.responseJSON;
+          if (error.jqXHR.status === 422 && json.user_count) {
+            const controller = showModal("group-default-notifications", {
+              model: { count: json.user_count },
+            });
+
+            controller.set("onClose", () => {
+              this.updateExistingUsers = controller.updateExistingUsers;
+              this.send("save");
+            });
+          } else {
+            popupAjaxError(error);
+          }
+        })
        .finally(() => this.set("saving", false));
    },
  },
--- a/app/assets/javascripts/discourse/app/lib/ajax.js
+++ b/app/assets/javascripts/discourse/app/lib/ajax.js
@ -29,14 +29,9 @@ export function handleLogoff(xhr) {
  }
 }

-function handleRedirect(data) {
-  if (
-    data &&
-    data.getResponseHeader &&
-    data.getResponseHeader("Discourse-Xhr-Redirect")
-  ) {
-    window.location.replace(data.responseText);
-    window.location.reload();
+function handleRedirect(xhr) {
+  if (xhr && xhr.getResponseHeader("Discourse-Xhr-Redirect")) {
+    window.location = xhr.responseText;
  }
 }

@ -99,7 +94,7 @@ export function ajax() {
    }

    args.success = (data, textStatus, xhr) => {
-      handleRedirect(data);
+      handleRedirect(xhr);
      handleLogoff(xhr);

      run(() => {
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@ -260,7 +260,7 @@ class ApplicationController < ActionController::Base
    render json: { error: I18n.t(e.error_translation_key) }, status: e.status_code
  end

-  def redirect_with_client_support(url, options)
+  def redirect_with_client_support(url, options = {})
    if request.xhr?
      response.headers['Discourse-Xhr-Redirect'] = 'true'
      render plain: url
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@ -150,38 +150,32 @@ class GroupsController < ApplicationController

  def update
    group = Group.find(params[:id])
-    guardian.ensure_can_edit!(group) unless guardian.can_admin_group?(group)
+    guardian.ensure_can_edit!(group) if !guardian.can_admin_group?(group)

-    params_with_permitted = group_params(automatic: group.automatic)
-    clear_disabled_email_settings(group, params_with_permitted)
+    group_attributes = group_params(automatic: group.automatic)
+    reset_group_email_settings_if_disabled!(group, group_attributes)

    categories, tags = []
    if !group.automatic || current_user.admin
-      notification_level, categories, tags = user_default_notifications(group, params_with_permitted)
+      notification_level, categories, tags = user_default_notifications(group, group_attributes)

      if params[:update_existing_users].blank?
        user_count = count_existing_users(group.group_users, notification_level, categories, tags)
-
-        if user_count > 0
-          render json: { user_count: user_count }
-          return
-        end
+        return render status: 422, json: { user_count: user_count, errors: [I18n.t('invalid_params', message: :update_existing_users)] } if user_count > 0
      end
    end

-    if group.update(params_with_permitted)
+    if group.update(group_attributes)
      GroupActionLogger.new(current_user, group).log_change_group_settings
      group.record_email_setting_changes!(current_user)
      group.expire_imap_mailbox_cache
      update_existing_users(group.group_users, notification_level, categories, tags) if params[:update_existing_users] == "true"
      AdminDashboardData.clear_found_problem("group_#{group.id}_email_credentials")

-      if guardian.can_see?(group)
-        render json: success_json
-      else
-        # They can no longer see the group after changing permissions
-        render json: { route_to: '/g' }
-      end
+      # Redirect user to groups index page if they can no longer see the group
+      return redirect_with_client_support groups_path if !guardian.can_see?(group)
+
+      render json: success_json
    else
      render_json_error(group)
    end
@ -658,87 +652,74 @@ class GroupsController < ApplicationController
  end

  def group_params(automatic: false)
-    permitted_params =
-      if automatic
-        %i{
-          visibility_level
-          mentionable_level
-          messageable_level
-          default_notification_level
-          bio_raw
-          flair_icon
-          flair_upload_id
-          flair_bg_color
-          flair_color
-        }
-      else
-        default_params = %i{
-          mentionable_level
-          messageable_level
-          title
-          flair_icon
-          flair_upload_id
-          flair_bg_color
-          flair_color
-          bio_raw
-          public_admission
-          public_exit
-          allow_membership_requests
-          full_name
-          default_notification_level
-          membership_request_template
-        }
+    attributes = %i{
+      bio_raw
+      default_notification_level
+      messageable_level
+      mentionable_level
+      flair_bg_color
+      flair_color
+      flair_icon
+      flair_upload_id
+    }

-        if current_user.staff?
-          default_params.push(*[
-            :incoming_email,
-            :smtp_server,
-            :smtp_port,
-            :smtp_ssl,
-            :smtp_enabled,
-            :smtp_updated_by,
-            :smtp_updated_at,
-            :imap_server,
-            :imap_port,
-            :imap_ssl,
-            :imap_mailbox_name,
-            :imap_enabled,
-            :imap_updated_by,
-            :imap_updated_at,
-            :email_username,
-            :email_password,
-            :email_from_alias,
-            :primary_group,
-            :visibility_level,
-            :members_visibility_level,
-            :name,
-            :grant_trust_level,
-            :automatic_membership_email_domains,
-            :publish_read_state,
-            :allow_unknown_sender_topic_replies
-          ])
+    if automatic
+      attributes.push(:visibility_level)
+    else
+      attributes.push(
+        :title,
+        :allow_membership_requests,
+        :full_name,
+        :public_exit,
+        :public_admission,
+        :membership_request_template
+      )
+    end

-          custom_fields = DiscoursePluginRegistry.editable_group_custom_fields
-          default_params << { custom_fields: custom_fields } unless custom_fields.blank?
-        end
+    if !automatic && current_user.staff?
+      attributes.push(
+        :incoming_email,
+        :smtp_server,
+        :smtp_port,
+        :smtp_ssl,
+        :smtp_enabled,
+        :smtp_updated_by,
+        :smtp_updated_at,
+        :imap_server,
+        :imap_port,
+        :imap_ssl,
+        :imap_mailbox_name,
+        :imap_enabled,
+        :imap_updated_by,
+        :imap_updated_at,
+        :email_username,
+        :email_password,
+        :email_from_alias,
+        :primary_group,
+        :visibility_level,
+        :members_visibility_level,
+        :name,
+        :grant_trust_level,
+        :automatic_membership_email_domains,
+        :publish_read_state,
+        :allow_unknown_sender_topic_replies
+      )

-        default_params
-      end
+      custom_fields = DiscoursePluginRegistry.editable_group_custom_fields
+      attributes << { custom_fields: custom_fields } if custom_fields.present?
+    end

    if !automatic || current_user.admin
      [:muted, :regular, :tracking, :watching, :watching_first_post].each do |level|
-        permitted_params << { "#{level}_category_ids" => [] }
-        permitted_params << { "#{level}_tags" => [] }
+        attributes << { "#{level}_category_ids" => [] }
+        attributes << { "#{level}_tags" => [] }
      end
    end

-    if guardian.can_associate_groups?
-      permitted_params << { associated_group_ids: [] }
-    end
+    attributes << { associated_group_ids: [] } if guardian.can_associate_groups?
+    attributes.concat(DiscoursePluginRegistry.group_params)

-    permitted_params = permitted_params | DiscoursePluginRegistry.group_params
-
-    params.require(:group).permit(*permitted_params)
+    params.require(:group).permit(*attributes)
  end

  def find_group(param_name, ensure_can_see: true)
@ -767,23 +748,23 @@ class GroupsController < ApplicationController
    users
  end

-  def clear_disabled_email_settings(group, params_with_permitted)
-    should_clear_imap = group.imap_enabled && params_with_permitted.key?(:imap_enabled) && params_with_permitted[:imap_enabled] == "false"
-    should_clear_smtp = group.smtp_enabled && params_with_permitted.key?(:smtp_enabled) && params_with_permitted[:smtp_enabled] == "false"
+  def reset_group_email_settings_if_disabled!(group, attributes)
+    should_clear_imap = group.imap_enabled && attributes[:imap_enabled] == "false"
+    should_clear_smtp = group.smtp_enabled && attributes[:smtp_enabled] == "false"

    if should_clear_imap || should_clear_smtp
-      params_with_permitted[:imap_server] = nil
-      params_with_permitted[:imap_ssl] = false
-      params_with_permitted[:imap_port] = nil
-      params_with_permitted[:imap_mailbox_name] = ""
+      attributes[:imap_server] = nil
+      attributes[:imap_ssl] = false
+      attributes[:imap_port] = nil
+      attributes[:imap_mailbox_name] = ""
    end

    if should_clear_smtp
-      params_with_permitted[:smtp_server] = nil
-      params_with_permitted[:smtp_ssl] = false
-      params_with_permitted[:smtp_port] = nil
-      params_with_permitted[:email_username] = nil
-      params_with_permitted[:email_password] = nil
+      attributes[:smtp_server] = nil
+      attributes[:smtp_ssl] = false
+      attributes[:smtp_port] = nil
+      attributes[:email_username] = nil
+      attributes[:email_password] = nil
    end
  end

--- a/spec/requests/groups_controller_spec.rb
+++ b/spec/requests/groups_controller_spec.rb
@ -904,14 +904,12 @@ describe GroupsController do
            }
          }

-          expect(response.status).to eq(200)
-
+          expect(response.status).to eq(422)
+          expect(response.parsed_body["user_count"]).to eq(group.group_users.count)
+          expect(response.parsed_body["errors"].first).to include("update_existing_users")
          expect(group_user1.reload.notification_level).to eq(NotificationLevels.all[:watching])
          expect(group_user2.reload.notification_level).to eq(NotificationLevels.all[:watching])

-          group_users = group.group_users
-          expect(response.parsed_body["user_count"]).to eq(group_users.count)
-
          group_user1.update!(notification_level: NotificationLevels.all[:regular])

          put "/groups/#{group.id}.json", params: {
@ -920,7 +918,7 @@ describe GroupsController do
            }
          }

-          expect(response.status).to eq(200)
+          expect(response.status).to eq(422)
          expect(response.parsed_body["user_count"]).to eq(group.group_users.count - 1)
          expect(group_user1.reload.notification_level).to eq(NotificationLevels.all[:regular])
          expect(group_user2.reload.notification_level).to eq(NotificationLevels.all[:watching])
@ -973,7 +971,7 @@ describe GroupsController do
            }
          }

-          expect(response.status).to eq(200)
+          expect(response.status).to eq(422)
          expect(response.parsed_body["user_count"]).to eq(group.group_users.count - 1)

          put "/groups/#{group.id}.json", params: {