Merge branch '27954-remove-hipchat-service' into 'master'

Remove HipChat service and records

See merge request gitlab-org/gitlab!59769

app/controllers/concerns/service_params.rb

@@ -9,7 +9,6 @@ module ServiceParams
     :alert_events,
     :api_key,
     :api_url,
-    :api_version,
     :bamboo_url,
     :branches_to_be_notified,
     :labels_to_be_notified,
@@ -52,7 +51,6 @@ module ServiceParams
     :mock_service_url,
     :namespace,
     :new_issue_url,
-    :notify,
     :notify_only_broken_pipelines,
     :password,
     :priority,

app/models/project.rb

@@ -161,7 +161,6 @@ class Project < ApplicationRecord
   has_one :pipelines_email_service
   has_one :irker_service
   has_one :pivotaltracker_service
-  has_one :hipchat_service
   has_one :flowdock_service
   has_one :assembla_service
   has_one :asana_service

app/models/project_services/hipchat_service.rb

 # frozen_string_literal: true
 
+# This service is scheduled for removal. All records must
+# be deleted before the class can be removed.
+# https://gitlab.com/gitlab-org/gitlab/-/issues/27954
 class HipchatService < Service
-  include ActionView::Helpers::SanitizeHelper
-
-  MAX_COMMITS = 3
-  HIPCHAT_ALLOWED_TAGS = %w[
-    a b i strong em br img pre code
-    table th tr td caption colgroup col thead tbody tfoot
-    ul ol li dl dt dd
-  ].freeze
-
-  prop_accessor :token, :room, :server, :color, :api_version
-  boolean_accessor :notify_only_broken_pipelines, :notify
-  validates :token, presence: true, if: :activated?
-
-  def initialize_properties
-    if properties.nil?
-      self.properties = {}
-      self.notify_only_broken_pipelines = true
-    end
-  end
-
-  def title
-    'HipChat'
-  end
-
-  def description
-    'Private group chat and IM'
-  end
+  before_save :prevent_save
 
   def self.to_param
     'hipchat'
   end
 
-  def fields
-    [
-      { type: 'text', name: 'token',     placeholder: 'Room token', required: true },
-      { type: 'text', name: 'room',      placeholder: 'Room name or ID' },
-      { type: 'checkbox', name: 'notify' },
-      { type: 'select', name: 'color', choices: %w(yellow red green purple gray random) },
-      { type: 'text', name: 'api_version', title: _('API version'),
-        placeholder: 'Leave blank for default (v2)' },
-      { type: 'text', name: 'server',
-        placeholder: 'Leave blank for default. https://hipchat.example.com' },
-      { type: 'checkbox', name: 'notify_only_broken_pipelines' }
-    ]
-  end
-
   def self.supported_events
-    %w(push issue confidential_issue merge_request note confidential_note tag_push pipeline)
+    []
   end
 
   def execute(data)
@@ -56,96 +19,14 @@ class HipchatService < Service
     # HipChat is unusable anyway, so do nothing in this method
   end
 
-  def test(data)
-    begin
-      result = execute(data)
-    rescue StandardError => error
-      return { success: false, result: error }
-    end
-
-    { success: true, result: result }
-  end
-
   private
 
-  def message_options(data = nil)
-    { notify: notify.present? && Gitlab::Utils.to_boolean(notify), color: message_color(data) }
-  end
-
-  def render_line(text)
-    markdown(text.lines.first.chomp, pipeline: :single_line) if text
-  end
-
-  def markdown(text, options = {})
-    return "" unless text
-
-    context = {
-      project: project,
-      pipeline: :email
-    }
-
-    Banzai.render(text, context)
-
-    context.merge!(options)
-
-    html = Banzai.render_and_post_process(text, context)
-    sanitized_html = sanitize(html, tags: HIPCHAT_ALLOWED_TAGS, attributes: %w[href title alt])
+  def prevent_save
+    errors.add(:base, _('HipChat endpoint is deprecated and should not be created or modified.'))
 
-    sanitized_html.truncate(200, separator: ' ', omission: '...')
-  end
-
-  def format_title(title)
-    "<b>#{render_line(title)}</b>"
-  end
-
-  def message_color(data)
-    pipeline_status_color(data) || color || 'yellow'
-  end
-
-  def pipeline_status_color(data)
-    return unless data && data[:object_kind] == 'pipeline'
-
-    case data[:object_attributes][:status]
-    when 'success'
-      'green'
-    else
-      'red'
-    end
-  end
-
-  def project_name
-    project.full_name.gsub(/\s/, '')
-  end
-
-  def project_url
-    project.web_url
-  end
-
-  def project_link
-    "<a href=\"#{project_url}\">#{project_name}</a>"
-  end
-
-  def update?(data)
-    data[:object_attributes][:action] == 'update'
-  end
-
-  def humanized_status(status)
-    case status
-    when 'success'
-      'passed'
-    else
-      status
-    end
-  end
-
-  def should_pipeline_be_notified?(data)
-    case data[:object_attributes][:status]
-    when 'success'
-      !notify_only_broken_pipelines?
-    when 'failed'
-      true
-    else
-      false
-    end
+    # Stops execution of callbacks and database operation while
+    # preserving expectations of #save (will not raise) & #save! (raises)
+    # https://guides.rubyonrails.org/active_record_callbacks.html#halting-execution
+    throw :abort # rubocop:disable Cop/BanCatchThrow
   end
 end

changelogs/unreleased/27954-remove-hipchat-service.yml

+---
+title: Delete HipChat service database records
+merge_request: 59769
+author:
+type: removed

db/post_migrate/20210420103955_remove_hipchat_service_records.rb

+# frozen_string_literal: true
+
+class RemoveHipchatServiceRecords < ActiveRecord::Migration[6.0]
+  disable_ddl_transaction!
+
+  class Service < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'services'
+  end
+
+  def up
+    Service.each_batch(of: 100_000, column: :id) do |relation|
+      relation.delete_by(type: 'HipchatService')
+    end
+  end
+
+  def down
+    # no-op
+  end
+end

db/schema_migrations/20210420103955

+1ee3df66a0e7d1802196740cc1c0a899724c1f5b3dd8be4316915b354446f238
\ No newline at end of file

lib/api/helpers/services_helpers.rb

@@ -420,44 +420,6 @@ module API
             },
             chat_notification_events
           ].flatten,
-          'hipchat' => [
-            {
-              required: true,
-              name: :token,
-              type: String,
-              desc: 'The room token'
-            },
-            {
-              required: false,
-              name: :room,
-              type: String,
-              desc: 'The room name or ID'
-            },
-            {
-              required: false,
-              name: :color,
-              type: String,
-              desc: 'The room color'
-            },
-            {
-              required: false,
-              name: :notify,
-              type: Boolean,
-              desc: 'Enable notifications'
-            },
-            {
-              required: false,
-              name: :api_version,
-              type: String,
-              desc: 'Leave blank for default (v2)'
-            },
-            {
-              required: false,
-              name: :server,
-              type: String,
-              desc: 'Leave blank for default. https://hipchat.example.com'
-            }
-          ],
           'irker' => [
             {
               required: true,
@@ -828,7 +790,6 @@ module API
           ::ExternalWikiService,
           ::FlowdockService,
           ::HangoutsChatService,
-          ::HipchatService,
           ::IrkerService,
           ::JenkinsService,
           ::JiraService,

locale/gitlab.pot

@@ -1490,9 +1490,6 @@ msgstr ""
 msgid "API key"
 msgstr ""
 
-msgid "API version"
-msgstr ""
-
 msgid "API?"
 msgstr ""
 
@@ -16010,6 +16007,9 @@ msgstr ""
 msgid "HighlightBar|Time to SLA:"
 msgstr ""
 
+msgid "HipChat endpoint is deprecated and should not be created or modified."
+msgstr ""
+
 msgid "History"
 msgstr ""
 

spec/factories/services.rb

@@ -159,12 +159,6 @@ FactoryBot.define do
     password { 'my-secret-password' }
   end
 
-  factory :hipchat_service do
-    project
-    type { 'HipchatService' }
-    token { 'test_token' }
-  end
-
   factory :slack_service do
     project
     active { true }

spec/lib/gitlab/import_export/all_models.yml

@@ -369,7 +369,6 @@ project:
 - packagist_service
 - pivotaltracker_service
 - prometheus_service
-- hipchat_service
 - flowdock_service
 - assembla_service
 - asana_service

spec/migrations/remove_hipchat_service_records_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20210420103955_remove_hipchat_service_records.rb')
+
+RSpec.describe RemoveHipchatServiceRecords do
+  let(:services) { table(:services) }
+
+  before do
+    services.create!(type: 'HipchatService')
+    services.create!(type: 'SomeOtherType')
+  end
+
+  it 'removes services records of type HipchatService' do
+    expect(services.count).to eq(2)
+
+    migrate!
+
+    expect(services.count).to eq(1)
+    expect(services.first.type).to eq('SomeOtherType')
+    expect(services.where(type: 'HipchatService')).to be_empty
+  end
+end

spec/models/project_services/hipchat_service_spec.rb

@@ -2,91 +2,35 @@
 
 require 'spec_helper'
 
+# HipchatService is partially removed and it will be remove completely
+# after the deletion of all the database records.
+# https://gitlab.com/gitlab-org/gitlab/-/issues/27954
 RSpec.describe HipchatService do
-  describe "Associations" do
-    it { is_expected.to belong_to :project }
-    it { is_expected.to have_one :service_hook }
-  end
-
-  describe 'Validations' do
-    context 'when service is active' do
-      before do
-        subject.active = true
-      end
-
-      it { is_expected.to validate_presence_of(:token) }
-    end
-
-    context 'when service is inactive' do
-      before do
-        subject.active = false
-      end
-
-      it { is_expected.not_to validate_presence_of(:token) }
-    end
-  end
-
-  describe "Execute" do
-    let(:hipchat) { described_class.new }
-    let(:user)    { create(:user) }
-    let(:project) { create(:project, :repository) }
-    let(:api_url) { 'https://hipchat.example.com/v2/room/123456/notification?auth_token=verySecret' }
-    let(:project_name) { project.full_name.gsub(/\s/, '') }
-    let(:token) { 'verySecret' }
-    let(:server_url) { 'https://hipchat.example.com'}
-    let(:push_sample_data) do
-      Gitlab::DataBuilder::Push.build_sample(project, user)
-    end
+  let_it_be(:project) { create(:project) }
 
-    before do
-      allow(hipchat).to receive_messages(
-        project_id: project.id,
-        project: project,
-        room: 123456,
-        server: server_url,
-        token: token
-      )
-      WebMock.stub_request(:post, api_url)
-    end
-
-    it 'does nothing' do
-      expect { hipchat.execute(push_sample_data) }.not_to raise_error
-    end
-
-    describe "#message_options" do
-      it "is set to the defaults" do
-        expect(hipchat.__send__(:message_options)).to eq({ notify: false, color: 'yellow' })
-      end
+  subject(:service) { described_class.new(project: project) }
 
-      it "sets notify to true" do
-        allow(hipchat).to receive(:notify).and_return('1')
+  it { is_expected.to be_valid }
 
-        expect(hipchat.__send__(:message_options)).to eq({ notify: true, color: 'yellow' })
-      end
-
-      it "sets the color" do
-        allow(hipchat).to receive(:color).and_return('red')
+  describe '#to_param' do
+    subject { service.to_param }
 
-        expect(hipchat.__send__(:message_options)).to eq({ notify: false, color: 'red' })
+    it { is_expected.to eq('hipchat') }
   end
 
-      context 'with a successful build' do
-        it 'uses the green color' do
-          data = { object_kind: 'pipeline',
-                   object_attributes: { status: 'success' } }
+  describe '#supported_events' do
+    subject { service.supported_events }
 
-          expect(hipchat.__send__(:message_options, data)).to eq({ notify: false, color: 'green' })
-        end
+    it { is_expected.to be_empty }
   end
 
-      context 'with a failed build' do
-        it 'uses the red color' do
-          data = { object_kind: 'pipeline',
-                   object_attributes: { status: 'failed' } }
+  describe '#save' do
+    it 'prevents records from being created or updated' do
+      expect(service.save).to be_falsey
 
-          expect(hipchat.__send__(:message_options, data)).to eq({ notify: false, color: 'red' })
-        end
-      end
+      expect(service.errors.full_messages).to include(
+        'HipChat endpoint is deprecated and should not be created or modified.'
+      )
     end
   end
 end

spec/models/project_spec.rb

@@ -52,7 +52,6 @@ RSpec.describe Project, factory_default: :keep do
     it { is_expected.to have_one(:pipelines_email_service) }
     it { is_expected.to have_one(:irker_service) }
     it { is_expected.to have_one(:pivotaltracker_service) }
-    it { is_expected.to have_one(:hipchat_service) }
     it { is_expected.to have_one(:flowdock_service) }
     it { is_expected.to have_one(:assembla_service) }
     it { is_expected.to have_one(:slack_slash_commands_service) }