feedback

app/controllers/admin/color_schemes_controller.rb

@@ -31,11 +31,7 @@ class Admin::ColorSchemesController < Admin::AdminController
       end
     end
 
-    diverge_from_remote =
+    color_scheme = ColorSchemeRevisor.revise(@color_scheme, update_params)
-      update_params[:colors].present? && @color_scheme.theme_id.present? &&
-        @color_scheme.base_scheme_id.blank?
-
-    color_scheme = ColorSchemeRevisor.revise(@color_scheme, update_params, diverge_from_remote:)
     update_theme_default_scheme!
     if color_scheme.valid?
       render json: color_scheme, root: false

app/models/color_scheme.rb

@@ -349,6 +349,7 @@ class ColorScheme < ActiveRecord::Base
   belongs_to :theme
   belongs_to :base_scheme, -> { unscope(where: :remote_copy) }, class_name: "ColorScheme"
 
+  validate :no_edits_for_remote_copies, on: :update
   validates_associated :color_scheme_colors
 
   BASE_COLORS_FILE = "#{Rails.root}/app/assets/stylesheets/common/foundation/colors.scss"
@@ -588,15 +589,19 @@ class ColorScheme < ActiveRecord::Base
     DistributedMutex.synchronize("color_scheme_fork_#{self.id}") do
       self.reload
       if self.base_scheme_id.blank?
-        new_scheme.save!
+        self.transaction do
-        self.base_scheme_id = new_scheme.id
+          new_scheme.save!
-        self.save!
+          self.base_scheme_id = new_scheme.id
+          self.save!
+        end
       end
     end
 
     self
   end
 
+  private
+
   def destroy_remote_original
     return if theme_id.blank?
     return if base_scheme_id.blank? || base_scheme_id < 0
@@ -606,6 +611,16 @@ class ColorScheme < ActiveRecord::Base
       .where(theme_id: theme_id, id: base_scheme_id, remote_copy: true)
       .destroy_all
   end
+
+  def no_edits_for_remote_copies
+    if remote_copy &&
+         (
+           will_save_change_to_base_scheme_id? || will_save_change_to_user_selectable? ||
+             will_save_change_to_remote_copy? || will_save_change_to_theme_id?
+         )
+      errors.add(:base, I18n.t("color_schemes.errors.cannot_edit_remote_copies"))
+    end
+  end
 end
 
 # == Schema Information

app/models/color_scheme_color.rb

@@ -5,13 +5,22 @@ class ColorSchemeColor < ActiveRecord::Base
     "dark_hex", # TODO: Remove when 20250821155127_drop_dark_hex_from_color_scheme_color has been promoted to pre-deploy
   ]
 
-  belongs_to :color_scheme
+  belongs_to :color_scheme, -> { unscope(where: :remote_copy) }
 
   validates :hex, format: { with: /\A([0-9a-fA-F]{3}|[0-9a-fA-F]{6})\z/ }
+  validate :no_edits_for_remote_copies, on: :update
 
   def hex_with_hash
     "##{hex}"
   end
+
+  private
+
+  def no_edits_for_remote_copies
+    if color_scheme&.remote_copy && will_save_change_to_hex?
+      errors.add(:base, I18n.t("color_schemes.errors.cannot_edit_remote_copies"))
+    end
+  end
 end
 
 # == Schema Information

app/models/remote_theme.rb

@@ -420,7 +420,11 @@ class RemoteTheme < ActiveRecord::Base
     existing_schemes = ColorScheme.unscoped.where(theme_id: theme.id)
 
     missing_scheme_names =
-      Hash[*existing_schemes.reject(&:remote_copy).map { |cs| [cs.name, cs] }.flatten]
+      existing_schemes.reduce({}) do |hash, cs|
+        hash[cs.name] = cs if !cs.remote_copy
+        hash
+      end
+
     ordered_schemes = []
 
     schemes&.each do |name, colors|

app/services/color_scheme_revisor.rb

@@ -6,11 +6,11 @@ class ColorSchemeRevisor
     @params = params
   end
 
-  def self.revise(color_scheme, params, diverge_from_remote: false)
+  def self.revise(color_scheme, params)
-    self.new(color_scheme, params).revise(diverge_from_remote:)
+    self.new(color_scheme, params).revise
   end
 
-  def revise(diverge_from_remote: false)
+  def revise
     ColorScheme.transaction do
       @color_scheme.name = @params[:name] if @params.has_key?(:name)
       @color_scheme.user_selectable = @params[:user_selectable] if @params.has_key?(
@@ -20,7 +20,10 @@ class ColorSchemeRevisor
       @color_scheme.base_scheme_id = @params[:base_scheme_id] if @params.has_key?(:base_scheme_id)
       has_colors = @params[:colors]
 
-      @color_scheme.diverge_from_remote if diverge_from_remote
+      if @params[:colors].present? && @color_scheme.theme_id.present? &&
+           @color_scheme.base_scheme_id.blank?
+        @color_scheme.diverge_from_remote
+      end
 
       if has_colors
         @params[:colors].each do |c|

config/locales/server.en.yml

@@ -4805,6 +4805,8 @@ en:
     latte_theme_name: "Latte"
     summer_theme_name: "Summer"
     dark_rose_theme_name: "Dark Rose"
+    errors:
+      cannot_edit_remote_copies: "Edits to remote copies of color palettes are not allowed."
 
   edit_this_page: "Edit this page"
 

spec/integration/remote_theme_color_schemes_spec.rb

@@ -0,0 +1,152 @@
+# frozen_string_literal: true
+
+RSpec.describe "Remote theme update" do
+  let :about_json do
+    <<~JSON
+      {
+        "name": "awesome theme",
+        "about_url": "https://www.site.com/about",
+        "license_url": "https://www.site.com/license",
+        "theme_version": "1.0",
+        "minimum_discourse_version": "1.0.0",
+        "assets": {
+          "font": "assets/font.woff2"
+        },
+        "color_schemes": {
+          "Amazing": {
+            "love": "FAFAFA",
+            "tertiary-low": "FFFFFF"
+          }
+        }
+      }
+    JSON
+  end
+
+  let :initial_repo do
+    setup_git_repo("about.json" => about_json)
+  end
+
+  let :initial_repo_url do
+    MockGitImporter.register("https://example.com/initial_repo.git", initial_repo)
+  end
+
+  after { `rm -fr #{initial_repo}` }
+
+  around(:each) { |group| MockGitImporter.with_mock { group.run } }
+
+  it "updates the base schemes for schemes that have diverged colors" do
+    add_to_git_repo(
+      initial_repo,
+      "about.json" =>
+        JSON
+          .parse(about_json)
+          .merge(
+            color_schemes: {
+              scheme1: {
+                love: "FFFFFF",
+                tertiary_low: "000000",
+              },
+              scheme2: {
+                love: "AACCDD",
+                tertiary_low: "99AAFF",
+              },
+            },
+          )
+          .to_json,
+    )
+
+    theme = RemoteTheme.import_theme(initial_repo_url)
+    scheme1 = theme.color_schemes.find_by(name: "scheme1")
+    scheme2 = theme.color_schemes.find_by(name: "scheme2")
+    expect(scheme1.base_scheme_id).to eq(nil)
+    expect(scheme2.base_scheme_id).to eq(nil)
+
+    ColorSchemeRevisor.revise(scheme1, { colors: [{ name: "love", hex: "111111" }] })
+
+    scheme1_base_scheme_id = scheme1.reload.base_scheme_id
+    expect(scheme1_base_scheme_id).to be_present
+    expect(scheme1.colors.find_by(name: "love").hex).to eq("111111")
+    expect(
+      ColorScheme.unscoped.find(scheme1_base_scheme_id).colors.find_by(name: "love").hex,
+    ).to eq("ffffff")
+    expect(scheme2.base_scheme_id).to eq(nil)
+
+    add_to_git_repo(
+      initial_repo,
+      "about.json" =>
+        JSON
+          .parse(about_json)
+          .merge(
+            color_schemes: {
+              scheme1: {
+                love: "EEEEEE",
+                tertiary_low: "000000",
+              },
+              scheme2: {
+                love: "AACCDD",
+                tertiary_low: "99AAFF",
+              },
+            },
+          )
+          .to_json,
+    )
+    theme.remote_theme.update_from_remote
+
+    scheme1.reload
+    expect(scheme1.base_scheme_id).to eq(scheme1_base_scheme_id)
+    expect(scheme1.colors.find_by(name: "love").hex).to eq("111111")
+    expect(
+      ColorScheme.unscoped.find(scheme1_base_scheme_id).colors.find_by(name: "love").hex,
+    ).to eq("eeeeee")
+    expect(scheme2.base_scheme_id).to eq(nil)
+  end
+
+  it "deletes color schemes that haven't been modified and deletes the bases of schemes that have been modified" do
+    add_to_git_repo(
+      initial_repo,
+      "about.json" =>
+        JSON
+          .parse(about_json)
+          .merge(
+            color_schemes: {
+              scheme1: {
+                love: "FFFFFF",
+                tertiary_low: "000000",
+              },
+              scheme2: {
+                love: "AACCDD",
+                tertiary_low: "99AAFF",
+              },
+            },
+          )
+          .to_json,
+    )
+
+    theme = RemoteTheme.import_theme(initial_repo_url)
+
+    scheme1 = theme.color_schemes.find_by(name: "scheme1")
+    scheme2 = theme.color_schemes.find_by(name: "scheme2")
+
+    expect do
+      ColorSchemeRevisor.revise(scheme1, { colors: [{ name: "love", hex: "111111" }] })
+    end.to change { ColorScheme.unscoped.count }.by(1)
+
+    expect(scheme1.reload.base_scheme_id).to be_present
+    expect(scheme2.reload.base_scheme_id).to be_blank
+
+    scheme1_base_scheme_id = scheme1.base_scheme_id
+
+    add_to_git_repo(
+      initial_repo,
+      "about.json" => JSON.parse(about_json).merge(color_schemes: {}).to_json,
+    )
+
+    expect do theme.remote_theme.update_from_remote end.to change { ColorScheme.unscoped.count }.by(
+      -2,
+    )
+
+    expect(ColorScheme.unscoped.exists?(id: scheme1_base_scheme_id)).to eq(false)
+    expect(ColorScheme.unscoped.exists?(id: scheme1.id)).to eq(true)
+    expect(ColorScheme.unscoped.exists?(id: scheme2.id)).to eq(false)
+  end
+end

spec/models/color_scheme_color_spec.rb

@@ -26,4 +26,24 @@ RSpec.describe ColorSchemeColor do
       "555 666",
     ].each { |hex| test_invalid_hex(hex) }
   end
+
+  describe "#no_edits_for_remote_copies" do
+    it "prevents editing colors of remote copies" do
+      remote_copy =
+        Fabricate(
+          :color_scheme,
+          remote_copy: true,
+          color_scheme_colors: [
+            Fabricate(:color_scheme_color, name: "primary", hex: "998877"),
+            Fabricate(:color_scheme_color, name: "secondary", hex: "553322"),
+          ],
+        )
+      color = remote_copy.color_scheme_colors.first
+      color.hex = "111111"
+      expect(color.valid?).to eq(false)
+      expect(color.errors.full_messages).to include(
+        I18n.t("color_schemes.errors.cannot_edit_remote_copies"),
+      )
+    end
+  end
 end

spec/models/color_scheme_spec.rb

@@ -214,9 +214,10 @@ RSpec.describe ColorScheme do
       expect(original_scheme.base_scheme.colors.map { |c| [c.name, c.hex] }.sort_by(&:first)).to eq(
         original_scheme.colors.map { |c| [c.name, c.hex] }.sort_by(&:first),
       )
-      expect(original_scheme.base_scheme.theme.id).to eq(theme.id)
+      expect(original_scheme.base_scheme.theme_id).to eq(theme.id)
       expect(original_scheme.base_scheme.user_selectable).to eq(false)
       expect(original_scheme.base_scheme.remote_copy).to eq(true)
+      expect(original_scheme.base_scheme.via_wizard).to eq(false)
     end
   end
 
@@ -239,4 +240,21 @@ RSpec.describe ColorScheme do
       expect(ColorScheme.unscoped.exists?(id: original_scheme.base_scheme_id)).to eq(false)
     end
   end
+
+  describe "#no_edits_for_remote_copies" do
+    it "prevents editing remote copies of color schemes" do
+      remote_copy =
+        Fabricate(
+          :color_scheme,
+          remote_copy: true,
+          user_selectable: false,
+          theme_id: Fabricate(:theme).id,
+        )
+      remote_copy.user_selectable = true
+      expect(remote_copy.valid?).to eq(false)
+      expect(remote_copy.errors.full_messages).to include(
+        I18n.t("color_schemes.errors.cannot_edit_remote_copies"),
+      )
+    end
+  end
 end

spec/models/remote_theme_spec.rb

@@ -468,136 +468,6 @@ RSpec.describe RemoteTheme do
       end
     end
 
-    describe "color schemes" do
-      it "updates the base schemes for schemes that have diverged colors" do
-        add_to_git_repo(
-          initial_repo,
-          "about.json" =>
-            JSON
-              .parse(about_json)
-              .merge(
-                color_schemes: {
-                  scheme1: {
-                    love: "FFFFFF",
-                    tertiary_low: "000000",
-                  },
-                  scheme2: {
-                    love: "AACCDD",
-                    tertiary_low: "99AAFF",
-                  },
-                },
-              )
-              .to_json,
-        )
-
-        theme = RemoteTheme.import_theme(initial_repo_url)
-        scheme1 = theme.color_schemes.find_by(name: "scheme1")
-        scheme2 = theme.color_schemes.find_by(name: "scheme1")
-        expect(scheme1.base_scheme_id).to eq(nil)
-        expect(scheme2.base_scheme_id).to eq(nil)
-
-        expect do
-          ColorSchemeRevisor.revise(
-            scheme1,
-            { colors: [{ name: "love", hex: "111111" }] },
-            diverge_from_remote: true,
-          )
-        end.to change { ColorScheme.unscoped.count }.by(1)
-
-        scheme1_base_scheme_id = scheme1.reload.base_scheme_id
-        expect(scheme1_base_scheme_id).to be_present
-        expect(scheme1.colors.find_by(name: "love").hex).to eq("111111")
-        expect(
-          ColorScheme.unscoped.find(scheme1_base_scheme_id).colors.find_by(name: "love").hex,
-        ).to eq("ffffff")
-        expect(scheme2.base_scheme_id).to eq(nil)
-
-        add_to_git_repo(
-          initial_repo,
-          "about.json" =>
-            JSON
-              .parse(about_json)
-              .merge(
-                color_schemes: {
-                  scheme1: {
-                    love: "EEEEEE",
-                    tertiary_low: "000000",
-                  },
-                  scheme2: {
-                    love: "AACCDD",
-                    tertiary_low: "99AAFF",
-                  },
-                },
-              )
-              .to_json,
-        )
-        theme.remote_theme.update_from_remote
-
-        scheme1.reload
-        expect(scheme1.base_scheme_id).to eq(scheme1_base_scheme_id)
-        expect(scheme1.colors.find_by(name: "love").hex).to eq("111111")
-        expect(
-          ColorScheme.unscoped.find(scheme1_base_scheme_id).colors.find_by(name: "love").hex,
-        ).to eq("eeeeee")
-        expect(scheme2.base_scheme_id).to eq(nil)
-      end
-
-      it "deletes color schemes that haven't been modified and deletes the bases of schemes that have been modified" do
-        add_to_git_repo(
-          initial_repo,
-          "about.json" =>
-            JSON
-              .parse(about_json)
-              .merge(
-                color_schemes: {
-                  scheme1: {
-                    love: "FFFFFF",
-                    tertiary_low: "000000",
-                  },
-                  scheme2: {
-                    love: "AACCDD",
-                    tertiary_low: "99AAFF",
-                  },
-                },
-              )
-              .to_json,
-        )
-
-        theme = RemoteTheme.import_theme(initial_repo_url)
-
-        expect(theme.color_schemes.pluck(:name)).to contain_exactly("scheme1", "scheme2")
-
-        scheme1 = theme.color_schemes.find_by(name: "scheme1")
-        scheme2 = theme.color_schemes.find_by(name: "scheme2")
-
-        expect do
-          ColorSchemeRevisor.revise(
-            scheme1,
-            { colors: [{ name: "love", hex: "111111" }] },
-            diverge_from_remote: true,
-          )
-        end.to change { ColorScheme.unscoped.count }.by(1)
-
-        expect(scheme1.reload.base_scheme_id).to be_present
-        expect(scheme2.reload.base_scheme_id).to be_blank
-
-        scheme1_base_scheme_id = scheme1.base_scheme_id
-
-        add_to_git_repo(
-          initial_repo,
-          "about.json" => JSON.parse(about_json).merge(color_schemes: {}).to_json,
-        )
-
-        expect do theme.remote_theme.update_from_remote end.to change {
-          ColorScheme.unscoped.count
-        }.by(-2)
-
-        expect(ColorScheme.unscoped.exists?(id: scheme1_base_scheme_id)).to eq(false)
-        expect(ColorScheme.unscoped.exists?(id: scheme1.id)).to eq(true)
-        expect(ColorScheme.unscoped.exists?(id: scheme2.id)).to eq(false)
-      end
-    end
-
     describe "theme site settings" do
       it "creates theme site settings defined in about.json" do
         add_to_git_repo(

spec/requests/admin/color_schemes_controller_spec.rb

@@ -201,7 +201,7 @@ RSpec.describe Admin::ColorSchemesController do
                   colors: [{ name: "primary", hex: "7711EE" }],
                 },
               }
-        end.to change { ColorScheme.unscoped.count }.by(1)
+        end.to change { ColorScheme.unscoped.where(remote_copy: true).count }.by(1)
         expect(response.status).to eq(200)
         expect(response.parsed_body["colors"].find { |c| c["name"] == "primary" }["hex"]).to eq(
           "7711EE",