fix GoodJob::Setting duplicate keys (#1498)

Co-authored-by: Ben Sheldon [he/him] <bensheldon@gmail.com>

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

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