From d0dcae47768e670482cf8a1441cce764ae046b35 Mon Sep 17 00:00:00 2001
From: Mantas Masalskis <mm@zammad.com>
Date: Tue, 16 Mar 2021 07:50:12 +0000
Subject: [PATCH] Refactoring: Use Rails validation callback to ensure valid
 Webhook endpoint and extended test coverage.

---
 app/models/webhook.rb       | 10 ++++-----
 spec/models/webhook_spec.rb | 41 ++++++++++++++++++++++++++++---------
 2 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/app/models/webhook.rb b/app/models/webhook.rb
index 232d05141..716ae5b1b 100644
--- a/app/models/webhook.rb
+++ b/app/models/webhook.rb
@@ -5,19 +5,19 @@ class Webhook < ApplicationModel
   include ChecksLatestChangeObserved
   include HasCollectionUpdate
 
-  before_create  :validate_endpoint
-  before_update  :validate_endpoint
   before_destroy Webhook::EnsureNoRelatedObjects
 
   validates :name, presence: true
+  validate :validate_endpoint
 
   private
 
   def validate_endpoint
     uri = URI.parse(endpoint)
-    raise Exceptions::UnprocessableEntity, 'Invalid endpoint (no http/https)!' if !uri.is_a?(URI::HTTP)
-    raise Exceptions::UnprocessableEntity, 'Invalid endpoint (no hostname)!' if uri.host.nil?
+
+    errors.add(:endpoint, 'Invalid endpoint (no http/https)!') if !uri.is_a?(URI::HTTP)
+    errors.add(:endpoint, 'Invalid endpoint (no hostname)!') if uri.host.nil?
   rescue URI::InvalidURIError
-    raise Exceptions::UnprocessableEntity, 'Invalid endpoint!'
+    errors.add :endpoint, 'Invalid endpoint!'
   end
 end
diff --git a/spec/models/webhook_spec.rb b/spec/models/webhook_spec.rb
index 0a5ccb829..3846367c0 100644
--- a/spec/models/webhook_spec.rb
+++ b/spec/models/webhook_spec.rb
@@ -3,40 +3,61 @@ require 'rails_helper'
 RSpec.describe Webhook, type: :model do
 
   describe 'check endpoint' do
-    subject(:webhook) { create(:webhook, endpoint: endpoint) }
+    subject(:webhook) { build(:webhook, endpoint: endpoint) }
 
-    let(:endpoint) { 'example.com' }
+    before { webhook.valid? }
+
+    let(:endpoint_errors) { webhook.errors.messages[:endpoint] }
 
     context 'with missing http type' do
-      it 'raise an error' do
-        expect { webhook }.to raise_error(Exceptions::UnprocessableEntity, 'Invalid endpoint (no http/https)!')
+      let(:endpoint) { 'example.com' }
+
+      it { is_expected.not_to be_valid }
+
+      it 'has an error' do
+        expect(endpoint_errors).to include 'Invalid endpoint (no http/https)!'
       end
     end
 
     context 'with spaces in invalid hostname' do
       let(:endpoint) { 'http://   example.com' }
 
-      it 'raise an error' do
-        expect { webhook }.to raise_error(Exceptions::UnprocessableEntity, 'Invalid endpoint!')
+      it { is_expected.not_to be_valid }
+
+      it 'has an error' do
+        expect(endpoint_errors).to include 'Invalid endpoint!'
       end
     end
 
     context 'with ? in hostname' do
       let(:endpoint) { 'http://?example.com' }
 
-      it 'raise an error' do
-        expect { webhook }.to raise_error(Exceptions::UnprocessableEntity, 'Invalid endpoint (no hostname)!')
+      it { is_expected.not_to be_valid }
+
+      it 'has an error' do
+        expect(endpoint_errors).to include 'Invalid endpoint (no hostname)!'
       end
     end
 
     context 'with nil in endpoint' do
       let(:endpoint) { nil }
 
-      it 'raise an error' do
-        expect { webhook }.to raise_error(Exceptions::UnprocessableEntity, 'Invalid endpoint!')
+      it { is_expected.not_to be_valid }
+
+      it 'has an error' do
+        expect(endpoint_errors).to include 'Invalid endpoint!'
       end
     end
 
+    context 'with a valid endpoint' do
+      let(:endpoint) { 'https://example.com/endpoint' }
+
+      it { is_expected.to be_valid }
+
+      it 'has no errors' do
+        expect(endpoint_errors).to be_empty
+      end
+    end
   end
 
   describe '#destroy' do