From 194fc8092aa464ecc28370496a5981f95ed90f48 Mon Sep 17 00:00:00 2001
From: Mohammed Nasser <135416851+mohammednasser-32@users.noreply.github.com>
Date: Tue, 8 Oct 2024 23:58:54 +0300
Subject: [PATCH] fix GoodJob::Setting duplicate keys (#1498)

Co-authored-by: Ben Sheldon [he/him] <bensheldon@gmail.com>
---
 app/models/good_job/setting.rb           | 14 +++++----
 spec/app/models/good_job/setting_spec.rb | 39 ++++++++++++++++++++++--
 2 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/app/models/good_job/setting.rb b/app/models/good_job/setting.rb
index 1fb1328ed..7201bd603 100644
--- a/app/models/good_job/setting.rb
+++ b/app/models/good_job/setting.rb
@@ -19,30 +19,32 @@ def self.cron_key_enabled?(key, default: true)
     end
 
     def self.cron_key_enable(key)
+      key_string = key.to_s
       enabled_setting = find_or_initialize_by(key: CRON_KEYS_ENABLED) do |record|
         record.value = []
       end
-      enabled_setting.value << key unless enabled_setting.value.include?(key)
+      enabled_setting.value << key unless enabled_setting.value.include?(key_string)
       enabled_setting.save!
 
       disabled_setting = GoodJob::Setting.find_by(key: CRON_KEYS_DISABLED)
-      return unless disabled_setting&.value&.include?(key.to_s)
+      return unless disabled_setting&.value&.include?(key_string)
 
-      disabled_setting.value.delete(key.to_s)
+      disabled_setting.value.delete(key_string)
       disabled_setting.save!
     end
 
     def self.cron_key_disable(key)
       enabled_setting = GoodJob::Setting.find_by(key: CRON_KEYS_ENABLED)
-      if enabled_setting&.value&.include?(key.to_s)
-        enabled_setting.value.delete(key.to_s)
+      key_string = key.to_s
+      if enabled_setting&.value&.include?(key_string)
+        enabled_setting.value.delete(key_string)
         enabled_setting.save!
       end
 
       disabled_setting = find_or_initialize_by(key: CRON_KEYS_DISABLED) do |record|
         record.value = []
       end
-      disabled_setting.value << key unless disabled_setting.value.include?(key)
+      disabled_setting.value << key unless disabled_setting.value.include?(key_string)
       disabled_setting.save!
     end
   end
diff --git a/spec/app/models/good_job/setting_spec.rb b/spec/app/models/good_job/setting_spec.rb
index 9239d0250..73994b552 100644
--- a/spec/app/models/good_job/setting_spec.rb
+++ b/spec/app/models/good_job/setting_spec.rb
@@ -45,14 +45,49 @@
         described_class.cron_key_enable(:test_2)
         expect(described_class.find_by(key: described_class::CRON_KEYS_DISABLED).value).to eq []
       end
+
+      it 'does not insert duplicate keys' do
+        expect(described_class.where(key: described_class::CRON_KEYS_DISABLED).count).to eq 0
+
+        described_class.cron_key_disable(:test)
+        described_class.cron_key_disable(:test)
+        expect(described_class.where(key: described_class::CRON_KEYS_DISABLED).count).to eq 1
+        expect(described_class.find_by(key: described_class::CRON_KEYS_DISABLED).value).to contain_exactly 'test'
+
+        described_class.cron_key_enable(:test)
+        expect(described_class.find_by(key: described_class::CRON_KEYS_DISABLED).value).to eq []
+      end
     end
+  end
 
+  describe 'cron_key_enabled setting' do
     describe '.cron_key_enable' do
-      it 'removes values from a json array' do
+      it 'inserts values into a json array' do
+        expect(described_class.where(key: described_class::CRON_KEYS_ENABLED).count).to eq 0
+
+        described_class.cron_key_enable(:test)
+        expect(described_class.where(key: described_class::CRON_KEYS_ENABLED).count).to eq 1
+        expect(described_class.find_by(key: described_class::CRON_KEYS_ENABLED).value).to contain_exactly 'test'
+
+        described_class.cron_key_enable(:test_2)
+        expect(described_class.where(key: described_class::CRON_KEYS_ENABLED).count).to eq 1
+        expect(described_class.find_by(key: described_class::CRON_KEYS_ENABLED).value).to contain_exactly "test", "test_2"
+
         described_class.cron_key_disable(:test)
+        described_class.cron_key_disable(:test_2)
+        expect(described_class.find_by(key: described_class::CRON_KEYS_ENABLED).value).to eq []
+      end
+
+      it 'does not insert duplicate keys' do
+        expect(described_class.where(key: described_class::CRON_KEYS_ENABLED).count).to eq 0
+
         described_class.cron_key_enable(:test)
+        described_class.cron_key_enable(:test)
+        expect(described_class.where(key: described_class::CRON_KEYS_ENABLED).count).to eq 1
+        expect(described_class.find_by(key: described_class::CRON_KEYS_ENABLED).value).to contain_exactly 'test'
 
-        expect(described_class.find_by(key: described_class::CRON_KEYS_DISABLED).value).to eq []
+        described_class.cron_key_disable(:test)
+        expect(described_class.find_by(key: described_class::CRON_KEYS_ENABLED).value).to eq []
       end
     end
   end