From 0388886f9439fa93efea29a159522aec5643f7c8 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 12 Feb 2020 09:09:11 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- .../boards/mixins/sortable_default_options.js | 1 + app/controllers/concerns/spammable_actions.rb | 4 +- .../mutations/snippets/mark_as_spam.rb | 2 +- app/services/concerns/akismet_methods.rb | 10 +-- app/services/concerns/spam_check_methods.rb | 6 +- app/services/spam/ham_service.rb | 16 ++--- app/services/spam/mark_as_spam_service.rb | 14 ++-- app/services/spam/spam_check_service.rb | 26 +++---- .../refactoring-entities-file-16.yml | 5 ++ danger/commit_messages/Dangerfile | 4 +- lib/api/entities.rb | 72 ------------------- .../entities/global_notification_setting.rb | 11 +++ lib/api/entities/project_service.rb | 17 +++++ lib/api/entities/project_service_basic.rb | 17 +++++ lib/api/entities/project_with_access.rb | 47 ++++++++++++ spec/services/spam/ham_service_spec.rb | 14 ++-- .../spam/mark_as_spam_service_spec.rb | 14 ++-- spec/services/spam/spam_check_service_spec.rb | 12 ++-- .../spam_check_shared_examples.rb | 2 +- yarn.lock | 6 +- 20 files changed, 162 insertions(+), 138 deletions(-) create mode 100644 changelogs/unreleased/refactoring-entities-file-16.yml create mode 100644 lib/api/entities/global_notification_setting.rb create mode 100644 lib/api/entities/project_service.rb create mode 100644 lib/api/entities/project_service_basic.rb create mode 100644 lib/api/entities/project_with_access.rb diff --git a/app/assets/javascripts/boards/mixins/sortable_default_options.js b/app/assets/javascripts/boards/mixins/sortable_default_options.js index 68ea28e68d9..f77f131c71a 100644 --- a/app/assets/javascripts/boards/mixins/sortable_default_options.js +++ b/app/assets/javascripts/boards/mixins/sortable_default_options.js @@ -26,6 +26,7 @@ export function getBoardSortableDefaultOptions(obj) { scrollSpeed: 20, onStart: sortableStart, onEnd: sortableEnd, + fallbackTolerance: 1, }); Object.keys(obj).forEach(key => { diff --git a/app/controllers/concerns/spammable_actions.rb b/app/controllers/concerns/spammable_actions.rb index 9ec8f930a78..e9bba904dab 100644 --- a/app/controllers/concerns/spammable_actions.rb +++ b/app/controllers/concerns/spammable_actions.rb @@ -11,7 +11,7 @@ module SpammableActions end def mark_as_spam - if Spam::MarkAsSpamService.new(spammable: spammable).execute + if Spam::MarkAsSpamService.new(target: spammable).execute redirect_to spammable_path, notice: _("%{spammable_titlecase} was submitted to Akismet successfully.") % { spammable_titlecase: spammable.spammable_entity_type.titlecase } else redirect_to spammable_path, alert: _('Error with Akismet. Please check the logs for more info.') @@ -42,7 +42,7 @@ module SpammableActions end format.json do - locals = { spammable: spammable, script: false, has_submit: false } + locals = { target: spammable, script: false, has_submit: false } recaptcha_html = render_to_string(partial: 'shared/recaptcha_form', formats: :html, locals: locals) render json: { recaptcha_html: recaptcha_html } diff --git a/app/graphql/mutations/snippets/mark_as_spam.rb b/app/graphql/mutations/snippets/mark_as_spam.rb index 8cfbbae7c08..3171f4645ca 100644 --- a/app/graphql/mutations/snippets/mark_as_spam.rb +++ b/app/graphql/mutations/snippets/mark_as_spam.rb @@ -24,7 +24,7 @@ module Mutations private def mark_as_spam(snippet) - Spam::MarkAsSpamService.new(spammable: snippet).execute + Spam::MarkAsSpamService.new(target: snippet).execute end def authorized_resource?(snippet) diff --git a/app/services/concerns/akismet_methods.rb b/app/services/concerns/akismet_methods.rb index 105b79785bd..4e554ddac4c 100644 --- a/app/services/concerns/akismet_methods.rb +++ b/app/services/concerns/akismet_methods.rb @@ -1,15 +1,15 @@ # frozen_string_literal: true module AkismetMethods - def spammable_owner - @user ||= User.find(spammable.author_id) + def target_owner + @user ||= User.find(target.author_id) end def akismet @akismet ||= Spam::AkismetService.new( - spammable_owner.name, - spammable_owner.email, - spammable.try(:spammable_text) || spammable&.text, + target_owner.name, + target_owner.email, + target.try(:spammable_text) || target&.text, options ) end diff --git a/app/services/concerns/spam_check_methods.rb b/app/services/concerns/spam_check_methods.rb index 695bdf92b49..62836881099 100644 --- a/app/services/concerns/spam_check_methods.rb +++ b/app/services/concerns/spam_check_methods.rb @@ -2,7 +2,7 @@ # SpamCheckMethods # -# Provide helper methods for checking if a given spammable object has +# Provide helper methods for checking if a given target spammable object has # potential spam data. # # Dependencies: @@ -18,13 +18,13 @@ module SpamCheckMethods end # rubocop:enable Gitlab/ModuleWithInstanceVariables - # In order to be proceed to the spam check process, @spammable has to be + # In order to be proceed to the spam check process, @target has to be # a dirty instance, which means it should be already assigned with the new # attribute values. # rubocop:disable Gitlab/ModuleWithInstanceVariables def spam_check(spammable, user) Spam::SpamCheckService.new( - spammable: spammable, + target: spammable, request: @request ).execute( api: @api, diff --git a/app/services/spam/ham_service.rb b/app/services/spam/ham_service.rb index d0f53eea90c..cedb9f02f3d 100644 --- a/app/services/spam/ham_service.rb +++ b/app/services/spam/ham_service.rb @@ -4,25 +4,23 @@ module Spam class HamService include AkismetMethods - attr_accessor :spam_log, :options + attr_accessor :target, :options - def initialize(spam_log) - @spam_log = spam_log - @user = spam_log.user + def initialize(target) + @target = target + @user = target.user @options = { - ip_address: spam_log.source_ip, - user_agent: spam_log.user_agent + ip_address: target.source_ip, + user_agent: target.user_agent } end def execute if akismet.submit_ham - spam_log.update_attribute(:submitted_as_ham, true) + target.update_attribute(:submitted_as_ham, true) else false end end - - alias_method :spammable, :spam_log end end diff --git a/app/services/spam/mark_as_spam_service.rb b/app/services/spam/mark_as_spam_service.rb index 0ebcf17927a..ed5e674d8e9 100644 --- a/app/services/spam/mark_as_spam_service.rb +++ b/app/services/spam/mark_as_spam_service.rb @@ -4,21 +4,21 @@ module Spam class MarkAsSpamService include ::AkismetMethods - attr_accessor :spammable, :options + attr_accessor :target, :options - def initialize(spammable:) - @spammable = spammable + def initialize(target:) + @target = target @options = {} - @options[:ip_address] = @spammable.ip_address - @options[:user_agent] = @spammable.user_agent + @options[:ip_address] = @target.ip_address + @options[:user_agent] = @target.user_agent end def execute - return unless spammable.submittable_as_spam? + return unless target.submittable_as_spam? return unless akismet.submit_spam - spammable.user_agent_detail.update_attribute(:submitted, true) + target.user_agent_detail.update_attribute(:submitted, true) end end end diff --git a/app/services/spam/spam_check_service.rb b/app/services/spam/spam_check_service.rb index d19ef03976f..a922b0d96e7 100644 --- a/app/services/spam/spam_check_service.rb +++ b/app/services/spam/spam_check_service.rb @@ -4,11 +4,11 @@ module Spam class SpamCheckService include AkismetMethods - attr_accessor :spammable, :request, :options + attr_accessor :target, :request, :options attr_reader :spam_log - def initialize(spammable:, request:) - @spammable = spammable + def initialize(target:, request:) + @target = target @request = request @options = {} @@ -17,8 +17,8 @@ module Spam @options[:user_agent] = @request.env['HTTP_USER_AGENT'] @options[:referrer] = @request.env['HTTP_REFERRER'] else - @options[:ip_address] = @spammable.ip_address - @options[:user_agent] = @spammable.user_agent + @options[:ip_address] = @target.ip_address + @options[:user_agent] = @target.user_agent end end @@ -29,10 +29,10 @@ module Spam SpamLog.verify_recaptcha!(user_id: user_id, id: spam_log_id) else # Otherwise, it goes to Akismet for spam check. - # If so, it assigns spammable object as "spam" and creates a SpamLog record. + # If so, it assigns target spammable object as "spam" and creates a SpamLog record. possible_spam = check(api) - spammable.spam = possible_spam unless spammable.allow_possible_spam? - spammable.spam_log = spam_log + target.spam = possible_spam unless target.allow_possible_spam? + target.spam_log = spam_log end end @@ -48,18 +48,18 @@ module Spam end def check_for_spam? - spammable.check_for_spam? + target.check_for_spam? end def create_spam_log(api) @spam_log = SpamLog.create!( { - user_id: spammable.author_id, - title: spammable.spam_title, - description: spammable.spam_description, + user_id: target.author_id, + title: target.spam_title, + description: target.spam_description, source_ip: options[:ip_address], user_agent: options[:user_agent], - noteable_type: spammable.class.to_s, + noteable_type: target.class.to_s, via_api: api } ) diff --git a/changelogs/unreleased/refactoring-entities-file-16.yml b/changelogs/unreleased/refactoring-entities-file-16.yml new file mode 100644 index 00000000000..135440f84b9 --- /dev/null +++ b/changelogs/unreleased/refactoring-entities-file-16.yml @@ -0,0 +1,5 @@ +--- +title: Separate service entities into own class files +merge_request: 24936 +author: Rajendra Kadam +type: added diff --git a/danger/commit_messages/Dangerfile b/danger/commit_messages/Dangerfile index d6eb050930c..2db49f60710 100644 --- a/danger/commit_messages/Dangerfile +++ b/danger/commit_messages/Dangerfile @@ -89,9 +89,9 @@ def lint_commits(commits) end if squash_mr? - multi_line_commit_linter = commit_linters.detect { |commit_linter| commit_linter.multi_line? } + multi_line_commit_linter = commit_linters.detect { |commit_linter| !commit_linter.merge? && commit_linter.multi_line? } - if multi_line_commit_linter && multi_line_commit_linter.lint.failed? + if multi_line_commit_linter && multi_line_commit_linter.failed? warn_or_fail_commits(multi_line_commit_linter) fail_message('The commit message that will be used in the squash commit does not meet our Git commit message standards.') else diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 5c46a9f21e8..aa2cbd3fed3 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -163,78 +163,6 @@ module API end end - class GlobalNotificationSetting < NotificationSetting - expose :notification_email do |notification_setting, options| - notification_setting.user.notification_email - end - end - - class ProjectServiceBasic < Grape::Entity - expose :id, :title - expose :slug do |service| - service.to_param.dasherize - end - expose :created_at, :updated_at, :active - expose :commit_events, :push_events, :issues_events, :confidential_issues_events - expose :merge_requests_events, :tag_push_events, :note_events - expose :confidential_note_events, :pipeline_events, :wiki_page_events - expose :job_events, :comment_on_event_enabled - end - - class ProjectService < ProjectServiceBasic - # Expose serialized properties - expose :properties do |service, options| - # TODO: Simplify as part of https://gitlab.com/gitlab-org/gitlab/issues/29404 - if service.data_fields_present? - service.data_fields.as_json.slice(*service.api_field_names) - else - service.properties.slice(*service.api_field_names) - end - end - end - - class ProjectWithAccess < Project - expose :permissions do - expose :project_access, using: Entities::ProjectAccess do |project, options| - if options[:project_members] - options[:project_members].find { |member| member.source_id == project.id } - else - project.project_member(options[:current_user]) - end - end - - expose :group_access, using: Entities::GroupAccess do |project, options| - if project.group - if options[:group_members] - options[:group_members].find { |member| member.source_id == project.namespace_id } - else - project.group.highest_group_member(options[:current_user]) - end - end - end - end - - # rubocop: disable CodeReuse/ActiveRecord - def self.preload_relation(projects_relation, options = {}) - relation = super(projects_relation, options) - project_ids = relation.select('projects.id') - namespace_ids = relation.select(:namespace_id) - - options[:project_members] = options[:current_user] - .project_members - .where(source_id: project_ids) - .preload(:source, user: [notification_settings: :source]) - - options[:group_members] = options[:current_user] - .group_members - .where(source_id: namespace_ids) - .preload(:source, user: [notification_settings: :source]) - - relation - end - # rubocop: enable CodeReuse/ActiveRecord - end - class LabelBasic < Grape::Entity expose :id, :name, :color, :description, :description_html, :text_color end diff --git a/lib/api/entities/global_notification_setting.rb b/lib/api/entities/global_notification_setting.rb new file mode 100644 index 00000000000..f3ca64347f0 --- /dev/null +++ b/lib/api/entities/global_notification_setting.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module API + module Entities + class GlobalNotificationSetting < Entities::NotificationSetting + expose :notification_email do |notification_setting, options| + notification_setting.user.notification_email + end + end + end +end diff --git a/lib/api/entities/project_service.rb b/lib/api/entities/project_service.rb new file mode 100644 index 00000000000..947cec1e3cd --- /dev/null +++ b/lib/api/entities/project_service.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module API + module Entities + class ProjectService < Entities::ProjectServiceBasic + # Expose serialized properties + expose :properties do |service, options| + # TODO: Simplify as part of https://gitlab.com/gitlab-org/gitlab/issues/29404 + if service.data_fields_present? + service.data_fields.as_json.slice(*service.api_field_names) + else + service.properties.slice(*service.api_field_names) + end + end + end + end +end diff --git a/lib/api/entities/project_service_basic.rb b/lib/api/entities/project_service_basic.rb new file mode 100644 index 00000000000..eb97ca69a82 --- /dev/null +++ b/lib/api/entities/project_service_basic.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module API + module Entities + class ProjectServiceBasic < Grape::Entity + expose :id, :title + expose :slug do |service| + service.to_param.dasherize + end + expose :created_at, :updated_at, :active + expose :commit_events, :push_events, :issues_events, :confidential_issues_events + expose :merge_requests_events, :tag_push_events, :note_events + expose :confidential_note_events, :pipeline_events, :wiki_page_events + expose :job_events, :comment_on_event_enabled + end + end +end diff --git a/lib/api/entities/project_with_access.rb b/lib/api/entities/project_with_access.rb new file mode 100644 index 00000000000..c53a712a879 --- /dev/null +++ b/lib/api/entities/project_with_access.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +module API + module Entities + class ProjectWithAccess < Project + expose :permissions do + expose :project_access, using: Entities::ProjectAccess do |project, options| + if options[:project_members] + options[:project_members].find { |member| member.source_id == project.id } + else + project.project_member(options[:current_user]) + end + end + + expose :group_access, using: Entities::GroupAccess do |project, options| + if project.group + if options[:group_members] + options[:group_members].find { |member| member.source_id == project.namespace_id } + else + project.group.highest_group_member(options[:current_user]) + end + end + end + end + + # rubocop: disable CodeReuse/ActiveRecord + def self.preload_relation(projects_relation, options = {}) + relation = super(projects_relation, options) + project_ids = relation.select('projects.id') + namespace_ids = relation.select(:namespace_id) + + options[:project_members] = options[:current_user] + .project_members + .where(source_id: project_ids) + .preload(:source, user: [notification_settings: :source]) + + options[:group_members] = options[:current_user] + .group_members + .where(source_id: namespace_ids) + .preload(:source, user: [notification_settings: :source]) + + relation + end + # rubocop: enable CodeReuse/ActiveRecord + end + end +end diff --git a/spec/services/spam/ham_service_spec.rb b/spec/services/spam/ham_service_spec.rb index 9848f48def2..a4583108dba 100644 --- a/spec/services/spam/ham_service_spec.rb +++ b/spec/services/spam/ham_service_spec.rb @@ -4,10 +4,10 @@ require 'spec_helper' describe Spam::HamService do let_it_be(:user) { create(:user) } - let!(:spam_log) { create(:spam_log, user: user, submitted_as_ham: false) } + let!(:target) { create(:spam_log, user: user, submitted_as_ham: false) } let(:fake_akismet_service) { double(:akismet_service) } - subject { described_class.new(spam_log) } + subject { described_class.new(target) } before do allow(Spam::AkismetService).to receive(:new).and_return fake_akismet_service @@ -24,23 +24,23 @@ describe Spam::HamService do end it 'does not update the record' do - expect { subject.execute }.not_to change { spam_log.submitted_as_ham } + expect { subject.execute }.not_to change { target.submitted_as_ham } end context 'if spam log record has already been marked as spam' do before do - spam_log.update_attribute(:submitted_as_ham, true) + target.update_attribute(:submitted_as_ham, true) end it 'does not update the record' do - expect { subject.execute }.not_to change { spam_log.submitted_as_ham } + expect { subject.execute }.not_to change { target.submitted_as_ham } end end end context 'Akismet ham submission is successful' do before do - spam_log.update_attribute(:submitted_as_ham, false) + target.update_attribute(:submitted_as_ham, false) allow(fake_akismet_service).to receive(:submit_ham).and_return true end @@ -49,7 +49,7 @@ describe Spam::HamService do end it 'updates the record' do - expect { subject.execute }.to change { spam_log.submitted_as_ham }.from(false).to(true) + expect { subject.execute }.to change { target.submitted_as_ham }.from(false).to(true) end end end diff --git a/spec/services/spam/mark_as_spam_service_spec.rb b/spec/services/spam/mark_as_spam_service_spec.rb index cba9d6a39cb..3824a8244bb 100644 --- a/spec/services/spam/mark_as_spam_service_spec.rb +++ b/spec/services/spam/mark_as_spam_service_spec.rb @@ -4,19 +4,19 @@ require 'spec_helper' describe Spam::MarkAsSpamService do let(:user_agent_detail) { build(:user_agent_detail) } - let(:spammable) { build(:issue, user_agent_detail: user_agent_detail) } + let(:target) { build(:issue, user_agent_detail: user_agent_detail) } let(:fake_akismet_service) { double(:akismet_service, submit_spam: true) } - subject { described_class.new(spammable: spammable) } + subject { described_class.new(target: target) } describe '#execute' do before do allow(subject).to receive(:akismet).and_return(fake_akismet_service) end - context 'when the spammable object is not submittable' do + context 'when the target object is not submittable' do before do - allow(spammable).to receive(:submittable_as_spam?).and_return false + allow(target).to receive(:submittable_as_spam?).and_return false end it 'does not submit as spam' do @@ -26,7 +26,7 @@ describe Spam::MarkAsSpamService do context 'spam is submitted successfully' do before do - allow(spammable).to receive(:submittable_as_spam?).and_return true + allow(target).to receive(:submittable_as_spam?).and_return true allow(fake_akismet_service).to receive(:submit_spam).and_return true end @@ -34,14 +34,14 @@ describe Spam::MarkAsSpamService do expect(subject.execute).to be_truthy end - it "updates the spammable object's user agent detail as being submitted as spam" do + it "updates the target object's user agent detail as being submitted as spam" do expect(user_agent_detail).to receive(:update_attribute) subject.execute end context 'when Akismet does not consider it spam' do - it 'does not update the spammable object as spam' do + it 'does not update the target object as spam' do allow(fake_akismet_service).to receive(:submit_spam).and_return false expect(subject.execute).to be_falsey diff --git a/spec/services/spam/spam_check_service_spec.rb b/spec/services/spam/spam_check_service_spec.rb index 732b64b52a0..376afc78b5f 100644 --- a/spec/services/spam/spam_check_service_spec.rb +++ b/spec/services/spam/spam_check_service_spec.rb @@ -22,12 +22,12 @@ describe Spam::SpamCheckService do end describe '#initialize' do - subject { described_class.new(spammable: issue, request: request) } + subject { described_class.new(target: issue, request: request) } context 'when the request is nil' do let(:request) { nil } - it 'assembles the options with information from the spammable' do + it 'assembles the options with information from the target' do aggregate_failures do expect(subject.options[:ip_address]).to eq(issue.ip_address) expect(subject.options[:user_agent]).to eq(issue.user_agent) @@ -39,7 +39,7 @@ describe Spam::SpamCheckService do context 'when the request is present' do let(:request) { double(:request, env: env) } - it 'assembles the options with information from the spammable' do + it 'assembles the options with information from the target' do aggregate_failures do expect(subject.options[:ip_address]).to eq(fake_ip) expect(subject.options[:user_agent]).to eq(fake_user_agent) @@ -55,7 +55,7 @@ describe Spam::SpamCheckService do let_it_be(:existing_spam_log) { create(:spam_log, user: user, recaptcha_verified: false) } subject do - described_service = described_class.new(spammable: issue, request: request) + described_service = described_class.new(target: issue, request: request) described_service.execute(user_id: user.id, api: nil, recaptcha_verified: recaptcha_verified, spam_log_id: existing_spam_log.id) end @@ -81,7 +81,7 @@ describe Spam::SpamCheckService do context 'when recaptcha was not verified' do let(:recaptcha_verified) { false } - context 'when spammable attributes have not changed' do + context 'when target attributes have not changed' do before do issue.closed_at = Time.zone.now @@ -98,7 +98,7 @@ describe Spam::SpamCheckService do end end - context 'when spammable attributes have changed' do + context 'when target attributes have changed' do before do issue.description = 'SPAM!' end diff --git a/spec/support/shared_examples/spam_check_shared_examples.rb b/spec/support/shared_examples/spam_check_shared_examples.rb index 3ecae16db39..a16934b7d77 100644 --- a/spec/support/shared_examples/spam_check_shared_examples.rb +++ b/spec/support/shared_examples/spam_check_shared_examples.rb @@ -2,7 +2,7 @@ shared_examples 'akismet spam' do context 'when request is missing' do - subject { described_class.new(spammable: issue, request: nil) } + subject { described_class.new(target: issue, request: nil) } it "doesn't check as spam" do subject diff --git a/yarn.lock b/yarn.lock index 1a4a8f42074..cffb5809a2b 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2446,9 +2446,9 @@ camelcase@^5.0.0: integrity sha512-L28STB170nwWS63UjtlEOE3dldQApaJXZkOI1uMFfzf3rRuPegHaHesyee+YxQ+W6SvRDQV6UrdOdRiR153wJg== caniuse-lite@^1.0.30000980, caniuse-lite@^1.0.30000984: - version "1.0.30000985" - resolved "https://registry.yarnpkg.com/caniuse-lite/-/caniuse-lite-1.0.30000985.tgz#0eb40f6c8a8c219155cbe43c4975c0efb4a0f77f" - integrity sha512-1ngiwkgqAYPG0JSSUp3PUDGPKKY59EK7NrGGX+VOxaKCNzRbNc7uXMny+c3VJfZxtoK3wSImTvG9T9sXiTw2+w== + version "1.0.30001025" + resolved "https://registry.yarnpkg.com/caniuse-lite/-/caniuse-lite-1.0.30001025.tgz#30336a8aca7f98618eb3cf38e35184e13d4e5fe6" + integrity sha512-SKyFdHYfXUZf5V85+PJgLYyit27q4wgvZuf8QTOk1osbypcROihMBlx9GRar2/pIcKH2r4OehdlBr9x6PXetAQ== capture-exit@^2.0.0: version "2.0.0"