From 66dd70cc3a4cfacb6f2dae596c5b91d84530467e Mon Sep 17 00:00:00 2001
From: rpereira2 <rpereira@gitlab.com>
Date: Wed, 6 Feb 2019 12:05:27 +0530
Subject: Add back reactive caching

---
 .../project_error_tracking_setting.rb              | 18 +++++-
 .../error_tracking/list_projects_service.rb        | 15 +++--
 .../project_error_tracking_setting_spec.rb         | 60 +++++++++++++++--
 .../error_tracking/list_projects_service_spec.rb   | 75 ++++++++++++++++------
 4 files changed, 135 insertions(+), 33 deletions(-)

diff --git a/app/models/error_tracking/project_error_tracking_setting.rb b/app/models/error_tracking/project_error_tracking_setting.rb
index 31084c54bdc..8fb70f080d7 100644
--- a/app/models/error_tracking/project_error_tracking_setting.rb
+++ b/app/models/error_tracking/project_error_tracking_setting.rb
@@ -63,13 +63,29 @@ module ErrorTracking
     end
 
     def list_sentry_projects
-      { projects: sentry_client.list_projects }
+      with_reactive_cache('list_projects', {}) do |result|
+        result
+      end
     end
 
     def calculate_reactive_cache(request, opts)
       case request
       when 'list_issues'
         sentry_client.list_issues(**opts.symbolize_keys)
+      when 'list_projects'
+        begin
+          { projects: sentry_client.list_projects }
+        rescue Sentry::Client::SentryError => e
+          {
+            error: e.message,
+            http_status: :unprocessable_entity
+          }
+        rescue Sentry::Client::Error => e
+          {
+            error: e.message,
+            http_status: :bad_request
+          }
+        end
       end
     end
 
diff --git a/app/services/error_tracking/list_projects_service.rb b/app/services/error_tracking/list_projects_service.rb
index c6e8be0f2be..f2d0904a41f 100644
--- a/app/services/error_tracking/list_projects_service.rb
+++ b/app/services/error_tracking/list_projects_service.rb
@@ -11,12 +11,15 @@ module ErrorTracking
         return error(setting.errors.full_messages.join(', '), :bad_request)
       end
 
-      begin
-        result = setting.list_sentry_projects
-      rescue Sentry::Client::Error => e
-        return error(e.message, :bad_request)
-      rescue Sentry::Client::SentryError => e
-        return error(e.message, :unprocessable_entity)
+      result = setting.list_sentry_projects
+
+      # our results are not yet ready
+      if result.nil?
+        return error('not ready', :no_content)
+      end
+
+      if result[:error].present?
+        return error(result[:error], result[:http_status] || :bad_request)
       end
 
       success(projects: result[:projects])
diff --git a/spec/models/error_tracking/project_error_tracking_setting_spec.rb b/spec/models/error_tracking/project_error_tracking_setting_spec.rb
index d30228b863c..65f849fa5a4 100644
--- a/spec/models/error_tracking/project_error_tracking_setting_spec.rb
+++ b/spec/models/error_tracking/project_error_tracking_setting_spec.rb
@@ -149,15 +149,63 @@ describe ErrorTracking::ProjectErrorTrackingSetting do
 
   describe '#list_sentry_projects' do
     let(:projects) { [:list, :of, :projects] }
-    let(:sentry_client) { spy(:sentry_client) }
 
-    it 'calls sentry client' do
-      expect(subject).to receive(:sentry_client).and_return(sentry_client)
-      expect(sentry_client).to receive(:list_projects).and_return(projects)
+    let(:result) do
+      subject.list_sentry_projects
+    end
 
-      result = subject.list_sentry_projects
+    context 'when cached' do
+      let(:sentry_client) { spy(:sentry_client) }
 
-      expect(result).to eq(projects: projects)
+      before do
+        stub_reactive_cache(subject, projects, {})
+        synchronous_reactive_cache(subject)
+
+        expect(subject).to receive(:sentry_client).and_return(sentry_client)
+        expect(sentry_client).to receive(:list_projects)
+          .and_return(projects)
+      end
+
+      it 'returns cached projects' do
+        expect(result).to eq(projects: projects)
+      end
+    end
+
+    context 'when not cached' do
+      it 'returns nil' do
+        expect(subject).not_to receive(:sentry_client)
+        expect(result).to be_nil
+      end
+    end
+
+    context 'when sentry client raises exception' do
+      let(:sentry_client) { spy(:sentry_client) }
+
+      before do
+        synchronous_reactive_cache(subject)
+
+        expect(subject).to receive(:sentry_client).and_return(sentry_client)
+      end
+
+      it 'returns error for Sentry::Client::Error exception' do
+        expect(sentry_client).to receive(:list_projects)
+          .and_raise(Sentry::Client::Error, 'error message')
+
+        expect(result).to eq(
+          error: 'error message',
+          http_status: :bad_request
+        )
+      end
+
+      it 'returns error for Sentry::Client::SentryError exception' do
+        expect(sentry_client).to receive(:list_projects)
+          .and_raise(Sentry::Client::SentryError, 'error message')
+
+        expect(result).to eq(
+          error: 'error message',
+          http_status: :unprocessable_entity
+        )
+      end
     end
   end
 
diff --git a/spec/services/error_tracking/list_projects_service_spec.rb b/spec/services/error_tracking/list_projects_service_spec.rb
index eca99273007..9b0bcbf2ee4 100644
--- a/spec/services/error_tracking/list_projects_service_spec.rb
+++ b/spec/services/error_tracking/list_projects_service_spec.rb
@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe ErrorTracking::ListProjectsService do
+  include ReactiveCachingHelpers
+
   set(:user) { create(:user) }
   set(:project) { create(:project) }
 
@@ -26,32 +28,29 @@ describe ErrorTracking::ListProjectsService do
     let(:result) { subject.execute }
 
     context 'with authorized user' do
-      let(:sentry_client) { spy(:sentry_client) }
-
       before do
         expect(project).to receive(:error_tracking_setting).at_least(:once)
           .and_return(error_tracking_setting)
       end
 
       context 'call sentry client' do
+        let(:sentry_client) { spy(:sentry_client) }
+
+        before do
+          synchronous_reactive_cache(error_tracking_setting)
+        end
+
         it 'uses new api_url and token' do
           expect(Sentry::Client).to receive(:new)
-            .with(new_api_host + 'api/0/projects/', new_token).and_return(sentry_client)
+            .with(new_api_host + 'api/0/projects/', new_token)
+            .and_return(sentry_client)
           expect(sentry_client).to receive(:list_projects).and_return([])
 
           subject.execute
-        end
-      end
 
-      context 'sentry client raises exception' do
-        before do
-          expect(error_tracking_setting).to receive(:list_sentry_projects)
-            .and_raise(Sentry::Client::Error, 'Sentry response error: 500')
-        end
-
-        it 'returns error response' do
-          expect(result[:message]).to eq('Sentry response error: 500')
-          expect(result[:http_status]).to eq(:bad_request)
+          error_tracking_setting.reload
+          expect(error_tracking_setting.api_url).to eq(sentry_url)
+          expect(error_tracking_setting.token).to eq(token)
         end
       end
 
@@ -85,6 +84,39 @@ describe ErrorTracking::ListProjectsService do
           expect(result).to eq(status: :success, projects: projects)
         end
       end
+
+      context 'when list_sentry_projects returns nil' do
+        before do
+          expect(error_tracking_setting)
+            .to receive(:list_sentry_projects).and_return(nil)
+        end
+
+        it 'result is not ready' do
+          result = subject.execute
+
+          expect(result).to eq(
+            status: :error,
+            http_status: :no_content,
+            message: 'not ready'
+          )
+        end
+      end
+
+      context 'when list_sentry_projects returns empty array' do
+        before do
+          expect(error_tracking_setting)
+            .to receive(:list_sentry_projects).and_return({ projects: [] })
+        end
+
+        it 'returns the empty array' do
+          result = subject.execute
+
+          expect(result).to eq(
+            status: :success,
+            projects: []
+          )
+        end
+      end
     end
 
     context 'with unauthorized user' do
@@ -105,26 +137,29 @@ describe ErrorTracking::ListProjectsService do
           .to receive(:list_sentry_projects).and_return(projects: [])
 
         error_tracking_setting.enabled = false
+        error_tracking_setting.save!
       end
 
       it 'ignores enabled flag' do
         expect(result).to include(status: :success, projects: [])
+
+        error_tracking_setting.reload
+        expect(error_tracking_setting.enabled).to be false
       end
     end
 
     context 'error_tracking_setting is nil' do
-      let(:sentry_client) { spy(:sentry_client) }
+      let(:error_tracking_setting) { build(:project_error_tracking_setting) }
 
       before do
         expect(project).to receive(:error_tracking_setting).at_least(:once)
           .and_return(nil)
 
-        expect(Sentry::Client).to receive(:new)
-          .with(new_api_host + 'api/0/projects/', new_token)
-          .and_return(sentry_client)
+        expect(project).to receive(:build_error_tracking_setting).once
+          .and_return(error_tracking_setting)
 
-        expect(sentry_client).to receive(:list_projects)
-          .and_return([:project1, :project2])
+        expect(error_tracking_setting).to receive(:list_sentry_projects)
+          .and_return(projects: [:project1, :project2])
       end
 
       it 'builds a new error_tracking_setting' do
-- 
cgit v1.2.1


From 0eff270c6b089ec5882dbb63eb4e5cdcc7e87967 Mon Sep 17 00:00:00 2001
From: rpereira2 <rpereira@gitlab.com>
Date: Wed, 6 Feb 2019 14:50:38 +0530
Subject: Don't stub Sentry client

---
 spec/services/error_tracking/list_projects_service_spec.rb | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/spec/services/error_tracking/list_projects_service_spec.rb b/spec/services/error_tracking/list_projects_service_spec.rb
index 9b0bcbf2ee4..df058a18bdb 100644
--- a/spec/services/error_tracking/list_projects_service_spec.rb
+++ b/spec/services/error_tracking/list_projects_service_spec.rb
@@ -33,21 +33,21 @@ describe ErrorTracking::ListProjectsService do
           .and_return(error_tracking_setting)
       end
 
-      context 'call sentry client' do
-        let(:sentry_client) { spy(:sentry_client) }
+      context 'set model attributes to new values' do
+        let(:new_api_url) { new_api_host + 'api/0/projects/' }
 
         before do
           synchronous_reactive_cache(error_tracking_setting)
+
+          expect(error_tracking_setting).to receive(:list_sentry_projects)
+            .and_return({ projects: [] })
         end
 
         it 'uses new api_url and token' do
-          expect(Sentry::Client).to receive(:new)
-            .with(new_api_host + 'api/0/projects/', new_token)
-            .and_return(sentry_client)
-          expect(sentry_client).to receive(:list_projects).and_return([])
-
           subject.execute
 
+          expect(error_tracking_setting.api_url).to eq(new_api_url)
+          expect(error_tracking_setting.token).to eq(new_token)
           error_tracking_setting.reload
           expect(error_tracking_setting.api_url).to eq(sentry_url)
           expect(error_tracking_setting.token).to eq(token)
-- 
cgit v1.2.1


From 8b2422a34a65eab59399a1fd91190849b9ef07b3 Mon Sep 17 00:00:00 2001
From: rpereira2 <rpereira@gitlab.com>
Date: Wed, 6 Feb 2019 18:04:09 +0530
Subject: Revert "Add back reactive caching"

This reverts commit 66dd70cc3a4cfacb6f2dae596c5b91d84530467e.
---
 .../project_error_tracking_setting.rb              | 18 +-----
 .../error_tracking/list_projects_service.rb        | 15 ++---
 .../project_error_tracking_setting_spec.rb         | 60 ++------------------
 .../error_tracking/list_projects_service_spec.rb   | 66 +++++++---------------
 4 files changed, 33 insertions(+), 126 deletions(-)

diff --git a/app/models/error_tracking/project_error_tracking_setting.rb b/app/models/error_tracking/project_error_tracking_setting.rb
index 8fb70f080d7..31084c54bdc 100644
--- a/app/models/error_tracking/project_error_tracking_setting.rb
+++ b/app/models/error_tracking/project_error_tracking_setting.rb
@@ -63,29 +63,13 @@ module ErrorTracking
     end
 
     def list_sentry_projects
-      with_reactive_cache('list_projects', {}) do |result|
-        result
-      end
+      { projects: sentry_client.list_projects }
     end
 
     def calculate_reactive_cache(request, opts)
       case request
       when 'list_issues'
         sentry_client.list_issues(**opts.symbolize_keys)
-      when 'list_projects'
-        begin
-          { projects: sentry_client.list_projects }
-        rescue Sentry::Client::SentryError => e
-          {
-            error: e.message,
-            http_status: :unprocessable_entity
-          }
-        rescue Sentry::Client::Error => e
-          {
-            error: e.message,
-            http_status: :bad_request
-          }
-        end
       end
     end
 
diff --git a/app/services/error_tracking/list_projects_service.rb b/app/services/error_tracking/list_projects_service.rb
index f2d0904a41f..c6e8be0f2be 100644
--- a/app/services/error_tracking/list_projects_service.rb
+++ b/app/services/error_tracking/list_projects_service.rb
@@ -11,15 +11,12 @@ module ErrorTracking
         return error(setting.errors.full_messages.join(', '), :bad_request)
       end
 
-      result = setting.list_sentry_projects
-
-      # our results are not yet ready
-      if result.nil?
-        return error('not ready', :no_content)
-      end
-
-      if result[:error].present?
-        return error(result[:error], result[:http_status] || :bad_request)
+      begin
+        result = setting.list_sentry_projects
+      rescue Sentry::Client::Error => e
+        return error(e.message, :bad_request)
+      rescue Sentry::Client::SentryError => e
+        return error(e.message, :unprocessable_entity)
       end
 
       success(projects: result[:projects])
diff --git a/spec/models/error_tracking/project_error_tracking_setting_spec.rb b/spec/models/error_tracking/project_error_tracking_setting_spec.rb
index 65f849fa5a4..d30228b863c 100644
--- a/spec/models/error_tracking/project_error_tracking_setting_spec.rb
+++ b/spec/models/error_tracking/project_error_tracking_setting_spec.rb
@@ -149,63 +149,15 @@ describe ErrorTracking::ProjectErrorTrackingSetting do
 
   describe '#list_sentry_projects' do
     let(:projects) { [:list, :of, :projects] }
+    let(:sentry_client) { spy(:sentry_client) }
 
-    let(:result) do
-      subject.list_sentry_projects
-    end
+    it 'calls sentry client' do
+      expect(subject).to receive(:sentry_client).and_return(sentry_client)
+      expect(sentry_client).to receive(:list_projects).and_return(projects)
 
-    context 'when cached' do
-      let(:sentry_client) { spy(:sentry_client) }
+      result = subject.list_sentry_projects
 
-      before do
-        stub_reactive_cache(subject, projects, {})
-        synchronous_reactive_cache(subject)
-
-        expect(subject).to receive(:sentry_client).and_return(sentry_client)
-        expect(sentry_client).to receive(:list_projects)
-          .and_return(projects)
-      end
-
-      it 'returns cached projects' do
-        expect(result).to eq(projects: projects)
-      end
-    end
-
-    context 'when not cached' do
-      it 'returns nil' do
-        expect(subject).not_to receive(:sentry_client)
-        expect(result).to be_nil
-      end
-    end
-
-    context 'when sentry client raises exception' do
-      let(:sentry_client) { spy(:sentry_client) }
-
-      before do
-        synchronous_reactive_cache(subject)
-
-        expect(subject).to receive(:sentry_client).and_return(sentry_client)
-      end
-
-      it 'returns error for Sentry::Client::Error exception' do
-        expect(sentry_client).to receive(:list_projects)
-          .and_raise(Sentry::Client::Error, 'error message')
-
-        expect(result).to eq(
-          error: 'error message',
-          http_status: :bad_request
-        )
-      end
-
-      it 'returns error for Sentry::Client::SentryError exception' do
-        expect(sentry_client).to receive(:list_projects)
-          .and_raise(Sentry::Client::SentryError, 'error message')
-
-        expect(result).to eq(
-          error: 'error message',
-          http_status: :unprocessable_entity
-        )
-      end
+      expect(result).to eq(projects: projects)
     end
   end
 
diff --git a/spec/services/error_tracking/list_projects_service_spec.rb b/spec/services/error_tracking/list_projects_service_spec.rb
index df058a18bdb..c3363036c78 100644
--- a/spec/services/error_tracking/list_projects_service_spec.rb
+++ b/spec/services/error_tracking/list_projects_service_spec.rb
@@ -3,8 +3,6 @@
 require 'spec_helper'
 
 describe ErrorTracking::ListProjectsService do
-  include ReactiveCachingHelpers
-
   set(:user) { create(:user) }
   set(:project) { create(:project) }
 
@@ -28,6 +26,8 @@ describe ErrorTracking::ListProjectsService do
     let(:result) { subject.execute }
 
     context 'with authorized user' do
+      let(:sentry_client) { spy(:sentry_client) }
+
       before do
         expect(project).to receive(:error_tracking_setting).at_least(:once)
           .and_return(error_tracking_setting)
@@ -37,8 +37,6 @@ describe ErrorTracking::ListProjectsService do
         let(:new_api_url) { new_api_host + 'api/0/projects/' }
 
         before do
-          synchronous_reactive_cache(error_tracking_setting)
-
           expect(error_tracking_setting).to receive(:list_sentry_projects)
             .and_return({ projects: [] })
         end
@@ -54,6 +52,18 @@ describe ErrorTracking::ListProjectsService do
         end
       end
 
+      context 'sentry client raises exception' do
+        before do
+          expect(error_tracking_setting).to receive(:list_sentry_projects)
+            .and_raise(Sentry::Client::Error, 'Sentry response error: 500')
+        end
+
+        it 'returns error response' do
+          expect(result[:message]).to eq('Sentry response error: 500')
+          expect(result[:http_status]).to eq(:bad_request)
+        end
+      end
+
       context 'with invalid url' do
         let(:params) do
           ActionController::Parameters.new(
@@ -84,39 +94,6 @@ describe ErrorTracking::ListProjectsService do
           expect(result).to eq(status: :success, projects: projects)
         end
       end
-
-      context 'when list_sentry_projects returns nil' do
-        before do
-          expect(error_tracking_setting)
-            .to receive(:list_sentry_projects).and_return(nil)
-        end
-
-        it 'result is not ready' do
-          result = subject.execute
-
-          expect(result).to eq(
-            status: :error,
-            http_status: :no_content,
-            message: 'not ready'
-          )
-        end
-      end
-
-      context 'when list_sentry_projects returns empty array' do
-        before do
-          expect(error_tracking_setting)
-            .to receive(:list_sentry_projects).and_return({ projects: [] })
-        end
-
-        it 'returns the empty array' do
-          result = subject.execute
-
-          expect(result).to eq(
-            status: :success,
-            projects: []
-          )
-        end
-      end
     end
 
     context 'with unauthorized user' do
@@ -137,29 +114,26 @@ describe ErrorTracking::ListProjectsService do
           .to receive(:list_sentry_projects).and_return(projects: [])
 
         error_tracking_setting.enabled = false
-        error_tracking_setting.save!
       end
 
       it 'ignores enabled flag' do
         expect(result).to include(status: :success, projects: [])
-
-        error_tracking_setting.reload
-        expect(error_tracking_setting.enabled).to be false
       end
     end
 
     context 'error_tracking_setting is nil' do
-      let(:error_tracking_setting) { build(:project_error_tracking_setting) }
+      let(:sentry_client) { spy(:sentry_client) }
 
       before do
         expect(project).to receive(:error_tracking_setting).at_least(:once)
           .and_return(nil)
 
-        expect(project).to receive(:build_error_tracking_setting).once
-          .and_return(error_tracking_setting)
+        expect(Sentry::Client).to receive(:new)
+          .with(new_api_host + 'api/0/projects/', new_token)
+          .and_return(sentry_client)
 
-        expect(error_tracking_setting).to receive(:list_sentry_projects)
-          .and_return(projects: [:project1, :project2])
+        expect(sentry_client).to receive(:list_projects)
+          .and_return([:project1, :project2])
       end
 
       it 'builds a new error_tracking_setting' do
-- 
cgit v1.2.1


From 5d61a7dee4b47a08c549717d43b0b0cff146cb22 Mon Sep 17 00:00:00 2001
From: rpereira2 <rpereira@gitlab.com>
Date: Wed, 6 Feb 2019 18:22:18 +0530
Subject: Remove stubbed sentry client

---
 .../error_tracking/list_projects_service_spec.rb   | 26 +++++++++++++---------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/spec/services/error_tracking/list_projects_service_spec.rb b/spec/services/error_tracking/list_projects_service_spec.rb
index c3363036c78..ee9c59e3f65 100644
--- a/spec/services/error_tracking/list_projects_service_spec.rb
+++ b/spec/services/error_tracking/list_projects_service_spec.rb
@@ -26,8 +26,6 @@ describe ErrorTracking::ListProjectsService do
     let(:result) { subject.execute }
 
     context 'with authorized user' do
-      let(:sentry_client) { spy(:sentry_client) }
-
       before do
         expect(project).to receive(:error_tracking_setting).at_least(:once)
           .and_return(error_tracking_setting)
@@ -122,22 +120,28 @@ describe ErrorTracking::ListProjectsService do
     end
 
     context 'error_tracking_setting is nil' do
-      let(:sentry_client) { spy(:sentry_client) }
+      let(:error_tracking_setting) { build(:project_error_tracking_setting) }
+      let(:new_api_url) { new_api_host + 'api/0/projects/' }
 
       before do
-        expect(project).to receive(:error_tracking_setting).at_least(:once)
-          .and_return(nil)
-
-        expect(Sentry::Client).to receive(:new)
-          .with(new_api_host + 'api/0/projects/', new_token)
-          .and_return(sentry_client)
+        expect(project).to receive(:build_error_tracking_setting).once
+          .and_return(error_tracking_setting)
 
-        expect(sentry_client).to receive(:list_projects)
-          .and_return([:project1, :project2])
+        expect(error_tracking_setting).to receive(:list_sentry_projects)
+          .and_return(projects: [:project1, :project2])
       end
 
       it 'builds a new error_tracking_setting' do
+        expect(project.error_tracking_setting).to be_nil
+
         expect(result[:projects]).to eq([:project1, :project2])
+
+        expect(error_tracking_setting.api_url).to eq(new_api_url)
+        expect(error_tracking_setting.token).to eq(new_token)
+        expect(error_tracking_setting.enabled).to be true
+        expect(error_tracking_setting.persisted?).to be false
+        expect(error_tracking_setting.project_id).not_to be_nil
+
         expect(project.error_tracking_setting).to be_nil
       end
     end
-- 
cgit v1.2.1