From 814ea07b23df7ff8ba6227dc8734d26aa7613c17 Mon Sep 17 00:00:00 2001
From: Markus Koller <mkoller@gitlab.com>
Date: Thu, 23 Jun 2022 18:15:45 +0200
Subject: [PATCH] Fix `Integration#boolean_accessor` to work with data fields
Previously this only worked with fields defined with `prop_accessor`.
Change this so `prop_accessor` is only called if a getter doesn't
exist yet.
Changelog: fixed
---
app/models/integration.rb | 17 +++++++---
spec/models/integration_spec.rb | 57 +++++++++++++++++++++++++++++----
2 files changed, 63 insertions(+), 11 deletions(-)
diff --git a/app/models/integration.rb b/app/models/integration.rb
index 727f7d2eeba0..af8e8060d81e 100644
--- a/app/models/integration.rb
+++ b/app/models/integration.rb
@@ -200,14 +200,21 @@ def #{arg}_was
# Provide convenient boolean accessor methods for each serialized property.
# Also keep track of updated properties in a similar way as ActiveModel::Dirty
def self.boolean_accessor(*args)
- prop_accessor(*args)
-
args.each do |arg|
+ # TODO: Allow legacy usage of `.boolean_accessor`, once all integrations
+ # are converted to the field DSL we can remove this and only call
+ # `.boolean_accessor` through `.field`.
+ #
+ # See https://gitlab.com/groups/gitlab-org/-/epics/7652
+ prop_accessor(arg) unless method_defined?(arg)
+
class_eval <<~RUBY, __FILE__, __LINE__ + 1
- def #{arg}
- return if properties.blank?
+ # Make the original getter available as a private method.
+ alias_method :#{arg}_before_type_cast, :#{arg}
+ private(:#{arg}_before_type_cast)
- Gitlab::Utils.to_boolean(properties['#{arg}'])
+ def #{arg}
+ Gitlab::Utils.to_boolean(#{arg}_before_type_cast)
end
def #{arg}?
diff --git a/spec/models/integration_spec.rb b/spec/models/integration_spec.rb
index 63db1c9535ff..39fde0c9b779 100644
--- a/spec/models/integration_spec.rb
+++ b/spec/models/integration_spec.rb
@@ -1083,11 +1083,8 @@ def fields
end
it 'provides boolean accessors for checkbox fields' do
- integration.boolean = 'yes'
- expect(integration.boolean?).to be(true)
-
- integration.boolean = nil
- expect(integration.boolean?).to be(false)
+ expect(integration).to respond_to(:boolean)
+ expect(integration).to respond_to(:boolean?)
expect(integration).not_to respond_to(:foo?)
expect(integration).not_to respond_to(:bar?)
@@ -1129,11 +1126,12 @@ def fields
describe 'boolean_accessor' do
let(:klass) do
Class.new(Integration) do
+ prop_accessor :test_value
boolean_accessor :test_value
end
end
- let(:integration) { klass.new(properties: { test_value: input }) }
+ let(:integration) { klass.new(test_value: input) }
where(:input, :method_result, :predicate_method_result) do
true | true | true
@@ -1163,6 +1161,35 @@ def fields
test_value: be(method_result),
test_value?: be(predicate_method_result)
)
+
+ # Make sure the original value is stored correctly
+ expect(integration.send(:test_value_before_type_cast)).to eq(input)
+ expect(integration.properties).to include('test_value' => input)
+ end
+
+ context 'when using data fields' do
+ let(:klass) do
+ Class.new(Integration) do
+ field :project_url, storage: :data_fields, type: 'checkbox'
+
+ def data_fields
+ issue_tracker_data || self.build_issue_tracker_data
+ end
+ end
+ end
+
+ let(:integration) { klass.new(project_url: input) }
+
+ it 'has the correct value' do
+ expect(integration).to have_attributes(
+ project_url: be(method_result),
+ project_url?: be(predicate_method_result)
+ )
+
+ # Make sure the original value is stored correctly
+ expect(integration.send(:project_url_before_type_cast)).to eq(input == false ? 'false' : input)
+ expect(integration.properties).not_to include('project_url')
+ end
end
end
@@ -1174,6 +1201,24 @@ def fields
test_value?: be(false)
)
end
+
+ context 'when getter is not defined' do
+ let(:input) { true }
+ let(:klass) do
+ Class.new(Integration) do
+ boolean_accessor :test_value
+ end
+ end
+
+ it 'defines a prop_accessor' do
+ expect(integration).to have_attributes(
+ test_value: true,
+ test_value?: true
+ )
+
+ expect(integration.properties['test_value']).to be(true)
+ end
+ end
end
describe '#attributes' do
--
GitLab