From 46615333f96450f909420d8d1ef5fb8890e885e1 Mon Sep 17 00:00:00 2001 From: Ryan Lue Date: Mon, 28 Jan 2019 14:04:05 +0800 Subject: [PATCH] Refactoring: Eliminate explicit coupling of classes in ChecksImport concern --- app/models/application_model/checks_import.rb | 12 ++++++++---- app/models/concerns/can_be_imported.rb | 10 ++++++++++ app/models/group.rb | 1 + app/models/history.rb | 1 + app/models/role.rb | 1 + app/models/ticket.rb | 1 + app/models/ticket/article.rb | 1 + app/models/ticket/priority.rb | 1 + app/models/ticket/state.rb | 1 + app/models/ticket/state_type.rb | 1 + app/models/user.rb | 1 + .../application_model/checks_import_examples.rb | 14 +++++++------- spec/models/concerns/can_be_imported_examples.rb | 7 +++++++ spec/models/group_spec.rb | 2 ++ spec/models/history_spec.rb | 2 ++ spec/models/role_spec.rb | 2 ++ spec/models/ticket/article_spec.rb | 2 ++ spec/models/ticket/priority_spec.rb | 2 ++ spec/models/ticket/state_spec.rb | 2 ++ spec/models/ticket/state_type_spec.rb | 2 ++ spec/models/ticket_spec.rb | 2 ++ spec/models/user_spec.rb | 2 ++ 22 files changed, 59 insertions(+), 11 deletions(-) create mode 100644 app/models/concerns/can_be_imported.rb create mode 100644 spec/models/concerns/can_be_imported_examples.rb diff --git a/app/models/application_model/checks_import.rb b/app/models/application_model/checks_import.rb index dd37714db..b9ca881be 100644 --- a/app/models/application_model/checks_import.rb +++ b/app/models/application_model/checks_import.rb @@ -6,13 +6,17 @@ module ApplicationModel::ChecksImport before_create :check_attributes_protected end + class_methods do + # Use `include CanBeImported` in a class to override this method + def importable? + false + end + end + def check_attributes_protected - - import_class_list = ['Ticket', 'Ticket::Article', 'History', 'Ticket::State', 'Ticket::StateType', 'Ticket::Priority', 'Group', 'User', 'Role' ] - # do noting, use id as it is return if !Setting.get('system_init_done') - return if Setting.get('import_mode') && import_class_list.include?(self.class.to_s) + return if Setting.get('import_mode') && self.class.importable? return if !has_attribute?(:id) self[:id] = nil diff --git a/app/models/concerns/can_be_imported.rb b/app/models/concerns/can_be_imported.rb new file mode 100644 index 000000000..eea3e9746 --- /dev/null +++ b/app/models/concerns/can_be_imported.rb @@ -0,0 +1,10 @@ +module CanBeImported + extend ActiveSupport::Concern + + # methods defined here are going to extend the class, not the instance of it + class_methods do + def importable? + true + end + end +end diff --git a/app/models/group.rb b/app/models/group.rb index 67ec0041f..f324bd13c 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -1,6 +1,7 @@ # Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/ class Group < ApplicationModel + include CanBeImported include HasActivityStreamLog include ChecksClientNotification include ChecksLatestChangeObserved diff --git a/app/models/history.rb b/app/models/history.rb index 29012769a..23b170344 100644 --- a/app/models/history.rb +++ b/app/models/history.rb @@ -1,6 +1,7 @@ # Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/ class History < ApplicationModel + include CanBeImported include History::Assets self.table_name = 'histories' diff --git a/app/models/role.rb b/app/models/role.rb index 7cf35f07b..e4ad5f729 100644 --- a/app/models/role.rb +++ b/app/models/role.rb @@ -1,6 +1,7 @@ # Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/ class Role < ApplicationModel + include CanBeImported include HasActivityStreamLog include ChecksClientNotification include ChecksLatestChangeObserved diff --git a/app/models/ticket.rb b/app/models/ticket.rb index c5eef5a22..5a8eeda77 100644 --- a/app/models/ticket.rb +++ b/app/models/ticket.rb @@ -1,6 +1,7 @@ # Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/ class Ticket < ApplicationModel + include CanBeImported include HasActivityStreamLog include ChecksClientNotification include ChecksLatestChangeObserved diff --git a/app/models/ticket/article.rb b/app/models/ticket/article.rb index cc40862cb..7fc8b6771 100644 --- a/app/models/ticket/article.rb +++ b/app/models/ticket/article.rb @@ -1,5 +1,6 @@ # Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/ class Ticket::Article < ApplicationModel + include CanBeImported include HasActivityStreamLog include ChecksClientNotification include HasHistory diff --git a/app/models/ticket/priority.rb b/app/models/ticket/priority.rb index d2a75fd71..38392a7b5 100644 --- a/app/models/ticket/priority.rb +++ b/app/models/ticket/priority.rb @@ -1,5 +1,6 @@ # Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/ class Ticket::Priority < ApplicationModel + include CanBeImported self.table_name = 'ticket_priorities' validates :name, presence: true diff --git a/app/models/ticket/state.rb b/app/models/ticket/state.rb index 34a7e670b..e136edef8 100644 --- a/app/models/ticket/state.rb +++ b/app/models/ticket/state.rb @@ -1,5 +1,6 @@ # Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/ class Ticket::State < ApplicationModel + include CanBeImported include ChecksLatestChangeObserved belongs_to :state_type, class_name: 'Ticket::StateType', inverse_of: :states diff --git a/app/models/ticket/state_type.rb b/app/models/ticket/state_type.rb index a1bbb65f4..9696f9508 100644 --- a/app/models/ticket/state_type.rb +++ b/app/models/ticket/state_type.rb @@ -1,5 +1,6 @@ # Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/ class Ticket::StateType < ApplicationModel + include CanBeImported include ChecksLatestChangeObserved has_many :states, class_name: 'Ticket::State', inverse_of: :state_type diff --git a/app/models/user.rb b/app/models/user.rb index a0d097248..5d564f088 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1,5 +1,6 @@ # Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/ class User < ApplicationModel + include CanBeImported include HasActivityStreamLog include ChecksClientNotification include HasHistory diff --git a/spec/models/application_model/checks_import_examples.rb b/spec/models/application_model/checks_import_examples.rb index bb374a826..dd932c983 100644 --- a/spec/models/application_model/checks_import_examples.rb +++ b/spec/models/application_model/checks_import_examples.rb @@ -1,5 +1,5 @@ -RSpec.shared_examples 'ApplicationModel::ChecksImport' do |importable: false| - subject(:new_instance) { build(described_class.name.underscore, id: unused_id) } +RSpec.shared_examples 'ApplicationModel::ChecksImport' do + subject { build(described_class.name.underscore, id: unused_id) } let(:unused_id) { described_class.pluck(:id).max * 2 } context 'when Setting.get("system_init_done") is true AND Setting.get("import_mode") is false' do @@ -7,7 +7,7 @@ RSpec.shared_examples 'ApplicationModel::ChecksImport' do |importable: false| before { Setting.set('import_mode', false) } it 'prevents explicit setting of #id attribute' do - expect { new_instance.save }.to change { new_instance.id } + expect { subject.save }.to change { subject.id } end end @@ -15,7 +15,7 @@ RSpec.shared_examples 'ApplicationModel::ChecksImport' do |importable: false| before { Setting.set('system_init_done', false) } it 'allows explicit setting of #id attribute' do - expect { new_instance.save }.not_to change { new_instance.id } + expect { subject.save }.not_to change { subject.id } end end @@ -24,16 +24,16 @@ RSpec.shared_examples 'ApplicationModel::ChecksImport' do |importable: false| shared_examples 'importable classes' do it 'allows explicit setting of #id attribute' do - expect { new_instance.save }.not_to change { new_instance.id } + expect { subject.save }.not_to change { subject.id } end end shared_examples 'non-importable classes' do it 'prevents explicit setting of #id attribute' do - expect { new_instance.save }.to change { new_instance.id } + expect { subject.save }.to change { subject.id } end end - include_examples importable ? 'importable classes' : 'non-importable classes' + include_examples described_class.importable? ? 'importable classes' : 'non-importable classes' end end diff --git a/spec/models/concerns/can_be_imported_examples.rb b/spec/models/concerns/can_be_imported_examples.rb new file mode 100644 index 000000000..f3c185767 --- /dev/null +++ b/spec/models/concerns/can_be_imported_examples.rb @@ -0,0 +1,7 @@ +RSpec.shared_examples 'CanBeImported' do + describe '.importable?' do + it 'returns true' do + expect(described_class.importable?).to be(true) + end + end +end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 7a2f6116d..df56c77da 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -1,6 +1,8 @@ require 'rails_helper' require 'models/application_model_examples' +require 'models/concerns/can_be_imported_examples' RSpec.describe Group, type: :model do it_behaves_like 'ApplicationModel' + it_behaves_like 'CanBeImported' end diff --git a/spec/models/history_spec.rb b/spec/models/history_spec.rb index 05e752e44..ef7fb0aeb 100644 --- a/spec/models/history_spec.rb +++ b/spec/models/history_spec.rb @@ -1,6 +1,8 @@ require 'rails_helper' require 'models/application_model_examples' +require 'models/concerns/can_be_imported_examples' RSpec.describe History, type: :model do it_behaves_like 'ApplicationModel' + it_behaves_like 'CanBeImported' end diff --git a/spec/models/role_spec.rb b/spec/models/role_spec.rb index 253515a49..7194fb9a3 100644 --- a/spec/models/role_spec.rb +++ b/spec/models/role_spec.rb @@ -1,9 +1,11 @@ require 'rails_helper' require 'models/application_model_examples' +require 'models/concerns/can_be_imported_examples' require 'models/concerns/has_groups_examples' RSpec.describe Role do it_behaves_like 'ApplicationModel' + it_behaves_like 'CanBeImported' it_behaves_like 'HasGroups', group_access_factory: :role subject(:role) { create(:role) } diff --git a/spec/models/ticket/article_spec.rb b/spec/models/ticket/article_spec.rb index 787d400cc..5b2a87dbd 100644 --- a/spec/models/ticket/article_spec.rb +++ b/spec/models/ticket/article_spec.rb @@ -1,8 +1,10 @@ require 'rails_helper' require 'models/application_model_examples' +require 'models/concerns/can_be_imported_examples' RSpec.describe Ticket::Article, type: :model do it_behaves_like 'ApplicationModel' + it_behaves_like 'CanBeImported' describe '.create' do it 'handles NULL byte in subject or body' do diff --git a/spec/models/ticket/priority_spec.rb b/spec/models/ticket/priority_spec.rb index b0fc57aad..c775a4f18 100644 --- a/spec/models/ticket/priority_spec.rb +++ b/spec/models/ticket/priority_spec.rb @@ -1,8 +1,10 @@ require 'rails_helper' require 'models/application_model_examples' +require 'models/concerns/can_be_imported_examples' RSpec.describe Ticket::Priority, type: :model do it_behaves_like 'ApplicationModel' + it_behaves_like 'CanBeImported' describe 'Default state' do describe 'of whole table:' do diff --git a/spec/models/ticket/state_spec.rb b/spec/models/ticket/state_spec.rb index 5bbfd39ce..6635c400d 100644 --- a/spec/models/ticket/state_spec.rb +++ b/spec/models/ticket/state_spec.rb @@ -1,8 +1,10 @@ require 'rails_helper' require 'models/application_model_examples' +require 'models/concerns/can_be_imported_examples' RSpec.describe Ticket::State, type: :model do it_behaves_like 'ApplicationModel' + it_behaves_like 'CanBeImported' describe '.by_category' do it 'looks up states by category' do diff --git a/spec/models/ticket/state_type_spec.rb b/spec/models/ticket/state_type_spec.rb index fa7033f21..827765518 100644 --- a/spec/models/ticket/state_type_spec.rb +++ b/spec/models/ticket/state_type_spec.rb @@ -1,6 +1,8 @@ require 'rails_helper' require 'models/application_model_examples' +require 'models/concerns/can_be_imported_examples' RSpec.describe Ticket::StateType, type: :model do it_behaves_like 'ApplicationModel' + it_behaves_like 'CanBeImported' end diff --git a/spec/models/ticket_spec.rb b/spec/models/ticket_spec.rb index cfd261c2a..2d9fd13df 100644 --- a/spec/models/ticket_spec.rb +++ b/spec/models/ticket_spec.rb @@ -1,9 +1,11 @@ require 'rails_helper' require 'models/application_model_examples' +require 'models/concerns/can_be_imported_examples' require 'models/concerns/can_lookup_examples' RSpec.describe Ticket, type: :model do it_behaves_like 'ApplicationModel' + it_behaves_like 'CanBeImported' it_behaves_like 'CanLookup' describe '#merge_to' do diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 1be710d0d..0c10e7fc8 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -3,6 +3,7 @@ require 'models/application_model_examples' require 'models/concerns/has_groups_examples' require 'models/concerns/has_roles_examples' require 'models/concerns/has_groups_permissions_examples' +require 'models/concerns/can_be_imported_examples' require 'models/concerns/can_lookup_examples' RSpec.describe User do @@ -10,6 +11,7 @@ RSpec.describe User do it_behaves_like 'HasGroups', group_access_factory: :agent_user it_behaves_like 'HasRoles', group_access_factory: :agent_user it_behaves_like 'HasGroups and Permissions', group_access_no_permission_factory: :user + it_behaves_like 'CanBeImported' it_behaves_like 'CanLookup' subject(:user) { create(:user) }