From dde66b9e168c0568955b08696a63c3fc0ec7b322 Mon Sep 17 00:00:00 2001
From: Bianca Nenciu <nbianca@users.noreply.github.com>
Date: Tue, 14 Sep 2021 15:04:54 +0300
Subject: [PATCH] FIX: Update only present fields in request (#14310)

Some category fields were always updated, even if they were not present
in the request. When this happened, these field were erased.
---
 .../discourse/app/models/category.js          | 10 ++++--
 app/controllers/categories_controller.rb      | 16 +++++----
 spec/requests/categories_controller_spec.rb   | 36 ++++++++++++++++++-
 3 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/app/assets/javascripts/discourse/app/models/category.js b/app/assets/javascripts/discourse/app/models/category.js
index 940bb5ba39e..c2694e88fa6 100644
--- a/app/assets/javascripts/discourse/app/models/category.js
+++ b/app/assets/javascripts/discourse/app/models/category.js
@@ -212,8 +212,14 @@ const Category = RestModel.extend({
         all_topics_wiki: this.all_topics_wiki,
         allow_unlimited_owner_edits_on_first_post: this
           .allow_unlimited_owner_edits_on_first_post,
-        allowed_tags: this.allowed_tags,
-        allowed_tag_groups: this.allowed_tag_groups,
+        allowed_tags:
+          this.allowed_tags && this.allowed_tags.length > 0
+            ? this.allowed_tags
+            : null,
+        allowed_tag_groups:
+          this.allowed_tag_groups && this.allowed_tag_groups.length > 0
+            ? this.allowed_tag_groups
+            : null,
         allow_global_tags: this.allow_global_tags,
         required_tag_group_name: this.required_tag_groups
           ? this.required_tag_groups[0]
diff --git a/app/controllers/categories_controller.rb b/app/controllers/categories_controller.rb
index c22d6be7e31..153f84e68c7 100644
--- a/app/controllers/categories_controller.rb
+++ b/app/controllers/categories_controller.rb
@@ -286,9 +286,13 @@ class CategoriesController < ApplicationController
       end
 
       if SiteSetting.tagging_enabled
-        params[:allowed_tags] ||= []
-        params[:allowed_tag_groups] ||= []
-        params[:required_tag_group_name] ||= ''
+        params[:allowed_tags] = params[:allowed_tags].presence || [] if params[:allowed_tags]
+        params[:allowed_tag_groups] = params[:allowed_tag_groups].presence || [] if params[:allowed_tag_groups]
+        params[:required_tag_group_name] = params[:required_tag_group_name].presence || '' if params[:required_tag_group_name]
+      end
+
+      if SiteSetting.enable_category_group_moderation?
+        params[:reviewable_by_group_id] = Group.where(name: params[:reviewable_by_group_name]).pluck_first(:id) if params[:reviewable_by_group_name]
       end
 
       result = params.permit(
@@ -327,14 +331,12 @@ class CategoriesController < ApplicationController
         :min_tags_from_required_group,
         :read_only_banner,
         :default_list_filter,
+        :reviewable_by_group_id,
         custom_fields: [params[:custom_fields].try(:keys)],
         permissions: [*p.try(:keys)],
         allowed_tags: [],
-        allowed_tag_groups: []
+        allowed_tag_groups: [],
       )
-      if SiteSetting.enable_category_group_moderation?
-        result[:reviewable_by_group_id] = Group.find_by(name: params[:reviewable_by_group_name])&.id
-      end
 
       result
     end
diff --git a/spec/requests/categories_controller_spec.rb b/spec/requests/categories_controller_spec.rb
index ffbf60843c9..d8bb0b7a2e7 100644
--- a/spec/requests/categories_controller_spec.rb
+++ b/spec/requests/categories_controller_spec.rb
@@ -437,13 +437,47 @@ describe CategoriesController do
             color: category.color,
             text_color: category.text_color,
             allow_global_tags: 'false',
-            min_tags_from_required_group: 1
+            min_tags_from_required_group: 1,
+            required_tag_group_name: ''
           }
 
           expect(response.status).to eq(200)
           category.reload
           expect(category.required_tag_group).to be_nil
         end
+
+        it "does not update other fields" do
+          SiteSetting.tagging_enabled = true
+          tag_group_1 = Fabricate(:tag_group)
+          tag_group_2 = Fabricate(:tag_group)
+
+          category.update!(
+            allowed_tags: ["hello", "world"],
+            allowed_tag_groups: [tag_group_1.name],
+            required_tag_group_name: tag_group_2.name
+          )
+
+          put "/categories/#{category.id}.json"
+          expect(response.status).to eq(200)
+          category.reload
+          expect(category.tags.pluck(:name)).to contain_exactly("hello", "world")
+          expect(category.tag_groups.pluck(:name)).to contain_exactly(tag_group_1.name)
+          expect(category.required_tag_group).to eq(tag_group_2)
+
+          put "/categories/#{category.id}.json", params: { allowed_tags: [] }
+          expect(response.status).to eq(200)
+          category.reload
+          expect(category.tags).to be_blank
+          expect(category.tag_groups.pluck(:name)).to contain_exactly(tag_group_1.name)
+          expect(category.required_tag_group).to eq(tag_group_2)
+
+          put "/categories/#{category.id}.json", params: { allowed_tags: [], allowed_tag_groups: [], required_tag_group_name: nil }
+          expect(response.status).to eq(200)
+          category.reload
+          expect(category.tags).to be_blank
+          expect(category.tag_groups).to be_blank
+          expect(category.required_tag_group).to eq(nil)
+        end
       end
     end
   end